[NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

[danlark]

Summary

In sorting elements can compare with themselves and sometimes assert further down the line was triggered

[shafik]

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.

[rsmith]

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.

[danlark]

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 equality comparison), and then order them by that integer? That'd be more obviously a strict weak order.

In 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

[aaron.ballman]

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 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, @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?

[danlark]

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

[danlark]

Ping

[aaron.ballman]

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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.

[danlark]

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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

[aaron.ballman]

In D155809#4550654, @danlark wrote:

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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.

[danlark]

In D155809#4550663, @aaron.ballman wrote:

In D155809#4550654, @danlark wrote:

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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.

[aaron.ballman]

In D155809#4550674, @danlark wrote:

In D155809#4550663, @aaron.ballman wrote:

In D155809#4550654, @danlark wrote:

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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.

[danlark]

In D155809#4550682, @aaron.ballman wrote:

In D155809#4550674, @danlark wrote:

In D155809#4550663, @aaron.ballman wrote:

In D155809#4550654, @danlark wrote:

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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

[aaron.ballman]

In D155809#4550700, @danlark wrote:

In D155809#4550682, @aaron.ballman wrote:

In D155809#4550674, @danlark wrote:

In D155809#4550663, @aaron.ballman wrote:

In D155809#4550654, @danlark wrote:

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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

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).

[danlark]

In D155809#4550711, @aaron.ballman wrote:

In D155809#4550700, @danlark wrote:

In D155809#4550682, @aaron.ballman wrote:

In D155809#4550674, @danlark wrote:

In D155809#4550663, @aaron.ballman wrote:

In D155809#4550654, @danlark wrote:

In D155809#4550646, @aaron.ballman wrote:

In D155809#4527890, @danlark wrote:

In D155809#4527847, @aaron.ballman wrote:

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 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, @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

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

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-lit 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.

[aaron.ballman]

(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

• 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-lit with libc++ at head in debug mode.

I'm ignorant of how things are tested within libc++ which may be contributing to the confusion here. https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGenCXX/cxx2a-three-way-comparison.cpp does not include any header files, so I don't understand what libc++ has to do with the test. What I'm asking for is to add a clang-specific test that fires the assertion today (copied and reduced from whatever test code you've got in libc++ that demonstrates the issue) and no longer fires the assertion with your patch so that we can land the Clang changes. That should then allow you to enable the existing debug tests in libc++ without hitting the assertion from Clang.

[philnik]

@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.

[cor3ntin]

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, 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/

[danlark]

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 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.

[aaron.ballman]

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, @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.

[danlark]

In D155809#4553690, @aaron.ballman wrote:

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, @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. No implementation of std::stable_sort in any major standard library can reach this line

[cor3ntin]

@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, the patch make sense, and I'm happy to accept it as is.

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 somewhere. Is that something we could do @aaron.ballman ?

[aaron.ballman]

In D155809#4553694, @danlark wrote:

In D155809#4553690, @aaron.ballman wrote:

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, @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. 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.

[aaron.ballman]

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 somewhere. Is that something we could do @aaron.ballman ?

Agreed, that would be a beneficial. We'd have to figure out which bot(s) are in the best state to enable that mode and reach out to their owner to see if they'd be willing to make a configuration change. Alternatively, someone could volunteer to stand up an entirely new bot to test that configuration.

[ilya-biryukov]

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 buildbot last year and getting a new one running over the same infrastructure should be easy.

[danlark]

In D155809#4553727, @aaron.ballman wrote:

In D155809#4553694, @danlark wrote:

In D155809#4553690, @aaron.ballman wrote:

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, @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. 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. [e-mail]

[aaron.ballman]

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 buildbot last year and getting a new one running over the same infrastructure should be easy.

Thank you for the offer!

In D155809#4553772, @danlark wrote:

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

No worries, I'll learn how to read better too. :-)

I don't have commit rights. Danila Kutenin. [e-mail]

I'll land the changes on your behalf shortly, thank you for the fix!

[MaskRay]

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 llvm/llvm-project/clang/test/CodeGen:available-externally-hidden.cpp.test and llvm/llvm-project/clang/test/CodeGenCXX:cxx2a-three-way-comparison.cpp.test (

This is the gist and I think it should have been included in the patch summary/git message, so that readers can get the information from the commit message, not just by reading the long discussions on this page.

clang/lib/AST/VTableBuilder.cpp

if (MD->isImplicit())
       NewImplicitVirtualFunctions.push_back(MD);
     else
       NewVirtualFunctions.push_back(MD);
   }
 
   std::stable_sort(
       NewImplicitVirtualFunctions.begin(), NewImplicitVirtualFunctions.end(),
       [](const CXXMethodDecl *A, const CXXMethodDecl *B) {
+        if (A == B)
+          return false;
         if (A->isCopyAssignmentOperator() != B->isCopyAssignmentOperator())
           return A->isCopyAssignmentOperator();
         if (A->isMoveAssignmentOperator() != B->isMoveAssignmentOperator())
           return A->isMoveAssignmentOperator();
         if (isa<CXXDestructorDecl>(A) != isa<CXXDestructorDecl>(B))
           return isa<CXXDestructorDecl>(A);
         assert(A->getOverloadedOperator() == OO_EqualEqual &&
                B->getOverloadedOperator() == OO_EqualEqual &&