Add an off-by-default warning to complain about MSVC bitfield padding (llvm#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.

Author: ojhunt

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