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

This just adds a warning for bitfields placed next to other bitfields
where the underlying type has different storage. Under the MS struct
bitfield packing ABI such bitfields are not packed.

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
 ----------------------------------
 

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,

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 <langext-preferred_type_documentation>` 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">;

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<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">,

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<FieldDecl*, 32> RecFields;
-
+  const FieldDecl *PreviousField = nullptr;
   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.
@@ -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;

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 <stddef.h>
 
@@ -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);

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) \

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.

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
 

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);