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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

In D155809#4553727 <https://reviews.llvm.org/D155809#4553727>, @aaron.ballman wrote:

> In D155809#4553694 <https://reviews.llvm.org/D155809#4553694>, @danlark wrote:
>
>> In D155809#4553690 <https://reviews.llvm.org/D155809#4553690>, @aaron.ballman wrote:
>>
>>> In D155809#4551876 <https://reviews.llvm.org/D155809#4551876>, @danlark wrote:
>>>
>>>> In D155809#4551721 <https://reviews.llvm.org/D155809#4551721>, @cor3ntin wrote:
>>>>
>>>>> I think the test we want is a set of classes and virtuaal fuc
>>>>>
>>>>> In D155809#4551686 <https://reviews.llvm.org/D155809#4551686>, @philnik wrote:
>>>>>
>>>>>> @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 gets from the clang code. That's why he can't just add a test that fires the assertion. Currently, there are no bots that build clang against a libc++ with debug assertions enabled.
>>>>>
>>>>> What we are asking for is a standalone minimal test that doesn't depend on libc++ at all, living in `clang/test/`
>>>>
>>>> That's impossible. No test will fail because no implementation of std::stable_sort calls `comp(a, a)`. Only libcxx calls it in debug mode. But by C++ standard rules, `comp(a, a)` is allowed to be called.
>>>
>>> It's not impossible. :-) Can you share a link to which libc++ test case is failing in debug mode? We can work backwards from there to build up a test case.
>>
>> The issue is not in libc++, the assert fires if you build clang in the following way:
>>
>> 1. Build clang binary from source with the latest libc++ at head.
>> 2. Build clang binary from source with -D_LIBCPP_ENABLE_DEBUG_MODE=1 -D_LIBCPP_ENABLE_HARDENED_MODE=1 (that's a libc++ define controlling standard library behavior which every file in the clang project uses, including this piece of code)
>> 3. Run llvm-lit on clang tests
>> 4. You will see 2 failures in  llvm/llvm-project/clang/test/CodeGen:available-externally-hidden.cpp.test and llvm/llvm-project/clang/test/CodeGenCXX:cxx2a-three-way-comparison.cpp.test (
>>
>> Without steps 1 and 2, there is no way to have assert on line 1569 <https://github.com/llvm/llvm-project/blob/2773098ee3187d5f9daca8938d57657dd89dd36f/clang/lib/AST/VTableBuilder.cpp#L1569>. No implementation of std::stable_sort in any major standard library can reach this line
>
> OH! The critical bit I was missing that you're building *Clang with libc++* in order to see this issue, and the problem is that this particular call to `std::stable_sort` has UB because of a violated precondition. Thanks for sticking with the discussion until we had this clarity.
>
> The changes LGTM now that I understand the context better. This doesn't seem to warrant a release note though, as users wouldn't see any differences.

Thank you, I'll improve my communication so that we reach to an agreement faster.

I don't have commit rights. Danila Kutenin. kutdanila at yandex.ru


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809