Allow packing fields into tail padding of record fields #122197
+91
−26
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Don't use isPotentiallyOverlapping to control behavior that allows overwriting prevoius field data, because the `[[no_unique_address]]` attribute is not available in 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.
llvmbot
added
clang
|
@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.cpp
index 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.cpp
index 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.cpp
index 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.cpp
index 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.cppView the diff from clang-format here.diff --git a/clang/lib/CodeGen/CGExprConstant.cpp b/clang/lib/CodeGen/CGExprConstant.cpp
index 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;
}
|
dwblaikie
reviewed
Jan 9, 2025
There was a problem hiding this comment.
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.
Comment on lines
+810
to
+811
| // without checking for it, since it is not necessarily present in debug | ||
| // info. |
There was a problem hiding this comment.
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"?
| if (const CXXRecordDecl *FieldRD = | ||
| Field->getType()->getAsCXXRecordDecl()) { | ||
| CharUnits NextOffset = Layout.getNonVirtualSize(); | ||
| auto NextField = std::next(Field); |
There was a problem hiding this comment.
I think NextField needs to be the next non-empty field, specifically, not just any field.
| // 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.
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.
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 other hasAttr<NoUniqueAddressAttr>() in CGExprConstant.cpp?
|
I don't feel qualified to review this, but thank you for working on it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.