[clang] [Sema] Add check for bitfield assignments to larger integral types (PR #68276)

via cfe-commits cfe-commits at lists.llvm.org
Wed Oct 11 14:41:46 PDT 2023


vabridgers wrote:

> > 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?
> > ![image](https://user-images.githubusercontent.com/58314289/273438293-44313d33-6763-434b-92cb-37df8173209a.png)
> 
> 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...

`
  1
  2 enum myenum {
  3     red,
  4     blue,
  5     numenums,
  6 };
  7
  8 struct bits1 {
  9     int bf:1;
 10 };
 11
 12 struct bits1 xx;
 13
 14 int test(enum myenum value) {
 15     xx.bf = value;
 16 }
`
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,  

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


More information about the cfe-commits mailing list