aaron.ballman added a comment. In D155809#4550700 <https://reviews.llvm.org/D155809#4550700>, @danlark wrote:
> In D155809#4550682 <https://reviews.llvm.org/D155809#4550682>, @aaron.ballman > wrote: > >> In D155809#4550674 <https://reviews.llvm.org/D155809#4550674>, @danlark >> wrote: >> >>> In D155809#4550663 <https://reviews.llvm.org/D155809#4550663>, >>> @aaron.ballman wrote: >>> >>>> In D155809#4550654 <https://reviews.llvm.org/D155809#4550654>, @danlark >>>> wrote: >>>> >>>>> In D155809#4550646 <https://reviews.llvm.org/D155809#4550646>, >>>>> @aaron.ballman wrote: >>>>> >>>>>> In D155809#4527890 <https://reviews.llvm.org/D155809#4527890>, @danlark >>>>>> wrote: >>>>>> >>>>>>> In D155809#4527847 <https://reviews.llvm.org/D155809#4527847>, >>>>>>> @aaron.ballman wrote: >>>>>>> >>>>>>>> In D155809#4527199 <https://reviews.llvm.org/D155809#4527199>, >>>>>>>> @danlark wrote: >>>>>>>> >>>>>>>>> In D155809#4521494 <https://reviews.llvm.org/D155809#4521494>, >>>>>>>>> @rsmith wrote: >>>>>>>>> >>>>>>>>>> This looks correct to me, but it's still a little subtle. Perhaps >>>>>>>>>> it'd be clearer to map the method to an integer (0 for copy >>>>>>>>>> assignment, 1 for move assignment, 2 for destructor, 3 for equality >>>>>>>>>> comparison), and then order them by that integer? That'd be more >>>>>>>>>> obviously a strict weak order. >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> In D155809#4520765 <https://reviews.llvm.org/D155809#4520765>, >>>>>>>>> @shafik wrote: >>>>>>>>> >>>>>>>>>> I am not sure about this change but I think we at least need a test >>>>>>>>>> and this does not seem non-functional if it prevents a crash. >>>>>>>>> >>>>>>>>> This is NFC as it only prevents further assert to fire when >>>>>>>>> stable_sort compares the element with itself >>>>>>>> >>>>>>>> Richard's suggestion makes sense to me as a clarifying change to the >>>>>>>> code. I also agree with Shafik -- if this prevents an assertion from >>>>>>>> firing in practice, then it's a functional change that should come >>>>>>>> with tests. Or are you saying the assertion isn't happening in >>>>>>>> practice and this is a defensive change? >>>>>>> >>>>>>> The assertion happens in debug libcxx mode after >>>>>>> https://reviews.llvm.org/D150264. This is a defensive change, in >>>>>>> practice, 2 same functions cannot happen in this comparator, this is >>>>>>> only for preventing assertions on line 1568 >>>>>>> <https://github.com/llvm/llvm-project/blob/2773098ee3187d5f9daca8938d57657dd89dd36f/clang/lib/AST/VTableBuilder.cpp#L1569> >>>>>> >>>>>> I apologize, but I'm still confused. If this assertion triggers in >>>>>> practice in debug modes with libc++, we should be able to make a >>>>>> stand-alone reproducer that we test as part of these changes within >>>>>> Clang. >>>>> >>>>> This assertion triggers in debug mode for various tests but clang is not >>>>> tested against libc++ debug mode for now. In non debug mode the assertion >>>>> is impossible to reach because in practice comp(a, a) is not called for >>>>> all implementations of sorting in all major standard libraries >>>> >>>> Okay, I think you should take the existing tests that trigger the >>>> assertion and reduce the code down to just what's needed to trigger the >>>> assertion, then add that code as a test case to Clang so that we can >>>> demonstrate the assert happens before your patch and doesn't happen after >>>> your patch. We've got a special lit mode (`// REQUIRES: asserts` as a >>>> comment near the `RUN` line) to enable the test only in asserts builds so >>>> you don't have to worry about assertions disabled changing the test >>>> behavior. >>> >>> libc++ debug mode is different from assertions in LLVM main library (first >>> is controlled with -D_LIBCPP_ENABLE_DEBUG_MODE, second is with -UNDEBUG). I >>> claim that now I cannot write the test which fails in any existing CI >>> configuration. And I cannot add a new version of CI because of failing >>> tests. That's why I defensively clean up comparators to enable CI in more >>> modes. I made the change as easy as possible to prove that it does not harm >>> the sorting overall. >> >> If it fails in any libc++ CI pipeline, you should be able to craft the same >> code to fail within Clang's test suite. The changes in the patch all look >> correct to me; the problem is that the patch claims to be NFC when it's not. >> It is making a functional change (it fixes assertions you were able to hit) >> and it has no test coverage for that. > > @ldionne is it possible to run clang test suite in the libc++ debug mode? I > looked through the CI and my opinion it's an exclusively libc++ feature > <https://github.com/llvm/llvm-project/blob/e3821e47da66eb10abb10d5b68cce5fcb167201b/libcxx/utils/libcxx/test/params.py#L297> To be clear, the request is not to run Clang's test suite in libc++ debug mode; it's to add a new test to Clang's test suite that demonstrates the fix. e.g., adding a test to https://github.com/llvm/llvm-project/tree/main/clang/test/SemaCXX (either a new test file or updating an existing test file). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155809/new/ https://reviews.llvm.org/D155809 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
