Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,8 @@ Modified Compiler Flags

- The compiler flag `-fbracket-depth` default value is increased from 256 to 2048. (#GH94728)

- `-Wpadded` option implemented for the `x86_64-windows-msvc` target. Fixes #61702

Removed Compiler Flags
-------------------------

Expand Down
68 changes: 56 additions & 12 deletions clang/lib/AST/RecordLayoutBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2274,9 +2274,9 @@ static unsigned getPaddingDiagFromTagKind(TagTypeKind Tag) {
}
}

void ItaniumRecordLayoutBuilder::CheckFieldPadding(
uint64_t Offset, uint64_t UnpaddedOffset, uint64_t UnpackedOffset,
unsigned UnpackedAlign, bool isPacked, const FieldDecl *D) {
static void CheckFieldPadding(const ASTContext &Context, bool IsUnion,
uint64_t Offset, uint64_t UnpaddedOffset,
const FieldDecl *D) {
// We let objc ivars without warning, objc interfaces generally are not used
// for padding tricks.
if (isa<ObjCIvarDecl>(D))
Expand All @@ -2300,23 +2300,31 @@ void ItaniumRecordLayoutBuilder::CheckFieldPadding(
if (D->getIdentifier()) {
auto Diagnostic = D->isBitField() ? diag::warn_padded_struct_bitfield
: diag::warn_padded_struct_field;
Diag(D->getLocation(), Diagnostic)
Context.getDiagnostics().Report(D->getLocation(),
Diagnostic)
<< getPaddingDiagFromTagKind(D->getParent()->getTagKind())
<< Context.getTypeDeclType(D->getParent()) << PadSize
<< (InBits ? 1 : 0) // (byte|bit)
<< D->getIdentifier();
} else {
auto Diagnostic = D->isBitField() ? diag::warn_padded_struct_anon_bitfield
: diag::warn_padded_struct_anon_field;
Diag(D->getLocation(), Diagnostic)
Context.getDiagnostics().Report(D->getLocation(),
Diagnostic)
<< getPaddingDiagFromTagKind(D->getParent()->getTagKind())
<< Context.getTypeDeclType(D->getParent()) << PadSize
<< (InBits ? 1 : 0); // (byte|bit)
}
}
if (isPacked && Offset != UnpackedOffset) {
HasPackedField = true;
}
}
}

void ItaniumRecordLayoutBuilder::CheckFieldPadding(
uint64_t Offset, uint64_t UnpaddedOffset, uint64_t UnpackedOffset,
unsigned UnpackedAlign, bool isPacked, const FieldDecl *D) {
::CheckFieldPadding(Context, IsUnion, Offset, UnpaddedOffset, D);
if (isPacked && Offset != UnpackedOffset) {
HasPackedField = true;
}
}

static const CXXMethodDecl *computeKeyFunction(ASTContext &Context,
Expand Down Expand Up @@ -2555,7 +2563,8 @@ struct MicrosoftRecordLayoutBuilder {
typedef llvm::DenseMap<const CXXRecordDecl *, CharUnits> BaseOffsetsMapTy;
MicrosoftRecordLayoutBuilder(const ASTContext &Context,
EmptySubobjectMap *EmptySubobjects)
: Context(Context), EmptySubobjects(EmptySubobjects) {}
: Context(Context), EmptySubobjects(EmptySubobjects),
RemainingBitsInField(0) {}

private:
MicrosoftRecordLayoutBuilder(const MicrosoftRecordLayoutBuilder &) = delete;
Expand Down Expand Up @@ -2642,8 +2651,6 @@ struct MicrosoftRecordLayoutBuilder {
/// virtual base classes and their offsets in the record.
ASTRecordLayout::VBaseOffsetsMapTy VBases;
/// The number of remaining bits in our last bitfield allocation.
/// This value isn't meaningful unless LastFieldIsNonZeroWidthBitfield is
/// true.
unsigned RemainingBitsInField;
bool IsUnion : 1;
/// True if the last field laid out was a bitfield and was not 0
Expand Down Expand Up @@ -3004,6 +3011,15 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
} else {
FieldOffset = Size.alignTo(Info.Alignment);
}

uint64_t UnpaddedFielddOffsetInBits =
Context.toBits(DataSize) - RemainingBitsInField;

::CheckFieldPadding(Context, IsUnion, Context.toBits(FieldOffset),
UnpaddedFielddOffsetInBits, FD);

RemainingBitsInField = 0;

placeFieldAtOffset(FieldOffset);

if (!IsOverlappingEmptyField)
Expand Down Expand Up @@ -3049,10 +3065,14 @@ void MicrosoftRecordLayoutBuilder::layoutBitField(const FieldDecl *FD) {
} else {
// Allocate a new block of memory and place the bitfield in it.
CharUnits FieldOffset = Size.alignTo(Info.Alignment);
uint64_t UnpaddedFieldOffsetInBits =
Context.toBits(DataSize) - RemainingBitsInField;
placeFieldAtOffset(FieldOffset);
Size = FieldOffset + Info.Size;
Alignment = std::max(Alignment, Info.Alignment);
RemainingBitsInField = Context.toBits(Info.Size) - Width;
::CheckFieldPadding(Context, IsUnion, Context.toBits(FieldOffset),
UnpaddedFieldOffsetInBits, FD);
}
DataSize = Size;
}
Expand All @@ -3076,9 +3096,14 @@ MicrosoftRecordLayoutBuilder::layoutZeroWidthBitField(const FieldDecl *FD) {
} else {
// Round up the current record size to the field's alignment boundary.
CharUnits FieldOffset = Size.alignTo(Info.Alignment);
uint64_t UnpaddedFieldOffsetInBits =
Context.toBits(DataSize) - RemainingBitsInField;
placeFieldAtOffset(FieldOffset);
RemainingBitsInField = 0;
Size = FieldOffset;
Alignment = std::max(Alignment, Info.Alignment);
::CheckFieldPadding(Context, IsUnion, Context.toBits(FieldOffset),
UnpaddedFieldOffsetInBits, FD);
}
DataSize = Size;
}
Expand Down Expand Up @@ -3203,6 +3228,9 @@ void MicrosoftRecordLayoutBuilder::layoutVirtualBases(const CXXRecordDecl *RD) {
}

