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

[Oliver Hunt via cfe-commits]

https://github.com/ojhunt closed 
https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 01/10] 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 df9bf94b5d039..57bdda4b8f8b4 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 eb05a6a77978a..aa13e3438d373 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, 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, 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 74b0e5ad23bd4..18aeca3b34f65 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 0..001086de5bbd1
--- /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 

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

[Reid Kleckner via cfe-commits]

@@ -0,0 +1,180 @@
+
+// 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 MSVC 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 MSVC 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);
+

rnk wrote:

Wouldn't this negatively affect cases like Value, where we have alternating 
small integeters and bitfields? The current layout won't be affected, but it 
seems like you'll get warnings on cases like this that you can't fix without 
adding padding:

```
struct Value {
  char kind;
  unsigned char b1 : 1;
  unsigned char b7 : 7; // 8
  unsigned short b2 : 2;
  unsigned short b14 : 14; // 16
  unsigned int b12 : 12;
  unsigned int b10a : 10;
  unsigned int b10b : 10; // 32
};
```

Does your warning fire on this case, and what types would one use to silence 
the warning and get compact layouts on all platforms?

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

ojhunt wrote:

@AaronBallman are you ok with this now? I'm re-requesting the review as it's 
been a while, and I want to confirm I addressed things as you wanted

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -6519,6 +6519,13 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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">,

ojhunt wrote:

I've switched to Microsoft ABI, and the diagnostic is now `ms-bitfield-padding`

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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

ojhunt wrote:

Fixed

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 01/10] 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 df9bf94b5d039..57bdda4b8f8b4 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 eb05a6a77978a..aa13e3438d373 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, 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, 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 74b0e5ad23bd4..18aeca3b34f65 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 0..001086de5bbd1
--- /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 

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

[Oliver Hunt via cfe-commits]

ojhunt wrote:

(Finally getting back to this one)

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -0,0 +1,180 @@
+
+// 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 MSVC 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 MSVC 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);
+

ojhunt wrote:

@rnk thoughts?

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -19001,9 +19001,9 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, 
Decl *EnclosingDecl,
 
   // Verify that all the fields are okay.
   SmallVector RecFields;
-
+  std::optional PreviousField;

ojhunt wrote:

@rnk this is sufficiently constrained/non-escaping but in general I prefer 
optional<> to null pointers as there's no other signal to indicate whether a 
pointer can be legitimately null. (It would be nice if optional<&> was a thing 
:D )

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -631,6 +631,9 @@ Improvements to Clang's diagnostics
 
 - Clang now diagnoses dangling references for C++20's parenthesized aggregate 
initialization (#101957).
 
+- A new off-by-default warning ``-Wms-bitfield-compatibility`` has been added 
to alert to cases where bit-field

ojhunt wrote:

I think we'll probably want a general MSVC abi compatibility warning that 
encompasses this PR, the above alignment example, the signed nonsense, etc for 
projects that want to have some kind of compatibility with out using the 
various force-msvc-abi flags on their projects

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -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">;

ojhunt wrote:

I've removed -packing- because I could see us extending this to also warn in 
cases where the ms "signed by default" nonsense could cause issues - e.g. 
rather than a new flag for every possible scenario we can just have one.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

ojhunt wrote:

No one commented on this test case, but for the record:

* It runs in both ms_bitfields and sane (:D) mode
* The static assertions are used to force struct layout
* We assert the structs are the same size or different sizes depending on 
layout mode, so we can then just ensure the structs with different layouts get 
warnings.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

ojhunt wrote:

this matches my thoughts - this warning exists specifically for projects where 
people are building for non ms platforms where the behavior on ms platforms 
matters, but there isn't necessarily a strict abi match


https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

ojhunt wrote:

Oh, what are the semantics of `-fms-compatibility`? does it include the codegen 
changing options? In which case I'm not sure if this warning should be on?

My intention for this warning was so that code that compiled under both ms and 
and non-ms ABIs with the expectation of sane padding (e.g. llvm+clang) can 
diagnose the padding difference.

But for someone compiling under the ms abi that's presumably the intent so the 
warning becomes "you aren't packing these perfectly" (which might be something 
to subsume into a more usual packing warning?), but enabling this warning by 
default is basically going to mean we'll be making a warning along the lines of 
"warning: packing of these fields matches the expected platform packing" which 
seems less than great.

My long term goal is to allow us to switch to enum bitfields in llvm/clang 
because we'll have warnings mitigating the issues that apparently caused us to 
use unsigned instead of enums everywhere.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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

ojhunt wrote:

@rnk eep, thanks! I think the reason I switched to getQuantity was because 
diagprinter can't handle char units (I may just add that in a later patch as it 
seems like an obvious thing to support)

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -0,0 +1,180 @@
+
+// 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 MSVC 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 MSVC 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);
+

