danlark added a comment. In D155809#4550711 <https://reviews.llvm.org/D155809#4550711>, @aaron.ballman wrote:
> 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). Okay, but I still don't understand how I can satisfy the following conditions at the same time - Enable libc++ debug mode without failing existing tests like llvm/llvm-project/clang/test/CodeGen:available-externally-hidden.cpp.test and llvm/llvm-project/clang/test/CodeGenCXX:cxx2a-three-way-comparison.cpp.test (yes, these are the tests that reach the assertion I am talking about). They already exist. You can reproduce by building and running llvm with libc++ at head in debug mode. - Write a test that reliably fails given that in any existing CI mode the assert will not fire To fire the assert I need to enable libc++ debug mode. I looked through and right now I don't know how to do it. 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
