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

clang/docs/ReleaseNotes.rst

@@ -734,6 +734,9 @@ Improvements to Clang's diagnostics
       require(scope); // Warning!  Requires mu1.
     }
 
+- A new off-by-default warning ``-Wms-bitfield-compatibility`` 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

@@ -8242,6 +8242,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

@@ -635,6 +635,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-compatibility"> {
+  code Documentation = [{
+    Under the MS Windows platform 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 MS Windows platform 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

@@ -6437,6 +6437,13 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup<BitFieldEnumConversion>, DefaultIgnore;
 def note_change_bitfield_sign : Note<
   "consider making the bit-field type %select{unsigned|signed}0">;
+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 MS Windows platform 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

@@ -19028,9 +19028,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.
@@ -19240,6 +19240,25 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
 
     if (Record && FD->getType().isVolatileQualified())
       Record->setHasVolatileMember(true);
+    auto IsNonDependentBitField = [](const FieldDecl *FD) {
+      return FD->isBitField() && !FD->getType()->isDependentType();
+    };
+
+    if (Record && PreviousField && 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/SemaCXX/ms_struct-bitfield-padding.cpp

@@ -0,0 +1,196 @@
+
+// RUN: %clang_cc1 -fsyntax-only -Wms-bitfield-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 than the preceding bit-field (8 vs 4 bytes) and will not be packed under the MS Windows platform 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 MS Windows platform 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 MS Windows platform 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 MS Windows platform 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 MS Windows platform 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 MS Windows platform 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 MS Windows platform 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 MS Windows platform 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 MS Windows platform 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);