Skip to content

[Sema] Add check for bitfield assignments to integral types #69049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 18, 2023
Merged

[Sema] Add check for bitfield assignments to integral types #69049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ New Compiler Flags
the preprocessed text to the output. This can greatly reduce the size of the
preprocessed output, which can be helpful when trying to reduce a test case.

* ``-Wbitfield-conversion`` was added to detect assignments of integral
types to a bitfield that may change the value.

Deprecated Compiler Flags
-------------------------

Expand Down
2 changes: 2 additions & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ def SingleBitBitFieldConstantConversion :
def BitFieldConstantConversion : DiagGroup<"bitfield-constant-conversion",
[SingleBitBitFieldConstantConversion]>;
def BitFieldEnumConversion : DiagGroup<"bitfield-enum-conversion">;
def BitFieldConversion : DiagGroup<"bitfield-conversion">;
def BitFieldWidth : DiagGroup<"bitfield-width">;
def CompoundTokenSplitByMacro : DiagGroup<"compound-token-split-by-macro">;
def CompoundTokenSplitBySpace : DiagGroup<"compound-token-split-by-space">;
Expand Down Expand Up @@ -933,6 +934,7 @@ def Conversion : DiagGroup<"conversion",
ConstantConversion,
EnumConversion,
BitFieldEnumConversion,
BitFieldConversion,
FloatConversion,
Shorten64To32,
IntConversion,
Expand Down
3 changes: 3 additions & 0 deletions clang/include/clang/Basic/DiagnosticSemaKinds.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6171,6 +6171,9 @@ def warn_signed_bitfield_enum_conversion : Warning<
"signed bit-field %0 needs an extra bit to represent the largest positive "
"enumerators of %1">,
InGroup<BitFieldEnumConversion>, DefaultIgnore;
def warn_bitfield_too_small_for_integral_type : Warning<
"conversion from %2 (%3 bits) to bit-field %0 (%1 bits) may change value">,
InGroup<BitFieldConversion>, DefaultIgnore;
def note_change_bitfield_sign : Note<
"consider making the bitfield type %select{unsigned|signed}0">;

Expand Down
13 changes: 12 additions & 1 deletion clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14298,6 +14298,18 @@ static bool AnalyzeBitFieldAssignment(Sema &S, FieldDecl *Bitfield, Expr *Init,
S.Diag(WidthExpr->getExprLoc(), diag::note_widen_bitfield)
<< BitsNeeded << ED << WidthExpr->getSourceRange();
}
} else if (OriginalInit->getType()->isIntegralType(S.Context)) {
IntRange LikelySourceRange =
GetExprRange(S.Context, Init, S.isConstantEvaluatedContext(),
/*Approximate=*/true);

if (LikelySourceRange.Width > FieldWidth) {
Expr *WidthExpr = Bitfield->getBitWidth();
S.Diag(InitLoc, diag::warn_bitfield_too_small_for_integral_type)
<< Bitfield << FieldWidth << OriginalInit->getType()
<< LikelySourceRange.Width;
S.Diag(WidthExpr->getExprLoc(), diag::note_declared_at);
}
}

return false;
Expand Down Expand Up @@ -15195,7 +15207,6 @@ static void CheckImplicitConversion(Sema &S, Expr *E, QualType T,

if (LikelySourceRange.Width > TargetRange.Width) {
// If the source is a constant, use a default-on diagnostic.
// TODO: this should happen for bitfield stores, too.
Expr::EvalResult Result;
if (E->EvaluateAsInt(Result, S.Context, Expr::SE_AllowSideEffects,
S.isConstantEvaluatedContext())) {
Expand Down
42 changes: 42 additions & 0 deletions clang/test/SemaCXX/bitfield-width.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// RUN: %clang_cc1 -Wconversion -fsyntax-only -verify %s
// RUN: %clang_cc1 -Wbitfield-conversion -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple armebv7-unknown-linux -Wbitfield-conversion \
// RUN: -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple arm64-unknown-linux -Wbitfield-conversion \
// RUN: -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple arm-unknown-linux -Wbitfield-conversion \
// RUN: -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple aarch64-unknown-linux -Wbitfield-conversion \
// RUN: -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple mipsel-unknown-linux -Wbitfield-conversion \
// RUN: -fsyntax-only -verify %s
// RUN: %clang_cc1 -triple mips64el-unknown-linux -Wbitfield-conversion \
// RUN: -fsyntax-only -verify %s

typedef struct _xx {
int bf:9; // expected-note 3{{declared here}}
} xx, *pxx;

xx vxx;

void foo1(int x) {
vxx.bf = x; // expected-warning{{conversion from 'int' (32 bits) to bit-field 'bf' (9 bits) may change value}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will fire on the code pattern that I have been recommending in Clang to detect bitfield truncation:

  int value : 6;
  void setBitfield(int v) {
    value = v;
    assert(value == v && "truncation");
  }

Example:

llvm-project/clang/include/clang/AST/Decl.h

Lines 1901 to 1902 in c35939b

ParmVarDeclBits.ParameterIndex = parameterIndex;
assert(ParmVarDeclBits.ParameterIndex == parameterIndex && "truncation!");

This isn't appropriate for all projects because not everyone uses assertions, but what's nice about it is that you don't have to do the truncation twice: the compiler figures out the bitwidth and truncates appropriately, so the developer doesn't have to duplicate information or create an extra enum to track the bitwidth to compute a mask. The assert can't really inform the warning because they are pre-processed away in release builds, but I wonder if we could detect equality comparisons between the assigned bitfield and the RHS of the assignment to suppress the warning. On second thought, that seems infeasible.

Without a better code pattern to recommend for developers, I'm worried that most of our users are going to turn this warning off. Can anyone more creative than me come up with a better recommended code pattern to suppress the warning?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm certainly not opposed to reducing the chattiness if there's an idea on how to do so. However, this is an off-by-default warning grouped under -Wconversion with its own diagnostic group (-Wbitfield-conversion) specifically to allow people to opt into conversion warnings while opting out of this new class of diagnostics. Is that insufficient?

Also, this is diagnosing code that GCC also diagnoses under -Wconversion.

https://godbolt.org/z/qYE8Kzr7f

}
void foo2(short x) {
vxx.bf = x; // expected-warning{{conversion from 'short' (16 bits) to bit-field 'bf' (9 bits) may change value}}
}
void foo3(char x) {
vxx.bf = x; // no warning expected
}
void foo4(short x) {
vxx.bf = 0xff & x; // no warning expected
}
void foo5(short x) {
vxx.bf = 0x1ff & x; // no warning expected
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really non-ergonomic code pattern. Are we sure we can't come up with a better recommended code pattern for detecting and handling bitfield truncation?

}
void foo6(short x) {
vxx.bf = 0x3ff & x; // expected-warning{{conversion from 'int' (10 bits) to bit-field 'bf' (9 bits) may change value}}
}
int fee(void) {
return 0;
}