This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by lukasza on Aug 22 2023, 10:57 AM.

Details

Reviewers
gribozavr2
Summary

This is a reland of https://reviews.llvm.org/D155895.

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]] S {
  S(S&& other) {}
  S& operator=(S&& other) { return *this; }
  ~S() {}
  union { Trivial field; };
};
static_assert(__is_trivially_relocatable(S), "");

Before the fix Clang would warn that 'trivial_abi' is disallowed on 'S'
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 that contain
fields with a non-trivial copy constructors or move constructors.

Diff Detail

Event Timeline

lukasza created this revision.Aug 22 2023, 10:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 10:57 AM
Herald added a subscriber: mstorsjo. · View Herald Transcript
lukasza requested review of this revision.Aug 22 2023, 10:57 AM

The delta from https://reviews.llvm.org/D155895 is in clang/test/SemaCXX/attr-trivial-abi.cpp where 1) a comment has been added above the non-trivial move constructor of the Trivial test helper and 2) test expectations have been tweaked to account for _WIN64 and __MINGW32__ (cargo-culting these conditions from the already-existing tests in the top part of this test file).

I have considered making Windows and non-Windows behaving in the same way, which would require making Trivial trivially-copyable (and consequently trivially-movable). This wouldn't repro the original problem, so I abandoned this direction.

gribozavr2 accepted this revision.Sep 19 2023, 11:29 AM
This revision is now accepted and ready to land.Sep 19 2023, 11:29 AM