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

clang/docs/ReleaseNotes.rst

@@ -52,6 +52,9 @@ C++ Specific Potentially Breaking Changes
 ABI Changes in This Version
 ---------------------------
 
+- Fixed tail padding not being reused on types with oversized bit-fields and
+  potentially-overlapping members (#GH50766).
+
 AST Dumping Potentially Breaking Changes
 ----------------------------------------
 

clang/include/clang/AST/Decl.h

@@ -3177,9 +3177,10 @@ class FieldDecl : public DeclaratorDecl, public Mergeable<FieldDecl> {
   /// [[no_unique_address]] attribute.
   bool isZeroSize(const ASTContext &Ctx) const;
 
-  /// Determine if this field is of potentially-overlapping class type, that
+  /// Determine if this field is of potentially-overlapping type, that
   /// is, subobject with the [[no_unique_address]] attribute
-  bool isPotentiallyOverlapping() const;
+  /// If ClassOnly is true, the field also has to be of class type.
+  bool isPotentiallyOverlapping(bool ClassOnly = true) const;
 
   /// Get the kind of (C++11) default member initializer that this field has.
   InClassInitStyle getInClassInitStyle() const {

clang/include/clang/Basic/LangOptions.h

@@ -236,6 +236,12 @@ class LangOptionsBase {
     /// in the initializers of members of local classes.
     Ver18,
 
+    /// Attempt to be ABI-compatible with code generated by Clang 19.0.x.
+    /// This causes clang to not reuse tail padding on structs with
+    /// potentially-overlapping members or oversized bit-fields under Itanium
+    /// and some derived ABIs.
+    Ver19,
+
     /// Conform to the underlying platform's C and C++ ABIs as closely
     /// as we can.
     Latest

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();
   }

clang/lib/AST/Decl.cpp

@@ -4623,8 +4623,9 @@ bool FieldDecl::isZeroSize(const ASTContext &Ctx) const {
   });
 }
 
-bool FieldDecl::isPotentiallyOverlapping() const {
-  return hasAttr<NoUniqueAddressAttr>() && getType()->getAsCXXRecordDecl();
+bool FieldDecl::isPotentiallyOverlapping(bool ClassOnly) const {
+  return hasAttr<NoUniqueAddressAttr>() &&
+         (!ClassOnly || getType()->getAsCXXRecordDecl());
 }
 
 unsigned FieldDecl::getFieldIndex() const {

clang/lib/AST/RecordLayoutBuilder.cpp

@@ -2415,46 +2415,120 @@ 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()) {
+      QualType DeclaredType = FD->getType();
+      unsigned DeclaredWidth;
+      if (const auto *BIT = DeclaredType->getAs<BitIntType>())
+        DeclaredWidth = BIT->getNumBits();
+      else
+        DeclaredWidth = Context.getTypeSize(DeclaredType);
+
+      if (FD->getBitWidthValue(Context) > DeclaredWidth)
+        return true;
+    }
+
+    return FD->isPotentiallyOverlapping(/*ClassOnly=*/false);
+  };
+
+  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;
+///
+/// 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.
+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
+  // consistent semantics to the type-traits intrinsics (or at
+  // least as many of them as possible).
+  auto IsCXX11PODType = [&]() -> bool {
+    return RD->isTrivial() && RD->isCXX11StandardLayout();
+  };
 
-  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.
+  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:
+    if (!RD->isPOD())
+      return false;
+    // Prior to Clang 20, over-large bitfields and potentially overlapping
+    // members were not checked
+    return (Context.getLangOpts().getClangABICompat() <=
+            LangOptions::ClangABI::Ver19) ||
+           isItaniumPOD(Context, RD);
+
+  // The same as generic Itanium but does not honor the exception about classes
+  // with over-large bit-fields.
+  // FIXME: do the iOS/AppleARM64/WatchOS ABI care about potentially-overlapping
+  // members?
+  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:
+    if (!IsCXX11PODType())
+      return false;
+    return (Context.getLangOpts().getClangABICompat() <=
+            LangOptions::ClangABI::Ver19) ||
+           isItaniumPOD(Context, RD);
+
+  // Also use C++11 POD but without honoring the exception about classes with
+  // over-large bit-fields.
+  case TargetCXXABI::AppleARM64:
+  case TargetCXXABI::WatchOS:
+    return IsCXX11PODType();
+
+  // FIXME: do these two ABIs need to check isItaniumPOD?
+  case TargetCXXABI::XL:
+    return RD->isPOD();
+  case TargetCXXABI::Fuchsia:
+    return IsCXX11PODType();
 
-  llvm_unreachable("bad tail-padding use kind");
+  // 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 ABI kind");
 }
 
 static bool isMsLayout(const ASTContext &Context) {
@@ -3388,7 +3462,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 =

clang/lib/Frontend/CompilerInvocation.cpp

@@ -3796,6 +3796,9 @@ void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts,
   case LangOptions::ClangABI::Ver18:
     GenerateArg(Consumer, OPT_fclang_abi_compat_EQ, "18.0");
     break;
+  case LangOptions::ClangABI::Ver19:
+    GenerateArg(Consumer, OPT_fclang_abi_compat_EQ, "19.0");
+    break;
   case LangOptions::ClangABI::Latest:
     break;
   }
@@ -4305,6 +4308,8 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,
         Opts.setClangABICompat(LangOptions::ClangABI::Ver17);
       else if (Major <= 18)
         Opts.setClangABICompat(LangOptions::ClangABI::Ver18);
+      else if (Major <= 19)
+        Opts.setClangABICompat(LangOptions::ClangABI::Ver19);
     } else if (Ver != "latest") {
       Diags.Report(diag::err_drv_invalid_value)
           << A->getAsString(Args) << A->getValue();

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=19.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=19.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=19.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=19.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=19.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;

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++20 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,NEW --allow-unused-prefixes
+// RUN: %clang_cc1 -std=c++20 %s -emit-llvm -o - -triple x86_64-linux-gnu -O2 -disable-llvm-passes | FileCheck %s --check-prefixes=CHECK-OPT,NEW-OPT --allow-unused-prefixes
+// RUN: %clang_cc1 -fclang-abi-compat=19.0 -std=c++20 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=CHECK,OLD --allow-unused-prefixes
+// RUN: %clang_cc1 -fclang-abi-compat=19.0 -std=c++20 %s -emit-llvm -o - -triple x86_64-linux-gnu -O2 -disable-llvm-passes | FileCheck %s --check-prefixes=CHECK-OPT,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 {