[Sema] Add check for bitfield assignments to larger integral types

[vabridgers]

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 -Wconversion compiler option.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

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 option is disabled by default, and is enabled by usign the -Wbitfield-width option.

---

Full diff: https://github.com/llvm/llvm-project/pull/68276.diff

3 Files Affected:

• (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+3)
• (modified) clang/lib/Sema/SemaChecking.cpp (+10)
• (added) clang/test/SemaCXX/bitfield-width.c (+30)

diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index e2eaeb885e2ea77..9ffb7baacac22d7 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -6165,6 +6165,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<
+  "bit-field %0 is not wide enough to store type of %1 ">,
+  InGroup<BitFieldWidth>, 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 3b0cc154c2e46ab..eb567bc4935e768 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -14300,6 +14300,16 @@ 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)) {
+      TypeInfo TI = S.Context.getTypeInfo(OriginalInit->getType());
+      if (TI.Width > FieldWidth) {
+        Expr *WidthExpr = Bitfield->getBitWidth();
+        S.Diag(InitLoc, diag::warn_bitfield_too_small_for_integral_type)
+            << Bitfield << OriginalInit->getType();
+        S.Diag(WidthExpr->getExprLoc(), diag::note_widen_bitfield)
+            << TI.Width << OriginalInit->getType()
+            << WidthExpr->getSourceRange();
+      }
     }
 
     return false;
diff --git a/clang/test/SemaCXX/bitfield-width.c b/clang/test/SemaCXX/bitfield-width.c
new file mode 100644
index 000000000000000..69aa2e229869906
--- /dev/null
+++ b/clang/test/SemaCXX/bitfield-width.c
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -Wbitfield-width -fsyntax-only -verify %s
+
+typedef struct _xx {
+     int bf:9; // expected-note{{widen this field to 32 bits to store all values of 'int'}}
+     // expected-note@-1{{widen this field to 16 bits to store all values of 'short'}}
+     // expected-note@-2{{widen this field to 64 bits to store all values of 'long'}}
+     // expected-note@-3{{widen this field to 32 bits to store all values of 'int'}}
+ } xx, *pxx; 
+
+ xx vxx;
+
+ void foo1(int x) {     
+     vxx.bf = x; // expected-warning{{bit-field 'bf' is not wide enough to store type of 'int'}}
+ } 
+ void foo2(short x) {     
+     vxx.bf = x; // expected-warning{{bit-field 'bf' is not wide enough to store type of 'short'}}
+ } 
+ void foo3(char x) {     
+     vxx.bf = x;
+ } 
+ void foo4(long x) {     
+     vxx.bf = x; // expected-warning{{bit-field 'bf' is not wide enough to store type of 'long'}}
+ } 
+ void foo5(void * x) {     
+     vxx.bf = (int)x; // expected-warning{{cast to smaller integer type 'int' from 'void *'}}
+     // expected-warning@-1{{bit-field 'bf' is not wide enough to store type of 'int'}}
+ } 
+ int fee(void) {
+     return 0;
+ }

[vabridgers]

I don't claim this is ready to merge, but I would like to get the review party started. I'm looking for constructive comments to advance this patch to a mergeable state. I don't do a lot of these patches, so have probably missed some basic things. Thanks.

[shafik]

Why not wrap this into -Wconversion on clang as well?

[vabridgers]

@shafik , I updated the check into -Wconversion. Temporarily addressed the CI failure (to get a passing baseline). Awaiting further review comments. Thank you.

[vabridgers]

Hi @AaronBallman and @erichkeane , any improvements you could suggest? I could expand the testing for sure. Before I invest time in that, I'd like to be sure the general approach is ok in Sema. Thanks.

[erichkeane]

Also needs a release note.

[vabridgers]

I'll add a release note also in the next iteration. Thanks

[vabridgers]

Updates

• Moved check to a subcat of conversion
• Added release notes
• Updated diagnostic message

TBD: Provide information on real findings in an llvm/clang compile using this check

[vabridgers]

Hi @AaronBallman , I ran a compile using this change on clang as you asked and have results. The compile ran with no crashes or errors, and produced 1478 bitfield-conversion warnings. I'll show a few examples below. To put that number into context, I enabled -Wconversion to compare the new warning - bitfield-conversion - with the other conversion checks. You can see from the results that while 1478 findings is large, it's less than 10% of the total conversion findings that exist - so looking at those numbers in context is helpful.

Are there any other suggested improvements for this patch, or do you think it's ok to merge?

[AaronBallman]

Hi @AaronBallman , I ran a compile using this change on clang as you asked and have results. The compile ran with no crashes or errors, and produced 1478 bitfield-conversion warnings. I'll show a few examples below. To put that number into context, I enabled -Wconversion to compare the new warning - bitfield-conversion - with the other conversion checks. You can see from the results that while 1478 findings is large, it's less than 10% of the total conversion findings that exist - so looking at those numbers in context is helpful.

Are there any other suggested improvements for this patch, or do you think it's ok to merge?

Thank you for running the experiment! From spot-checking the results, did they help you to find any actual bugs? And did you notice any patterns in the results that could be used to safely silence the diagnostic in some cases?

[vabridgers]

Hi @AaronBallman , I ran a compile using this change on clang as you asked and have results. The compile ran with no crashes or errors, and produced 1478 bitfield-conversion warnings. I'll show a few examples below. To put that number into context, I enabled -Wconversion to compare the new warning - bitfield-conversion - with the other conversion checks. You can see from the results that while 1478 findings is large, it's less than 10% of the total conversion findings that exist - so looking at those numbers in context is helpful.
Are there any other suggested improvements for this patch, or do you think it's ok to merge?

Thank you for running the experiment! From spot-checking the results, did they help you to find any actual bugs? And did you notice any patterns in the results that could be used to safely silence the diagnostic in some cases?

We did find issues in our code base, and an "interesting" study in coding style related to the bitfield-enum-conversion check I'll describe.

For the bitfield-conversion check, we found no real interesting patterns other than maybe the developer could use a mask on the RHS. But we observed that can mask problems (pun intended!!!) because a value could actually be greater in rank than the bitfield. An improvement to be made is a static analysis check (flow and context sensitive) to check the range of a value to be assigned to a bitfield. This would be an additional check to bitfield-conversion, and could be used to "vote" on the presence of a real defect or not. This may seem a bit too pedantic, but it is what it is :)

Back to the interesting topic of bitfield-enum-conversion, you may know some programmers use a style to define C enums where the last element of an enum is the number of enums. An example looks like this...

enum myenum {
  red,
  blue,
  numenums,
};
struct bits1 {
   int bf:1;
};
struct bits1 xx;
int test(enum myenum value) {
  xx.bf = value;
}

We see false positives related to this style, but this actually caught a real bug where the bitfield was not large enough to contain the enum. The trouble is suppressing the false positives - and this is certainly possible. It's also possible to change the coding style to accommodate the compiler diagnostic behavior.

But tying these two together, it's more difficult to detect unintended use for the bitfield-conversion check compared to the bitfield-enum-conversion check - and I think the contrast is important to highlight (because it helps to comprehend the problem). It's more difficult to infer the programmers intent for the bitfield-conversion check compared to disambiguating user intent for the bitfield-enum diagnostic. Understanding a bitfield-conversion diagnostic well enough to comprehend a true positive still requires the user to recognize the value assigned may take on a rank greater than be defined in the bitfield - which is why we think a static analysis check is appropriate for the code we develop.

Hope this was helpful. Please let me know if you require any other changes for this to be accepted. Thank you!

[vabridgers]

Hi all, I believe all comments have been addressed to date. Please let me know if there's anything else required. Thank you!

[AaronBallman]

Just some minor things to cleanup, but this otherwise LGTM, thank you!

[antmox]

Hi!
This commit broke some armv8/armv7 bots:
https://lab.llvm.org/buildbot/#/builders/245/builds/15263
https://lab.llvm.org/buildbot/#/builders/187/builds/13069
Int & Void* have the same size there, so no warning on bitfield-width.c:20.

[vabridgers]

Hi! This commit broke some armv8/armv7 bots: https://lab.llvm.org/buildbot/#/builders/245/builds/15263 https://lab.llvm.org/buildbot/#/builders/187/builds/13069 Int & Void* have the same size there, so no warning on bitfield-width.c:20.

I'll revert this change for now then.

[davidstone]

The documentation for this should also explain how to write code such that the warning does not fire (by giving an example with x & bitmask)

[joanahalili]

Hello,
This commit is causing clang crashes on our end. Here is a reproducer:

repro Cmd: clang -O0 -std=gnu++20 -fsized-deallocation -Xclang -target-feature -Xclang +sse4.2 -c fileName.ii

where fileName.ii is

template <class a, class... b> bool c = __is_constructible(a, b...);
template <bool, class> using d = int;
template <class> bool e;
template <typename> struct f;
template <typename g> struct h {
  static void i() { c<g, char>; }
};
struct j {
  template <typename k, typename g, d<e<k>, int> = 0>
  void l(unsigned long, f<g> (k::*)());
};
template <typename k, typename g, int> void j::l(unsigned long, f<g> (k::*)()) {
  h<g>::i;
}
struct m : j {
  struct n;
  void o() { l(8, &m::p); }
  f<n> p();
};
struct m::n {
  int q : 1;
};

Could you please revert?

[AaronBallman]

Could you please revert?

It was already reverted in 2e955c0.