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

Endilll · 2024-03-07T12:18:41Z

This patch implements CWG2586 "Common initial sequence should consider over-alignment". Note that alignment of union members doesn't have to match, as layout compatibility of unions is not defined in terms of common initial sequence (http://eel.is/c++draft/class.mem.general#25).

…f structs This patch implements [CWG2586](https://cplusplus.github.io/CWG/issues/2583.html) "Common initial sequence should consider over-alignment ". Note that alignment of union members doesn't have to match, as layout compatibility of unions is not defined in terms of common initial sequence (http://eel.is/c++draft/class.mem.general#25).

llvmbot · 2024-03-07T12:19:12Z

@llvm/pr-subscribers-clang

Author: Vlad Serebrennikov (Endilll)

Changes

This patch implements CWG2586 "Common initial sequence should consider over-alignment". Note that alignment of union members doesn't have to match, as layout compatibility of unions is not defined in terms of common initial sequence (http://eel.is/c++draft/class.mem.general#25).

Full diff: https://github.com/llvm/llvm-project/pull/84313.diff

5 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+4)
(modified) clang/lib/Sema/SemaChecking.cpp (+7-2)
(modified) clang/test/CXX/drs/dr25xx.cpp (+26)
(modified) clang/test/SemaCXX/type-traits.cpp (+12-1)
(modified) clang/www/cxx_dr_status.html (+1-1)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 1b901a27fd19d1..62cc6aae4ee58b 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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>`_).
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 3597f93a017136..5f607608cf7a53 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -19185,7 +19185,8 @@ static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {
 
 /// Check if two fields are layout-compatible.
 static bool isLayoutCompatible(ASTContext &C, FieldDecl *Field1,
-                               FieldDecl *Field2) {
+                               FieldDecl *Field2,
+                               bool IgnoreAlignment = false) {
   if (!isLayoutCompatible(C, Field1->getType(), Field2->getType()))
     return false;
 
@@ -19204,6 +19205,10 @@ static bool isLayoutCompatible(ASTContext &C, FieldDecl *Field1,
   if (Field1->hasAttr<clang::NoUniqueAddressAttr>() ||
       Field2->hasAttr<clang::NoUniqueAddressAttr>())
     return false;
+
+  if (!IgnoreAlignment &&
+      Field1->getMaxAlignment() != Field2->getMaxAlignment())
+    return false;
   return true;
 }
 
@@ -19265,7 +19270,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, /*IgnoreAlignment=*/true)) {
         bool Result = UnmatchedFields.erase(*I);
         (void) Result;
         assert(Result);
diff --git a/clang/test/CXX/drs/dr25xx.cpp b/clang/test/CXX/drs/dr25xx.cpp
index 9fc7cf59485caa..46532486e50e53 100644
--- a/clang/test/CXX/drs/dr25xx.cpp
+++ b/clang/test/CXX/drs/dr25xx.cpp
@@ -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
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index 23c339ebdf0826..831de2589dcb9e 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -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;
@@ -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), "");
diff --git a/clang/www/cxx_dr_status.html b/clang/www/cxx_dr_status.html
index 774c71bc1cb6b7..494c0e1e9007bd 100755
--- a/clang/www/cxx_dr_status.html
+++ b/clang/www/cxx_dr_status.html
@@ -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>

cor3ntin

I only have a comment otherwise LGTM

cor3ntin · 2024-03-07T12:39:55Z

clang/lib/Sema/SemaChecking.cpp

@@ -19185,7 +19185,8 @@ static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {

 /// Check if two fields are layout-compatible.
 static bool isLayoutCompatible(ASTContext &C, FieldDecl *Field1,
-                               FieldDecl *Field2) {
+                               FieldDecl *Field2,
+                               bool IgnoreAlignment = false) {


I think I would prefer calling that IsUnionMember or something like that (and reverse the condition everywhere)

Yeah,I agree with Corentin, but mostly because it is from the wrong 'direction'. Bools like this should be named in a way that makes it easy for a user of this function, with no understanding of the implementation to understand.

So it should be, "I want to know if these two are layout compatible... Why would I ignore alignment?" vs "Oh, I'm not dealing with a union member, so I can set this false!".

but mostly because it is from the wrong 'direction'

This is spot-on observation. I wasn't sure which direction should I look at this from, but seeing both of you agree, I'm happy to change this.

erichkeane · 2024-03-07T19:25:14Z

clang/lib/Sema/SemaChecking.cpp

@@ -19185,7 +19185,7 @@ static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {

 /// Check if two fields are layout-compatible.


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

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.

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

cor3ntin

I am happy with that once @erichkeane's comment is addressed

erichkeane

I approve too, i trust you to update the comment for the new PARAMETER (not member, I'm a dummy) to make it clear.

Endilll added c++11 clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 7, 2024

Endilll requested review from cor3ntin, AaronBallman and shafik March 7, 2024 12:18

llvmbot added the clang Clang issues not falling into any other category label Mar 7, 2024

Endilll changed the title ~~[clang] Respect field alignment when evaluating layout compatiblity of structs~~ [clang] Respect field alignment when evaluating layout compatibility of structs Mar 7, 2024

Endilll changed the title ~~[clang] Respect field alignment when evaluating layout compatibility of structs~~ [clang] Respect field alignment in layout compatibility of structs Mar 7, 2024

cor3ntin reviewed Mar 7, 2024

View reviewed changes

Rename IgnoreAlignment parameter to IsUnionMember

c0110bd

erichkeane reviewed Mar 7, 2024

View reviewed changes

cor3ntin approved these changes Mar 7, 2024

View reviewed changes

erichkeane approved these changes Mar 7, 2024

View reviewed changes

Endilll added 2 commits March 7, 2024 23:42

Add a comment for new boolean parameter and an assert

5454f15

Add an additional assert for the value of AreUnionMembers

cc2039b

Endilll merged commit b6a3400 into llvm:main Mar 8, 2024
5 checks passed

Endilll deleted the layout-alignment branch March 8, 2024 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Endilll commented Mar 7, 2024

llvmbot commented Mar 7, 2024

cor3ntin left a comment

cor3ntin Mar 7, 2024

erichkeane Mar 7, 2024

cor3ntin Mar 7, 2024

Endilll Mar 7, 2024

erichkeane Mar 7, 2024

Endilll Mar 7, 2024

Endilll Mar 7, 2024

cor3ntin left a comment

erichkeane left a comment

		@@ -19185,7 +19185,7 @@ static bool isLayoutCompatible(ASTContext &C, EnumDecl ED1, EnumDecl ED2) {

		/// Check if two fields are layout-compatible.

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

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

Conversation

Endilll commented Mar 7, 2024

llvmbot commented Mar 7, 2024

cor3ntin left a comment

Choose a reason for hiding this comment

cor3ntin Mar 7, 2024

Choose a reason for hiding this comment

erichkeane Mar 7, 2024

Choose a reason for hiding this comment

cor3ntin Mar 7, 2024

Choose a reason for hiding this comment

Endilll Mar 7, 2024

Choose a reason for hiding this comment

erichkeane Mar 7, 2024

Choose a reason for hiding this comment

Endilll Mar 7, 2024

Choose a reason for hiding this comment

Endilll Mar 7, 2024

Choose a reason for hiding this comment

cor3ntin left a comment

Choose a reason for hiding this comment

erichkeane left a comment

Choose a reason for hiding this comment