[clang] Add an off-by-default warning to complain about MSVC bitfield padding (PR #117428)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Nov 23 03:01:56 PST 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-clang
Author: Oliver Hunt (ojhunt)
<details>
<summary>Changes</summary>
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<BitFieldEnumConversion>, DefaultIgnore;
def note_change_bitfield_sign : Note<
"consider making the bitfield type %select{unsigned|signed}0">;
+def warn_ms_bitfield_mismatched_storage_packing : Warning<
+ "bit-field %0 of type %1 has a different storage size (%2 vs %3 bytes) than the "
+ "preceding bit-field and may not be packed under MSVC ABI">,
+ InGroup<MSBitfieldCompatibility>, DefaultIgnore;
+def note_ms_bitfield_mismatched_storage_size_previous : Note<
+ "preceding bit-field %0 declared here with type %1">;
def warn_missing_braces : Warning<
"suggest braces around initialization of subobject">,
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 74b0e5ad23bd48..18aeca3b34f659 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -19001,9 +19001,9 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
// Verify that all the fields are okay.
SmallVector<FieldDecl*, 32> RecFields;
-
+ std::optional<const FieldDecl *> PreviousField;
for (ArrayRef<Decl *>::iterator i = Fields.begin(), end = Fields.end();
- i != end; ++i) {
+ i != end; PreviousField = cast<FieldDecl>(*i), ++i) {
FieldDecl *FD = cast<FieldDecl>(*i);
// Get the type for the field.
@@ -19213,6 +19213,29 @@ void Sema::ActOnFields(Scope *S, SourceLocation RecLoc, Decl *EnclosingDecl,
if (Record && FD->getType().isVolatileQualified())
Record->setHasVolatileMember(true);
+ auto IsNonDependentBitField = [](const FieldDecl *FD) {
+ if (!FD->isBitField())
+ return false;
+ if (FD->getType()->isDependentType())
+ return false;
+ return true;
+ };
+
+ if (Record && PreviousField && IsNonDependentBitField(FD) &&
+ IsNonDependentBitField(*PreviousField)) {
+ unsigned FDStorageSize =
+ Context.getTypeSizeInChars(FD->getType()).getQuantity();
+ unsigned PreviousFieldStorageSize =
+ Context.getTypeSizeInChars((*PreviousField)->getType()).getQuantity();
+ if (FDStorageSize != PreviousFieldStorageSize) {
+ Diag(FD->getLocation(),
+ diag::warn_ms_bitfield_mismatched_storage_packing)
+ << FD << FD->getType() << FDStorageSize << PreviousFieldStorageSize;
+ Diag((*PreviousField)->getLocation(),
+ diag::note_ms_bitfield_mismatched_storage_size_previous)
+ << *PreviousField << (*PreviousField)->getType();
+ }
+ }
// Keep track of the number of named members.
if (FD->getIdentifier())
++NumNamedMembers;
diff --git a/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
new file mode 100644
index 00000000000000..001086de5bbd10
--- /dev/null
+++ b/clang/test/SemaCXX/ms_struct-bitfield-padding.cpp
@@ -0,0 +1,180 @@
+
+// RUN: %clang_cc1 -fsyntax-only -Wms-bitfield-packing-compatibility -verify -triple armv8 -std=c++23 %s
+// RUN: %clang_cc1 -fsyntax-only -DMS_BITFIELDS -mms-bitfields -verify=msbitfields -triple armv8-apple-macos10.15 -std=c++23 %s
+
+// msbitfields-no-diagnostics
+
+enum Enum1 { Enum1_A, Enum1_B };
+enum Enum2 { Enum2_A, Enum2_B };
+
+enum class EnumU32_1 : unsigned { A, B };
+enum class EnumU32_2 : unsigned { A, B };
+enum class EnumU64 : unsigned long long { A, B };
+enum class EnumI32 : int { A, B };
+enum class EnumU8 : unsigned char { A, B };
+enum class EnumI8 : char { A, B };
+enum class EnumU16 : unsigned short { A, B };
+enum class EnumI16 : short { A, B };
+
+struct A {
+ unsigned int a : 15;
+ unsigned int b : 15;
+};
+static_assert(sizeof(A) == 4);
+
+struct B {
+ unsigned int a : 15;
+ int b : 15;
+};
+static_assert(sizeof(B) == 4);
+
+struct C {
+ unsigned int a : 15;
+ int b : 15;
+};
+static_assert(sizeof(C) == 4);
+
+struct D {
+ Enum1 a : 15;
+ Enum1 b : 15;
+};
+static_assert(sizeof(D) == 4);
+
+struct E {
+ Enum1 a : 15;
+ Enum2 b : 15;
+};
+static_assert(sizeof(E) == 4);
+
+struct F {
+ EnumU32_1 a : 15;
+ EnumU32_2 b : 15;
+};
+static_assert(sizeof(F) == 4);
+
+struct G {
+ EnumU32_1 a : 15;
+ EnumU64 b : 15;
+ // expected-warning at -1 {{bit-field 'b' of type 'EnumU64' has a different storage size (8 vs 4 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
+};
+
+#ifdef MS_BITFIELDS
+ static_assert(sizeof(G) == 16);
+#else
+ static_assert(sizeof(G) == 8);
+#endif
+
+struct H {
+ EnumU32_1 a : 10;
+ EnumI32 b : 10;
+ EnumU32_1 c : 10;
+};
+static_assert(sizeof(H) == 4);
+
+struct I {
+ EnumU8 a : 3;
+ EnumI8 b : 5;
+ EnumU32_1 c : 10;
+ // expected-warning at -1 {{bit-field 'c' of type 'EnumU32_1' has a different storage size (4 vs 1 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'b' declared here with type 'EnumI8'}}
+};
+#ifdef MS_BITFIELDS
+static_assert(sizeof(I) == 8);
+#else
+static_assert(sizeof(I) == 4);
+#endif
+
+struct J {
+ EnumU8 : 0;
+ EnumU8 b : 4;
+};
+static_assert(sizeof(J) == 1);
+
+struct K {
+ EnumU8 a : 4;
+ EnumU8 : 0;
+};
+static_assert(sizeof(K) == 1);
+
+struct L {
+ EnumU32_1 a : 10;
+ EnumU32_2 b : 10;
+ EnumU32_1 c : 10;
+};
+
+static_assert(sizeof(L) == 4);
+
+struct M {
+ EnumU32_1 a : 10;
+ EnumI32 b : 10;
+ EnumU32_1 c : 10;
+};
+
+static_assert(sizeof(M) == 4);
+
+struct N {
+ EnumU32_1 a : 10;
+ EnumU64 b : 10;
+ // expected-warning at -1 {{bit-field 'b' of type 'EnumU64' has a different storage size (8 vs 4 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
+ EnumU32_1 c : 10;
+ // expected-warning at -1 {{bit-field 'c' of type 'EnumU32_1' has a different storage size (4 vs 8 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -5 {{preceding bit-field 'b' declared here with type 'EnumU64'}}
+};
+
+#ifdef MS_BITFIELDS
+static_assert(sizeof(N) == 24);
+#else
+static_assert(sizeof(N) == 8);
+#endif
+
+struct O {
+ EnumU16 a : 10;
+ EnumU32_1 b : 10;
+ // expected-warning at -1 {{bit-field 'b' of type 'EnumU32_1' has a different storage size (4 vs 2 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU16'}}
+};
+#ifdef MS_BITFIELDS
+static_assert(sizeof(O) == 8);
+#else
+static_assert(sizeof(O) == 4);
+#endif
+
+struct P {
+ EnumU32_1 a : 10;
+ EnumU16 b : 10;
+ // expected-warning at -1 {{bit-field 'b' of type 'EnumU16' has a different storage size (2 vs 4 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU32_1'}}
+};
+#ifdef MS_BITFIELDS
+static_assert(sizeof(P) == 8);
+#else
+static_assert(sizeof(P) == 4);
+#endif
+
+struct Q {
+ EnumU8 a : 6;
+ EnumU16 b : 6;
+ // expected-warning at -1 {{bit-field 'b' of type 'EnumU16' has a different storage size (2 vs 1 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'a' declared here with type 'EnumU8'}}
+};
+#ifdef MS_BITFIELDS
+static_assert(sizeof(Q) == 4);
+#else
+static_assert(sizeof(Q) == 2);
+#endif
+
+struct R {
+ EnumU16 a : 9;
+ EnumU16 b : 9;
+ EnumU8 c : 6;
+ // expected-warning at -1 {{bit-field 'c' of type 'EnumU8' has a different storage size (1 vs 2 bytes) than the preceding bit-field and may not be packed under MSVC ABI}}
+ // expected-note at -3 {{preceding bit-field 'b' declared here with type 'EnumU16'}}
+};
+
+#ifdef MS_BITFIELDS
+static_assert(sizeof(R) == 6);
+#else
+static_assert(sizeof(R) == 4);
+#endif
``````````
</details>
https://github.com/llvm/llvm-project/pull/117428
More information about the cfe-commits
mailing list