- Notifications
You must be signed in to change notification settings - Fork14.5k
Allow packing fields into tail padding of record fields#122197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Don't use isPotentiallyOverlapping to control behavior that allowsoverwriting prevoius field data, because the `[[no_unique_address]]`attribute is not available in debug info, DWARF or PDB. Instead, justtrust the layout given by the frontend and use the smaller basesubobject LLVM struct type when the layout requires it.This extra complexity mainly exists to produce prettier LLVM structtypes, which are very vestigial at this point. The main value of usingthe complete struct type is that it avoids disturbing all of the Clangtests cases that pattern match the existing LLVM struct layout patterns.
llvmbot commentedJan 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Reid Kleckner (rnk) ChangesDon't use isPotentiallyOverlapping to control behavior that allows overwriting previous field data, because the This extra complexity mainly exists to produce prettier LLVM struct types, which are very vestigial at this point. The main value of using the complete struct type is that it avoids disturbing all of the Clang tests cases that pattern match the existing LLVM struct layout patterns. Full diff:https://github.com/llvm/llvm-project/pull/122197.diff 4 Files Affected:
diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cppindex 655fc3dc954c81..00f21e393a27f3 100644--- a/clang/lib/CodeGen/CGExprConstant.cpp+++ b/clang/lib/CodeGen/CGExprConstant.cpp@@ -733,6 +733,7 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) { for (FieldDecl *Field : RD->fields()) { ++FieldNo;+ QualType FieldTy = Field->getType(); // If this is a union, skip all the fields that aren't being initialized. if (RD->isUnion() &&@@ -773,23 +774,23 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) { // represents additional overwriting of our current constant value, and not // a new constant to emit independently. if (AllowOverwrite &&- (Field->getType()->isArrayType() || Field->getType()->isRecordType())) {+ (FieldTy->isArrayType() || FieldTy->isRecordType())) { if (auto *SubILE = dyn_cast<InitListExpr>(Init)) { CharUnits Offset = CGM.getContext().toCharUnitsFromBits( Layout.getFieldOffset(FieldNo)); if (!EmitDesignatedInitUpdater(Emitter, Builder, StartOffset + Offset,- Field->getType(), SubILE))+ FieldTy, SubILE)) return false; // If we split apart the field's value, try to collapse it down to a // single value now. Builder.condense(StartOffset + Offset,- CGM.getTypes().ConvertTypeForMem(Field->getType()));+ CGM.getTypes().ConvertTypeForMem(FieldTy)); continue; } } llvm::Constant *EltInit =- Init ? Emitter.tryEmitPrivateForMemory(Init, Field->getType())+ Init ? Emitter.tryEmitPrivateForMemory(Init, FieldTy) : Emitter.emitNullForMemory(Field->getType()); if (!EltInit) return false;@@ -803,10 +804,16 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) { if (!AppendField(Field, Layout.getFieldOffset(FieldNo), EltInit, AllowOverwrite)) return false;- // After emitting a non-empty field with [[no_unique_address]], we may- // need to overwrite its tail padding.- if (Field->hasAttr<NoUniqueAddressAttr>())- AllowOverwrite = true;++ // Allow overwrites after a field with tail padding. This allows+ // overwriting tail padding of fields carrying [[no_unique_address]]+ // without checking for it, since it is not necessarily present in debug+ // info.+ if (const CXXRecordDecl *FieldRD = FieldTy->getAsCXXRecordDecl()) {+ const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(FieldRD);+ if (Layout.getDataSize() < Layout.getSize())+ AllowOverwrite = true;+ } } else { // Otherwise we have a bitfield. if (!AppendBitField(Field, Layout.getFieldOffset(FieldNo), EltInit,diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cppindex ea44e6f21f3c86..9b36ff4e0beadb 100644--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp@@ -198,7 +198,7 @@ struct CGRecordLowering { const CXXRecordDecl *Query) const; void calculateZeroInit(); CharUnits calculateTailClippingOffset(bool isNonVirtualBaseType) const;- void checkBitfieldClipping(bool isNonVirtualBaseType) const;+ void checkForOverlap(bool isNonVirtualBaseType); /// Determines if we need a packed llvm struct. void determinePacked(bool NVBaseType); /// Inserts padding everywhere it's needed.@@ -300,7 +300,7 @@ void CGRecordLowering::lower(bool NVBaseType) { accumulateVBases(); } llvm::stable_sort(Members);- checkBitfieldClipping(NVBaseType);+ checkForOverlap(NVBaseType); Members.push_back(StorageInfo(Size, getIntNType(8))); determinePacked(NVBaseType); insertPadding();@@ -389,14 +389,28 @@ void CGRecordLowering::accumulateFields(bool isNonVirtualBaseType) { // Empty fields have no storage. ++Field; } else {- // Use base subobject layout for the potentially-overlapping field,- // as it is done in RecordLayoutBuilder- Members.push_back(MemberInfo(- bitsToCharUnits(getFieldBitOffset(*Field)), MemberInfo::Field,- Field->isPotentiallyOverlapping()- ? getStorageType(Field->getType()->getAsCXXRecordDecl())- : getStorageType(*Field),- *Field));+ CharUnits CurOffset = bitsToCharUnits(getFieldBitOffset(*Field));+ llvm::Type *StorageType = getStorageType(*Field);++ // Detect cases when the next field needs to be packed into tail padding+ // of a record field. This is typically caused by [[no_unique_address]],+ // but we try to infer when that is the case rather than checking for the+ // attribute explicitly because the attribute is typically not present in+ // debug info. Use the base subobject LLVM struct type in these cases,+ // which will be less than data size bytes.+ if (const CXXRecordDecl *FieldRD =+ Field->getType()->getAsCXXRecordDecl()) {+ CharUnits NextOffset = Layout.getNonVirtualSize();+ auto NextField = std::next(Field);+ if (NextField != FieldEnd)+ NextOffset = bitsToCharUnits(getFieldBitOffset(*NextField));+ if (CurOffset + getSize(StorageType) > NextOffset) {+ StorageType = getStorageType(FieldRD);+ }+ }++ Members.push_back(+ MemberInfo(CurOffset, MemberInfo::Field, StorageType, *Field)); ++Field; } }@@ -948,19 +962,19 @@ void CGRecordLowering::calculateZeroInit() { } // Verify accumulateBitfields computed the correct storage representations.-void CGRecordLowering::checkBitfieldClipping(bool IsNonVirtualBaseType) const {+void CGRecordLowering::checkForOverlap(bool IsNonVirtualBaseType) { #ifndef NDEBUG auto ScissorOffset = calculateTailClippingOffset(IsNonVirtualBaseType); auto Tail = CharUnits::Zero();- for (const auto &M : Members) {+ for (MemberInfo &M : Members) { // Only members with data could possibly overlap. if (!M.Data) continue;- assert(M.Offset >= Tail && "Bitfield access unit is not clipped");+ assert(M.Offset >= Tail && "LLVM struct member types overlap"); Tail = M.Offset + getSize(M.Data); assert((Tail <= ScissorOffset || M.Offset >= ScissorOffset) &&- "Bitfield straddles scissor offset");+ "Member straddles scissor offset"); } #endif }diff --git a/clang/test/CodeGenCXX/no-unique-address-3.cpp b/clang/test/CodeGenCXX/no-unique-address-3.cppindex c1e84f1d312768..17480030ef7ba2 100644--- a/clang/test/CodeGenCXX/no-unique-address-3.cpp+++ b/clang/test/CodeGenCXX/no-unique-address-3.cpp@@ -65,8 +65,8 @@ Bar J; // CHECK-NEXT: | [sizeof=8, dsize=8, align=4, // CHECK-NEXT: | nvsize=8, nvalign=4]-// CHECK-LABEL: LLVMType:%class.IntFieldClass = type { [2 x i8], %class.Second.base, i32 }-// CHECK-NEXT: NonVirtualBaseLLVMType:%class.IntFieldClass = type { [2 x i8], %class.Second.base, i32 }+// CHECK-LABEL: LLVMType:%class.IntFieldClass = type { [2 x i8], %class.Second, i32 }+// CHECK-NEXT: NonVirtualBaseLLVMType:%class.IntFieldClass = type { [2 x i8], %class.Second, i32 } class IntFieldClass : Empty { [[no_unique_address]] Second Field; int C;diff --git a/clang/test/CodeGenCXX/override-layout.cpp b/clang/test/CodeGenCXX/override-layout.cppindex 07cd31580726e3..de5c18966688f0 100644--- a/clang/test/CodeGenCXX/override-layout.cpp+++ b/clang/test/CodeGenCXX/override-layout.cpp@@ -1,15 +1,19 @@ // RUN: %clang_cc1 %std_cxx98-14 -w -fdump-record-layouts-simple %s > %t.layouts // RUN: %clang_cc1 %std_cxx98-14 -w -fdump-record-layouts-simple %s > %t.before-// RUN: %clang_cc1 %std_cxx98-14 -w -DPACKED= -DALIGNED16= -fdump-record-layouts-simple -foverride-record-layout=%t.layouts %s > %t.after+// RUN: %clang_cc1 %std_cxx98-14 -w -DPACKED= -DALIGNED16= -DNO_UNIQUE_ADDRESS= -fdump-record-layouts-simple -foverride-record-layout=%t.layouts %s > %t.after // RUN: diff -u %t.before %t.after // RUN: FileCheck --check-prefixes=CHECK,PRE17 %s < %t.after // RUN: %clang_cc1 -std=c++17 -w -fdump-record-layouts-simple %s > %t.layouts // RUN: %clang_cc1 -std=c++17 -w -fdump-record-layouts-simple %s > %t.before-// RUN: %clang_cc1 -std=c++17 -w -DPACKED= -DALIGNED16= -fdump-record-layouts-simple -foverride-record-layout=%t.layouts %s > %t.after+// RUN: %clang_cc1 -std=c++17 -w -DPACKED= -DALIGNED16= -DNO_UNIQUE_ADDRESS= -fdump-record-layouts-simple -foverride-record-layout=%t.layouts %s > %t.after // RUN: diff -u %t.before %t.after // RUN: FileCheck --check-prefixes=CHECK,CXX17 %s < %t.after+// Generate IR with the externally defined layout to exercise IR generation the+// way LLDB does.+// RUN: %clang_cc1 -std=c++17 -w -DPACKED= -DALIGNED16= -DNO_UNIQUE_ADDRESS= -foverride-record-layout=%t.layouts %s -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-IR+ // CXX17: Type: struct X6 // If not explicitly disabled, set PACKED to the packed attribute.@@ -102,3 +106,43 @@ void use_structs() { x4s[1].a = 1; x5s[1].a = 17; }++// Test various uses of no_unique_address.+#ifndef NO_UNIQUE_ADDRESS+#define NO_UNIQUE_ADDRESS [[no_unique_address]]+#endif++struct HasTailPadding {+ HasTailPadding() = default;+ constexpr HasTailPadding(int x, short y) : x(x), y(y) {}+private:+ int x;+ short y;+};+static_assert(sizeof(HasTailPadding) == 8);+// CHECK: Type: struct HasTailPadding++struct NoTailPacking {+ HasTailPadding xy;+ char z;+};+static_assert(sizeof(NoTailPacking) == 12);+// CHECK: Type: struct NoTailPacking++struct NUAUsesTailPadding {+ NO_UNIQUE_ADDRESS HasTailPadding xy;+ NO_UNIQUE_ADDRESS char z;+};++// CHECK: Type: struct NUAUsesTailPadding+static_assert(sizeof(NUAUsesTailPadding) == 8);++NUAUsesTailPadding nua_gv_zeroinit;+NUAUsesTailPadding nua_gv_braces{};+NUAUsesTailPadding nua_gv_123{{1, 2}, 3};+// CHECK-IR: %struct.NUAUsesTailPadding = type { %struct.HasTailPadding.base, i8, i8 }+// CHECK-IR: %struct.HasTailPadding.base = type <{ i32, i16 }>++// CHECK-IR: @nua_gv_zeroinit = global %struct.NUAUsesTailPadding zeroinitializer, align 4+// CHECK-IR: @nua_gv_braces = global { [6 x i8], i8, [1 x i8] } zeroinitializer, align 4+// CHECK-IR: @nua_gv_123 = global { i32, i16, i8 } { i32 1, i16 2, i8 3 }, align 4 |
You can test this locally with the following command:git-clang-format --diff e93181bf13b289823810d3b43bcc3c2df1eda70b 54eaf769c085d8efeab957a5f385bca59a3f1f32 --extensions cpp -- clang/lib/CodeGen/CGExprConstant.cpp clang/lib/CodeGen/CGRecordLayoutBuilder.cpp clang/test/CodeGenCXX/no-unique-address-3.cpp clang/test/CodeGenCXX/override-layout.cpp View the diff from clang-format here.diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cppindex 00f21e393a..060385ca92 100644--- a/clang/lib/CodeGen/CGExprConstant.cpp+++ b/clang/lib/CodeGen/CGExprConstant.cpp@@ -773,8 +773,7 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) { // When emitting a DesignatedInitUpdateExpr, a nested InitListExpr // represents additional overwriting of our current constant value, and not // a new constant to emit independently.- if (AllowOverwrite &&- (FieldTy->isArrayType() || FieldTy->isRecordType())) {+ if (AllowOverwrite && (FieldTy->isArrayType() || FieldTy->isRecordType())) { if (auto *SubILE = dyn_cast<InitListExpr>(Init)) { CharUnits Offset = CGM.getContext().toCharUnitsFromBits( Layout.getFieldOffset(FieldNo));@@ -810,7 +809,8 @@ bool ConstStructBuilder::Build(const InitListExpr *ILE, bool AllowOverwrite) { // without checking for it, since it is not necessarily present in debug // info. if (const CXXRecordDecl *FieldRD = FieldTy->getAsCXXRecordDecl()) {- const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(FieldRD);+ const ASTRecordLayout &Layout =+ CGM.getContext().getASTRecordLayout(FieldRD); if (Layout.getDataSize() < Layout.getSize()) AllowOverwrite = true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thanks! Looks plausible to me, would be great to get@Michael137's take on it, since he's been looking at this sort of thing a fair bit lately.
| // without checking for it, since it is not necessarily present in debug | ||
| // info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd probably describe this more generally - something about external AST providers "if an external AST provider (such as lldb) says this is what the layout they want"?
Oh, I see, you aren't actually checking for an external AST provider here - but for the layout that implies no_unique_address or similar.
I guess then, maybe "even in the absence of [[no_unique_address]], such as when using an external AST provider like lldb"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BTW, I attempted making this logic conditional on whether an external AST provider is present, but consider that PCH and PCM files are implemented as external AST sources, so that approach disturbed record layouts of too many unrelated tests.
There is no easy way to query whether a record has an externally dictated layout, although we could probably pipe that through as an ASTRecordLayout field if we wanted to. I decided that the best approach is to have the fewest possible conditions, and to only look at the layout data, which is the same between compilation and debugging.
| if (const CXXRecordDecl *FieldRD = | ||
| Field->getType()->getAsCXXRecordDecl()) { | ||
| CharUnits NextOffset = Layout.getNonVirtualSize(); | ||
| auto NextField = std::next(Field); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think NextField needs to be the next non-empty field, specifically, not just any field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is annoying. I think the whole purpose of building theMembers vector is to sort the subobjects by offset to avoid dealing with special cases where field offsets are non-ascending, so perhaps I should be computing the LLVM storage types in a second pass altogether.
| // of a record field. This is typically caused by [[no_unique_address]], | ||
| // but we try to infer when that is the case rather than checking for the | ||
| // attribute explicitly because the attribute is typically not present in | ||
| // debug info. Use the base subobject LLVM struct type in these cases, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe state more explicitly that this is an issue when the class layout is computed from debug info?
| if (const CXXRecordDecl *FieldRD = FieldTy->getAsCXXRecordDecl()) { | ||
| const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(FieldRD); | ||
| if (Layout.getDataSize() < Layout.getSize()) | ||
| AllowOverwrite = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
At first glance, I'm not sure what this is doing... if you have a non-empty field, only the tail padding can be overwritten... but the CGRecordLayoutBuilder change should ensure the tail padding doesn't actually count as part of the field in that case? I'm probably missing something obvious.
Do we need to adjust the otherhasAttr<NoUniqueAddressAttr>() in CGExprConstant.cpp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I hadn't caught the other one! I only happened to find this one through testing, not inspection. I'll have to figure out how to exercise the other codepath.
I don't feel qualified to review this, but thank you for working on it. |
I ran into this again. I don't know what it would take to fix this, but I filed an bug (#148838) with steps on how to crash lldb with this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I had some pending draft comments from back when this was active.
My recollection is that this is actually quite difficult and I am not likely to have time in the near future to work on it.
| // without checking for it, since it is not necessarily present in debug | ||
| // info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
BTW, I attempted making this logic conditional on whether an external AST provider is present, but consider that PCH and PCM files are implemented as external AST sources, so that approach disturbed record layouts of too many unrelated tests.
There is no easy way to query whether a record has an externally dictated layout, although we could probably pipe that through as an ASTRecordLayout field if we wanted to. I decided that the best approach is to have the fewest possible conditions, and to only look at the layout data, which is the same between compilation and debugging.
| if (const CXXRecordDecl *FieldRD = FieldTy->getAsCXXRecordDecl()) { | ||
| const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(FieldRD); | ||
| if (Layout.getDataSize() < Layout.getSize()) | ||
| AllowOverwrite = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I hadn't caught the other one! I only happened to find this one through testing, not inspection. I'll have to figure out how to exercise the other codepath.
| if (const CXXRecordDecl *FieldRD = | ||
| Field->getType()->getAsCXXRecordDecl()) { | ||
| CharUnits NextOffset = Layout.getNonVirtualSize(); | ||
| auto NextField = std::next(Field); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That is annoying. I think the whole purpose of building theMembers vector is to sort the subobjects by offset to avoid dealing with special cases where field offsets are non-ascending, so perhaps I should be computing the LLVM storage types in a second pass altogether.
Don't use isPotentiallyOverlapping to control behavior that allows overwriting previous field data, because the
[[no_unique_address]]attribute is not available in any debug info, DWARF or PDB. Instead, just trust the layout given by the frontend and use the smaller base subobject LLVM struct type when the layout requires it.This extra complexity mainly exists to produce prettier LLVM struct types, which are very vestigial at this point. The main value of using the complete struct type is that it avoids disturbing all of the Clang tests cases that pattern match the existing LLVM struct layout patterns.