void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {
uint64_t UnpaddedSizeInBits = Context.toBits(DataSize);
UnpaddedSizeInBits -= RemainingBitsInField;

// Respect required alignment. Note that in 32-bit mode Required alignment
// may be 0 and cause size not to be updated.
DataSize = Size;
Expand Down Expand Up @@ -3231,6 +3259,22 @@ void MicrosoftRecordLayoutBuilder::finalizeLayout(const RecordDecl *RD) {
Size = Context.toCharUnitsFromBits(External.Size);
if (External.Align)
Alignment = Context.toCharUnitsFromBits(External.Align);
return;
}
unsigned CharBitNum = Context.getTargetInfo().getCharWidth();
uint64_t DataSizeInBits = Context.toBits(DataSize);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand all the implications of using DataSize... yes, it handles empty structs, but there's also the interaction with RequiredAlignment, which I'm not sure behaves the way you want for non-empty structs. (I think you can test this using an "aligned" attribute on a struct.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right, using DataSize did create issues while using certain alignment attributes. I added some in the test file.
Now, 1 byte is artificially added to UnpaddedSizeInBits when Size is zero before aligning the empty struct to its alignment. This allows to keep the checks as they used to be (using Size instead of DataSize) and also takes care of the empty struct case.

Thank you for catching that case!

if (DataSizeInBits > UnpaddedSizeInBits) {
unsigned int PadSize = DataSizeInBits - UnpaddedSizeInBits;
bool InBits = true;
if (PadSize % CharBitNum == 0) {
PadSize = PadSize / CharBitNum;
InBits = false;
}

Context.getDiagnostics().Report(RD->getLocation(),
diag::warn_padded_struct_size)
<< Context.getTypeDeclType(RD) << PadSize
<< (InBits ? 1 : 0); // (byte|bit)
}
}

Expand Down
32 changes: 32 additions & 0 deletions clang/test/SemaCXX/windows-Wpadded-bitfield.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded %s

struct __attribute__((ms_struct)) BitfieldStruct { // expected-warning {{padding size of 'BitfieldStruct' with 3 bytes to alignment boundary}}
char c : 1;
int : 0; // expected-warning {{padding struct 'BitfieldStruct' with 31 bits to align anonymous bit-field}}
char i;
};

struct __attribute__((ms_struct)) SevenBitfieldStruct { // expected-warning {{padding size of 'SevenBitfieldStruct' with 3 bytes to alignment boundary}}
char c : 7;
int : 0; // expected-warning {{padding struct 'SevenBitfieldStruct' with 25 bits to align anonymous bit-field}}
char i;
};

struct __attribute__((ms_struct)) SameUnitSizeBitfield {
char c : 7;
char : 1; // Same unit size attributes fall in the same unit + they fill the unit -> no padding
char i;
};

struct __attribute__((ms_struct)) DifferentUnitSizeBitfield { // expected-warning {{padding size of 'DifferentUnitSizeBitfield' with 3 bytes to alignment boundary}}
char c : 7;
int : 1; // expected-warning {{padding struct 'DifferentUnitSizeBitfield' with 25 bits to align anonymous bit-field}}
char i; // expected-warning {{padding struct 'DifferentUnitSizeBitfield' with 31 bits to align 'i'}}
};

int main() {
BitfieldStruct b;
SevenBitfieldStruct s;
SameUnitSizeBitfield su;
DifferentUnitSizeBitfield du;
}
45 changes: 45 additions & 0 deletions clang/test/SemaCXX/windows-Wpadded.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// RUN: %clang_cc1 -triple x86_64-windows-msvc -fsyntax-only -verify -Wpadded %s
// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only -verify -Wpadded %s

struct __attribute__((ms_struct)) Foo { // expected-warning {{padding size of 'Foo' with 3 bytes to alignment boundary}}
int b : 1;
char a; // expected-warning {{padding struct 'Foo' with 31 bits to align 'a'}}
};

struct __attribute__((ms_struct)) AlignedStruct { // expected-warning {{padding size of 'AlignedStruct' with 4 bytes to alignment boundary}}
char c;
alignas(8) int i; // expected-warning {{padding struct 'AlignedStruct' with 7 bytes to align 'i'}}
};


struct Base {
int b;
};

struct Derived : public Base { // expected-warning {{padding size of 'Derived' with 3 bytes to alignment boundary}}
char c;
};

union __attribute__((ms_struct)) Union {
char c;
long long u;
};

struct __attribute__((ms_struct)) StructWithUnion { // expected-warning {{padding size of 'StructWithUnion' with 6 bytes to alignment boundary}}
char c;
int : 0;
Union t; // expected-warning {{padding struct 'StructWithUnion' with 7 bytes to align 't'}}
short i;
};

struct __attribute__((ms_struct)) EmptyStruct {

};

int main() {
Foo f;
AlignedStruct a;
Derived d;
StructWithUnion swu;
EmptyStruct e;
}
Loading