ojhunt wrote:

@rnk fun story: my first pass at this was in record layout and it involved 
layout out the record twice and was awful :D - it also reminded me that there 
are a bunch of warnings that we don't issue until the point of use rather than 
declaration which is irksome.

That said, while my original implementation was in record layout and was 
intended to be 100% accurate,  I realized that the actual issue is different 
type storage sizes resulting in radically different behavior with *seemingly 
unrelated* changes later on. So in cases where it matters we just want all 
bit-fields to be the same storage size, and the warning currently just drives 
"all bit-fields shall be the same storage size" which is the safe option. 
Pushing it to actual accurate warnings seems like it _might_ be less than 
ideal, as you get cases like this trivial change to the above false positive

```cpp
struct Foo {
  char a : 4;
  char b : 4;
  char c : 4;
  char d : 4;
  char e : 1; // +1 bit field
  short x : 6; // -1 bit width
  short y : 9;
};
```

which would now issue a warning saying that it is no longer packed on msvc, so 
the fix will be to go through and restructure/re-type (type system type, not 
keyboard) the entire structure to ensure everything has the same storage type, 
when it may have been easier to have it "correct" (from msvc point of view) 
from the start.

e.g the early false positive forces the code to be written in a way that avoids 
a real warning down the line that requires more changes. That's also why the 
warning text originally said "may" :D 

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

ojhunt wrote:

ah good point, I'll update msvc references (there are others in the docs) to 
say windows platform abi.

I don't believe this should be an on by default warning at all - it's a warning 
specifically about difference in packing/padding on a platform you aren't 
targeting. In the overwhelming majority of cases that is not something that 
matters. For folk actually targeting the windows abi, the existing warnings 
will fire (because the sad packing/padding would actually occur).

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -633,6 +633,50 @@ 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 MSVC ABI adjacemt 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 MSVC 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 `preferred_type` attribute can be used 
to
+convey the actual field type to debuggers and other tooling.

ojhunt wrote:

Took a while to work out how, but done

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

ojhunt wrote:

What's the appropriate way to do that? (I don't know how DefaultIgnore and 
similar interact - or are there other options where the semantics change on 
compilation mode that I can look at)

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

@@ -6519,6 +6519,13 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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">,

AaronBallman wrote:

SGTM!

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

@@ -6519,6 +6519,13 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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">,

AaronBallman wrote:

Thanks for the explanation!

Would you recommend we go with `Windows ABI` and leave the `Microsoft` off? Or 
should we use `Microsoft Windows ABI`? I mostly want to avoid `MS`.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

@@ -6519,6 +6519,13 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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">,

rnk wrote:

