Skip to content

Commit

Permalink
[Sema] Add check for bitfield assignments to larger integral types (#…
Browse files Browse the repository at this point in the history
…68276)

We noticed that clang does not check for bitfield assignment widths,
while gcc does check this.

gcc produced a warning like so for it's -Wconversion flag:
```
$ gcc -Wconversion -c test.c
test.c: In function 'foo':
test.c:10:15: warning: conversion from 'int' to 'signed char:7' may change value [-Wconversion]
   10 |      vxx.bf = x; // no warning
      |               ^
```

This change simply adds this check for integral types under the
-Wbitfield-conversion compiler option.
  • Loading branch information
vabridgers committed Oct 12, 2023
1 parent 4c6cba3 commit dd0f642
Show file tree
Hide file tree
Showing 5 changed files with 54 additions and 1 deletion.
3 changes: 3 additions & 0 deletions clang/docs/ReleaseNotes.rst
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
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
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
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
34 changes: 34 additions & 0 deletions clang/test/SemaCXX/bitfield-width.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// RUN: %clang_cc1 -Wconversion -fsyntax-only -verify %s
// RUN: %clang_cc1 -Wbitfield-conversion -fsyntax-only -verify %s

typedef struct _xx {
int bf:9; // expected-note 4{{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}}
}
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 foo5(void * x) {
vxx.bf = (int)x; // expected-warning{{cast to smaller integer type 'int' from 'void *'}}
// expected-warning@-1{{conversion from 'int' (32 bits) to bit-field 'bf' (9 bits) may change value}}
}

This comment has been minimized.

Copy link
@DavidSpickett

DavidSpickett Oct 13, 2023

Collaborator

This test line fails on a 32 bit system, https://lab.llvm.org/buildbot/#/builders/245/builds/15263. As sizeof(void*) == sizeof(int).

If I get time today I'll see if I can fix it.

This comment has been minimized.

Copy link
@DavidSpickett

DavidSpickett Oct 13, 2023

Collaborator

Sounds like someone else has taken care of it.

void foo6(short x) {
vxx.bf = 0xff & x; // no warning expected
}
void foo7(short x) {
vxx.bf = 0x1ff & x; // no warning expected
}
void foo8(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;
}

0 comments on commit dd0f642

Please sign in to comment.