https://github.com/MitalAshok created
https://github.com/llvm/llvm-project/pull/90462
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
>From 96ff21d5126ebb4b9a538b8eef11f8ac9e2194c5 Mon Sep 17 00:00:00 2001
From: Mital Ashok <mi...@mitalashok.co.uk>
Date: Mon, 29 Apr 2024 12:27:04 +0100
Subject: [PATCH] [Clang] Reuse tail-padding for more types that are not POD
for the purpose 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 #50766
---
clang/docs/ReleaseNotes.rst | 3 +
clang/include/clang/Basic/LangOptions.h | 4 +-
clang/include/clang/Basic/TargetCXXABI.h | 51 --------
clang/lib/AST/RecordLayoutBuilder.cpp | 134 +++++++++++++++-----
clang/test/CodeGenCXX/bitfield-layout.cpp | 20 ++-
clang/test/CodeGenCXX/no-unique-address.cpp | 9 +-
clang/test/Layout/no-unique-address.cpp | 20 ++-
7 files changed, 138 insertions(+), 103 deletions(-)
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 {
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