In the codebase, we tend to use "Windows" to cover behaviors that need to be 
shared with GCC and MinGW, and "Microsoft" to cover behaviors that are unique 
to MSVC. This tends to boil down to C vs. C++. If it's part of the C ABI, it's 
a Windows behavior, under the rationale that any C compiler for the platform 
would need to match this behavior, like [enums always using a fixed type of 
`int`](https://godbolt.org/z/P41YYGEah) rather than expanding to `long long` if 
the members require it. C++ behaviors are high cost and "optional".

This mostly reflects the actual coding logic required, which is to check 
whether the OS is Windows, or whether the "environment" is Microsoft, so 
perhaps this is not the best language to communicate with users

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> > > > So I take it we decided not to enable it by default in 
> > > > `-fms-compatibility` mode then?
> > > 
> > > 
> > > I don't believe it is appropriate to do so. The intent of this warning is 
> > > to indicate MSVC compatibility issues when building in 
> > > non-ms-compatibility modes. The existing padding warnings already trigger 
> > > for these layouts in the relevant ms compatibility modes
> > 
> > 
> > We don't typically add new off-by-default warnings though; users don't 
> > enable them often enough to be worth the costs. My thought process is: if 
> > we enable this diagnostic by default in `-fms-compatibility` mode as 
> > telling the user their bit-fields aren't packing, then it's on by default 
> > for some configurations so it meets our bar for inclusion, and it helps us 
> > directly because we have Windows precommit (and post-commit) CI coverage 
> > for building Clang and LLVM. Users on other platforms can opt-in to the 
> > diagnostic if they want the diagnostics for compatibility reasons.
> 
> My concerns with attaching it to `-fms-compatibility` is that projects using 
> that and that care about padding already have the padding/packing warnings 
> enabled. For every other user we will be warning them that the padding 
> matches the abi they're targeting. The problem I'm wanting to address is not 
> "I am targeting the ms abi" it is "I am not targeting ms abi but want to be 
> told about packing/padding that would be different on ms platforms".
> 
> The intention would be to then enable the warning (maybe with a `-Werror=...` 
> once the build is clean :D ) for all llvm projects, not just the windows 
> targets - maybe with some evangelism so larger projects that have similar 
> issues can be made aware of the flag. Then we can start migrating away from 
> unending unsigned bit-fields and adopt enum bit-fields
> 
> > If we want to leave it off by default, then I wonder if we want to roll the 
> > functionality into `-Wpadded-bitfield` which already exists and is off by 
> > default.
> 
> The problem with this is that that people who do not care about windows/ms 
> abi will then be getting what to them are spurious warnings.

My opinion on this is evolving -- this morally is similar to all the padding 
warnings we have in that it's telling you "hey, this packing is suboptimal". 
Those warnings are all off-by-default because they're not identifying a logical 
mistake in your code; your code still *works* even though it may be possible to 
improve it. So I think leaving this as an off-by-default warning is reasonable.

And it is a distinct diagnostic from `-Wpadded-bitfield` in that it's 
identifying new problematic patterns which may not be problematic on all 
platforms, so it should get its own warning group. Maybe that warning group 
sits under `-Wpadded-bitfield`, but we should give users a way to say "I want 
to know about bitfield padding issues, but not ones specific to how Visual 
Studio does things".

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/7] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[via cfe-commits]

@@ -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) &&

cor3ntin wrote:

If this is not enabled by default, maybe we should check the diagnostic is not 
ignored to avoid unnecessary work (`DiagnosticsEngine::isIgnored`)

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/7] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/7] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Oliver Hunt via cfe-commits]

ojhunt wrote:

> > > So I take it we decided not to enable it by default in 
> > > `-fms-compatibility` mode then?
> > 
> > 
> > I don't believe it is appropriate to do so. The intent of this warning is 
> > to indicate MSVC compatibility issues when building in non-ms-compatibility 
> > modes. The existing padding warnings already trigger for these layouts in 
> > the relevant ms compatibility modes
> 
> We don't typically add new off-by-default warnings though; users don't enable 
> them often enough to be worth the costs. My thought process is: if we enable 
> this diagnostic by default in `-fms-compatibility` mode as telling the user 
> their bit-fields aren't packing, then it's on by default for some 
> configurations so it meets our bar for inclusion, and it helps us directly 
> because we have Windows precommit (and post-commit) CI coverage for building 
> Clang and LLVM. Users on other platforms can opt-in to the diagnostic if they 
> want the diagnostics for compatibility reasons.

My concerns with attaching it to `-fms-compatibility` is that projects using 
that and that care about padding already have the padding/packing warnings 
enabled. For every other user we will be warning them that the padding matches 
the abi they're targeting. The problem I'm wanting to address is not "I am 
targeting the ms abi" it is "I am not targeting ms abi but want to be told 
about packing/padding that would be different on ms platforms".

The intention would be to then enable the warning (maybe with a `-Werror=...` 
once the build is clean :D ) for all llvm projects, not just the windows 
targets - maybe with some evangelism so larger projects that have similar 
issues can be made aware of the flag. Then we can start migrating away from 
unending unsigned bit-fields and adopt enum bit-fields

> 
> If we want to leave it off by default, then I wonder if we want to roll the 
> functionality into `-Wpadded-bitfield` which already exists and is off by 
> default.

The problem with this is that that people who do not care about windows/ms abi 
will then be getting what to them are spurious warnings.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/6] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> > So I take it we decided not to enable it by default in `-fms-compatibility` 
> > mode then?
> 
> I don't believe it is appropriate to do so. The intent of this warning is to 
> indicate MSVC compatibility issues when building in non-ms-compatibility 
> modes. The existing padding warnings already trigger for these layouts in the 
> relevant ms compatibility modes

We don't typically add new off-by-default warnings though; users don't enable 
them often enough to be worth the costs. My thought process is: if we enable 
this diagnostic by default in `-fms-compatibility` mode as telling the user 
their bit-fields aren't packing, then it's on by default for some 
configurations so it meets our bar for inclusion, and it helps us directly 
because we have Windows precommit (and post-commit) CI coverage for building 
Clang and LLVM. Users on other platforms can opt-in to the diagnostic if they 
want the diagnostics for compatibility reasons.

If we want to leave it off by default, then I wonder if we want to roll the 
functionality into `-Wpadded-bitfield` which already exists and is off by 
default.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Martin Storsjö via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

mstorsjo wrote:

Yes; mingw also uses the MS bitfield packing logic. (Originally, for mingw 
targets, users would need to opt in to this with `-mms-bitfields`; GCC enabled 
this option by default for mingw targets in 4.7 and we followed suit a bit 
belatedly in Clang 11 - see e3fd9dc9734c5775dc6824d0a839702e8d43e7f6.)

As for whether this should be enabled by deault; I think it's entirely up to 
each individual project whether they want it as an early portability headsup 
that they're writing code which is behaving differently on Windows or not. 
People writing windows-exclusive code might not care, and they may want to 
intentionally keep their structs as they are to match some existing ABI.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/6] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Reid Kleckner via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

rnk wrote:

If we did do this, it would be a Windows-thing, not an MSVC-thing, since it 
affects mingw. cc @mstorsjo  for a mingw PoV

I don't think we should enable it by default until we have more confidence that 
there aren't false positives. I don't see that as a blocker to land it, though.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

@@ -0,0 +1,180 @@
+
+// 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 MSVC 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 MSVC 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);
+

rnk wrote:

Can we add a test for a case for what happens when there is no layout 
difference, so something like this:
```
struct Foo {
  char a : 4;
  char b : 4;
  char c : 4;
  char d : 4;
  short x : 7;
  short y : 9;
};
```

As written, this code will warn, but both compilers use the same / similar 
struct layouts ([link](https://godbolt.org/z/qf6ccq65f)). Does this represent a 
false positive, or should we encourage users to use a consistent bitfield width?

`llvm::Value` doesn't have this problem because it happens to alternate between 
small bitfields and small integers, but this seems like a category of false 
positive that is hard to identify.

The only reasonable way to overcome this limitation would be to move this 
warning into record layout, in which case we'd have to implement it twice, once 
in the MS and non-MS record layout code.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

@@ -631,6 +631,9 @@ Improvements to Clang's diagnostics
 
 - Clang now diagnoses dangling references for C++20's parenthesized aggregate 
initialization (#101957).
 
+- A new off-by-default warning ``-Wms-bitfield-compatibility`` has been added 
to alert to cases where bit-field

rnk wrote:

I wanted to bikeshed the name. Is this really a compatibility warning? It's a 
compatibility issue for someone trying to use C structs to do memory-mapped I/O 
or something, but for LLVM's application, it's supposed to be a warning about 
extra bitfield padding, suggesting a name like `-Wms-bitfield-padding` or 
`-Wms-bitfield-packing`.

It also doesn't catch all bitfield layout differences. There are cases like 
[this](https://godbolt.org/z/hvd8aY8ba). Do we care about that, or is it better 
to leave that to `-Wpadding`?
```
struct Foo {
  char a;
  int x : 7; // msvc aligns to 4, gcc doesn't
  int y : 9;
};
```

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

ojhunt wrote:

> So I take it we decided not to enable it by default in `-fms-compatibility` 
> mode then?

I don't believe it is appropriate to do so. The intent of this warning is to 
indicate MSVC compatibility issues when building in non-ms-compatibility modes. 
The existing padding warnings already trigger for these layouts in the relevant 
ms compatibility modes

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/5] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[via cfe-commits]

@@ -633,6 +633,50 @@ 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 MSVC ABI adjacemt 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 MSVC 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 `preferred_type` attribute can be used 
to
+convey the actual field type to debuggers and other tooling.

Sirraide wrote:

Can we link to the documentation of that attribute here maybe?

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

https://github.com/Sirraide edited 
https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

https://github.com/Sirraide commented:

So I take it we decided not to enable it by default in `-fms-compatibility` 
mode then?

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/4] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/4] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH 1/3] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Oliver Hunt via cfe-commits]

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 
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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };

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

[Oliver Hunt via cfe-commits]

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 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 };
+enu

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

[Dan Klishch via cfe-commits]

DanShaders wrote:

Just FYI, there's a similar diagnostic `-Wpadded-bitfield` that warns about any 
padding inserted into bitfields in Itanium mode. They are somehow emitted in 
the completely different parts of the layout code, so it doesn't show up in the 
diff at all.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Andrew Kaster via cfe-commits]

