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

[Fangrui Song via Phabricator via cfe-commits]

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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 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
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,8 @@
   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())


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,8 @@
   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())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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 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. kutdan...@yandex.ru

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


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

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

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. kutdan...@yandex.ru


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

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

[Ilya Biryukov via Phabricator via cfe-commits]

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

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Corentin Jabot via Phabricator via cfe-commits]

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


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

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Danila Kutenin via Phabricator via cfe-commits]

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


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

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

[Corentin Jabot via Phabricator via cfe-commits]

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


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

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

[Nikolas Klauser via Phabricator via cfe-commits]

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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 

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a subscriber: ldionne.
danlark added a comment.

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 



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

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


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

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

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.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

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


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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.


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

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

[Danila Kutenin via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Danila Kutenin via Phabricator via cfe-commits]

danlark added a comment.

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 



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

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

[Aaron Ballman via Phabricator via cfe-commits]

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


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

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

[Danila Kutenin via Phabricator via cfe-commits]

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


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

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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 strict weak order.


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

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

[Shafik Yaghmour via Phabricator via cfe-commits]

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

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

[Danila Kutenin via Phabricator via cfe-commits]

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
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,8 @@
   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())


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,8 @@
   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())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Danila Kutenin via Phabricator via cfe-commits]

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:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155809

Files:
  clang/lib/AST/VTableBuilder.cpp


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,7 @@
   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())


Index: clang/lib/AST/VTableBuilder.cpp
===
--- clang/lib/AST/VTableBuilder.cpp
+++ clang/lib/AST/VTableBuilder.cpp
@@ -1560,6 +1560,7 @@
   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())
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits