[clang] Separate bit-field padding diagnostics into -Wpadded-bitfield

[DanShaders]

The -Wpadded diagnostics are usually very noisy and generally not helpful. However, reporting padding that was introduced in bit-fields is rather helpful. For example, yesterday in SerenityOS's discord we had very unpleasant experience of debugging Windows portability issue, and its root cause was that under x86_64-pc-windows-msvc a padding was introduced for one of the bit-fields.

So, this PR separates bit-field-related padding diagnostics into a new -Wpadded-bitfield. The diagnostic group is also enabled by -Wpadded for compatibility reasons.

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (DanShaders)

Changes

The -Wpadded diagnostics are usually very noisy and generally not helpful. However, reporting padding that was introduced in bit-fields is rather helpful. For example, yesterday in SerenityOS's discord we had very unpleasant experience of debugging Windows portability issue, and its root cause was that under x86_64-pc-windows-msvc a padding was introduced for one of the bit-fields.

So, this PR separates bit-field-related padding diagnostics into a new -Wpadded-bitfield. The diagnostic group is also enabled by -Wpadded for compatibility reasons.

---

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

5 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticASTKinds.td (+10-4)
• (modified) clang/include/clang/Basic/DiagnosticGroups.td (+2-1)
• (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+12-8)
• (modified) clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp (+2-2)
• (added) clang/test/CodeGenCXX/warn-padded-bitfields.cpp (+29)

diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index 031117f2c4137a4..94d1907bce6d044 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -998,14 +998,20 @@ def warn_npot_ms_struct : Warning<
   "data types with sizes that aren't a power of two">,
   DefaultError, InGroup<IncompatibleMSStruct>;
 
-// -Wpadded, -Wpacked
-def warn_padded_struct_field : Warning<
+// -Wpadded-bitfield
+def warn_padded_struct_bitfield : Warning<
   "padding %select{struct|interface|class}0 %1 with %2 "
   "%select{byte|bit}3%s2 to align %4">,
-  InGroup<Padded>, DefaultIgnore;
-def warn_padded_struct_anon_field : Warning<
+  InGroup<PaddedBitField>, DefaultIgnore;
+def warn_padded_struct_anon_bitfield : Warning<
   "padding %select{struct|interface|class}0 %1 with %2 "
   "%select{byte|bit}3%s2 to align anonymous bit-field">,
+  InGroup<PaddedBitField>, DefaultIgnore;
+
+// -Wpadded, -Wpacked
+def warn_padded_struct_field : Warning<
+  "padding %select{struct|interface|class}0 %1 with %2 "
+  "%select{byte|bit}3%s2 to align %4">,
   InGroup<Padded>, DefaultIgnore;
 def warn_padded_struct_size : Warning<
   "padding size of %0 with %1 %select{byte|bit}2%s1 to alignment boundary">,
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 9a8f3f03b39d165..bfda89945d635bd 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -586,7 +586,8 @@ def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def OrderedCompareFunctionPointers : DiagGroup<"ordered-compare-function-pointers">;
 def PackedNonPod : DiagGroup<"packed-non-pod">;
 def Packed : DiagGroup<"packed", [PackedNonPod]>;
-def Padded : DiagGroup<"padded">;
+def PaddedBitField : DiagGroup<"padded-bitfield">;
+def Padded : DiagGroup<"padded", [PaddedBitField]>;
 def UnalignedAccess : DiagGroup<"unaligned-access">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index f1f2275da44dcad..982266488d488e2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2297,19 +2297,23 @@ void ItaniumRecordLayoutBuilder::CheckFieldPadding(
       PadSize = PadSize / CharBitNum;
       InBits = false;
     }
-    if (D->getIdentifier())
-      Diag(D->getLocation(), diag::warn_padded_struct_field)
+    if (D->getIdentifier()) {
+      auto Diagnostic = D->isBitField() ? diag::warn_padded_struct_bitfield
+                                        : diag::warn_padded_struct_field;
+      Diag(D->getLocation(), Diagnostic)
           << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
-          << Context.getTypeDeclType(D->getParent())
-          << PadSize
+          << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0) // (byte|bit)
           << D->getIdentifier();
-    else
-      Diag(D->getLocation(), diag::warn_padded_struct_anon_field)
+    } else {
+      assert(
+          D->isBitField() &&
+          "Introduced padding for an anonymous field which is not a bit-field");
+      Diag(D->getLocation(), diag::warn_padded_struct_anon_bitfield)
           << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
-          << Context.getTypeDeclType(D->getParent())
-          << PadSize
+          << Context.getTypeDeclType(D->getParent()) << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
+    }
  }
  if (isPacked && Offset != UnpackedOffset) {
    HasPackedField = true;
diff --git a/clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp b/clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp
index 2a75498d87197a4..5e166deba0a3f05 100644
--- a/clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp
+++ b/clang/test/CodeGenCXX/warn-all-padded-packed-packed-non-pod.cpp
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,top %s -emit-llvm-only
-// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -Wno-padded-bitfield -verify=expected,top %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -Wno-padded-bitfield -verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
 // -Wpacked-non-pod itself should not emit the "packed attribute is unnecessary" warnings.
 // RUN: %clang_cc1 -triple=x86_64-none-none -Wpacked-non-pod -verify=top %s -emit-llvm-only
 // -Wall should not emit the "packed attribute is unnecessary" warnings without -Wpacked.
diff --git a/clang/test/CodeGenCXX/warn-padded-bitfields.cpp b/clang/test/CodeGenCXX/warn-padded-bitfields.cpp
new file mode 100644
index 000000000000000..1206346ebfbc2c2
--- /dev/null
+++ b/clang/test/CodeGenCXX/warn-padded-bitfields.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded-bitfield -verify=expected %s -emit-llvm-only
+
+struct S1 {
+  unsigned a : 1;
+  unsigned long long : 0; // expected-warning {{padding struct 'S1' with 63 bits to align anonymous bit-field}}
+};
+
+struct S2 {
+  unsigned a : 1;
+  unsigned long long b : 64; // expected-warning {{padding struct 'S2' with 63 bits to align 'b'}}
+};
+
+struct S3 {
+  char a : 1;
+  short b : 16; // expected-warning {{padding struct 'S3' with 15 bits to align 'b'}}
+};
+
+struct [[gnu::packed]] S4 {
+  char a : 1;
+  short b : 16;
+};
+
+struct S5 {
+  unsigned a : 1;
+  unsigned long long b : 63;
+};
+
+// The warnings are emitted when the layout of the structs is computed, so we have to use them.
+void f(S1, S2, S3, S4, S5){}

[nico]

lgtm