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

[lukasza]

Summary

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

Consider the test input below:

struct [[clang::trivial_abi]] Trivial {
  Trivial() {}
  Trivial(Trivial&& other) {}
  Trivial& operator=(Trivial&& other) { return *this; }
  ~Trivial() {}
};
static_assert(__is_trivially_relocatable(Trivial), "");

struct [[clang::trivial_abi]] S2 {
  S2(S2&& other) {}
  S2& operator=(S2&& other) { return *this; }
  ~S2() {}
  union { Trivial field; };
};
static_assert(__is_trivially_relocatable(S2), "");

Before the fix Clang would warn that 'trivial_abi' is disallowed on 'S2'
because it has a field of a non-trivial class type (the type of the
anonymous union is non-trivial, because it doesn't have the
[[clang::trivial_abi]] attribute applied to it). Consequently, before
the fix the static_assert about __is_trivially_relocatable would
fail.

Note that [[clang::trivial_abi]] cannot be applied to the anonymous
union, because Clang warns that 'trivial_abi' is disallowed on '(unnamed
union at ...)' because its copy constructors and move constructors are
all deleted. Also note that it is impossible to provide copy nor move
constructors for anonymous unions and structs.

[lukasza]

clang/lib/Sema/SemaDeclCXX.cpp

10299

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.

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.

[gribozavr2]

clang/lib/Sema/SemaDeclCXX.cpp

10325

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

10330

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.

clang/test/SemaCXX/attr-trivial-abi.cpp

201–202

232

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.

251

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

[lukasza]

clang/lib/Sema/SemaDeclCXX.cpp

10325

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?

[gribozavr2]

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

[lukasza]

Addressed feedback from @gribozavr2

[lukasza]

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.

[rsmith]

clang/lib/Sema/SemaDeclCXX.cpp

10339–10343

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.

[lukasza]

Added support for -fclang-abi-compat=17

[lukasza]

Rebasing...

[rsmith]

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.

[gribozavr2]

clang/test/SemaCXX/attr-trivial-abi.cpp

258

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.

[lukasza]

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

[thakis]

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.

[lukasza]

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?

[thakis]

Reverted in 0342bbf223fa12701a0570a23f9eac433b8b341c for now, thanks!

clang/lib/Sema/SemaDeclCXX.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include <map>
 #include <optional>

     }
   }
 
-  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.
+  llvm::SmallVector<const FieldDecl *> FieldsToCheck{RD.fields()};
+  while (!FieldsToCheck.empty()) {
+    const FieldDecl *FD = FieldsToCheck.pop_back_val();
+
+    // Ill-formed if the field is an ObjectiveC pointer.
     QualType FT = FD->getType();
     if (FT.getObjCLifetime() == Qualifiers::OCL_Weak) {
       PrintDiagAndRemoveAttr(4);
       return;
     }
 
-    if (const auto *RT = FT->getBaseElementTypeUnsafe()->getAs<RecordType>())
+    if (const auto *RT = FT->getBaseElementTypeUnsafe()->getAs<RecordType>()) {
+      // Check the fields of anonymous structs (and/or unions) instead of
+      // checking the type of the anonynous struct itself.
+      if (RT->getDecl()->isAnonymousStructOrUnion()) {
+        FieldsToCheck.append(RT->getDecl()->field_begin(),
+                             RT->getDecl()->field_end());
+        continue;
+      }
+
+      // Ill-formed if the field is of a type that is non-trivial for the
+      // purpose of calls.
       if (!RT->isDependentType() &&
           !cast<CXXRecordDecl>(RT->getDecl())->canPassInRegisters()) {
         PrintDiagAndRemoveAttr(5);
         return;
       }
+    }
   }
 }
 

clang/test/SemaCXX/attr-trivial-abi.cpp

 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& operator=(StructWithAnonymousUnionWithNonTrivialField&& other) { return *this; }
+    ~StructWithAnonymousUnionWithNonTrivialField() {}
+    union {
+      Nontrivial field;
+    };
+  };
+  static_assert(!__is_trivially_relocatable(StructWithAnonymousUnionWithNonTrivialField), "");
+
+}  // namespace anonymousStructsAndUnions
+