[clang] Allow packing fields into tail padding of record fields (PR #122197)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jan 8 16:05:59 PST 2025
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang-codegen
Author: Reid Kleckner (rnk)
<details>
<summary>Changes</summary>
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.
---
Full diff: https://github.com/llvm/llvm-project/pull/122197.diff
4 Files Affected:
- (modified) clang/lib/CodeGen/CGExprConstant.cpp (+15-8)
- (modified) clang/lib/CodeGen/CGRecordLayoutBuilder.cpp (+28-14)
- (modified) clang/test/CodeGenCXX/no-unique-address-3.cpp (+2-2)
- (modified) clang/test/CodeGenCXX/override-layout.cpp (+46-2)
``````````diff
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
``````````
</details>
https://github.com/llvm/llvm-project/pull/122197
More information about the cfe-commits
mailing list