Skip to content

Commit

Permalink
[clang] Separate bit-field padding diagnostics into -Wpadded-bitfield (
Browse files Browse the repository at this point in the history
…#70978)

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.
  • Loading branch information
DanShaders committed Nov 2, 2023
1 parent 8f59c16 commit 43feb3e
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 12 deletions.
12 changes: 11 additions & 1 deletion clang/include/clang/Basic/DiagnosticASTKinds.td
Original file line number Diff line number Diff line change
Expand Up @@ -998,14 +998,24 @@ 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 "
"%select{byte|bit}3%s2 to align %4">,
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">,
Expand Down
3 changes: 2 additions & 1 deletion clang/include/clang/Basic/DiagnosticGroups.td
Original file line number Diff line number Diff line change
Expand Up @@ -585,7 +585,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">;
Expand Down
19 changes: 11 additions & 8 deletions clang/lib/AST/RecordLayoutBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand Down
41 changes: 41 additions & 0 deletions clang/test/CodeGenCXX/warn-padded-bitfields.cpp
Original file line number Diff line number Diff line change
@@ -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){}

0 comments on commit 43feb3e

Please sign in to comment.