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.

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

[CLANG-CL] ignores wpadded #130182

Merged
merged 12 commits into from
Apr 2, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
65 changes: 54 additions & 11 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 @@ -2642,8 +2650,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 +3010,15 @@ void MicrosoftRecordLayoutBuilder::layoutField(const FieldDecl *FD) {
} else {
FieldOffset = Size.alignTo(Info.Alignment);
}

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

Choose a reason for hiding this comment

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

Could it be that RemainingBitsInField is not always initialized here?
When I add some printouts and compare a failing and passing build I see that when it fails RemainingBitsInField is 2767481344 which looks a bit odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, this is also my main suspicion. I will fix this later today.

Thank you for your thoroughness, I did not think to check with other compilers or do a valgrind check. Sorry for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you both! In line with our patch reversion policy https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy I've gone ahead and reverted so that the issue can be fixed without undue time pressure, and so bots can go green again in the meanwhile.

Copy link
Collaborator

@mikaelholmen mikaelholmen Apr 3, 2025

Choose a reason for hiding this comment

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

Do we know why the failed bots didn't show up as comments in this PR or is that something someone/somewhere should look into?

Copy link
Contributor

@asb asb Apr 3, 2025

Choose a reason for hiding this comment

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

I can't see an obvious reason why that didn't happen here - I've filed a bug on llvm-zorg llvm/llvm-zorg#427

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, there was an obvious reason. The GitHub PR reporting only kicks in if a builder built with this as the only commit that changed since the last build. The failing builders had more PRs than just this one in their changelist, so no comment on GitHub.


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

RemainingBitsInField = 0;

placeFieldAtOffset(FieldOffset);

if (!IsOverlappingEmptyField)
Expand Down Expand Up @@ -3049,10 +3064,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 +3095,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 +3227,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 +3258,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 SizeInBits = Context.toBits(Size);
if (SizeInBits > UnpaddedSizeInBits) {
unsigned int PadSize = SizeInBits - 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;
}
40 changes: 40 additions & 0 deletions clang/test/SemaCXX/windows-Wpadded.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// 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;
};

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