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

Oliver Hunt via cfe-commits cfe-commits at lists.llvm.org
Sat Nov 30 19:37:18 PST 2024


https://github.com/ojhunt updated https://github.com/llvm/llvm-project/pull/117428

>From 3e25d7ef2e223942298078dace8979905956d05c Mon Sep 17 00:00:00 2001
From: Oliver Hunt <oliver at apple.com>
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/2] Add an off-by-default warning to complain about MSVC
 bitfield padding

---
 clang/include/clang/Basic/DiagnosticGroups.td |   1 +
 .../clang/Basic/DiagnosticSemaKinds.td        |   6 +
 clang/lib/Sema/SemaDecl.cpp                   |  27 ++-
 .../SemaCXX/ms_struct-bitfield-padding.cpp    | 180 ++++++++++++++++++
 4 files changed, 212 insertions(+), 2 deletions(-)
 create mode 100644 clang/test/SemaCXX/ms_struct-bitfield-padding.cpp

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index df9bf94b5d0398..57bdda4b8f8b47 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -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">;
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index eb05a6a77978af..aa13e3438d3739 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -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">,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74b0e5ad23bd48..18aeca3b34f659 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -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.
@@ -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;
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 00000000000000..001086de5bbd10
--- /dev/null
+++ b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
@@ -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 at -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 at -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 at -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 at -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 at -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 at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
+  EnumU32_1 c : 10;
+  // expected-warning at -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 at -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 at -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 at -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 at -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 at -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 at -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 at -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 at -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 at -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

>From 11447fea7197a0902c0b1c23071b35de3b782f13 Mon Sep 17 00:00:00 2001
From: Oliver Hunt <oliver at apple.com>
Date: Sat, 30 Nov 2024 19:36:33 -0800
Subject: [PATCH 2/2] Address feedback

---
 .../clang/Basic/DiagnosticSemaKinds.td        |  5 ++--
 clang/lib/Sema/SemaDecl.cpp                   | 24 ++++++++-----------
 .../SemaCXX/ms_struct-bitfield-padding.cpp    | 16 ++++++-------
 3 files changed, 21 insertions(+), 24 deletions(-)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index 98ed070e398a50..ce4f68bbb8bfc0 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6423,8 +6423,9 @@ def warn_signed_bitfield_enum_conversion : Warning<
 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 (%2 vs %3 bytes) than the "
-  "preceding bit-field and may not be packed under MSVC ABI">,
+  "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 MSVC ABI">,
   InGroup<MSBitfieldCompatibility>, DefaultIgnore;
 def note_ms_bitfield_mismatched_storage_size_previous : Note<
   "preceding bit-field %0 declared here with type %1">;
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 123e061404add0..a71c31b0a5ae36 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -19016,7 +19016,7 @@ 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;
+  const FieldDecl *PreviousField = nullptr;
   for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
        i != end; PreviousField = cast<FieldDecl>(*i), ++i) {
     FieldDecl *FD = cast<FieldDecl>(*i);
@@ -19229,26 +19229,22 @@ 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;
+      return FD->isBitField() && !FD->getType()->isDependentType();
     };
 
     if (Record && PreviousField && IsNonDependentBitField(FD) &&
-        IsNonDependentBitField(*PreviousField)) {
-      unsigned FDStorageSize =
-          Context.getTypeSizeInChars(FD->getType()).getQuantity();
-      unsigned PreviousFieldStorageSize =
-          Context.getTypeSizeInChars((*PreviousField)->getType()).getQuantity();
+        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 << PreviousFieldStorageSize;
-        Diag((*PreviousField)->getLocation(),
+            << FD << FD->getType() << FDStorageSize.getQuantity()
+            << PreviousFieldStorageSize.getQuantity();
+        Diag(PreviousField->getLocation(),
              diag::note_ms_bitfield_mismatched_storage_size_previous)
-            << *PreviousField << (*PreviousField)->getType();
+            << PreviousField << PreviousField->getType();
       }
     }
     // Keep track of the number of named members.
diff --git a/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
index 001086de5bbd10..11a786d47dd0ac 100644
--- a/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
+++ b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
@@ -55,7 +55,7 @@ static_assert(sizeof(F) == 4);
 struct G {
   EnumU32_1 a : 15;
   EnumU64 b : 15;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
 };
 
@@ -76,7 +76,7 @@ struct I {
   EnumU8 a : 3;
   EnumI8 b : 5;
   EnumU32_1 c : 10;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'b' declared here with type 'EnumI8'}}
 };
 #ifdef MS_BITFIELDS
@@ -116,10 +116,10 @@ static_assert(sizeof(M) == 4);
 struct N {
   EnumU32_1 a : 10;
   EnumU64 b : 10;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
   EnumU32_1 c : 10;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -5 {{preceding bit-field 'b' declared here with type 'EnumU64'}}
 };
 
@@ -132,7 +132,7 @@ static_assert(sizeof(N) == 8);
 struct O {
   EnumU16 a : 10;
   EnumU32_1 b : 10;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU16'}}
 };
 #ifdef MS_BITFIELDS
@@ -144,7 +144,7 @@ static_assert(sizeof(O) == 4);
 struct P {
   EnumU32_1 a : 10;
   EnumU16 b : 10;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
 };
 #ifdef MS_BITFIELDS
@@ -156,7 +156,7 @@ static_assert(sizeof(P) == 4);
 struct Q {
   EnumU8 a : 6;
   EnumU16 b : 6;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU8'}}
 };
 #ifdef MS_BITFIELDS
@@ -169,7 +169,7 @@ struct R {
   EnumU16 a : 9;
   EnumU16 b : 9;
   EnumU8 c : 6;
-  // expected-warning at -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-warning at -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 MSVC ABI}}
   // expected-note at -3 {{preceding bit-field 'b' declared here with type 'EnumU16'}}
 };
 



More information about the cfe-commits mailing list