[Clang] Fix definition of layout-compatible to ignore empty classes #92103

MitalAshok · 2024-05-14T11:52:25Z

Also changes the behaviour of __builtin_is_layout_compatible

None of the historic nor the current definition of layout-compatible classes mention anything about base classes (other than implicitly through being standard-layout) and are defined in terms of members, not direct members.

llvmbot · 2024-05-14T11:52:56Z

@llvm/pr-subscribers-llvm-adt

@llvm/pr-subscribers-clang

Author: Mital Ashok (MitalAshok)

Changes

Also changes the behaviour of __builtin_is_layout_compatible

None of the historic nor the current definition of layout-compatible classes mention anything about base classes (other than implicitly through being standard-layout) and are defined in terms of members, not direct members.

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

5 Files Affected:

(modified) clang/include/clang/AST/DeclCXX.h (+7)
(modified) clang/lib/AST/DeclCXX.cpp (+36)
(modified) clang/lib/Sema/SemaChecking.cpp (+19-44)
(modified) clang/test/SemaCXX/type-traits.cpp (+11)
(modified) llvm/include/llvm/ADT/STLExtras.h (+6)

diff --git a/clang/include/clang/AST/DeclCXX.h b/clang/include/clang/AST/DeclCXX.h
index fb52ac804849d..f29592e1e7520 100644
--- a/clang/include/clang/AST/DeclCXX.h
+++ b/clang/include/clang/AST/DeclCXX.h
@@ -1229,6 +1229,13 @@ class CXXRecordDecl : public RecordDecl {
   /// C++11 [class]p7, specifically using the C++11 rules without any DRs.
   bool isCXX11StandardLayout() const { return data().IsCXX11StandardLayout; }
 
+  /// If this is a standard-layout class or union per C++11 rules,
+  /// any and all data members will be declared in the same type.
+  ///
+  /// This retrieves the type if this class has any data members,
+  /// or the current class if there is no class with fields.
+  const CXXRecordDecl *getStandardLayoutBaseWithFields() const;
+
   /// Determine whether this class, or any of its class subobjects,
   /// contains a mutable field.
   bool hasMutableFields() const { return data().HasMutableFields; }
diff --git a/clang/lib/AST/DeclCXX.cpp b/clang/lib/AST/DeclCXX.cpp
index 75c441293d62e..a101b61ad7392 100644
--- a/clang/lib/AST/DeclCXX.cpp
+++ b/clang/lib/AST/DeclCXX.cpp
@@ -561,6 +561,42 @@ void CXXRecordDecl::addedClassSubobject(CXXRecordDecl *Subobj) {
     data().StructuralIfLiteral = false;
 }
 
+const CXXRecordDecl *CXXRecordDecl::getStandardLayoutBaseWithFields() const {
+#ifndef NDEBUG
+  {
+    assert(
+        isCXX11StandardLayout() &&
+        "getStandardLayoutBaseWithFields called on a non-standard-layout type");
+    unsigned NumberOfBasesWithFields = 0;
+    if (!field_empty())
+      ++NumberOfBasesWithFields;
+    std::set<const CXXRecordDecl *> UniqueBases;
+    forallBases([&](const CXXRecordDecl *Base) -> bool {
+      if (!Base->field_empty())
+        ++NumberOfBasesWithFields;
+      assert(
+          UniqueBases.insert(Base->getCanonicalDecl()).second &&
+          "Standard layout struct has multiple base classes of the same type");
+      return true;
+    });
+    assert(NumberOfBasesWithFields <= 1 &&
+           "Standard layout struct has fields declared in more than one class");
+  }
+#endif
+  if (!field_empty())
+    return this;
+  const CXXRecordDecl *Result = this;
+  forallBases([&](const CXXRecordDecl *Base) -> bool {
+    if (!Base->field_empty()) {
+      // This is the base where the fields are declared; return early
+      Result = Base;
+      return false;
+    }
+    return true;
+  });
+  return Result;
+}
+
 bool CXXRecordDecl::hasConstexprDestructor() const {
   auto *Dtor = getDestructor();
   return Dtor ? Dtor->isConstexpr() : defaultedDestructorIsConstexpr();
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index ecd1821651140..acd142c1932ad 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -19084,7 +19084,8 @@ void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
 static bool isLayoutCompatible(ASTContext &C, QualType T1, QualType T2);
 
 /// Check if two enumeration types are layout-compatible.
-static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {
+static bool isLayoutCompatible(ASTContext &C, const EnumDecl *ED1,
+                               const EnumDecl *ED2) {
   // C++11 [dcl.enum] p8:
   // Two enumeration types are layout-compatible if they have the same
   // underlying type.
@@ -19095,8 +19096,8 @@ static bool isLayoutCompatible(ASTContext &C, EnumDecl *ED1, EnumDecl *ED2) {
 /// Check if two fields are layout-compatible.
 /// 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,
+static bool isLayoutCompatible(ASTContext &C, const FieldDecl *Field1,
+                               const FieldDecl *Field2,
                                bool AreUnionMembers = false) {
   [[maybe_unused]] const Type *Field1Parent =
       Field1->getParent()->getTypeForDecl();
@@ -19139,52 +19140,26 @@ static bool isLayoutCompatible(ASTContext &C, FieldDecl *Field1,
 
 /// Check if two standard-layout structs are layout-compatible.
 /// (C++11 [class.mem] p17)
-static bool isLayoutCompatibleStruct(ASTContext &C, RecordDecl *RD1,
-                                     RecordDecl *RD2) {
-  // If both records are C++ classes, check that base classes match.
-  if (const CXXRecordDecl *D1CXX = dyn_cast<CXXRecordDecl>(RD1)) {
-    // If one of records is a CXXRecordDecl we are in C++ mode,
-    // thus the other one is a CXXRecordDecl, too.
-    const CXXRecordDecl *D2CXX = cast<CXXRecordDecl>(RD2);
-    // Check number of base classes.
-    if (D1CXX->getNumBases() != D2CXX->getNumBases())
-      return false;
+static bool isLayoutCompatibleStruct(ASTContext &C, const RecordDecl *RD1,
+                                     const RecordDecl *RD2) {
+  // Get to the class where the fields are declared
+  if (const CXXRecordDecl *D1CXX = dyn_cast<CXXRecordDecl>(RD1))
+    RD1 = D1CXX->getStandardLayoutBaseWithFields();
 
-    // Check the base classes.
-    for (CXXRecordDecl::base_class_const_iterator
-               Base1 = D1CXX->bases_begin(),
-           BaseEnd1 = D1CXX->bases_end(),
-              Base2 = D2CXX->bases_begin();
-         Base1 != BaseEnd1;
-         ++Base1, ++Base2) {
-      if (!isLayoutCompatible(C, Base1->getType(), Base2->getType()))
-        return false;
-    }
-  } else if (const CXXRecordDecl *D2CXX = dyn_cast<CXXRecordDecl>(RD2)) {
-    // If only RD2 is a C++ class, it should have zero base classes.
-    if (D2CXX->getNumBases() > 0)
-      return false;
-  }
+  if (const CXXRecordDecl *D2CXX = dyn_cast<CXXRecordDecl>(RD2))
+    RD2 = D2CXX->getStandardLayoutBaseWithFields();
 
   // Check the fields.
-  RecordDecl::field_iterator Field2 = RD2->field_begin(),
-                             Field2End = RD2->field_end(),
-                             Field1 = RD1->field_begin(),
-                             Field1End = RD1->field_end();
-  for ( ; Field1 != Field1End && Field2 != Field2End; ++Field1, ++Field2) {
-    if (!isLayoutCompatible(C, *Field1, *Field2))
-      return false;
-  }
-  if (Field1 != Field1End || Field2 != Field2End)
-    return false;
-
-  return true;
+  return llvm::equal(RD1->fields(), RD2->fields(),
+                     [&C](const FieldDecl *F1, const FieldDecl *F2) -> bool {
+                       return isLayoutCompatible(C, F1, F2);
+                     });
 }
 
 /// Check if two standard-layout unions are layout-compatible.
 /// (C++11 [class.mem] p18)
-static bool isLayoutCompatibleUnion(ASTContext &C, RecordDecl *RD1,
-                                    RecordDecl *RD2) {
+static bool isLayoutCompatibleUnion(ASTContext &C, const RecordDecl *RD1,
+                                    const RecordDecl *RD2) {
   llvm::SmallPtrSet<FieldDecl *, 8> UnmatchedFields;
   for (auto *Field2 : RD2->fields())
     UnmatchedFields.insert(Field2);
@@ -19209,8 +19184,8 @@ static bool isLayoutCompatibleUnion(ASTContext &C, RecordDecl *RD1,
   return UnmatchedFields.empty();
 }
 
-static bool isLayoutCompatible(ASTContext &C, RecordDecl *RD1,
-                               RecordDecl *RD2) {
+static bool isLayoutCompatible(ASTContext &C, const RecordDecl *RD1,
+                               const RecordDecl *RD2) {
   if (RD1->isUnion() != RD2->isUnion())
     return false;
 
diff --git a/clang/test/SemaCXX/type-traits.cpp b/clang/test/SemaCXX/type-traits.cpp
index f2fd45762abf8..c32bb1b62828f 100644
--- a/clang/test/SemaCXX/type-traits.cpp
+++ b/clang/test/SemaCXX/type-traits.cpp
@@ -1734,6 +1734,11 @@ struct CStructWithFMA2 {
   int f[];
 };
 
+template<int N>
+struct UniqueEmpty {};
+template<typename... Bases>
+struct D : Bases... {};
+
 void is_layout_compatible(int n)
 {
   static_assert(__is_layout_compatible(void, void));
@@ -1837,6 +1842,12 @@ void is_layout_compatible(int n)
   static_assert(!__is_layout_compatible(EnumClassLayout, int));
   static_assert(!__is_layout_compatible(EnumForward, int));
   static_assert(!__is_layout_compatible(EnumClassForward, int));
+  static_assert(__is_layout_compatible(CStruct, D<CStruct>));
+  static_assert(__is_layout_compatible(CStruct, D<UniqueEmpty<0>, CStruct>));
+  static_assert(__is_layout_compatible(CStruct, D<UniqueEmpty<0>, D<UniqueEmpty<1>, CStruct>, D<UniqueEmpty<3>>>));
+  static_assert(__is_layout_compatible(CStruct, D<CStructWithQualifiers>));
+  static_assert(__is_layout_compatible(CStruct, D<UniqueEmpty<0>, CStructWithQualifiers>));
+  static_assert(__is_layout_compatible(CStructWithQualifiers, D<UniqueEmpty<0>, D<UniqueEmpty<1>, CStruct>, D<UniqueEmpty<3>>>));
 }
 
 namespace IPIBO {
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index 08a708e5c5871..3592dc3b9a4eb 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -2027,6 +2027,12 @@ template <typename L, typename R> bool equal(L &&LRange, R &&RRange) {
                     adl_end(RRange));
 }
 
+template <typename L, typename R, typename Predicate>
+bool equal(L &&LRange, R &&RRange, Predicate P) {
+  return std::equal(adl_begin(LRange), adl_end(LRange), adl_begin(RRange),
+                    adl_end(RRange), P);
+}
+
 /// Returns true if all elements in Range are equal or when the Range is empty.
 template <typename R> bool all_equal(R &&Range) {
   auto Begin = adl_begin(Range);

Also changes the behaviour of __builtin_is_layout_compatible None of the historic nor the current definition of layout-compatible classes mention anything about base classes (other than implicitly through being standard-layout) and are defined in terms of members, not direct members.

dwblaikie · 2024-05-20T20:37:02Z

How's this compare with other implementations clang is trying to be compatible with (gcc (in the normal clang driver mode) and msvc (in clang-cl mode))? Would this be an ABI break against them, or is this intended as an ABI fix to align better with them? Or some third option?

Endilll

Thank you for working on this!

clang/lib/AST/DeclCXX.cpp

MitalAshok · 2024-05-21T11:57:13Z

@dwblaikie This patch will bring Clang in line with GCC and MSVC: https://godbolt.org/z/nj715zbsW

dwblaikie · 2024-05-28T19:02:24Z

@dwblaikie This patch will bring Clang in line with GCC and MSVC: https://godbolt.org/z/nj715zbsW

would the change be ABI incompatible with previous versions of clang? If so, then it'll need to be versioned in Clang's ClangABI handling, so that platforms (like Sony's Playstation, and Apple's platforms) that are only interested in Clang ABI compatibility can retain the old behavior.

@rjmccall might have an interest in whether this is an ABI thing Apple cares about
@pogo59 not sure who over at Sony's the best person to check on sony ABI things

MitalAshok · 2024-05-28T20:29:39Z

Clang only uses Sema::isLayoutCompatible in two places: to implement __is_layout_compatible and Clang type safety checking attributes, which warn if a pointer does not point to a layout compatible type. This only affects compiler warnings.

The type safety attributes were introduced by e4a5a90 and they were exposed as __is_layout_compatible by d5922cf this February, so I don't think it was available in Clang 18

dwblaikie · 2024-05-29T18:13:37Z

Ah, OK - guess this might not be an ABI issue, then - carry on :) (I'll leave it to other Clang-y folks to do the rest of the review, the ABI issues were my only concern)

rjmccall · 2024-05-29T18:26:44Z

Changing a type trait is generally always an ABI problem, but yeah, if this hasn't been released yet, we're still free to fix basic bugs.

MitalAshok · 2024-06-22T08:31:58Z

llvm-project/clang/docs/ReleaseNotes.rst

Lines 210 to 212 in b0ae923

    
           - Implemented the `__is_layout_compatible` and `__is_pointer_interconvertible_base_of` 
        
             intrinsics to support 
        
             `P0466R5: Layout-compatibility and Pointer-interconvertibility Traits <https://wg21.link/P0466R5>`_.

Should be enough for ReleaseNotes, since not much has changed since Clang 18

MitalAshok · 2024-07-08T17:55:12Z

Ping

Will be ABI breaking for Clang 20

MitalAshok · 2024-07-31T17:31:14Z

I am also looking to cherry-pick this for Clang 19 so there won't be a difference between Clang 19 (when __is_layout_compatible was added) and Clang 20

rjmccall

The code changes look right to me.

rjmccall · 2024-07-31T22:07:50Z

clang/docs/ReleaseNotes.rst

@@ -160,6 +160,9 @@ Bug Fixes in This Version
 Bug Fixes to Compiler Builtins
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

+- ``__is_layout_compatible`` no longer requires the empty bases to be the same in two
+  standard-layout classes. It now only compares non-static data members.


Hmm. I was under the impression that this feature had not been previously released; is that incorrect?

That is correct, but the feature made into the 19 branch, whereas this fix did not (I hope we can backport it), hence a release note.

Is the plan to remove the release note from LLVM 20 if we're able to backport the fix?

I'm not sure, but this indeed is a possible course of action.
If you have preferences how this should be handled, let us know.
Personally I'm more focused on getting this backported.

We should backport these changes, drop the Clang 20 release note, and no need for a Clang 19.x release note to be added.

AaronBallman

LGTM!

Endilll · 2024-08-01T11:35:29Z

@MitalAshok Let us know when you apply the changes suggested by Aaron, and consider this PR ready to be merged. Unfortunately, clock is ticking on backporting.

This reverts commit dedf30c.

MitalAshok · 2024-08-01T14:03:21Z

@Endilll @AaronBallman Removed the release note and the tests have passed again. Would appreciate if someone else could start the backport process afterwards since I don't know how to do it.

Endilll · 2024-08-01T14:06:18Z

/cherry-pick 5d7357c

…lvm#92103) Also changes the behaviour of `__builtin_is_layout_compatible` None of the historic nor the current definition of layout-compatible classes mention anything about base classes (other than implicitly through being standard-layout) and are defined in terms of members, not direct members.

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:adt labels May 14, 2024

MitalAshok force-pushed the fix-layout-compatible branch from 147f0a7 to 74e1332 Compare May 14, 2024 11:55

cor3ntin requested a review from Endilll May 14, 2024 12:41

MitalAshok force-pushed the fix-layout-compatible branch from 74e1332 to 5908130 Compare May 14, 2024 12:56

Endilll reviewed May 21, 2024

View reviewed changes

clang/lib/AST/DeclCXX.cpp Outdated Show resolved Hide resolved

clang/lib/AST/DeclCXX.cpp Outdated Show resolved Hide resolved

clang/lib/AST/DeclCXX.cpp Outdated Show resolved Hide resolved

MitalAshok added 2 commits May 21, 2024 12:42

Use EXPENSIVE_CHECKS instead of !NDEBUG

dfb4d2a

More const

adc82e6

MitalAshok added 2 commits June 22, 2024 09:22

Merge branch 'main' into fix-layout-compatible

f088a63

Reword comment, move CXXRecordDecl::getStandardLayoutBaseWithFields

0a72deb

MitalAshok added 2 commits July 31, 2024 18:20

Merge branch 'main' into fix-layout-compatible

ee55fbb

Add release note

dedf30c

Will be ABI breaking for Clang 20

rjmccall reviewed Jul 31, 2024

View reviewed changes

AaronBallman approved these changes Aug 1, 2024

View reviewed changes

Revert "Add release note"

79997d6

This reverts commit dedf30c.

Endilll merged commit 5d7357c into llvm:main Aug 1, 2024
7 checks passed

Endilll mentioned this pull request Aug 1, 2024

release/19.x: [clang] Fix definition of layout-compatible to ignore empty classes #101491

Merged

mgabka mentioned this pull request Aug 22, 2024

Add release note about ABI mgabka/llvm-project#6

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clang] Fix definition of layout-compatible to ignore empty classes #92103

[Clang] Fix definition of layout-compatible to ignore empty classes #92103

MitalAshok commented May 14, 2024

llvmbot commented May 14, 2024 •

edited

Loading

dwblaikie commented May 20, 2024

Endilll left a comment

MitalAshok commented May 21, 2024

dwblaikie commented May 28, 2024

MitalAshok commented May 28, 2024

dwblaikie commented May 29, 2024

rjmccall commented May 29, 2024

MitalAshok commented Jun 22, 2024

MitalAshok commented Jul 8, 2024

MitalAshok commented Jul 31, 2024

rjmccall left a comment

rjmccall Jul 31, 2024

Endilll Jul 31, 2024

rjmccall Jul 31, 2024

Endilll Jul 31, 2024

AaronBallman Aug 1, 2024

AaronBallman left a comment

Endilll commented Aug 1, 2024

MitalAshok commented Aug 1, 2024

Endilll commented Aug 1, 2024

[Clang] Fix definition of layout-compatible to ignore empty classes #92103

[Clang] Fix definition of layout-compatible to ignore empty classes #92103

Conversation

MitalAshok commented May 14, 2024

llvmbot commented May 14, 2024 • edited Loading

dwblaikie commented May 20, 2024

Endilll left a comment

Choose a reason for hiding this comment

MitalAshok commented May 21, 2024

dwblaikie commented May 28, 2024

MitalAshok commented May 28, 2024

dwblaikie commented May 29, 2024

rjmccall commented May 29, 2024

MitalAshok commented Jun 22, 2024

MitalAshok commented Jul 8, 2024

MitalAshok commented Jul 31, 2024

rjmccall left a comment

Choose a reason for hiding this comment

rjmccall Jul 31, 2024

Choose a reason for hiding this comment

Endilll Jul 31, 2024

Choose a reason for hiding this comment

rjmccall Jul 31, 2024

Choose a reason for hiding this comment

Endilll Jul 31, 2024

Choose a reason for hiding this comment

AaronBallman Aug 1, 2024

Choose a reason for hiding this comment

AaronBallman left a comment

Choose a reason for hiding this comment

Endilll commented Aug 1, 2024

MitalAshok commented Aug 1, 2024

Endilll commented Aug 1, 2024

llvmbot commented May 14, 2024 •

edited

Loading