[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

MaskRay added a comment. I think `assert((A == B || (A->getOverloadedOperator() == OO_EqualEqual && B->getOverloadedOperator() == OO_EqualEqual)) && ...);` would look better , but the current form is fine as well. > You will see 2 failures in >

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGecdded5692f9: [Clang] Fix strict weak ordering in ItaniumVTableBuilder (authored by danlark, committed by aaron.ballman). Repository: rG LLVM

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4553755 , @ilya-biryukov wrote: > If we don't find existing buildbots owners with easy addition for this > configuration, I can volunteer to add a new one with hardened libc++. > I have set up the C++20

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4553727 , @aaron.ballman wrote: > In D155809#4553694 , @danlark wrote: > >> In D155809#4553690 , >> @aaron.ballman wrote: >> >>> In

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

ilya-biryukov added a comment. If we don't find existing buildbots owners with this, I can volunteer to add a new one with hardened libc++. I have set up the C++20 buildbot last year and getting a new one running over the same infrastructure should be easy. Repository: rG LLVM Github

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4553718 , @cor3ntin wrote: > However, it might be interesting to enable LIBCPP_ENABLE_HARDENED_MODE on > some build bots - I am assuming we already test to build clang with libc++ in > a two stage bot

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D155809#4553694 , @danlark wrote: > In D155809#4553690 , @aaron.ballman > wrote: > >> In

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

cor3ntin added a comment. @danlark OH! It make sense now! Thanks for the detailed explanation, having the whole context definitively helps understand the issue. Clang is violating the `sable_sort` preconditions, and that is only observed in some standard library implementations. In that light,

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4553690 , @aaron.ballman wrote: > In D155809#4551876 , @danlark wrote: > >> In D155809#4551721 , @cor3ntin >> wrote: >> >>> I think

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4551876 , @danlark wrote: > In D155809#4551721 , @cor3ntin > wrote: > >> I think the test we want is a set of classes and virtuaal fuc >> >> In D155809#4551686

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4551721 , @cor3ntin wrote: > I think the test we want is a set of classes and virtuaal fuc > > In D155809#4551686 , @philnik wrote: > >> @aaron.ballman I think what @danlark

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

cor3ntin added a comment. I think the test we want is a set of classes and virtuaal fuc In D155809#4551686 , @philnik wrote: > @aaron.ballman I think what @danlark means is that when building clang > against a libc++ which has debug assertions enabled,

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

philnik added a comment. @aaron.ballman I think what @danlark means is that when building clang against a libc++ which has debug assertions enabled, the clang tests he mentioned result in an assertion firing within libc++. i.e. the libc++ debug mode catches the non-strict weak ordering that it

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. (I snipped off earlier context because it was starting to get hard to read.) In D155809#4550727 , @danlark wrote: > Okay, but I still don't understand how I can satisfy the following conditions > at the same time > > -

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4550711 , @aaron.ballman wrote: > In D155809#4550700 , @danlark wrote: > >> In D155809#4550682 , >> @aaron.ballman wrote: >> >>> In

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4550700 , @danlark wrote: > In D155809#4550682 , @aaron.ballman > wrote: > >> In D155809#4550674 , @danlark >> wrote: >> >>>

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a subscriber: ldionne. danlark added a comment. In D155809#4550682 , @aaron.ballman wrote: > In D155809#4550674 , @danlark wrote: > >> In D155809#4550663 ,

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4550674 , @danlark wrote: > In D155809#4550663 , @aaron.ballman > wrote: > >> In D155809#4550654 , @danlark >> wrote: >> >>>

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4550663 , @aaron.ballman wrote: > In D155809#4550654 , @danlark wrote: > >> In D155809#4550646 , >> @aaron.ballman wrote: >> >>> In

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4550654 , @danlark wrote: > In D155809#4550646 , @aaron.ballman > wrote: > >> In D155809#4527890 , @danlark >> wrote: >> >>>

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4550646 , @aaron.ballman wrote: > In D155809#4527890 , @danlark wrote: > >> In D155809#4527847 , >> @aaron.ballman wrote: >> >>> In

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4527890 , @danlark wrote: > In D155809#4527847 , @aaron.ballman > wrote: > >> In D155809#4527199 , @danlark >> wrote: >> >>>

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. Ping 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

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In D155809#4527847 , @aaron.ballman wrote: > In D155809#4527199 , @danlark wrote: > >> In D155809#4521494 , @rsmith wrote: >> >>> This looks

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

aaron.ballman added a comment. In D155809#4527199 , @danlark wrote: > In D155809#4521494 , @rsmith wrote: > >> This looks correct to me, but it's still a little subtle. Perhaps it'd be >> clearer to map the

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark added a comment. In 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

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

rsmith added a comment. 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

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

shafik added a reviewer: clang-language-wg. shafik added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark updated this revision to Diff 542492. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155809/new/ https://reviews.llvm.org/D155809 Files: clang/lib/AST/VTableBuilder.cpp Index: clang/lib/AST/VTableBuilder.cpp

[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

danlark created this revision. Herald added a project: All. danlark requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. In sorting elements can compare with themselves and sometimes assert further down the line was triggered Repository: