[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Reverted in 0342bbf223fa12701a0570a23f9eac433b8b341c for now, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza added a comment.

In D155895#4566903 , @thakis wrote:

> Looks like this breaks tests on windows: 
> http://45.33.8.238/win/82239/step_7.txt

Thanks for the report!  It seems that `#if defined(_WIN64) && 
!defined(__MINGW32__)` from `clang/test/SemaCXX/attr-trivial-abi.cpp` needs to 
be applied to fix the failing expectations of the new tests:

  error: 'error' diagnostics seen but not expected: 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 182: 
static assertion failed due to requirement 
'__is_trivially_relocatable(anonymousUnionsAndStructs::Trivial)': 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 200: 
static assertion failed due to requirement 
'__is_trivially_relocatable(anonymousUnionsAndStructs::BasicStruct)': 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 225: 
static assertion failed due to requirement 
'__is_trivially_relocatable(anonymousUnionsAndStructs::StructWithAnonymousUnion)':
 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 239: 
static assertion failed due to requirement 
'__is_trivially_relocatable(anonymousUnionsAndStructs::StructWithAnonymousStruct)':
 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 266: 
static assertion failed due to requirement 
'__is_trivially_relocatable(anonymousUnionsAndStructs::TrivialAbiAttributeAppliedToAnonymousUnion)':
 
  error: 'warning' diagnostics seen but not expected: 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 194: 
'trivial_abi' cannot be applied to 'BasicStruct'
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 212: 
'trivial_abi' cannot be applied to 'StructWithAnonymousUnion'
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 233: 
'trivial_abi' cannot be applied to 'StructWithAnonymousStruct'
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 251: 
'trivial_abi' cannot be applied to 'TrivialAbiAttributeAppliedToAnonymousUnion'
  error: 'note' diagnostics seen but not expected: 
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 194: 
'trivial_abi' is disallowed on 'BasicStruct' because it has a field of a 
non-trivial class type
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 212: 
'trivial_abi' is disallowed on 'StructWithAnonymousUnion' because it has a 
field of a non-trivial class type
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 233: 
'trivial_abi' is disallowed on 'StructWithAnonymousStruct' because it has a 
field of a non-trivial class type
File C:\src\llvm-project\clang\test\SemaCXX\attr-trivial-abi.cpp Line 251: 
'trivial_abi' is disallowed on 'TrivialAbiAttributeAppliedToAnonymousUnion' 
because it has a field of a non-trivial class type

Alternatively, maybe the test classes can be made trivially copyable.

> Please take a look and revert for now if it takes a while to fix.

I need to take a few days off this week, so I think it will indeed be safest to 
revert for now.  I don't have write access to the repo though - I hope that 
@gribozavr2 can help with the revert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Looks like this breaks tests on windows: http://45.33.8.238/win/82239/step_7.txt

Please take a look and revert for now if it takes a while to fix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Dmitri Gribenko 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 rGbddaa3517786: Anonymous unions should be transparent wrt 
`[[clang::trivial_abi]]`. (authored by gribozavr).

Changed prior to commit:
  https://reviews.llvm.org/D155895?vs=547862=547878#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp

Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17
 
 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
 
@@ -169,3 +170,115 @@
 static_assert(__is_trivially_relocatable(S20), "");
 #endif
 } // namespace deletedCopyMoveConstructor
+
+namespace anonymousUnionsAndStructs {
+  // Test helper:
+  struct [[clang::trivial_abi]] Trivial {
+Trivial() {}
+Trivial(Trivial&& other) {}
+Trivial& operator=(Trivial&& other) { return *this; }
+~Trivial() {}
+  };
+  static_assert(__is_trivially_relocatable(Trivial), "");
+
+  // Test helper:
+  struct Nontrivial {
+Nontrivial() {}
+Nontrivial(Nontrivial&& other) {}
+Nontrivial& operator=(Nontrivial&& other) { return *this; }
+~Nontrivial() {}
+  };
+  static_assert(!__is_trivially_relocatable(Nontrivial), "");
+
+  // Basic smoke test, not yet related to anonymous unions or structs:
+  struct [[clang::trivial_abi]] BasicStruct {
+BasicStruct(BasicStruct&& other) {}
+BasicStruct& operator=(BasicStruct&& other) { return *this; }
+~BasicStruct() {}
+Trivial field;
+  };
+  static_assert(__is_trivially_relocatable(BasicStruct), "");
+
+  // `StructWithAnonymousUnion` is like `BasicStruct`, but `field` is wrapped in
+  // an anonymous union, and thus trivial relocatability of `BasicStruct` and
+  // `StructWithAnonymousUnion` should be the same).
+  //
+  // It's impossible to declare a constructor for an anonymous unions so to
+  // support applying `[[clang::trivial_abi]]` to structs containing anonymous
+  // unions, and therefore when processing fields of the struct containing the
+  // anonymous union, the trivial relocatability of the *union* is ignored and
+  // instead the union's fields are recursively inspected in
+  // `checkIllFormedTrivialABIStruct`.
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion {
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+// expected-warning@-2 {{'trivial_abi' cannot be applied to 'StructWithAnonymousUnion'}}
+// expected-note@-3 {{trivial_abi' is disallowed on 'StructWithAnonymousUnion' because it has a field of a non-trivial class type}}
+#endif
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+  static_assert(!__is_trivially_relocatable(StructWithAnonymousUnion), "");
+#else
+  static_assert(__is_trivially_relocatable(StructWithAnonymousUnion), "");
+#endif
+
+  // `StructWithAnonymousStruct` is like `StructWithAnonymousUnion` but uses an
+  // anonymous `struct` rather than an anonymous `union.  The same expectations
+  // can be applied to CLANG_ABI_COMPAT <= 17 and 18+, because the anonymous
+  // `struct` does have move constructors in the test below (unlike the
+  // anonymous `union` in the previous `StructWithAnonymousUnion` test).
+  struct [[clang::trivial_abi]] StructWithAnonymousStruct {
+StructWithAnonymousStruct(StructWithAnonymousStruct&& other) {}
+StructWithAnonymousStruct& operator=(StructWithAnonymousStruct&& other) { return *this; }
+~StructWithAnonymousStruct() {}
+struct { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousStruct), "");
+
+  // `TrivialAbiAttributeAppliedToAnonymousUnion` is like
+  // `StructWithAnonymousUnion` but with `[[clang::trivial_abi]]` also applied
+  // to the anonymous union.
+  //
+  // The example below shows that it is still *not* okay to explicitly apply
+  // `[[clang::trivial_abi]]` to anonymous unions. Handling this would require
+  // relaxing the `HasNonDeletedCopyOrMoveConstructor` check when
+  // `isAnonymousStructOrUnion` in `checkIllFormedTrivialABIStruct` but when
+  // that check runs `setAnonymousStructOrUnion` hasn't been called yet (i.e. at
+  // this point it's not possible to rely on 

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza updated this revision to Diff 547862.
lukasza added a comment.

More granular usage of `#if defined(CLANG_ABI_COMPAT) && ...` in tests


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

https://reviews.llvm.org/D155895

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp

Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17
 
 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
 
@@ -169,3 +170,113 @@
 static_assert(__is_trivially_relocatable(S20), "");
 #endif
 } // namespace deletedCopyMoveConstructor
+
+namespace anonymousUnionsAndStructs {
+  // Test helper:
+  struct [[clang::trivial_abi]] Trivial {
+Trivial() {}
+Trivial(Trivial&& other) {}
+Trivial& operator=(Trivial&& other) { return *this; }
+~Trivial() {}
+  };
+  static_assert(__is_trivially_relocatable(Trivial), "");
+
+  // Test helper:
+  struct Nontrivial {
+Nontrivial() {}
+Nontrivial(Nontrivial&& other) {}
+Nontrivial& operator=(Nontrivial&& other) { return *this; }
+~Nontrivial() {}
+  };
+  static_assert(!__is_trivially_relocatable(Nontrivial), "");
+
+  // Basic smoke test, not yet related to anonymous unions or structs:
+  struct [[clang::trivial_abi]] BasicStruct {
+BasicStruct(BasicStruct&& other) {}
+BasicStruct& operator=(BasicStruct&& other) { return *this; }
+~BasicStruct() {}
+Trivial field;
+  };
+  static_assert(__is_trivially_relocatable(BasicStruct), "");
+
+  // `StructWithAnonymousUnion` is like `BasicStruct`, but `field` is wrapped in
+  // an anonymous union, and thus trivial relocatability of `BasicStruct` and
+  // `StructWithAnonymousUnion` should be the same).
+  //
+  // It's impossible to declare a constructor for an anonymous unions so to
+  // support applying `[[clang::trivial_abi]]` to structs containing anonymous
+  // unions, and therefore when processing fields of the struct containing the
+  // anonymous union, the trivial relocatability of the *union* is ignored and
+  // instead the union's fields are recursively inspected in
+  // `checkIllFormedTrivialABIStruct`.
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion { // expected-warning {{'trivial_abi' cannot be applied to 'StructWithAnonymousUnion'}} expected-note {{trivial_abi' is disallowed on 'StructWithAnonymousUnion' because it has a field of a non-trivial class type}}
+#else
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion {
+#endif
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+  static_assert(!__is_trivially_relocatable(StructWithAnonymousUnion), "");
+#else
+  static_assert(__is_trivially_relocatable(StructWithAnonymousUnion), "");
+#endif
+
+  // `StructWithAnonymousStruct` is like `StructWithAnonymousUnion` but uses an
+  // anonymous `struct` rather than an anonymous `union.  The same expectations
+  // can be applied to CLANG_ABI_COMPAT <= 17 and 18+, because the anonymous
+  // `struct` does have move constructors in the test below (unlike the
+  // anonymous `union` in the previous `StructWithAnonymousUnion` test).
+  struct [[clang::trivial_abi]] StructWithAnonymousStruct {
+StructWithAnonymousStruct(StructWithAnonymousStruct&& other) {}
+StructWithAnonymousStruct& operator=(StructWithAnonymousStruct&& other) { return *this; }
+~StructWithAnonymousStruct() {}
+struct { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousStruct), "");
+
+  // `TrivialAbiAttributeAppliedToAnonymousUnion` is like
+  // `StructWithAnonymousUnion` but with `[[clang::trivial_abi]]` also applied
+  // to the anonymous union.
+  //
+  // The example below shows that it is still *not* okay to explicitly apply
+  // `[[clang::trivial_abi]]` to anonymous unions. Handling this would require
+  // relaxing the `HasNonDeletedCopyOrMoveConstructor` check when
+  // `isAnonymousStructOrUnion` in `checkIllFormedTrivialABIStruct` but when
+  // that check runs `setAnonymousStructOrUnion` hasn't been called yet (i.e. at
+  // this point it's not possible to rely on `RD->isAnonymousStructOrUnion()`).
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+  struct [[clang::trivial_abi]] TrivialAbiAttributeAppliedToAnonymousUnion { // 

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 accepted this revision.
gribozavr2 added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:258
+  
static_assert(__is_trivially_relocatable(TrivialAbiAttributeAppliedToAnonymousUnion),
 "");
+#endif
+

Is it possible to restructure the ifdefs to only wrap the different parts?

That is, you can place only the `expected-warning` line and `static_assert` 
lines within ifdefs. You can use the `@`-syntax on `expected-warning` to tell 
clang to expect the warning on a different line, for example see 
`test/SemaCXX/deprecated.cpp`.


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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

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

rsmith added a comment.

LGTM, though please wait a day or two for any more comments from @gribozavr2 
since he's looked at this more closely than I have.


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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza updated this revision to Diff 544478.
lukasza added a comment.

Rebasing...


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

https://reviews.llvm.org/D155895

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp

Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17
 
 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
 
@@ -169,3 +170,104 @@
 static_assert(__is_trivially_relocatable(S20), "");
 #endif
 } // namespace deletedCopyMoveConstructor
+
+namespace anonymousUnionsAndStructs {
+  // Test helper:
+  struct [[clang::trivial_abi]] Trivial {
+Trivial() {}
+Trivial(Trivial&& other) {}
+Trivial& operator=(Trivial&& other) { return *this; }
+~Trivial() {}
+  };
+  static_assert(__is_trivially_relocatable(Trivial), "");
+
+  // Test helper:
+  struct Nontrivial {
+Nontrivial() {}
+Nontrivial(Nontrivial&& other) {}
+Nontrivial& operator=(Nontrivial&& other) { return *this; }
+~Nontrivial() {}
+  };
+  static_assert(!__is_trivially_relocatable(Nontrivial), "");
+
+  // Basic smoke test, not yet related to anonymous unions or structs:
+  struct [[clang::trivial_abi]] BasicStruct {
+BasicStruct(BasicStruct&& other) {}
+BasicStruct& operator=(BasicStruct&& other) { return *this; }
+~BasicStruct() {}
+Trivial field;
+  };
+  static_assert(__is_trivially_relocatable(BasicStruct), "");
+
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion { // expected-warning {{'trivial_abi' cannot be applied to 'StructWithAnonymousUnion'}} expected-note {{trivial_abi' is disallowed on 'StructWithAnonymousUnion' because it has a field of a non-trivial class type}}
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+  static_assert(!__is_trivially_relocatable(StructWithAnonymousUnion), "");
+#else
+  // `StructWithAnonymousUnion` is like `BasicStruct`, but `field` is wrapped in
+  // an anonymous union, and thus trivial relocatability of `BasicStruct` and
+  // `StructWithAnonymousUnion` should be the same).
+  //
+  // It's impossible to declare a constructor for an anonymous unions so to
+  // support applying `[[clang::trivial_abi]]` to structs containing anonymous
+  // unions, and therefore when processing fields of the struct containing the
+  // anonymous union, the trivial relocatability of the *union* is ignored and
+  // instead the union's fields are recursively inspected in
+  // `checkIllFormedTrivialABIStruct`.
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion {
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousUnion), "");
+
+  // `StructWithAnonymousStruct` is like `StructWithAnonymousUnion` but uses an
+  // anonymous `struct` rather than an anonymous `union.
+  struct [[clang::trivial_abi]] StructWithAnonymousStruct {
+StructWithAnonymousStruct(StructWithAnonymousStruct&& other) {}
+StructWithAnonymousStruct& operator=(StructWithAnonymousStruct&& other) { return *this; }
+~StructWithAnonymousStruct() {}
+struct { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousStruct), "");
+
+  // `TrivialAbiAttributeAppliedToAnonymousUnion` is like
+  // `StructWithAnonymousUnion` but with `[[clang::trivial_abi]]` also applied
+  // to the anonymous union.
+  //
+  // The example below shows that it is still *not* okay to explicitly apply
+  // `[[clang::trivial_abi]]` to anonymous unions. Handling this would require
+  // relaxing the `HasNonDeletedCopyOrMoveConstructor` check when
+  // `isAnonymousStructOrUnion` in `checkIllFormedTrivialABIStruct` but when
+  // that check runs `setAnonymousStructOrUnion` hasn't been called yet (i.e. at
+  // this point it's not possible to rely on `RD->isAnonymousStructOrUnion()`).
+  struct [[clang::trivial_abi]] TrivialAbiAttributeAppliedToAnonymousUnion {
+TrivialAbiAttributeAppliedToAnonymousUnion(TrivialAbiAttributeAppliedToAnonymousUnion&& other) {}
+TrivialAbiAttributeAppliedToAnonymousUnion& operator=(TrivialAbiAttributeAppliedToAnonymousUnion&& other) { return *this; 

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza updated this revision to Diff 544476.
lukasza added a comment.

Added support for `-fclang-abi-compat=17`


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

https://reviews.llvm.org/D155895

Files:
  clang/include/clang/Basic/LangOptions.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp

Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -verify %s -std=c++11
+// RUN: %clang_cc1 -fsyntax-only -fclang-abi-compat=17 -verify %s -std=c++11 -DCLANG_ABI_COMPAT=17
 
 void __attribute__((trivial_abi)) foo(); // expected-warning {{'trivial_abi' attribute only applies to classes}}
 
@@ -169,3 +170,104 @@
 static_assert(__is_trivially_relocatable(S20), "");
 #endif
 } // namespace deletedCopyMoveConstructor
+
+namespace anonymousUnionsAndStructs {
+  // Test helper:
+  struct [[clang::trivial_abi]] Trivial {
+Trivial() {}
+Trivial(Trivial&& other) {}
+Trivial& operator=(Trivial&& other) { return *this; }
+~Trivial() {}
+  };
+  static_assert(__is_trivially_relocatable(Trivial), "");
+
+  // Test helper:
+  struct Nontrivial {
+Nontrivial() {}
+Nontrivial(Nontrivial&& other) {}
+Nontrivial& operator=(Nontrivial&& other) { return *this; }
+~Nontrivial() {}
+  };
+  static_assert(!__is_trivially_relocatable(Nontrivial), "");
+
+  // Basic smoke test, not yet related to anonymous unions or structs:
+  struct [[clang::trivial_abi]] BasicStruct {
+BasicStruct(BasicStruct&& other) {}
+BasicStruct& operator=(BasicStruct&& other) { return *this; }
+~BasicStruct() {}
+Trivial field;
+  };
+  static_assert(__is_trivially_relocatable(BasicStruct), "");
+
+#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 17
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion { // expected-warning {{'trivial_abi' cannot be applied to 'StructWithAnonymousUnion'}} expected-note {{trivial_abi' is disallowed on 'StructWithAnonymousUnion' because it has a field of a non-trivial class type}}
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+  static_assert(!__is_trivially_relocatable(StructWithAnonymousUnion), "");
+#else
+  // `StructWithAnonymousUnion` is like `BasicStruct`, but `field` is wrapped in
+  // an anonymous union, and thus trivial relocatability of `BasicStruct` and
+  // `StructWithAnonymousUnion` should be the same).
+  //
+  // It's impossible to declare a constructor for an anonymous unions so to
+  // support applying `[[clang::trivial_abi]]` to structs containing anonymous
+  // unions, and therefore when processing fields of the struct containing the
+  // anonymous union, the trivial relocatability of the *union* is ignored and
+  // instead the union's fields are recursively inspected in
+  // `checkIllFormedTrivialABIStruct`.
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion {
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousUnion), "");
+
+  // `StructWithAnonymousStruct` is like `StructWithAnonymousUnion` but uses an
+  // anonymous `struct` rather than an anonymous `union.
+  struct [[clang::trivial_abi]] StructWithAnonymousStruct {
+StructWithAnonymousStruct(StructWithAnonymousStruct&& other) {}
+StructWithAnonymousStruct& operator=(StructWithAnonymousStruct&& other) { return *this; }
+~StructWithAnonymousStruct() {}
+struct { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousStruct), "");
+
+  // `TrivialAbiAttributeAppliedToAnonymousUnion` is like
+  // `StructWithAnonymousUnion` but with `[[clang::trivial_abi]]` also applied
+  // to the anonymous union.
+  //
+  // The example below shows that it is still *not* okay to explicitly apply
+  // `[[clang::trivial_abi]]` to anonymous unions. Handling this would require
+  // relaxing the `HasNonDeletedCopyOrMoveConstructor` check when
+  // `isAnonymousStructOrUnion` in `checkIllFormedTrivialABIStruct` but when
+  // that check runs `setAnonymousStructOrUnion` hasn't been called yet (i.e. at
+  // this point it's not possible to rely on `RD->isAnonymousStructOrUnion()`).
+  struct [[clang::trivial_abi]] TrivialAbiAttributeAppliedToAnonymousUnion {
+TrivialAbiAttributeAppliedToAnonymousUnion(TrivialAbiAttributeAppliedToAnonymousUnion&& other) {}
+TrivialAbiAttributeAppliedToAnonymousUnion& 

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

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

rsmith added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10339-10343
+  if (RT->getDecl()->isAnonymousStructOrUnion()) {
+FieldsToCheck.append(RT->getDecl()->field_begin(),
+ RT->getDecl()->field_end());
+continue;
+  }

Please add a check for `ClangABICompat` here (only do the new thing if 
`getLangOpts().getClangABICompat() > LangOptions::ClangABI::Ver17`) so that 
people with a stable ABI requirement can use `-fclang-abi-compat=17.0` or 
earlier to turn off this ABI change. This is the first ABI change since we 
branched off the 17.x release, so you'll also need to add the new `Ver17` 
enumerator and parsing support for it; grep for `Ver15` to find the places you 
may need to update.


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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza added a comment.

In D155895#4525417 , @gribozavr2 
wrote:

> @lukasza I think you forgot to upload an updated version of the patch.

Ooops, sorry about that.  I've been trying to use `arc`, but it seems that it 
has uploaded separate reviews at https://reviews.llvm.org/D156003 and 
https://reviews.llvm.org/D156004.


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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza updated this revision to Diff 543594.
lukasza marked an inline comment as done.
lukasza added a comment.

Addressed feedback from @gribozavr2


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

https://reviews.llvm.org/D155895

Files:
  clang/lib/Sema/SemaDeclCXX.cpp
  clang/test/SemaCXX/attr-trivial-abi.cpp

Index: clang/test/SemaCXX/attr-trivial-abi.cpp
===
--- clang/test/SemaCXX/attr-trivial-abi.cpp
+++ clang/test/SemaCXX/attr-trivial-abi.cpp
@@ -169,3 +169,94 @@
 static_assert(__is_trivially_relocatable(S20), "");
 #endif
 } // namespace deletedCopyMoveConstructor
+
+namespace anonymousUnionsAndStructs {
+  // Test helper:
+  struct [[clang::trivial_abi]] Trivial {
+Trivial() {}
+Trivial(Trivial&& other) {}
+Trivial& operator=(Trivial&& other) { return *this; }
+~Trivial() {}
+  };
+  static_assert(__is_trivially_relocatable(Trivial), "");
+
+  // Test helper:
+  struct Nontrivial {
+Nontrivial() {}
+Nontrivial(Nontrivial&& other) {}
+Nontrivial& operator=(Nontrivial&& other) { return *this; }
+~Nontrivial() {}
+  };
+  static_assert(!__is_trivially_relocatable(Nontrivial), "");
+
+  // Basic smoke test, not yet related to anonymous unions or structs:
+  struct [[clang::trivial_abi]] BasicStruct {
+BasicStruct(BasicStruct&& other) {}
+BasicStruct& operator=(BasicStruct&& other) { return *this; }
+~BasicStruct() {}
+Trivial field;
+  };
+  static_assert(__is_trivially_relocatable(BasicStruct), "");
+
+  // `StructWithAnonymousUnion` is like `BasicStruct`, but `field` is wrapped in
+  // an anonymous union, and thus trivial relocatability of `BasicStruct` and
+  // `StructWithAnonymousUnion` should be the same).
+  //
+  // It's impossible to declare a constructor for an anonymous unions so to
+  // support applying `[[clang::trivial_abi]]` to structs containing anonymous
+  // unions, and therefore when processing fields of the struct containing the
+  // anonymous union, the trivial relocatability of the *union* is ignored and
+  // instead the union's fields are recursively inspected in
+  // `checkIllFormedTrivialABIStruct`.
+  struct [[clang::trivial_abi]] StructWithAnonymousUnion {
+StructWithAnonymousUnion(StructWithAnonymousUnion&& other) {}
+StructWithAnonymousUnion& operator=(StructWithAnonymousUnion&& other) { return *this; }
+~StructWithAnonymousUnion() {}
+union { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousUnion), "");
+
+  // `StructWithAnonymousStruct` is like `StructWithAnonymousUnion` but uses an
+  // anonymous `struct` rather than an anonymous `union.
+  struct [[clang::trivial_abi]] StructWithAnonymousStruct {
+StructWithAnonymousStruct(StructWithAnonymousStruct&& other) {}
+StructWithAnonymousStruct& operator=(StructWithAnonymousStruct&& other) { return *this; }
+~StructWithAnonymousStruct() {}
+struct { Trivial field; };
+  };
+  static_assert(__is_trivially_relocatable(StructWithAnonymousStruct), "");
+
+  // `TrivialAbiAttributeAppliedToAnonymousUnion` is like
+  // `StructWithAnonymousUnion` but with `[[clang::trivial_abi]]` also applied
+  // to the anonymous union.
+  //
+  // The example below shows that it is still *not* okay to explicitly apply
+  // `[[clang::trivial_abi]]` to anonymous unions. Handling this would require
+  // relaxing the `HasNonDeletedCopyOrMoveConstructor` check when
+  // `isAnonymousStructOrUnion` in `checkIllFormedTrivialABIStruct` but when
+  // that check runs `setAnonymousStructOrUnion` hasn't been called yet (i.e. at
+  // this point it's not possible to rely on `RD->isAnonymousStructOrUnion()`).
+  struct [[clang::trivial_abi]] TrivialAbiAttributeAppliedToAnonymousUnion {
+TrivialAbiAttributeAppliedToAnonymousUnion(TrivialAbiAttributeAppliedToAnonymousUnion&& other) {}
+TrivialAbiAttributeAppliedToAnonymousUnion& operator=(TrivialAbiAttributeAppliedToAnonymousUnion&& other) { return *this; }
+~TrivialAbiAttributeAppliedToAnonymousUnion() {}
+union [[clang::trivial_abi]] { // expected-warning {{'trivial_abi' cannot be applied to '(unnamed union}} expected-note {{copy constructors and move constructors are all deleted}}
+  Trivial field;
+};
+  };
+  static_assert(__is_trivially_relocatable(TrivialAbiAttributeAppliedToAnonymousUnion), "");
+
+  // Like `StructWithAnonymousUnion`, but the field of the anonymous union is
+  // *not* trivial.
+  struct [[clang::trivial_abi]] StructWithAnonymousUnionWithNonTrivialField { // expected-warning {{'trivial_abi' cannot be applied to 'StructWithAnonymousUnionWithNonTrivialField'}} expected-note {{trivial_abi' is disallowed on 'StructWithAnonymousUnionWithNonTrivialField' because it has a field of a non-trivial class type}}
+StructWithAnonymousUnionWithNonTrivialField(StructWithAnonymousUnionWithNonTrivialField&& other) {}
+StructWithAnonymousUnionWithNonTrivialField& 

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added a comment.

@lukasza I think you forgot to upload an updated version of the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza marked 5 inline comments as done.
lukasza added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10325
 
-  for (const auto *FD : RD.fields()) {
-// Ill-formed if the field is an ObjectiveC pointer or of a type that is
-// non-trivial for the purpose of calls.
-QualType FT = FD->getType();
-if (FT.getObjCLifetime() == Qualifiers::OCL_Weak) {
-  PrintDiagAndRemoveAttr(4);
-  return;
-}
-
-if (const auto *RT = FT->getBaseElementTypeUnsafe()->getAs())
-  if (!RT->isDependentType() &&
-  !cast(RT->getDecl())->canPassInRegisters()) {
-PrintDiagAndRemoveAttr(5);
+  std::queue RecordsWithFieldsToCheck;
+  RecordsWithFieldsToCheck.push();

gribozavr2 wrote:
> It looks like we don't need to visit the fields in a specific order. If 
> that's the case, please use SmallVector and pop_back_val. (See 
> AnalyzeImplicitConversions for an example of a typical worklist algorithm.)
There is no strong requirement to visit the fields in a specific order.  It 
seemed nice to report the first field in source order that causes trouble 
(rather than reporting an deterministically arbitrary field).  OTOH, using 
`SmallVector` will indeed result in simpler code.

Done?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr2 added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10325
 
-  for (const auto *FD : RD.fields()) {
-// Ill-formed if the field is an ObjectiveC pointer or of a type that is
-// non-trivial for the purpose of calls.
-QualType FT = FD->getType();
-if (FT.getObjCLifetime() == Qualifiers::OCL_Weak) {
-  PrintDiagAndRemoveAttr(4);
-  return;
-}
-
-if (const auto *RT = FT->getBaseElementTypeUnsafe()->getAs())
-  if (!RT->isDependentType() &&
-  !cast(RT->getDecl())->canPassInRegisters()) {
-PrintDiagAndRemoveAttr(5);
+  std::queue RecordsWithFieldsToCheck;
+  RecordsWithFieldsToCheck.push();

It looks like we don't need to visit the fields in a specific order. If that's 
the case, please use SmallVector and pop_back_val. (See 
AnalyzeImplicitConversions for an example of a typical worklist algorithm.)



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10330
+  // Ill-formed if the field is an ObjectiveC pointer or of a type that is
+  // non-trivial for the purpose of calls.
+  QualType FT = FD->getType();

Please split off the part of the comment that talks about "non-trivial for the 
purpose of calls", move it to the newly added if statement below, and enhance 
the comment to explain the new rule about anonymous unions.



Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:201-202
+
+  // `S2` is like `S1`, but `field` is wrapped in an anonymous union (and it
+  // seems that trivial relocatability of `S1` and `S2` should be the same).
+  //





Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:232
+  // `S2` and `S3` examples above show that `[[clang::trivial_abi]]`
+  // *implicitly* propagates into anonymous union and structs.  The example
+  // below shows that it is still *not* okay to *explicitly* apply

That's not exactly true, we don't propagate the trivial_abi attribute onto the 
union type. After this patch, the type of the union (which is impossible to get 
ahold of) is still non-trivial.

What we do though, is that we ignore the trivial relocatability of the union 
itself, and instead look at the members.



Comment at: clang/test/SemaCXX/attr-trivial-abi.cpp:251
+  // Like `S2`, but the field of the anonymous union is *not* trivial.
+  struct [[clang::trivial_abi]] S5 { // expected-warning {{'trivial_abi' 
cannot be applied to 'S5'}} expected-note {{trivial_abi' is disallowed on 'S5' 
because it has a field of a non-trivial class type}}
+S5(S5&& other) {}

Could you replace the numbers in S1-S5 with more meaningful phrases?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155895: Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`.

[Łukasz Anforowicz via Phabricator via cfe-commits]

lukasza added inline comments.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10299
 
   if (!HasNonDeletedCopyOrMoveConstructor()) {
 PrintDiagAndRemoveAttr(0);

I tried to change this condition to `!RD.isAnonymousStructOrUnion() && 
!HasNonDeletedCopyOrMoveConstructor()` but it seems that at this point 
`isAnonymousStructOrUnion` doesn't yet work because `setAnonymousStructOrUnion` 
is called at a later point.



Comment at: clang/lib/Sema/SemaDeclCXX.cpp:10350
   }
 }
 

Alternative fix I've considered:
1.1. `setHasTrivialSpecialMemberForCall` for anonymous unions *if* their 
`DeclContext` is trivial.
1.2. Don't check `!HasNonDeletedCopyOrMoveConstructor()` for anonymous unions.

I don't know how to do 1.1 - this seems like a chicken-and-egg problem.  We 
can't know if `DeclContext` is trivial until we check 
`checkIllFormedTrivialABIStruct`, but when `checkIllFormedTrivialABIStruct` 
checks fields it requires that the triviality of the anonymous union is already 
known.

I don't know how to do 1.2 - changing the condition above to 
`!RD.isAnonymousStructOrUnion() && !HasNonDeletedCopyOrMoveConstructor()` 
doesn't work because at that point `isAnonymousStructOrUnion` doesn't yet work 
because `setAnonymousStructOrUnion` is called at a later point.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155895

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits