[Sema] Add check for bitfield assignments to integral types #69049
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change introduces the bitfield conversion check after fixes to the test case. The original submission unfortunately did not comprehend differences in architecture for the diagnostic messages, leading to unanticipated failures in the arm build bots. Original PR: llvm#68276 Clang does not check for bitfield assignment widths, while gcc checks 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.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (vabridgers) ChangesThis change introduces the bitfield conversion check after fixes to the test case. The original submission unfortunately did not comprehend differences in architecture for the diagnostic messages, leading to unanticipated failures in the arm build bots. Original PR: #68276 Clang does not check for bitfield assignment widths, while gcc checks this. gcc produced a warning like so for it's -Wconversion flag: This change simply adds this check for integral types under the -Wbitfield-conversion compiler option. Full diff: https://github.com/llvm/llvm-project/pull/69049.diff 5 Files Affected:
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 2d918967e7f0b02..31969201a1cac8c 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -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
-------------------------
diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
index 0b09c002191848a..674eb9f4ef2e73f 100644
--- a/clang/include/clang/Basic/DiagnosticGroups.td
+++ b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -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">;
@@ -933,6 +934,7 @@ def Conversion : DiagGroup<"conversion",
ConstantConversion,
EnumConversion,
BitFieldEnumConversion,
+ BitFieldConversion,
FloatConversion,
Shorten64To32,
IntConversion,
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c1a6e3831127e56..ab7fe881976aad2 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -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">;
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 35b36db2049db09..cd61459cfbb13d6 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -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;
@@ -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())) {
diff --git a/clang/test/SemaCXX/bitfield-width.c b/clang/test/SemaCXX/bitfield-width.c
new file mode 100644
index 000000000000000..7b4e4444c245b0e
--- /dev/null
+++ b/clang/test/SemaCXX/bitfield-width.c
@@ -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}}
+ }
+ 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
+ }
+ 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;
+ }
|
|
Hi @AaronBallman , please let me know if you're ok with me merging this PR. This is a refinement of PR #68276. Thanks! |
AaronBallman
approved these changes
Oct 18, 2023
There was a problem hiding this comment.
LGTM! Btw, if you're just repairing tests like you had to do here, you generally are okay to not go through another round of review unless you want an extra set of eyes on things. The original approval is fine so long as you're not making substantial changes to the code.
Thanks @AaronBallman ! |
rnk
reviewed
Oct 19, 2023
| vxx.bf = 0xff & x; // no warning expected | ||
| } | ||
| void foo5(short x) { | ||
| vxx.bf = 0x1ff & x; // no warning expected |
There was a problem hiding this comment.
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?
| 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}} |
There was a problem hiding this comment.
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
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?
There was a problem hiding this comment.
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.
|
hi folks! this seems to be triggering crashes on some valid code, e.g: a.cc: |
kadircet
added a commit
that referenced
this pull request
Oct 23, 2023
…69049)" This reverts commit 708808e which is causing crashes on valid code, see #69049 (comment).
|
It looks like these changes surfaced an existing bug with llvm-project/clang/lib/Sema/SemaChecking.cpp Line 13734 in 4554eac
llvm-project/clang/lib/AST/Expr.cpp Line 3919 in 4554eac Note: I did not investigate whether the source expression should be null in this case, just verified that it can be and is in this code example. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change introduces the bitfield conversion check after fixes to the test case. The original submission unfortunately did not comprehend differences in architecture for the diagnostic messages, leading to unanticipated failures in the arm build bots.
Original PR: #68276
Clang does not check for bitfield assignment widths, while gcc checks this.
gcc produced a warning like so for it's -Wconversion flag:
This change simply adds this check for integral types under the -Wbitfield-conversion compiler option.