Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow packing fields into tail padding of record fields #122197

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+91 −26
Open

Allow packing fields into tail padding of record fields #122197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions clang/lib/CodeGen/CGExprConstant.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() &&
Expand Down Expand Up @@ -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;
Expand All @@ -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.
Comment on lines +810 to +811
Copy link
Collaborator

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"?

if (const CXXRecordDecl *FieldRD = FieldTy->getAsCXXRecordDecl()) {
const ASTRecordLayout &Layout = CGM.getContext().getASTRecordLayout(FieldRD);
if (Layout.getDataSize() < Layout.getSize())
AllowOverwrite = true;
Copy link
Collaborator

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 other hasAttr<NoUniqueAddressAttr>() in CGExprConstant.cpp?

}
} else {
// Otherwise we have a bitfield.
if (!AppendBitField(Field, Layout.getFieldOffset(FieldNo), EltInit,
Expand Down
42 changes: 28 additions & 14 deletions clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Copy link
Collaborator

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?

// which will be less than data size bytes.
if (const CXXRecordDecl *FieldRD =
Field->getType()->getAsCXXRecordDecl()) {
CharUnits NextOffset = Layout.getNonVirtualSize();
auto NextField = std::next(Field);
Copy link
Collaborator

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.

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;
}
}
Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 2 additions & 2 deletions clang/test/CodeGenCXX/no-unique-address-3.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
48 changes: 46 additions & 2 deletions clang/test/CodeGenCXX/override-layout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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
Loading