Skip to content

Commit

Permalink
[clang] Fix bitfield access unit for vbase corner case (#87238)
Browse files Browse the repository at this point in the history 
This fixes #87227, a vbase can be placed below nvsize when empty members and/or bases are in play. We must account for that.
  • Loading branch information
@urnathan
urnathan committed Apr 1, 2024
1 parent 4cd7bb0 commit ee99475
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 48 deletions.
57 changes: 40 additions & 17 deletions clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,18 +185,21 @@ struct CGRecordLowering {
/// Lowers an ASTRecordLayout to a llvm type.
void lower(bool NonVirtualBaseType);
void lowerUnion(bool isNoUniqueAddress);
void accumulateFields();
void accumulateFields(bool isNonVirtualBaseType);
RecordDecl::field_iterator
accumulateBitFields(RecordDecl::field_iterator Field,
accumulateBitFields(bool isNonVirtualBaseType,
RecordDecl::field_iterator Field,
RecordDecl::field_iterator FieldEnd);
void computeVolatileBitfields();
void accumulateBases();
void accumulateVPtrs();
void accumulateVBases();
/// Recursively searches all of the bases to find out if a vbase is
/// not the primary vbase of some base class.
bool hasOwnStorage(const CXXRecordDecl *Decl, const CXXRecordDecl *Query);
bool hasOwnStorage(const CXXRecordDecl *Decl,
const CXXRecordDecl *Query) const;
void calculateZeroInit();
CharUnits calculateTailClippingOffset(bool isNonVirtualBaseType) const;
/// Lowers bitfield storage types to I8 arrays for bitfields with tail
/// padding that is or can potentially be used.
void clipTailPadding();
Expand Down Expand Up @@ -287,7 +290,7 @@ void CGRecordLowering::lower(bool NVBaseType) {
computeVolatileBitfields();
return;
}
accumulateFields();
accumulateFields(NVBaseType);
// RD implies C++.
if (RD) {
accumulateVPtrs();
Expand Down Expand Up @@ -378,12 +381,12 @@ void CGRecordLowering::lowerUnion(bool isNoUniqueAddress) {
Packed = true;
}

void CGRecordLowering::accumulateFields() {
void CGRecordLowering::accumulateFields(bool isNonVirtualBaseType) {
for (RecordDecl::field_iterator Field = D->field_begin(),
FieldEnd = D->field_end();
Field != FieldEnd;) {
if (Field->isBitField()) {
Field = accumulateBitFields(Field, FieldEnd);
Field = accumulateBitFields(isNonVirtualBaseType, Field, FieldEnd);
assert((Field == FieldEnd || !Field->isBitField()) &&
"Failed to accumulate all the bitfields");
} else if (Field->isZeroSize(Context)) {
Expand All @@ -404,9 +407,12 @@ void CGRecordLowering::accumulateFields() {
}

// Create members for bitfields. Field is a bitfield, and FieldEnd is the end
// iterator of the record. Return the first non-bitfield encountered.
// iterator of the record. Return the first non-bitfield encountered. We need
// to know whether this is the base or complete layout, as virtual bases could
// affect the upper bound of bitfield access unit allocation.
RecordDecl::field_iterator
CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field,
CGRecordLowering::accumulateBitFields(bool isNonVirtualBaseType,
RecordDecl::field_iterator Field,
RecordDecl::field_iterator FieldEnd) {
if (isDiscreteBitFieldABI()) {
// Run stores the first element of the current run of bitfields. FieldEnd is
Expand Down Expand Up @@ -505,6 +511,10 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field,
bitsToCharUnits(Context.getTargetInfo().getRegisterWidth());
unsigned CharBits = Context.getCharWidth();

// Limit of useable tail padding at end of the record. Computed lazily and
// cached here.
CharUnits ScissorOffset = CharUnits::Zero();

// Data about the start of the span we're accumulating to create an access
// unit from. Begin is the first bitfield of the span. If Begin is FieldEnd,
// we've not got a current span. The span starts at the BeginOffset character
Expand Down Expand Up @@ -630,10 +640,14 @@ CGRecordLowering::accumulateBitFields(RecordDecl::field_iterator Field,
LimitOffset = bitsToCharUnits(getFieldBitOffset(*Probe));
goto FoundLimit;
}
// We reached the end of the fields. We can't necessarily use tail
// padding in C++ structs, so the NonVirtual size is what we must
// use there.
LimitOffset = RD ? Layout.getNonVirtualSize() : Layout.getDataSize();
// We reached the end of the fields, determine the bounds of useable
// tail padding. As this can be complex for C++, we cache the result.
if (ScissorOffset.isZero()) {
ScissorOffset = calculateTailClippingOffset(isNonVirtualBaseType);
assert(!ScissorOffset.isZero() && "Tail clipping at zero");
}

LimitOffset = ScissorOffset;
FoundLimit:;

CharUnits TypeSize = getSize(Type);
Expand Down Expand Up @@ -838,13 +852,17 @@ void CGRecordLowering::accumulateVPtrs() {
llvm::PointerType::getUnqual(Types.getLLVMContext())));
}

void CGRecordLowering::accumulateVBases() {
CharUnits
CGRecordLowering::calculateTailClippingOffset(bool isNonVirtualBaseType) const {
if (!RD)
return Layout.getDataSize();

CharUnits ScissorOffset = Layout.getNonVirtualSize();
// In the itanium ABI, it's possible to place a vbase at a dsize that is
// smaller than the nvsize. Here we check to see if such a base is placed
// before the nvsize and set the scissor offset to that, instead of the
// nvsize.
if (isOverlappingVBaseABI())
if (!isNonVirtualBaseType && isOverlappingVBaseABI())
for (const auto &Base : RD->vbases()) {
const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
if (BaseDecl->isEmpty())
Expand All @@ -856,8 +874,13 @@ void CGRecordLowering::accumulateVBases() {
ScissorOffset = std::min(ScissorOffset,
Layout.getVBaseClassOffset(BaseDecl));
}
Members.push_back(MemberInfo(ScissorOffset, MemberInfo::Scissor, nullptr,
RD));

return ScissorOffset;
}

void CGRecordLowering::accumulateVBases() {
Members.push_back(MemberInfo(calculateTailClippingOffset(false),
MemberInfo::Scissor, nullptr, RD));
for (const auto &Base : RD->vbases()) {
const CXXRecordDecl *BaseDecl = Base.getType()->getAsCXXRecordDecl();
if (BaseDecl->isEmpty())
Expand All @@ -882,7 +905,7 @@ void CGRecordLowering::accumulateVBases() {
}

bool CGRecordLowering::hasOwnStorage(const CXXRecordDecl *Decl,
const CXXRecordDecl *Query) {
const CXXRecordDecl *Query) const {
const ASTRecordLayout &DeclLayout = Context.getASTRecordLayout(Decl);
if (DeclLayout.isPrimaryBaseVirtual() && DeclLayout.getPrimaryBase() == Query)
return false;
Expand Down
104 changes: 73 additions & 31 deletions clang/test/CodeGenCXX/bitfield-access-tail.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,45 +2,45 @@

// Configs that have cheap unaligned access
// Little Endian
// RUN: %clang_cc1 -triple=aarch64-apple-darwin %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=aarch64-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=aarch64-apple-darwin %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=aarch64-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=arm-apple-darwin %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT-DWN32 %s
// RUN: %clang_cc1 -triple=arm-none-eabi %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=i686-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=loongarch64-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=powerpcle-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=ve-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=wasm32 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=wasm64 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=x86_64-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=arm-none-eabi %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=i686-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=loongarch64-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=powerpcle-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=ve-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=wasm32 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=wasm64 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=x86_64-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s

// Big Endian
// RUN: %clang_cc1 -triple=powerpc-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=powerpc64-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=systemz %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=powerpc-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=powerpc64-linux-gnu %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=systemz %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s

// Configs that have expensive unaligned access
// Little Endian
// RUN: %clang_cc1 -triple=amdgcn-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=arc-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=bpf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=csky %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=hexagon-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=le64-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=loongarch32-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=nvptx-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=riscv32 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=riscv64 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=spir-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=xcore-none-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=amdgcn-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=arc-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=bpf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=csky %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=hexagon-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=le64-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=loongarch32-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=nvptx-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=riscv32 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=riscv64 %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=spir-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=xcore-none-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s

// Big endian
// RUN: %clang_cc1 -triple=lanai-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=m68k-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=mips-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=mips64-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=sparc-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=tce-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT %s
// RUN: %clang_cc1 -triple=lanai-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=m68k-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=mips-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=mips64-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT64 %s
// RUN: %clang_cc1 -triple=sparc-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s
// RUN: %clang_cc1 -triple=tce-elf %s -emit-llvm -o /dev/null -fdump-record-layouts-simple | FileCheck --check-prefixes CHECK,LAYOUT,LAYOUT32 %s

// Can use tail padding
struct Pod {
Expand Down Expand Up @@ -113,3 +113,45 @@ struct __attribute__((packed)) PNonPod {
// LAYOUT-DWN32-NEXT: <CGBitFieldInfo Offset:{{[0-9]+}} Size:16 IsSigned:1 StorageSize:16 StorageOffset:0
// LAYOUT-DWN32-NEXT: <CGBitFieldInfo Offset:{{[0-9]+}} Size:8 IsSigned:1 StorageSize:8 StorageOffset:2
// CHECK-NEXT: ]>

struct __attribute__((aligned(4))) Empty {} empty;

struct Char { char a; } cbase;
struct D : virtual Char {
[[no_unique_address]] Empty e0;
[[no_unique_address]] Empty e1;
unsigned a : 24; // keep as 24bits
} d;
// CHECK-LABEL: LLVMType:%struct.D =
// LAYOUT64-SAME: type <{ ptr, [3 x i8], %struct.Char, [4 x i8] }>
// LAYOUT32-SAME: type { ptr, [3 x i8], %struct.Char }
// LAYOUT-DWN32-SAME: type { ptr, [3 x i8], %struct.Char }
// CHECK-NEXT: NonVirtualBaseLLVMType:
// LAYOUT64-SAME: %struct.D.base = type <{ ptr, i32 }>
// LAYOUT32-SAME: %struct.D = type { ptr, [3 x i8], %struct.Char }
// LAYOUT-DWN32-SAME: %struct.D = type { ptr, [3 x i8], %struct.Char }
// CHECK: BitFields:[
// LAYOUT-NEXT: <CGBitFieldInfo Offset:{{[0-9]+}} Size:24 IsSigned:0 StorageSize:24 StorageOffset:{{(4|8)}}

// LAYOUT-DWN32-NEXT: <CGBitFieldInfo Offset:{{[0-9]+}} Size:24 IsSigned:0 StorageSize:24 StorageOffset:{{(4|8)}}
// CHECK-NEXT: ]>

struct Int { int a; } ibase;
struct E : virtual Int {
[[no_unique_address]] Empty e0;
[[no_unique_address]] Empty e1;
unsigned a : 24; // expand to 32
} e;
// CHECK-LABEL: LLVMType:%struct.E =
// LAYOUT64-SAME: type <{ ptr, i32, %struct.Int }>
// LAYOUT32-SAME: type { ptr, i32, %struct.Int }
// LAYOUT-DWN32-SAME: type { ptr, i32, %struct.Int }
// CHECK-NEXT: NonVirtualBaseLLVMType:%struct.E.base =
// LAYOUT64-SAME: type <{ ptr, i32 }>
// LAYOUT32-SAME: type { ptr, i32 }
// LAYOUT-DWN32-SAME: type { ptr, i32 }
// CHECK: BitFields:[
// LAYOUT-NEXT: <CGBitFieldInfo Offset:{{[0-9]+}} Size:24 IsSigned:0 StorageSize:32 StorageOffset:{{(4|8)}}

// LAYOUT-DWN32-NEXT: <CGBitFieldInfo Offset:{{[0-9]+}} Size:24 IsSigned:0 StorageSize:32 StorageOffset:{{(4|8)}}
// CHECK-NEXT: ]>

0 comments on commit ee99475

Please sign in to comment.