search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
25.95k stars 10.58k forks source link

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

Open MitalAshok opened 2 weeks ago

MitalAshok commented 2 weeks ago

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

llvmbot commented 2 weeks ago

@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 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 commented 2 weeks ago

Some choices I've made: