Skip to content

Add an off-by-default warning to complain about MSVC bitfield padding #117428

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

Merged
merged 19 commits into from
May 13, 2025
Merged

Add an off-by-default warning to complain about MSVC bitfield padding #117428

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
3e25d7e
Add an off-by-default warning to complain about MSVC bitfield padding
ojhunt Nov 22, 2024
fbe679e
Merge remote-tracking branch 'origin/llvm.org/main' into users/ojhunt…
ojhunt Nov 30, 2024
11447fe
Address feedback
ojhunt Dec 1, 2024
a18c032
Documentation and renaming
ojhunt Dec 2, 2024
75a46fe
Add release note
ojhunt Dec 2, 2024
40dc8cf
Merge branch 'main' into users/ojhunt/warn-msvc-bitfield-packing
ojhunt Dec 2, 2024
bd3ffbe
Add links to other docs
ojhunt Dec 2, 2024
c4b2d4a
Address feedback
ojhunt Dec 3, 2024
e1b5296
Merge branch 'main' into users/ojhunt/warn-msvc-bitfield-packing
ojhunt Dec 4, 2024
ca50d1e
Update documentation label
ojhunt Dec 4, 2024
fa39d28
Merge remote-tracking branch 'origin/llvm.org/main' into users/ojhunt…
ojhunt Dec 31, 2024
196c034
Merge branch 'main' into users/ojhunt/warn-msvc-bitfield-packing
ojhunt Jan 4, 2025
d88b87b
Merge branch 'main' into users/ojhunt/warn-msvc-bitfield-packing
ojhunt Jan 16, 2025
bd2b006
Merge branch 'main' into users/ojhunt/warn-msvc-bitfield-packing
ojhunt Mar 3, 2025
ab3b7c3
Don't try to diagnose the mismatch unless the warning is enabled
ojhunt Mar 3, 2025
d3fb2f2
Sigh i really thought i had the formatting correct
ojhunt Mar 3, 2025
80e3549
Merge remote-tracking branch 'origin/llvm.org/main' into users/ojhunt…
ojhunt Apr 11, 2025
5e64d46
Change diagnostic spelling, switch to 'Microsoft ABI' to describe the…
ojhunt Apr 11, 2025
73128b6
Merge remote-tracking branch 'origin/llvm.org/main' into users/ojhunt…
ojhunt May 13, 2025
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
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,6 +631,7 @@ def Packed : DiagGroup<"packed", [PackedNonPod]>;
def PaddedBitField : DiagGroup<"padded-bitfield">;
def Padded : DiagGroup<"padded", [PaddedBitField]>;
def UnalignedAccess : DiagGroup<"unaligned-access">;
def MSBitfieldCompatibility : DiagGroup<"ms-bitfield-packing-compatibility">;

def PessimizingMove : DiagGroup<"pessimizing-move">;
def ReturnStdMove : DiagGroup<"return-std-move">;
Expand Down
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
InGroup<BitFieldEnumConversion>, DefaultIgnore;
def note_change_bitfield_sign : Note<
"consider making the bitfield type %select{unsigned|signed}0">;
def warn_ms_bitfield_mismatched_storage_packing : Warning<
"bit-field %0 of type %1 has a different storage size (%2 vs %3 bytes) than the "
"preceding bit-field and may not be packed under MSVC ABI">,
InGroup<MSBitfieldCompatibility>, DefaultIgnore;
def note_ms_bitfield_mismatched_storage_size_previous : Note<
"preceding bit-field %0 declared here with type %1">;

def warn_missing_braces : Warning<
"suggest braces around initialization of subobject">,
Expand Down
27 changes: 25 additions & 2 deletions clang/lib/Sema/SemaDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19001,9 +19001,9 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,

// Verify that all the fields are okay.
SmallVector<FieldDecl*, 32> RecFields;

