[clang] [clang] Separate bit-field padding diagnostics into -Wpadded-bitfield (PR #70978)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 1 19:01:01 PDT 2023
https://github.com/DanShaders updated https://github.com/llvm/llvm-project/pull/70978
>From 7906a4689d44a64ead5259f947049c862f6bf7e6 Mon Sep 17 00:00:00 2001
From: Dan Klishch <danilklishch at gmail.com>
Date: Wed, 1 Nov 2023 21:59:21 -0400
Subject: [PATCH] [clang] Separate bit-field padding diagnostics into
-Wpadded-bitfield
-Wpadded diagnostics on usual struct fields are generally really noisy
and are of interest only for memory microoptimizations. However, this is
not the case for bit-fields. If one uses a bit-field, they most likely
intuitively expect the adjacent struct fields to be packed.
Unfortunately, in reality, layout of the bit-fields follows extremely
complicated, target-depended rules, which do not always result in the
aforementioned intuitive behavior.
The motivating example for this change was the following code:
```
union DoubleExtractor {
double value;
struct {
unsigned long long mantissa : 52;
unsigned exponent : 11;
unsigned sign : 1;
}
};
```
which was used to extract parts of double-precision floating point
number memory representation. The snippet silently broke when compiled
for x86_64-pc-windows-msvc, since according to Microsoft ABI there
should be 12 bit padding between `mantissa` and `sign`.
Note however that this commit only alters `ItaniumRecordLayoutBuilder`,
so there will be no behavioral change for x86_64-pc-windows-msvc target.
The patch also revealed a bug in -Wpadded diagnostic text, where we
assumed that if it triggers for an anonymous field, that field must be a
bit-field.
---
.../include/clang/Basic/DiagnosticASTKinds.td | 12 +++++-
clang/include/clang/Basic/DiagnosticGroups.td | 3 +-
clang/lib/AST/RecordLayoutBuilder.cpp | 19 +++++----
.../warn-all-padded-packed-packed-non-pod.cpp | 4 +-
.../test/CodeGenCXX/warn-padded-bitfields.cpp | 41 +++++++++++++++++++
5 files changed, 67 insertions(+), 12 deletions(-)
create mode 100644 clang/test/CodeGenCXX/warn-padded-bitfields.cpp
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
index 031117f2c4137a4..c81d17ed641084a 100644
--- a/clang/include/clang/Basic/DiagnosticASTKinds.td
+++ b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -998,6 +998,16 @@ def warn_npot_ms_struct : Warning<
"data types with sizes that aren't a power of two">,
DefaultError, InGroup<IncompatibleMSStruct>;
+// -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<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 "
@@ -1005,7 +1015,7 @@ def warn_padded_struct_field : Warning<
InGroup<Padded>, DefaultIgnore;
def warn_padded_struct_anon_field : Warning<
"padding %select{struct|interface|class}0 %1 with %2 "
- "%select{byte|bit}3%s2 to align anonymous bit-field">,
+ "%select{byte|bit}3%s2 to align anonymous field">,
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..5d4f930fca50e2b 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2297,19 +2297,22 @@ 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 {
+ auto Diagnostic = D->isBitField() ? diag::warn_padded_struct_anon_bitfield
+ : diag::warn_padded_struct_anon_field;
+ Diag(D->getLocation(), Diagnostic)
<< 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..f9882d03564fe6e
--- /dev/null
+++ b/clang/test/CodeGenCXX/warn-padded-bitfields.cpp
@@ -0,0 +1,41 @@
+// 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;
+};
+
+struct S6 {
+ unsigned a : 1;
+ unsigned long long b;
+};
+
+struct S7 {
+ int word;
+ struct {
+ int filler __attribute__ ((aligned (8)));
+ };
+};
+
+// 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, S6, S7){}
More information about the cfe-commits
mailing list