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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155809