std::optional<const FieldDecl *> PreviousField;
for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
i != end; ++i) {
i != end; PreviousField = cast<FieldDecl>(*i), ++i) {
FieldDecl *FD = cast<FieldDecl>(*i);

// Get the type for the field.
Expand Down Expand Up @@ -19213,6 +19213,29 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,

if (Record && FD->getType().isVolatileQualified())
Record->setHasVolatileMember(true);
auto IsNonDependentBitField = [](const FieldDecl *FD) {
if (!FD->isBitField())
return false;
if (FD->getType()->isDependentType())
return false;
return true;
};

if (Record && PreviousField && IsNonDependentBitField(FD) &&
IsNonDependentBitField(*PreviousField)) {
unsigned FDStorageSize =
Context.getTypeSizeInChars(FD->getType()).getQuantity();
unsigned PreviousFieldStorageSize =
Context.getTypeSizeInChars((*PreviousField)->getType()).getQuantity();
if (FDStorageSize != PreviousFieldStorageSize) {
Diag(FD->getLocation(),
diag::warn_ms_bitfield_mismatched_storage_packing)
<< FD << FD->getType() << FDStorageSize << PreviousFieldStorageSize;
Diag((*PreviousField)->getLocation(),
diag::note_ms_bitfield_mismatched_storage_size_previous)
<< *PreviousField << (*PreviousField)->getType();
}
}
// Keep track of the number of named members.
if (FD->getIdentifier())
++NumNamedMembers;
Expand Down
180 changes: 180 additions & 0 deletions clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,180 @@

// RUN: %clang_cc1 -fsyntax-only -Wms-bitfield-packing-compatibility -verify -triple armv8 -std=c++23 %s
// RUN: %clang_cc1 -fsyntax-only -DMS_BITFIELDS -mms-bitfields -verify=msbitfields -triple armv8-apple-macos10.15 -std=c++23 %s

// msbitfields-no-diagnostics

enum Enum1 { Enum1_A, Enum1_B };
enum Enum2 { Enum2_A, Enum2_B };

enum class EnumU32_1 : unsigned { A, B };
enum class EnumU32_2 : unsigned { A, B };
enum class EnumU64 : unsigned long long { A, B };
enum class EnumI32 : int { A, B };
enum class EnumU8 : unsigned char { A, B };
enum class EnumI8 : char { A, B };
enum class EnumU16 : unsigned short { A, B };
enum class EnumI16 : short { A, B };

struct A {
unsigned int a : 15;
unsigned int b : 15;
};
static_assert(sizeof(A) == 4);

struct B {
unsigned int a : 15;
int b : 15;
};
static_assert(sizeof(B) == 4);

struct C {
unsigned int a : 15;
int b : 15;
};
static_assert(sizeof(C) == 4);

struct D {
Enum1 a : 15;
Enum1 b : 15;
};
static_assert(sizeof(D) == 4);

struct E {
Enum1 a : 15;
Enum2 b : 15;
};
static_assert(sizeof(E) == 4);

struct F {
EnumU32_1 a : 15;
EnumU32_2 b : 15;
};
static_assert(sizeof(F) == 4);

struct G {
EnumU32_1 a : 15;
EnumU64 b : 15;
// expected-warning@-1 {{bit-field 'b' of type 'EnumU64' has a different storage size (8 vs 4 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
};

#ifdef MS_BITFIELDS
static_assert(sizeof(G) == 16);
#else
static_assert(sizeof(G) == 8);
#endif

struct H {
EnumU32_1 a : 10;
EnumI32 b : 10;
EnumU32_1 c : 10;
};
static_assert(sizeof(H) == 4);

struct I {
EnumU8 a : 3;
EnumI8 b : 5;
EnumU32_1 c : 10;
// expected-warning@-1 {{bit-field 'c' of type 'EnumU32_1' has a different storage size (4 vs 1 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'b' declared here with type 'EnumI8'}}
};
#ifdef MS_BITFIELDS
static_assert(sizeof(I) == 8);
#else
static_assert(sizeof(I) == 4);
#endif

struct J {
EnumU8 : 0;
EnumU8 b : 4;
};
static_assert(sizeof(J) == 1);

struct K {
EnumU8 a : 4;
EnumU8 : 0;
};
static_assert(sizeof(K) == 1);

struct L {
EnumU32_1 a : 10;
EnumU32_2 b : 10;
EnumU32_1 c : 10;
};

static_assert(sizeof(L) == 4);

struct M {
EnumU32_1 a : 10;
EnumI32 b : 10;
EnumU32_1 c : 10;
};

static_assert(sizeof(M) == 4);

Copy link
Collaborator

@rnk rnk Dec 2, 2024

Choose a reason for hiding this comment

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

Can we add a test for a case for what happens when there is no layout difference, so something like this:

struct Foo {
  char a : 4;
  char b : 4;
  char c : 4;
  char d : 4;
  short x : 7;
  short y : 9;
};

As written, this code will warn, but both compilers use the same / similar struct layouts (link). Does this represent a false positive, or should we encourage users to use a consistent bitfield width?

llvm::Value doesn't have this problem because it happens to alternate between small bitfields and small integers, but this seems like a category of false positive that is hard to identify.

The only reasonable way to overcome this limitation would be to move this warning into record layout, in which case we'd have to implement it twice, once in the MS and non-MS record layout code.

Copy link
Contributor Author

@ojhunt ojhunt Dec 2, 2024

Choose a reason for hiding this comment

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

@rnk fun story: my first pass at this was in record layout and it involved layout out the record twice and was awful :D - it also reminded me that there are a bunch of warnings that we don't issue until the point of use rather than declaration which is irksome.

That said, while my original implementation was in record layout and was intended to be 100% accurate, I realized that the actual issue is different type storage sizes resulting in radically different behavior with seemingly unrelated changes later on. So in cases where it matters we just want all bit-fields to be the same storage size, and the warning currently just drives "all bit-fields shall be the same storage size" which is the safe option. Pushing it to actual accurate warnings seems like it might be less than ideal, as you get cases like this trivial change to the above false positive

struct Foo {
  char a : 4;
  char b : 4;
  char c : 4;
  char d : 4;
  char e : 1; // +1 bit field
  short x : 6; // -1 bit width
  short y : 9;
};

which would now issue a warning saying that it is no longer packed on msvc, so the fix will be to go through and restructure/re-type (type system type, not keyboard) the entire structure to ensure everything has the same storage type, when it may have been easier to have it "correct" (from msvc point of view) from the start.

e.g the early false positive forces the code to be written in a way that avoids a real warning down the line that requires more changes. That's also why the warning text originally said "may" :D

Copy link
Contributor Author

@ojhunt ojhunt Apr 11, 2025

Choose a reason for hiding this comment

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

@rnk thoughts?

Copy link
Collaborator

@rnk rnk Apr 14, 2025

Choose a reason for hiding this comment

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

Wouldn't this negatively affect cases like Value, where we have alternating small integeters and bitfields? The current layout won't be affected, but it seems like you'll get warnings on cases like this that you can't fix without adding padding:

struct Value {
  char kind;
  unsigned char b1 : 1;
  unsigned char b7 : 7; // 8
  unsigned short b2 : 2;
  unsigned short b14 : 14; // 16
  unsigned int b12 : 12;
  unsigned int b10a : 10;
  unsigned int b10b : 10; // 32
};

Does your warning fire on this case, and what types would one use to silence the warning and get compact layouts on all platforms?

struct N {
EnumU32_1 a : 10;
EnumU64 b : 10;
// expected-warning@-1 {{bit-field 'b' of type 'EnumU64' has a different storage size (8 vs 4 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
EnumU32_1 c : 10;
// expected-warning@-1 {{bit-field 'c' of type 'EnumU32_1' has a different storage size (4 vs 8 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-5 {{preceding bit-field 'b' declared here with type 'EnumU64'}}
};

#ifdef MS_BITFIELDS
static_assert(sizeof(N) == 24);
#else
static_assert(sizeof(N) == 8);
#endif

struct O {
EnumU16 a : 10;
EnumU32_1 b : 10;
// expected-warning@-1 {{bit-field 'b' of type 'EnumU32_1' has a different storage size (4 vs 2 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'a' declared here with type 'EnumU16'}}
};
#ifdef MS_BITFIELDS
static_assert(sizeof(O) == 8);
#else
static_assert(sizeof(O) == 4);
#endif

struct P {
EnumU32_1 a : 10;
EnumU16 b : 10;
// expected-warning@-1 {{bit-field 'b' of type 'EnumU16' has a different storage size (2 vs 4 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
};
#ifdef MS_BITFIELDS
static_assert(sizeof(P) == 8);
#else
static_assert(sizeof(P) == 4);
#endif

struct Q {
EnumU8 a : 6;
EnumU16 b : 6;
// expected-warning@-1 {{bit-field 'b' of type 'EnumU16' has a different storage size (2 vs 1 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'a' declared here with type 'EnumU8'}}
};
#ifdef MS_BITFIELDS
static_assert(sizeof(Q) == 4);
#else
static_assert(sizeof(Q) == 2);
#endif

struct R {
EnumU16 a : 9;
EnumU16 b : 9;
EnumU8 c : 6;
// expected-warning@-1 {{bit-field 'c' of type 'EnumU8' has a different storage size (1 vs 2 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
// expected-note@-3 {{preceding bit-field 'b' declared here with type 'EnumU16'}}
};

#ifdef MS_BITFIELDS
static_assert(sizeof(R) == 6);
#else
static_assert(sizeof(R) == 4);
#endif
Loading