diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index bc13d02e2d20b..44c38396c764f 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -524,6 +524,10 @@ Improvements to Clang's diagnostics - An error is now emitted when OpenMP ``collapse`` and ``ordered`` clauses have an argument larger than what can fit within a 64-bit integer. +- A new off-by-default warning ``-Wms-bitfield-padding`` has been added to alert to cases where bit-field + packing may differ under the MS struct ABI (#GH117428). + + Improvements to Clang's time-trace ---------------------------------- diff --git a/clang/include/clang/Basic/AttrDocs.td b/clang/include/clang/Basic/AttrDocs.td index 5fb5f16680b41..65d66dd398ad1 100644 --- a/clang/include/clang/Basic/AttrDocs.td +++ b/clang/include/clang/Basic/AttrDocs.td @@ -8680,6 +8680,7 @@ its underlying representation to be a WebAssembly ``funcref``. def PreferredTypeDocumentation : Documentation { let Category = DocCatField; + let Label = "langext-preferred_type_documentation"; let Content = [{ This attribute allows adjusting the type of a bit-field in debug information. This can be helpful when a bit-field is intended to store an enumeration value, diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 7b0dcde44296e..5a3e756f07ecc 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -690,6 +690,52 @@ 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-padding"> { + code Documentation = [{ + Under the Microsoft ABI, adjacent bit-fields are not packed if the + underlying type has a different storage size. This warning indicates that a + pair of adjacent bit-fields may not pack in the same way due to this behavioural + difference. + + This can occur when mixing different types explicitly: + + .. code-block:: c++ + + struct S { + uint16_t field1 : 1; + uint32_t field2 : 1; + }; + + or more subtly through enums + + .. code-block:: c++ + + enum Enum1 { /* ... */ }; + enum class Enum2 : unsigned char { /* ... */ }; + struct S { + Enum1 field1 : 1; + Enum2 field2 : 1; + }; + + In each of these cases under the Microsoft ABI the second bit-field + will not be packed with the preceding bit-field, and instead will be aligned + as if the fields were each separately defined integer fields of their respective + storage size. For binary compatibility this is obviously and observably + incompatible, however where bit-fields are being used solely for memory use + reduction this incomplete packing may silently increase the size of objects vs + what is expected. + + This issue can be addressed by ensuring the storage type of each bit-field is + the same, either by explicitly using the same integer type, or in the case of + enum types declaring the enum types with the same storage size. For enum types + where you cannot specify the underlying type, the options are to either switch + to int sized storage for all specifiers or to resort to declaring the + bit-fields with explicit integer storage types and cast in and out of the field. + If such a solution is required the + :ref:`preferred_type ` attribute can be + used to convey the actual field type to debuggers and other tooling. + }]; +} def PessimizingMove : DiagGroup<"pessimizing-move">; def ReturnStdMove : DiagGroup<"return-std-move">; diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 3efe9593b8633..d6afd5aebdb53 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -6541,6 +6541,13 @@ def note_change_bitfield_sign : Note< "consider making the bit-field type %select{unsigned|signed}0">; def note_bitfield_preferred_type : Note<"preferred type for bit-field %0 specified here">; +def warn_ms_bitfield_mismatched_storage_packing : Warning< + "bit-field %0 of type %1 has a different storage size than the " + "preceding bit-field (%2 vs %3 bytes) and will not be packed under " + "the Microsoft ABI">, + InGroup, 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">, diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp index 152f3f340cd50..a7d59ec232b64 100644 --- a/clang/lib/Sema/SemaDecl.cpp +++ b/clang/lib/Sema/SemaDecl.cpp @@ -19399,9 +19399,9 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, // Verify that all the fields are okay. SmallVector RecFields; - + const FieldDecl *PreviousField = nullptr; for (ArrayRef::iterator i = Fields.begin(), end = Fields.end(); - i != end; ++i) { + i != end; PreviousField = cast(*i), ++i) { FieldDecl *FD = cast(*i); // Get the type for the field. @@ -19617,6 +19617,29 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl, if (Record && FD->getType().isVolatileQualified()) Record->setHasVolatileMember(true); + bool ReportMSBitfieldStoragePacking = + Record && PreviousField && + !Diags.isIgnored(diag::warn_ms_bitfield_mismatched_storage_packing, + Record->getLocation()); + auto IsNonDependentBitField = [](const FieldDecl *FD) { + return FD->isBitField() && !FD->getType()->isDependentType(); + }; + + if (ReportMSBitfieldStoragePacking && IsNonDependentBitField(FD) && + IsNonDependentBitField(PreviousField)) { + CharUnits FDStorageSize = Context.getTypeSizeInChars(FD->getType()); + CharUnits PreviousFieldStorageSize = + Context.getTypeSizeInChars(PreviousField->getType()); + if (FDStorageSize != PreviousFieldStorageSize) { + Diag(FD->getLocation(), + diag::warn_ms_bitfield_mismatched_storage_packing) + << FD << FD->getType() << FDStorageSize.getQuantity() + << PreviousFieldStorageSize.getQuantity(); + 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; diff --git a/clang/test/Sema/bitfield-layout.c b/clang/test/Sema/bitfield-layout.c index 079720cc9b40b..595b24d3e857e 100644 --- a/clang/test/Sema/bitfield-layout.c +++ b/clang/test/Sema/bitfield-layout.c @@ -3,6 +3,8 @@ // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=aarch64-linux-gnu // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=x86_64-pc-linux-gnu // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=x86_64-scei-ps4 +// RUN: %clang_cc1 %s -fsyntax-only -verify=checkms -triple=i686-apple-darwin9 -Wms-bitfield-padding + // expected-no-diagnostics #include @@ -24,12 +26,27 @@ CHECK_ALIGN(struct, a, 1) #endif // Zero-width bit-fields with packed -struct __attribute__((packed)) a2 { short x : 9; char : 0; int y : 17; }; +struct __attribute__((packed)) a2 { + short x : 9; // #a2x + char : 0; // #a2anon + // checkms-warning@-1 {{bit-field '' of type 'char' has a different storage size than the preceding bit-field (1 vs 2 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#a2x {{preceding bit-field 'x' declared here with type 'short'}} + int y : 17; + // checkms-warning@-1 {{bit-field 'y' of type 'int' has a different storage size than the preceding bit-field (4 vs 1 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#a2anon {{preceding bit-field '' declared here with type 'char'}} +}; + CHECK_SIZE(struct, a2, 5) CHECK_ALIGN(struct, a2, 1) // Zero-width bit-fields at the end of packed struct -struct __attribute__((packed)) a3 { short x : 9; int : 0; }; +struct __attribute__((packed)) a3 { + short x : 9; // #a3x + int : 0; + // checkms-warning@-1 {{bit-field '' of type 'int' has a different storage size than the preceding bit-field (4 vs 2 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#a3x {{preceding bit-field 'x' declared here with type 'short'}} +}; + #if defined(__arm__) || defined(__aarch64__) CHECK_SIZE(struct, a3, 4) CHECK_ALIGN(struct, a3, 4) @@ -39,7 +56,12 @@ CHECK_ALIGN(struct, a3, 1) #endif // For comparison, non-zero-width bit-fields at the end of packed struct -struct __attribute__((packed)) a4 { short x : 9; int : 1; }; +struct __attribute__((packed)) a4 { + short x : 9; // #a4x + int : 1; + // checkms-warning@-1 {{bit-field '' of type 'int' has a different storage size than the preceding bit-field (4 vs 2 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#a4x {{preceding bit-field 'x' declared here with type 'short'}} +}; CHECK_SIZE(struct, a4, 2) CHECK_ALIGN(struct, a4, 1) @@ -165,22 +187,28 @@ CHECK_OFFSET(struct, g4, c, 3); #endif struct g5 { - char : 1; + char : 1; // #g5 __attribute__((aligned(1))) int n : 24; + // checkms-warning@-1 {{bit-field 'n' of type 'int' has a different storage size than the preceding bit-field (4 vs 1 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#g5 {{preceding bit-field '' declared here with type 'char'}} }; CHECK_SIZE(struct, g5, 4); CHECK_ALIGN(struct, g5, 4); struct __attribute__((packed)) g6 { - char : 1; + char : 1; // #g6 __attribute__((aligned(1))) int n : 24; + // checkms-warning@-1 {{bit-field 'n' of type 'int' has a different storage size than the preceding bit-field (4 vs 1 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#g6 {{preceding bit-field '' declared here with type 'char'}} }; CHECK_SIZE(struct, g6, 4); CHECK_ALIGN(struct, g6, 1); struct g7 { - char : 1; + char : 1; // #g7 __attribute__((aligned(1))) int n : 25; + // checkms-warning@-1 {{bit-field 'n' of type 'int' has a different storage size than the preceding bit-field (4 vs 1 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#g7 {{preceding bit-field '' declared here with type 'char'}} }; #if defined(__ORBIS__) CHECK_SIZE(struct, g7, 4); @@ -190,8 +218,10 @@ CHECK_SIZE(struct, g7, 8); CHECK_ALIGN(struct, g7, 4); struct __attribute__((packed)) g8 { - char : 1; + char : 1; // #g8 __attribute__((aligned(1))) int n : 25; + // checkms-warning@-1 {{bit-field 'n' of type 'int' has a different storage size than the preceding bit-field (4 vs 1 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#g8 {{preceding bit-field '' declared here with type 'char'}} }; #if defined(__ORBIS__) CHECK_SIZE(struct, g8, 4); diff --git a/clang/test/Sema/bitfield-layout_1.c b/clang/test/Sema/bitfield-layout_1.c index 24277c3911495..3db83c7463503 100644 --- a/clang/test/Sema/bitfield-layout_1.c +++ b/clang/test/Sema/bitfield-layout_1.c @@ -2,6 +2,7 @@ // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=arm-linux-gnueabihf // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=aarch64-linux-gnu // RUN: %clang_cc1 %s -fsyntax-only -verify -triple=x86_64-pc-linux-gnu +// RUN: %clang_cc1 %s -fsyntax-only -verify -triple=i686-apple-darwin9 -Wms-bitfield-padding // expected-no-diagnostics #define CHECK_SIZE(name, size) \ diff --git a/clang/test/Sema/mms-bitfields.c b/clang/test/Sema/mms-bitfields.c index cee5b0669d252..a976578845229 100644 --- a/clang/test/Sema/mms-bitfields.c +++ b/clang/test/Sema/mms-bitfields.c @@ -1,12 +1,16 @@ // RUN: %clang_cc1 -mms-bitfields -fsyntax-only -verify -triple x86_64-apple-darwin9 %s +// RUN: %clang_cc1 -mms-bitfields -fsyntax-only -Wms-bitfield-padding -verify=checkms -triple x86_64-apple-darwin9 %s + // expected-no-diagnostics // The -mms-bitfields commandline parameter should behave the same // as the ms_struct attribute. struct { - int a : 1; + int a : 1; // #a short b : 1; + // checkms-warning@-1 {{bit-field 'b' of type 'short' has a different storage size than the preceding bit-field (2 vs 4 bytes) and will not be packed under the Microsoft ABI}} + // checkms-note@#a {{preceding bit-field 'a' declared here with type 'int'}} } t; // MS pads out bitfields between different types. diff --git a/clang/test/SemaCXX/bitfield.cpp b/clang/test/SemaCXX/bitfield.cpp index 083c28ffbb3d4..bb3094561bea4 100644 --- a/clang/test/SemaCXX/bitfield.cpp +++ b/clang/test/SemaCXX/bitfield.cpp @@ -1,4 +1,5 @@ // RUN: %clang_cc1 %s -verify +// RUN: %clang_cc1 %s -verify -Wms-bitfield-padding // expected-no-diagnostics diff --git a/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp new file mode 100644 index 0000000000000..c0f90f798118a --- /dev/null +++ b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp @@ -0,0 +1,196 @@ + +// RUN: %clang_cc1 -fsyntax-only -Wms-bitfield-padding -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 than the preceding bit-field (8 vs 4 bytes) and will not be packed under the Microsoft 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 than the preceding bit-field (4 vs 1 bytes) and will not be packed under the Microsoft 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); + +struct N { + EnumU32_1 a : 10; + EnumU64 b : 10; + // expected-warning@-1 {{bit-field 'b' of type 'EnumU64' has a different storage size than the preceding bit-field (8 vs 4 bytes) and will not be packed under the Microsoft 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 than the preceding bit-field (4 vs 8 bytes) and will not be packed under the Microsoft 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 than the preceding bit-field (4 vs 2 bytes) and will not be packed under the Microsoft 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 than the preceding bit-field (2 vs 4 bytes) and will not be packed under the Microsoft 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 than the preceding bit-field (2 vs 1 bytes) and will not be packed under the Microsoft 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 than the preceding bit-field (1 vs 2 bytes) and will not be packed under the Microsoft 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 + +struct S { + char a : 4; + char b : 4; + char c : 4; + char d : 4; + short x : 7; + // expected-warning@-1 {{bit-field 'x' of type 'short' has a different storage size than the preceding bit-field (2 vs 1 bytes) and will not be packed under the Microsoft ABI}} + // expected-note@-3 {{preceding bit-field 'd' declared here with type 'char'}} + // This is a false positive. Reporting this correctly requires duplicating the record layout process + // in target and MS layout modes, and it's also unclear if that's the correct choice for users of + // this diagnostic. + short y : 9; +}; + +static_assert(sizeof(S) == 4);