ADKaster wrote:

https://github.com/llvm/llvm-project/pull/70978, which split -Wpadded into 
-Wpadded and -Wpadded-bitfield, may be relevant here

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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">,

ojhunt wrote:

Thanks, I'm terrible at wording (I prodded discord specifically to get 
diagnostic wording)

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

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

ojhunt wrote:

I think I did this intentionally as I sometimes feel like explicit true/false 
returns are clearer, I'm happy with either Aaron or @Sirraide's suggestion but 
I want to confirm that others don't have my clarity concerns :D

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

@@ -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">;

rnk wrote:

Bike shedding: Should we drop "packing"? Note that this covers compatibility 
with GCC `-mms-bitfields` and structs carrying `__attribute__((ms_struct))`, so 
naming parallel to `-mms-bitfields` seems good.

I think you could make this warning much more discoverable if you add 
documentation for it in DiagnosticDocs.td, which will be rendered to 
https://clang.llvm.org/docs/DiagnosticsReference.html , which is highly 
searchable.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

@@ -19001,9 +19001,9 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, 
Decl *EnclosingDecl,
 
   // Verify that all the fields are okay.
   SmallVector RecFields;
-
+  std::optional PreviousField;

rnk wrote:

It doesn't seem idiomatic to use an optional pointer, when nullable pointers 
are built into the language. Consider helping to reduce our collective carbon 
footprint by saving an extra template instantiation today, and using nullptr 
instead. =D

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

@@ -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())

rnk wrote:

Is this better as `return FD->isBitField() && 
!FD->getType()->isDependentType();`?

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via cfe-commits]

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

rnk wrote:

`CharUnits` are 64-bit, so you've got an implicit truncation here, not that 
anyone would ever define an `i(1<<32)` type and use it in a bit field. However, 
I think it's cleaner and more straightforward to skip `getQuantity` and just 
compare the CharUnits directly for equality. I'm also curious if the diagnostic 
printer can handle CharUnits, maybe it can, but if not, that's only one extra 
getQuantity call.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

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

Sirraide wrote:

Honestly, I’d even do this:
```c++
auto IsNonDependentBitField = [](const FieldDecl *FD) {
  return FD->isBitField() && !FD->getType()->isDependentType();
};
```
(I would have loved to format that as suggestion, but github isn’t letting me 
because this is in a reply...)

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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">,

Sirraide wrote:

```suggestion
  "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">,
```
I think it’s a bit clearer this way.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

Sirraide wrote:

I was about to point this out too but you beat me to it ;Þ

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman commented:

Thank you for working on this! Please be sure to also add a release note to 
`clang/docs/ReleaseNotes.rst` so users know about the new diagnostic.

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

@@ -6418,6 +6418,12 @@ def warn_signed_bitfield_enum_conversion : Warning<
   InGroup, 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, DefaultIgnore;

AaronBallman wrote:

This seems like a warning we should probably enable by default when the user 
specifies `-fms-compatibility` or is compiling for an MSVC target. WDYT @rnk?

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman edited 
https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

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

AaronBallman wrote:

```suggestion
  return !FD->getType()->isDependentType();
```

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Oliver Hunt via cfe-commits]

ojhunt wrote:

Oh, my goal for this warning is to have us turn it on in the llvm+clang builds, 
so we can then move away from endless use of unsigned bitfields and just use 
enums as nature intended :D (cc @AaronBallman )

https://github.com/llvm/llvm-project/pull/117428
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[via cfe-commits]

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Oliver Hunt (ojhunt)


Changes

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.

---
Full diff: https://github.com/llvm/llvm-project/pull/117428.diff


4 Files Affected:

- (modified) clang/include/clang/Basic/DiagnosticGroups.td (+1) 
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+6) 
- (modified) clang/lib/Sema/SemaDecl.cpp (+25-2) 
- (added) clang/test/SemaCXX/ms_struct-bitfield-padding.cpp (+180) 


``diff
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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 }

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

[Oliver Hunt via cfe-commits]

https://github.com/ojhunt created 
https://github.com/llvm/llvm-project/pull/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.

>From 3e25d7ef2e223942298078dace8979905956d05c Mon Sep 17 00:00:00 2001
From: Oliver Hunt 
Date: Fri, 22 Nov 2024 17:53:24 +0100
Subject: [PATCH] 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, 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, 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 RecFields;
-
+  std::optional PreviousField;
   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.
@@ -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 00..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 : unsi