Skip to content

[clang] Respect field alignment in layout compatibility of structs #84313

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,10 @@ Resolutions to C++ Defect Reports
of two types.
(`CWG1719: Layout compatibility and cv-qualification revisited <https://cplusplus.github.io/CWG/issues/1719.html>`_).

- Alignment of members is now respected when evaluating layout compatibility
of structs.
(`CWG2583: Common initial sequence should consider over-alignment <https://cplusplus.github.io/CWG/issues/2583.html>`_).

- ``[[no_unique_address]]`` is now respected when evaluating layout
compatibility of two types.
(`CWG2759: [[no_unique_address] and common initial sequence <https://cplusplus.github.io/CWG/issues/2759.html>`_).
Expand Down
23 changes: 21 additions & 2 deletions clang/lib/Sema/SemaChecking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19184,8 +19184,22 @@ static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {
}

/// Check if two fields are layout-compatible.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you update this comment for the new member? Is it both fields that are union members, or just one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! CWG2586 I implement here basically establish a different set of rules for layout compatibility of union members, and structs/classes are not layout compatible with unions by definition.

At this point I wonder if boolean is still needed, but my impression of our layout compatibility machinery is that it might be a bit too smart to infer the boolean. It's simple and recursive, without "looking back" so to say.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After an offline discussion, we agreed to keep the boolean, and to add an additional assert to ensure that the boolean is used correctly.

/// Can be used on union members, which are exempt from alignment requirement
/// of common initial sequence.
static bool isLayoutCompatible(ASTContext &C, FieldDecl *Field1,
FieldDecl *Field2) {
FieldDecl *Field2,
bool AreUnionMembers = false) {
const Type *Field1Parent = Field1->getParent()->getTypeForDecl();
const Type *Field2Parent = Field2->getParent()->getTypeForDecl();
assert(((Field1Parent->isStructureOrClassType() &&
Field2Parent->isStructureOrClassType()) ||
(Field1Parent->isUnionType() && Field2Parent->isUnionType())) &&
"Can't evaluate layout compatibility between a struct field and a "
"union field.");
assert(((!AreUnionMembers && Field1Parent->isStructureOrClassType()) ||
(AreUnionMembers && Field1Parent->isUnionType())) &&
"AreUnionMembers should be 'true' for union fields (only).");

if (!isLayoutCompatible(C, Field1->getType(), Field2->getType()))
return false;

Expand All @@ -19204,6 +19218,11 @@ static bool isLayoutCompatible(ASTContext &C, FieldDecl *Field1,
if (Field1->hasAttr<clang::NoUniqueAddressAttr>() ||
Field2->hasAttr<clang::NoUniqueAddressAttr>())
return false;

if (!AreUnionMembers &&
Field1->getMaxAlignment() != Field2->getMaxAlignment())
return false;

return true;
}

Expand Down Expand Up @@ -19265,7 +19284,7 @@ static bool isLayoutCompatibleUnion(ASTContext &C, RecordDecl *RD1,
E = UnmatchedFields.end();

for ( ; I != E; ++I) {
if (isLayoutCompatible(C, Field1, *I)) {
if (isLayoutCompatible(C, Field1, *I, /*IsUnionMember=*/true)) {
bool Result = UnmatchedFields.erase(*I);
(void) Result;
assert(Result);
Expand Down
26 changes: 26 additions & 0 deletions clang/test/CXX/drs/dr25xx.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,32 @@ namespace dr2565 { // dr2565: 16 open 2023-06-07
#endif
}

namespace dr2583 { // dr2583: 19
#if __cplusplus >= 201103L
struct A {
int i;
char c;
};

struct B {
int i;
alignas(8) char c;
};

union U {
A a;
B b;
};

union V {
A a;
alignas(64) B b;
};

static_assert(!__is_layout_compatible(A, B), "");
static_assert(__is_layout_compatible(U, V), "");
#endif
} // namespace dr2583

namespace dr2598 { // dr2598: 18
#if __cplusplus >= 201103L
Expand Down
13 changes: 12 additions & 1 deletion clang/test/SemaCXX/type-traits.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1681,6 +1681,16 @@ union UnionLayout3 {
[[no_unique_address]] CEmptyStruct d;
};

union UnionNoOveralignedMembers {
int a;
double b;
};

union UnionWithOveralignedMembers {
int a;
alignas(16) double b;
};

struct StructWithAnonUnion {
union {
int a;
Expand Down Expand Up @@ -1771,7 +1781,8 @@ void is_layout_compatible(int n)
static_assert(__is_layout_compatible(CStruct, CStructNoUniqueAddress) != bool(__has_cpp_attribute(no_unique_address)), "");
static_assert(__is_layout_compatible(CStructNoUniqueAddress, CStructNoUniqueAddress2) != bool(__has_cpp_attribute(no_unique_address)), "");
static_assert(__is_layout_compatible(CStruct, CStructAlignment), "");
static_assert(__is_layout_compatible(CStruct, CStructAlignedMembers), ""); // FIXME: alignment of members impact common initial sequence
static_assert(!__is_layout_compatible(CStruct, CStructAlignedMembers), "");
static_assert(__is_layout_compatible(UnionNoOveralignedMembers, UnionWithOveralignedMembers), "");
static_assert(__is_layout_compatible(CStructWithBitfelds, CStructWithBitfelds), "");
static_assert(__is_layout_compatible(CStructWithBitfelds, CStructWithBitfelds2), "");
static_assert(!__is_layout_compatible(CStructWithBitfelds, CStructWithBitfelds3), "");
Expand Down
2 changes: 1 addition & 1 deletion clang/www/cxx_dr_status.html
Original file line number Diff line number Diff line change
Expand Up @@ -15306,7 +15306,7 @@ <h2 id="cxxdr">C++ defect report implementation status</h2>
<td><a href="https://cplusplus.github.io/CWG/issues/2583.html">2583</a></td>
<td>C++23</td>
<td>Common initial sequence should consider over-alignment</td>
<td class="unknown" align="center">Unknown</td>
<td class="unreleased" align="center">Clang 19</td>
</tr>
<tr class="open" id="2584">
<td><a href="https://cplusplus.github.io/CWG/issues/2584.html">2584</a></td>
Expand Down