[Clang] Reuse tail-padding for more types that are not POD for the purpose of layout #90462

MitalAshok · 2024-04-29T12:27:10Z

This will be done for types with over-large bitfields and potentially-overlapping ([[no_unique_address]]) members

Compatible with old Clang 19 semantics with -fclang-abi-compat. New Ver19 added to that effect.

…rpose of layout This will be done for types with over-large bitfields and potentially-overlapping ([[no_unique_address]]) members Compatible with old Clang 18 semantics with -fclang-abi-compat Fixes llvm#50766

llvmbot · 2024-04-29T12:27:40Z

@llvm/pr-subscribers-clang

Author: Mital Ashok (MitalAshok)

Changes

This will be done for types with over-large bitfields and potentially-overlapping ([[no_unique_address]]) members

Compatible with old Clang 18 semantics with -fclang-abi-compat

Fixes #50766

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

7 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/include/clang/Basic/LangOptions.h (+3-1)
(modified) clang/include/clang/Basic/TargetCXXABI.h (-51)
(modified) clang/lib/AST/RecordLayoutBuilder.cpp (+100-34)
(modified) clang/test/CodeGenCXX/bitfield-layout.cpp (+13-7)
(modified) clang/test/CodeGenCXX/no-unique-address.cpp (+6-3)
(modified) clang/test/Layout/no-unique-address.cpp (+13-7)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 604782ca43dd54..b5d76c95a24bd1 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -63,6 +63,9 @@ ABI Changes in This Version
   MSVC uses a different mangling for these objects, compatibility is not affected.
   (#GH85423).
 
+- Fixed tail padding not being reused on types with oversized bit-fields and
+  potentially-overlapping members (#GH50766).
+
 AST Dumping Potentially Breaking Changes
 ----------------------------------------
 
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index e2a2aa71b880b3..3fe1362a2d767e 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -233,7 +233,9 @@ class LangOptionsBase {
 
     /// Attempt to be ABI-compatible with code generated by Clang 18.0.x.
     /// This causes clang to revert some fixes to the mangling of lambdas
-    /// in the initializers of members of local classes.
+    /// in the initializers of members of local classes and will not reuse
+    /// tail padding on structs with potentially-overlapping members or
+    /// oversized bit-fields under Itanium and some derived ABIs.
     Ver18,
 
     /// Conform to the underlying platform's C and C++ ABIs as closely
diff --git a/clang/include/clang/Basic/TargetCXXABI.h b/clang/include/clang/Basic/TargetCXXABI.h
index c113a6a048ad44..45d9dcc8f1b8a8 100644
--- a/clang/include/clang/Basic/TargetCXXABI.h
+++ b/clang/include/clang/Basic/TargetCXXABI.h
@@ -255,57 +255,6 @@ class TargetCXXABI {
     llvm_unreachable("bad ABI kind");
   }
 
-  /// When is record layout allowed to allocate objects in the tail
-  /// padding of a base class?
-  ///
-  /// This decision cannot be changed without breaking platform ABI
-  /// compatibility. In ISO C++98, tail padding reuse was only permitted for
-  /// non-POD base classes, but that restriction was removed retroactively by
-  /// DR 43, and tail padding reuse is always permitted in all de facto C++
-  /// language modes. However, many platforms use a variant of the old C++98
-  /// rule for compatibility.
-  enum TailPaddingUseRules {
-    /// The tail-padding of a base class is always theoretically
-    /// available, even if it's POD.
-    AlwaysUseTailPadding,
-
-    /// Only allocate objects in the tail padding of a base class if
-    /// the base class is not POD according to the rules of C++ TR1.
-    UseTailPaddingUnlessPOD03,
-
-    /// Only allocate objects in the tail padding of a base class if
-    /// the base class is not POD according to the rules of C++11.
-    UseTailPaddingUnlessPOD11
-  };
-  TailPaddingUseRules getTailPaddingUseRules() const {
-    switch (getKind()) {
-    // To preserve binary compatibility, the generic Itanium ABI has
-    // permanently locked the definition of POD to the rules of C++ TR1,
-    // and that trickles down to derived ABIs.
-    case GenericItanium:
-    case GenericAArch64:
-    case GenericARM:
-    case iOS:
-    case GenericMIPS:
-    case XL:
-      return UseTailPaddingUnlessPOD03;
-
-    // AppleARM64 and WebAssembly use the C++11 POD rules.  They do not honor
-    // the Itanium exception about classes with over-large bitfields.
-    case AppleARM64:
-    case Fuchsia:
-    case WebAssembly:
-    case WatchOS:
-      return UseTailPaddingUnlessPOD11;
-
-    // MSVC always allocates fields in the tail-padding of a base class
-    // subobject, even if they're POD.
-    case Microsoft:
-      return AlwaysUseTailPadding;
-    }
-    llvm_unreachable("bad ABI kind");
-  }
-
   friend bool operator==(const TargetCXXABI &left, const TargetCXXABI &right) {
     return left.getKind() == right.getKind();
   }
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index d9bf62c2bbb04a..0b6376381a53b7 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2415,46 +2415,112 @@ DiagnosticBuilder ItaniumRecordLayoutBuilder::Diag(SourceLocation Loc,
   return Context.getDiagnostics().Report(Loc, DiagID);
 }
 
+/// https://itanium-cxx-abi.github.io/cxx-abi/abi.html#POD
+/// POD for the purpose of layout
+///   In general, a type is considered a POD for the purposes of layout if it is
+///   a POD type (in the sense of ISO C++ [basic.types]). However, a type is not
+///   considered to be a POD for the purpose of layout if it is:
+///     - a POD-struct or POD-union (in the sense of ISO C++ [class]) with a
+///     bit-field whose declared width is wider than the declared type of the
+///     bit-field, or
+///     - an array type whose element type is not a POD for the purpose of
+///     layout, or
+///     - a POD-struct with one or more potentially-overlapping non-static data
+///     members.
+///   Where references to the ISO C++ are made in this paragraph, the Technical
+///   Corrigendum 1 version of the standard is intended.
+///
+/// This function does not check if the type is POD first
+static bool isItaniumPOD(const ASTContext &Context, const CXXRecordDecl *RD) {
+  const auto IsDisqualifying = [&](const FieldDecl *FD) -> bool {
+    if (FD->isBitField())
+      if (FD->getBitWidthValue(Context) > Context.getTypeSize(FD->getType()))
+        return true;
+
+    return FD->isPotentiallyOverlapping();
+  };
+
+  if (llvm::any_of(RD->fields(), IsDisqualifying))
+    return false;
+
+  return RD->forallBases([&](const CXXRecordDecl *Base) -> bool {
+    return llvm::any_of(Base->fields(), IsDisqualifying);
+  });
+}
+
 /// Does the target C++ ABI require us to skip over the tail-padding
 /// of the given class (considering it as a base class) when allocating
 /// objects?
-static bool mustSkipTailPadding(TargetCXXABI ABI, const CXXRecordDecl *RD) {
-  switch (ABI.getTailPaddingUseRules()) {
-  case TargetCXXABI::AlwaysUseTailPadding:
-    return false;
+static bool mustSkipTailPadding(const ASTContext &Context, TargetCXXABI ABI,
+                                const CXXRecordDecl *RD) {
+  // This is equivalent to
+  // Context.getTypeDeclType(RD).isCXX11PODType(Context),
+  // but with a lot of abstraction penalty stripped off.  This does
+  // assume that these properties are set correctly even in C++98
+  // mode; fortunately, that is true because we want to assign
+  // consistently semantics to the type-traits intrinsics (or at
+  // least as many of them as possible).
+  auto IsCXX11PODType = [&]() -> bool {
+    return RD->isTrivial() && RD->isCXX11StandardLayout();
+  };
+
+  if (Context.getLangOpts().getClangABICompat() <=
+      LangOptions::ClangABI::Ver18) {
+    switch (ABI.getKind()) {
+    // ABIs derived from Itanium: In Clang 18, did not check for over-large
+    // bit-fields or potentially overlapping members
+    case TargetCXXABI::GenericItanium:
+    case TargetCXXABI::GenericAArch64:
+    case TargetCXXABI::GenericARM:
+    case TargetCXXABI::iOS:
+    case TargetCXXABI::GenericMIPS:
+    case TargetCXXABI::XL:
+      return RD->isPOD();
+
+    case TargetCXXABI::AppleARM64:
+    case TargetCXXABI::Fuchsia:
+    case TargetCXXABI::WebAssembly:
+    case TargetCXXABI::WatchOS:
+      return IsCXX11PODType();
+
+    case TargetCXXABI::Microsoft:
+      return true;
+    }
+    llvm_unreachable("bad ABI kind");
+  }
+
+  switch (ABI.getKind()) {
+  // To preserve binary compatibility, the generic Itanium ABI has permanently
+  // locked the definition of POD to the rules of C++ TR1, and that trickles
+  // down to derived ABIs.
+  case TargetCXXABI::GenericItanium:
+  case TargetCXXABI::GenericAArch64:
+  case TargetCXXABI::GenericARM:
+  case TargetCXXABI::GenericMIPS:
+    return RD->isPOD() && isItaniumPOD(Context, RD);
 
-  case TargetCXXABI::UseTailPaddingUnlessPOD03:
-    // FIXME: To the extent that this is meant to cover the Itanium ABI
-    // rules, we should implement the restrictions about over-sized
-    // bitfields:
-    //
-    // http://itanium-cxx-abi.github.io/cxx-abi/abi.html#POD :
-    //   In general, a type is considered a POD for the purposes of
-    //   layout if it is a POD type (in the sense of ISO C++
-    //   [basic.types]). However, a POD-struct or POD-union (in the
-    //   sense of ISO C++ [class]) with a bitfield member whose
-    //   declared width is wider than the declared type of the
-    //   bitfield is not a POD for the purpose of layout.  Similarly,
-    //   an array type is not a POD for the purpose of layout if the
-    //   element type of the array is not a POD for the purpose of
-    //   layout.
-    //
-    //   Where references to the ISO C++ are made in this paragraph,
-    //   the Technical Corrigendum 1 version of the standard is
-    //   intended.
+  case TargetCXXABI::XL:
+  case TargetCXXABI::iOS:
     return RD->isPOD();
 
-  case TargetCXXABI::UseTailPaddingUnlessPOD11:
-    // This is equivalent to RD->getTypeForDecl().isCXX11PODType(),
-    // but with a lot of abstraction penalty stripped off.  This does
-    // assume that these properties are set correctly even in C++98
-    // mode; fortunately, that is true because we want to assign
-    // consistently semantics to the type-traits intrinsics (or at
-    // least as many of them as possible).
-    return RD->isTrivial() && RD->isCXX11StandardLayout();
+  // https://github.com/WebAssembly/tool-conventions/blob/cd83f847828336f10643d1f48aa60867c428c55c/ItaniumLikeC%2B%2BABI.md
+  // The same as Itanium except with C++11 POD instead of C++ TC1 POD
+  case TargetCXXABI::WebAssembly:
+    return IsCXX11PODType() && isItaniumPOD(Context, RD);
+
+  // Also uses C++11 POD but do not honor the Itanium exception about classes
+  // with over-large bitfields.
+  case TargetCXXABI::AppleARM64:
+  case TargetCXXABI::WatchOS:
+  case TargetCXXABI::Fuchsia:
+    return IsCXX11PODType();
+
+  // MSVC always allocates fields in the tail-padding of a base class
+  // subobject, even if they're POD.
+  case TargetCXXABI::Microsoft:
+    return true;
   }
-
-  llvm_unreachable("bad tail-padding use kind");
+  llvm_unreachable("bad ABI kind");
 }
 
 static bool isMsLayout(const ASTContext &Context) {
@@ -3388,7 +3454,7 @@ ASTContext::getASTRecordLayout(const RecordDecl *D) const {
       // tail-padding of base classes.  This is ABI-dependent.
       // FIXME: this should be stored in the record layout.
       bool skipTailPadding =
-          mustSkipTailPadding(getTargetInfo().getCXXABI(), RD);
+          mustSkipTailPadding(*this, getTargetInfo().getCXXABI(), RD);
 
       // FIXME: This should be done in FinalizeLayout.
       CharUnits DataSize =
diff --git a/clang/test/CodeGenCXX/bitfield-layout.cpp b/clang/test/CodeGenCXX/bitfield-layout.cpp
index 66a140a966c640..abf43b2f552801 100644
--- a/clang/test/CodeGenCXX/bitfield-layout.cpp
+++ b/clang/test/CodeGenCXX/bitfield-layout.cpp
@@ -1,10 +1,16 @@
-// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 %s
-// RUN: %clang_cc1 %s -triple=i386-apple-darwin10 -emit-llvm -o - -O3 | FileCheck %s
-// RUN: %clang_cc1 %s -triple=aarch64_be -emit-llvm -o - -O3 | FileCheck %s
-// RUN: %clang_cc1 %s -triple=thumbv7_be-none-eabi -emit-llvm -o - -O3 | FileCheck %s
-// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -emit-llvm -o - -O3 -std=c++11 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 %s
-
-// CHECK-LP64: %union.Test1 = type { i32, [4 x i8] }
+// RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 --check-prefix=NEW --check-prefix=NEW-LP64 %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -triple=i386-apple-darwin10 -emit-llvm -o - -O3 | FileCheck --check-prefix=CHECK --check-prefix=NEW %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -triple=aarch64_be -emit-llvm -o - -O3 | FileCheck --check-prefix=CHECK --check-prefix=NEW %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -triple=thumbv7_be-none-eabi -emit-llvm -o - -O3 | FileCheck --check-prefix=CHECK --check-prefix=NEW %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -emit-llvm -o - -O3 -std=c++11 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 --check-prefix=NEW --check-prefix=NEW-LP64 %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -fclang-abi-compat=18.0 -triple=x86_64-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 --check-prefix=OLD --check-prefix=OLD-LP64 %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -fclang-abi-compat=18.0 -triple=i386-apple-darwin10 -emit-llvm -o - -O3 | FileCheck --check-prefix=CHECK --check-prefix=OLD %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -fclang-abi-compat=18.0 -triple=aarch64_be -emit-llvm -o - -O3 | FileCheck --check-prefix=CHECK --check-prefix=OLD %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -fclang-abi-compat=18.0 -triple=thumbv7_be-none-eabi -emit-llvm -o - -O3 | FileCheck --check-prefix=CHECK --check-prefix=OLD %s --allow-unused-prefixes
+// RUN: %clang_cc1 %s -fclang-abi-compat=18.0 -triple=x86_64-unknown-unknown -emit-llvm -o - -O3 -std=c++11 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 --check-prefix=OLD --check-prefix=OLD-LP64 %s --allow-unused-prefixes
+
+// OLD-LP64: %union.Test1 = type { i32, [4 x i8] }
+// NEW-LP64: %union.Test1 = type <{ i32, [4 x i8] }>
 union Test1 {
   int a;
   int b: 39;
diff --git a/clang/test/CodeGenCXX/no-unique-address.cpp b/clang/test/CodeGenCXX/no-unique-address.cpp
index 7b4bbbf2a05d51..123069b79f2046 100644
--- a/clang/test/CodeGenCXX/no-unique-address.cpp
+++ b/clang/test/CodeGenCXX/no-unique-address.cpp
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 -std=c++2a %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s
-// RUN: %clang_cc1 -std=c++2a %s -emit-llvm -o - -triple x86_64-linux-gnu -O2 -disable-llvm-passes | FileCheck %s --check-prefix=CHECK-OPT
+// RUN: %clang_cc1 -std=c++2a %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=NEW --allow-unused-prefixes
+// RUN: %clang_cc1 -std=c++2a %s -emit-llvm -o - -triple x86_64-linux-gnu -O2 -disable-llvm-passes | FileCheck %s --check-prefix=CHECK-OPT --check-prefix=NEW-OPT --allow-unused-prefixes
+// RUN: %clang_cc1 -fclang-abi-compat=18.0 -std=c++2a %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefix=CHECK --check-prefix=OLD --allow-unused-prefixes
+// RUN: %clang_cc1 -fclang-abi-compat=18.0 -std=c++2a %s -emit-llvm -o - -triple x86_64-linux-gnu -O2 -disable-llvm-passes | FileCheck %s --check-prefix=CHECK-OPT --check-prefix=OLD-OPT --allow-unused-prefixes
 
 struct A { ~A(); int n; char c[3]; };
 struct B { [[no_unique_address]] A a; char k; };
@@ -39,7 +41,8 @@ Empty1 HasEmptyDuplicates::*off2 = &HasEmptyDuplicates::e2;
 // CHECK-DAG: @off3 ={{.*}} global i64 8
 Empty1 HasEmptyDuplicates::*off3 = &HasEmptyDuplicates::e3;
 
-// CHECK-DAG: @hed ={{.*}} global %{{[^ ]*}} { i32 1, i32 2, [4 x i8] undef }
+// OLD-DAG: @hed ={{.*}} global %struct.HasEmptyDuplicates { i32 1, i32 2, [4 x i8] undef }, align 4
+// NEW-DAG: @hed ={{.*}} global { i32, i32, [4 x i8] } { i32 1, i32 2, [4 x i8] undef }, align 4
 HasEmptyDuplicates hed = {{}, 1, {}, 2, {}};
 
 struct __attribute__((packed, aligned(2))) PackedAndPadded {
diff --git a/clang/test/Layout/no-unique-address.cpp b/clang/test/Layout/no-unique-address.cpp
index d5bb46647b88d0..60abd01ce5648a 100644
--- a/clang/test/Layout/no-unique-address.cpp
+++ b/clang/test/Layout/no-unique-address.cpp
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -std=c++2a -fsyntax-only -triple x86_64-linux-gnu -fdump-record-layouts %s | FileCheck %s
+// RUN: %clang_cc1 -std=c++2a -fsyntax-only -triple x86_64-linux-gnu -fdump-record-layouts %s | FileCheck %s --check-prefix=NEW --check-prefix=CHECK
+// RUN: %clang_cc1 -fclang-abi-compat=18.0 -DOLD_ABI=1 -std=c++2a -fsyntax-only -triple x86_64-linux-gnu -fdump-record-layouts %s | FileCheck %s --check-prefix=OLD --check-prefix=CHECK
 
 namespace Empty {
   struct A {};
@@ -40,7 +41,8 @@ namespace Empty {
   // CHECK-NEXT:     0 |   struct Empty::A a1 (empty)
   // CHECK-NEXT:     1 |   struct Empty::A a2 (empty)
   // CHECK-NEXT:     0 |   char e
-  // CHECK-NEXT:       | [sizeof=2, dsize=2, align=1,
+  // OLD-NEXT:         | [sizeof=2, dsize=2, align=1,
+  // NEW-NEXT:         | [sizeof=2, dsize=1, align=1,
   // CHECK-NEXT:       |  nvsize=2, nvalign=1]
 
   struct F {
@@ -120,8 +122,10 @@ namespace Empty {
   // CHECK-NEXT:     4 |   struct Empty::A b (empty)
   // CHECK-NEXT:     4 |   int y
   // CHECK-NEXT:     8 |   struct Empty::A c (empty)
-  // CHECK-NEXT:       | [sizeof=12, dsize=12, align=4,
-  // CHECK-NEXT:       |  nvsize=12, nvalign=4]
+  // OLD-NEXT:         | [sizeof=12, dsize=12, align=4,
+  // OLD-NEXT:         |  nvsize=12, nvalign=4]
+  // NEW-NEXT:         | [sizeof=12, dsize=8, align=4,
+  // NEW-NEXT:         |  nvsize=9, nvalign=4]
 
   struct EmptyWithNonzeroDSizeNonPOD {
     ~EmptyWithNonzeroDSizeNonPOD();
@@ -145,7 +149,7 @@ namespace Empty {
 }
 
 namespace POD {
-  // Cannot reuse tail padding of a PDO type.
+  // Cannot reuse tail padding of a POD type.
   struct A { int n; char c[3]; };
   struct B { [[no_unique_address]] A a; char d; };
   static_assert(sizeof(B) == 12);
@@ -156,8 +160,10 @@ namespace POD {
   // CHECK-NEXT:     0 |     int n
   // CHECK-NEXT:     4 |     char[3] c
   // CHECK-NEXT:     8 |   char d
-  // CHECK-NEXT:       | [sizeof=12, dsize=12, align=4,
-  // CHECK-NEXT:       |  nvsize=12, nvalign=4]
+  // OLD-NEXT:         | [sizeof=12, dsize=12, align=4,
+  // OLD-NEXT:         |  nvsize=12, nvalign=4]
+  // NEW-NEXT:         | [sizeof=12, dsize=9, align=4,
+  // NEW-NEXT:         |  nvsize=9, nvalign=4]
 }
 
 namespace NonPOD {

MitalAshok · 2024-04-29T12:39:15Z

Some choices I've made:

Removed TargetCXXABI::TailPaddingUseRules and TargetCXXABI::getTailPaddingUseRules(). Now there are 5 distinct tail padding rules (7 if you count tail padding rules that change with -fclang-abi-compat=18.0), and it was only used in the mustSkipTailPadding function anyways.
Ignored a comment that the Clang WebAssembley ABI should ignore bit-field sizes. I don't think that was actually true, but it didn't matter before because we didn't check bit-field sizes.
AppleARM64/WatchOS/iOS don't skip tail padding for types with potentially-overlapping members as well as the oversized bit-fields they were documented to ignore.
XL and Fuchsia are also unchanged, for no particular reason other than I could not find what the ABI for C++ layout was supposed to be

clang/lib/AST/RecordLayoutBuilder.cpp

rjmccall · 2024-05-22T16:28:10Z

clang/lib/AST/RecordLayoutBuilder.cpp

+      return true;
+    }
+    llvm_unreachable("bad ABI kind");
+  }


I think this would be a lot cleaner overall with a single switch over the C++ ABIs, even if it makes the individual conditions slightly more complex.

rjmccall

This LGTM. Please reach out to the Fuchsia and XL folks if you can, but we don't need to hold up the PR over it.

shafik · 2024-06-20T22:37:47Z

Looks like this is ready to land, if you have reached out to the Fuchsia and XL folks you should be good.

cor3ntin · 2024-07-17T12:05:26Z

@MitalAshok ping

MitalAshok · 2024-07-17T20:46:47Z

I have reached out to Fuschia on their discord and am still waiting for a response.
I was unable to find a place to ask someone related to IBM XL what is supposed to happen (and I couldn't get the xl++ compiler to check what it does). Would be grateful if someone more knowledgeable about this could help.

This patch currently changes nothing for Fuschia and XL. It could be merged as is and the Fuschia/XL behaviour could be changed for -fclang-abi-compat=20 if they need to be changed. Or this could be pushed back entirely for Clang 20

cor3ntin · 2024-08-07T15:40:01Z

@hubert-reinterpretcast

hubert-reinterpretcast · 2024-08-08T04:07:22Z

Can someone point me to the testing for this patch where the layout of a derived class is changed for an oversized bitfield in a base class?

MitalAshok · 2024-08-08T10:45:13Z

@hubert-reinterpretcast There were none, now I've added some: https://github.com/llvm/llvm-project/blob/cf8be1bac0eb37caaaecd47cb463ca58ee0fbe59/clang/test/Layout/itanium-padded-bit-field.cpp

…ping This agrees with existing GCC behaviour: https://godbolt.org/z/Grr8r4sYr

hubert-reinterpretcast · 2024-08-08T21:37:34Z

@hubert-reinterpretcast There were none, now I've added some: https://github.com/llvm/llvm-project/blob/cf8be1bac0eb37caaaecd47cb463ca58ee0fbe59/clang/test/Layout/itanium-padded-bit-field.cpp

Thanks. I am a bit concerned that the fix to match the Itanium ABI on the oversized-bitfield cases actually introduces a compatibility issue with GCC.

See https://godbolt.org/z/c85f8s9Ex (extracted from a test in this patch). GCC passes with -DOLD_ABI=true (and not with -DOLD_ABI=false.

[Clang] Reuse tail-padding for more types that are not POD for the pu…

96ff21d

…rpose of layout This will be done for types with over-large bitfields and potentially-overlapping ([[no_unique_address]]) members Compatible with old Clang 18 semantics with -fclang-abi-compat Fixes llvm#50766

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

cor3ntin requested review from rjmccall and erichkeane April 29, 2024 20:21

Merge branch 'main' into tail-padding

f2ca5f9

rjmccall reviewed May 22, 2024

View reviewed changes

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

rjmccall reviewed May 22, 2024

View reviewed changes

Reorganise into a single switch + comments

6c040b4

rjmccall approved these changes May 23, 2024

View reviewed changes

MitalAshok added 3 commits August 8, 2024 09:50

Merge branch 'main' into tail-padding

8dd85ac

Clang 19 -> Clang 20

b98b2e6

Consider "_BitInt(5) : 6" to be oversized; add tests for bit-fields

cf8be1b

MitalAshok added 2 commits August 8, 2024 11:55

[NFC] minor fixup

af1ff56

[[no_unique_address]] on non-class field is still potentially overlap…

48e0287

…ping This agrees with existing GCC behaviour: https://godbolt.org/z/Grr8r4sYr

MitalAshok force-pushed the tail-padding branch from 5fd11ed to 48e0287 Compare August 8, 2024 11:19

Michael137 mentioned this pull request Apr 8, 2025

[cxx-interop] Do not codegen zero-sized fields swiftlang/swift#79076

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Clang] Reuse tail-padding for more types that are not POD for the purpose of layout #90462

[Clang] Reuse tail-padding for more types that are not POD for the purpose of layout #90462

Uh oh!

MitalAshok commented Apr 29, 2024 •

edited

Loading

Uh oh!

llvmbot commented Apr 29, 2024

Uh oh!

MitalAshok commented Apr 29, 2024

Uh oh!

Uh oh!

rjmccall May 22, 2024

Uh oh!

rjmccall left a comment

Uh oh!

shafik commented Jun 20, 2024

Uh oh!

cor3ntin commented Jul 17, 2024

Uh oh!

MitalAshok commented Jul 17, 2024

Uh oh!

cor3ntin commented Aug 7, 2024

Uh oh!

hubert-reinterpretcast commented Aug 8, 2024

Uh oh!

MitalAshok commented Aug 8, 2024

Uh oh!

hubert-reinterpretcast commented Aug 8, 2024

Uh oh!

Uh oh!

[Clang] Reuse tail-padding for more types that are not POD for the purpose of layout #90462

Are you sure you want to change the base?

[Clang] Reuse tail-padding for more types that are not POD for the purpose of layout #90462

Uh oh!

Conversation

MitalAshok commented Apr 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 29, 2024

Uh oh!

MitalAshok commented Apr 29, 2024

Uh oh!

Uh oh!

rjmccall May 22, 2024

Choose a reason for hiding this comment

Uh oh!

rjmccall left a comment

Choose a reason for hiding this comment

Uh oh!

shafik commented Jun 20, 2024

Uh oh!

cor3ntin commented Jul 17, 2024

Uh oh!

MitalAshok commented Jul 17, 2024

Uh oh!

cor3ntin commented Aug 7, 2024

Uh oh!

hubert-reinterpretcast commented Aug 8, 2024

Uh oh!

MitalAshok commented Aug 8, 2024

Uh oh!

hubert-reinterpretcast commented Aug 8, 2024

Uh oh!

Uh oh!

MitalAshok commented Apr 29, 2024 •

edited

Loading