danlark added a subscriber: ldionne. danlark added a comment. 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> 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
