Page MenuHomePhabricator

Warn on enum assignment to bitfields that can't fit all values
ClosedPublic

Authored by rnk on Mar 13 2017, 4:51 PM.

Details

Summary

This adds -Wbitfield-enum-conversion, which warns on implicit
conversions that happen on bitfield assignment that change the value of
some enumerators.

Values of enum type typically take on a very small range of values, so
they are frequently stored in bitfields. Unfortunately, there is no
convenient way to calculate the minimum number of bits necessary to
store all possible values at compile time, so users usually hard code a
bitwidth that works today and widen it as necessary to pass basic
testing and validation. This is very error-prone, and leads to stale
widths as enums grow. This warning aims to catch such bugs.

This would have found two real bugs in clang and two instances of
questionable code. See r297680 and r297654 for the full description of
the issues.

This warning is currently disabled by default while we investigate its
usefulness outside of LLVM.

The major cause of false positives with this warning is this kind of
enum:

enum E { W, X, Y, Z, SENTINEL_LAST };

The last enumerator is an invalid value used to validate inputs or size
an array. Depending on the prevalance of this style of enum across a
codebase, this warning may be more or less feasible to deploy. It also
has trouble on sentinel values such as ~0U.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Mar 13 2017, 4:51 PM
thakis added inline comments.Mar 13 2017, 6:08 PM
test/Sema/warn-bitfield-enum-conversion.c
3 ↗(On Diff #91649)

can you add an enum with an explicit underlying type? will this warning always fire for those?

rnk updated this revision to Diff 91652.Mar 13 2017, 6:29 PM
  • Make test C++, add fixed type enum
rnk marked an inline comment as done.Mar 13 2017, 6:30 PM
rnk added inline comments.
test/Sema/warn-bitfield-enum-conversion.c
3 ↗(On Diff #91649)

I was worried for a moment, but it looks like EnumDecl::getNumPositiveBits does what I want and ignores the fixed underlying type.

rnk updated this revision to Diff 91653.Mar 13 2017, 6:32 PM
rnk marked an inline comment as done.
  • Actually make this warning off by default
thakis accepted this revision.Mar 14 2017, 8:44 AM

Looks like a cool warning. Two suggestions for more exhaustive testing, but I think this looks good.

test/SemaCXX/warn-bitfield-enum-conversion.cpp
1 ↗(On Diff #91653)

Consider also running this test with a triple where enums don't default to signed.

15 ↗(On Diff #91653)

Also add a : 4 version and check that that doesn't warn?

This revision is now accepted and ready to land.Mar 14 2017, 8:44 AM

Maybe it should have some "to suppress the warning, do X" fixit?

rnk marked 2 inline comments as done.Mar 14 2017, 8:49 AM

Maybe it should have some "to suppress the warning, do X" fixit?

I think we should at least include how many bits the field should have to fix the problem (pointing to the relevant field definition certainly seems helpful).

rnk added a comment.Mar 14 2017, 9:51 AM

Maybe it should have some "to suppress the warning, do X" fixit?

I think we should at least include how many bits the field should have to fix the problem (pointing to the relevant field definition certainly seems helpful).

Agreed, I was just hacking that together. :)

Do you think it's worth indicating that the error can be suppressed with an explicit cast, or is that wasted space?

rnk updated this revision to Diff 91741.Mar 14 2017, 9:51 AM
  • Beef up test as Nico suggests
  • Add two notes, one to change the bitfield signedness, one to indicate the required bitwidth
In D30923#700696, @rnk wrote:

Maybe it should have some "to suppress the warning, do X" fixit?

I think we should at least include how many bits the field should have to fix the problem (pointing to the relevant field definition certainly seems helpful).

Agreed, I was just hacking that together. :)

Do you think it's worth indicating that the error can be suppressed with an explicit cast, or is that wasted space?

What might this look like? Also, I don't see a regression test for this.

rnk updated this revision to Diff 91749.Mar 14 2017, 10:44 AM
  • Test that explicit casts suppress the warning
rnk added a comment.Mar 14 2017, 10:44 AM
In D30923#700696, @rnk wrote:

Do you think it's worth indicating that the error can be suppressed with an explicit cast, or is that wasted space?

What might this look like? Also, I don't see a regression test for this.

The warning only looks through implicit casts and paren exprs, so it could look like this: f.two_bits = (unsigned)three_bits; I added a test for it.

In D30923#700780, @rnk wrote:
In D30923#700696, @rnk wrote:

Do you think it's worth indicating that the error can be suppressed with an explicit cast, or is that wasted space?

What might this look like? Also, I don't see a regression test for this.

The warning only looks through implicit casts and paren exprs, so it could look like this: f.two_bits = (unsigned)three_bits; I added a test for it.

Great, thanks!

lib/Sema/SemaChecking.cpp
8765 ↗(On Diff #91749)

Line is too long.

rnk marked an inline comment as done.Mar 14 2017, 11:09 AM

Cool, thanks for the reviews, this is definitely in better shape now. This is off by default, so I'm going to commit and experiment with it on a few codebases.

I'd still like to hear if either of the Richards have diagnostic wording suggestions, but we can do that in a follow-up.

This revision was automatically updated to reflect the committed changes.