This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by lukasza on Jul 20 2023, 3:05 PM.

Details

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.

Diff Detail

Event Timeline

lukasza created this revision.Jul 20 2023, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 3:05 PM
lukasza requested review of this revision.Jul 20 2023, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2023, 3:05 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lukasza retitled this revision from git squash commit for trivial-abi-vs-anonymous-unions. to Anonymous unions should be transparent wrt `[[clang::trivial_abi]]`..Jul 20 2023, 3:06 PM
lukasza edited the summary of this revision. (Show Details)
lukasza added inline comments.Jul 20 2023, 3:13 PM
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 added inline comments.
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 marked 5 inline comments as done.Jul 21 2023, 3:37 PM
lukasza added inline comments.
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?

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

lukasza updated this revision to Diff 543594.Jul 24 2023, 9:33 AM
lukasza marked an inline comment as done.

Addressed feedback from @gribozavr2

@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 added inline comments.Jul 26 2023, 11:51 AM
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 updated this revision to Diff 544476.Jul 26 2023, 12:39 PM

Added support for -fclang-abi-compat=17

lukasza marked an inline comment as done.Jul 26 2023, 12:40 PM
lukasza updated this revision to Diff 544478.Jul 26 2023, 12:47 PM

Rebasing...

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 accepted this revision.Aug 4 2023, 1:35 PM
gribozavr2 added inline comments.
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.

This revision is now accepted and ready to land.Aug 4 2023, 1:35 PM
lukasza marked an inline comment as done.Aug 4 2023, 3:40 PM
lukasza updated this revision to Diff 547862.Aug 7 2023, 10:53 AM

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

This revision was landed with ongoing or failed builds.Aug 7 2023, 11:31 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Aug 7 2023, 12:06 PM

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.

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?