Page MenuHomePhabricator

[clang-tidy] Ignore narrowing conversions in case of bitfields
AcceptedPublic

Authored by steakhal on Wed, Nov 17, 10:17 AM.

Details

Summary

Bitfields are special. Due to integral promotion conv.prom/5 bitfield
member access expressions are frequently wrapped by an implicit cast to
int if that type can represent all the values of the bitfield.

Consider these examples:

struct SmallBitfield { unsigned int id : 4; };
x.id & 1;             (case-1)
x.id & 1u;            (case-2)
x.id << 1u;           (case-3)
(unsigned)x.id << 1;  (case-4)

Due to the promotion rules, we would get a warning for case-1. It's
debatable how useful this is, but the user at least has a convenient way of
'fixing' it by adding the u unsigned-suffix to the literal as
demonstrated by case-2. However, this won't work for shift operators like
the one in case-3. In case of a normal binary operator, both operands
contribute to the result type. However, the type of the shift expression is
the promoted type of the left operand. One could still suppress this
superfluous warning by explicitly casting the bitfield member access as
case-4 demonstrates, but why? The compiler already knew that the value from
the member access should safely fit into an int, why do we have this
warning in the first place? So, hereby we suppress this specific scenario, when a bitfield's value is implicitly cast to int (likely due to integral promotion).

Note that the e.g. bitshift operations might invoke unspecified/undefined
behavior, but that's another topic, this checker is about detecting
conversion-related defects.

Example AST for x.id << 1:

BinaryOperator 'int' '<<'
|-ImplicitCastExpr 'int' <IntegralCast>     <--- due to integral promotion
| `-ImplicitCastExpr 'unsigned int' <LValueToRValue>
|   `-MemberExpr 'unsigned int' lvalue bitfield .id
|     `-DeclRefExpr 'SmallBitfield' lvalue ParmVar 'x' 'SmallBitfield'
`-IntegerLiteral 'int' 1

Diff Detail

Event Timeline

steakhal created this revision.Wed, Nov 17, 10:17 AM
steakhal requested review of this revision.Wed, Nov 17, 10:17 AM
Herald added a project: Restricted Project. ยท View Herald TranscriptWed, Nov 17, 10:17 AM
Herald added a subscriber: cfe-commits. ยท View Herald Transcript
courbet added inline comments.Thu, Nov 18, 12:04 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
104

There needs to be some kind of check of the size of the bit field vs the size of the integer, because struct SmallBitfield { unsigned int id : 31; }; should warn.

105

What is specific about shifting operators ? What about x+1 for example ? You might have opened a pandora box here: you'll need to tech the check about the semantics of all binary operations.

BinaryOperator <line:7:3, col:10> 'int' '+'
      |-ImplicitCastExpr <col:3, col:5> 'int' <IntegralCast>
      | `-ImplicitCastExpr <col:3, col:5> 'unsigned int' <LValueToRValue>
      |   `-MemberExpr <col:3, col:5> 'unsigned int' lvalue bitfield .id 0x55e144e7eaa0
      |     `-DeclRefExpr <col:3> 'SmallBitfield' lvalue Var 0x55e144e7eb18 'x' 'SmallBitfield'
      `-IntegerLiteral <col:10> 'int' 1
106

There also needs to be some check of the constant value of the RHS, because x.id << 30 can actually overflow, so we want to warn in that case.

whisperity added inline comments.Thu, Nov 18, 12:06 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
87

(Terminology... ๐Ÿ˜•)

100

IntWidened? Just to point out that so far this is the only case we've found where the warning is superfluous.

Thanks for the valuable feedback.

D114105#inline-1089282

Let me clarify my motivation, and why I'm interested in bitfields, conversions, and shift expressions.
Bitfields can be quite tricky.
It turns out doing anything useful with a bitfield will eventually result in an integral promotion, which mandates that the type must be 'int' if it fits, otherwise 'unsigned int' or no integral promotion happens otherwise.
conv.prom/5:

A prvalue for an integral bit-field ([class.bit]) can be converted to a prvalue of type int if int can represent all the values of the bit-field; otherwise, it can be converted to unsigned int if unsigned int can represent all the values of the bit-field. If the bit-field is larger yet, no integral promotion applies to it. If the bit-field has an enumerated type, it is treated as any other value of that type for promotion purposes.

This behavior really surprises developers, but the context in which it happens is mostly binary operators.
At first glance, those reports seem to be false-positives but they are actually true-positives - and we are happy catching them.
A recommended workaround to this problem could be simply supplying the proper integer literal constant, by adding the u unsigned-suffix.
This will turn the int operand into unsigned int, at which point the binary operator will suddenly do the right thing and promote the bitfield expression to unsigned int as well, and the warning disappears.
This workaround works just fine for the regular binary operators, whose result type is the promoted type of the operands.

In the expr.compound section, the shift expression is the only one whose return type depends only on the left operand, making my plotted workaround inapplicable for them.
Note that the compound shift assignment operator expression doesn't suffer from this behavior.
expr.ass/1:

[...] The result in all cases is a bit-field if the left operand is a bit-field.

Given that the compiler chose int because that would be able to hold the value of the underlying bitfield without information loss, I think it makes sense to ignore these cases.
Of course, the shift operation might cause issues, but that's I think a different topic.
We might be able to do the extra mile and check if the other operand is a constant expression and the represented value would cause UB or unspecified behavior at the shift operation, but that's not really a conversion issue, what this check supposed to look for.

x.id & 4u; // no-warning: promotion will force x.id to be unsigned
x.id << 4u; // we still have a warning! the type of the LHS is unaffected by the type of the RHS
(unsigned)x.id << 4; // looks weird, but does the right thing

That being said, I think the matcher should look through paren expressions but other than that I don't think there is an issue.
Alternatively, we could say that the users *must* use an explicit cast when loading from a bitfield in bitshifts to make their intention clear.
However, it doesn't feel natural and I cannot immediately see how frequently those reports would be true-positives.

WDYT? How should I proceed?

clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
105

What is specific about shifting operators ? What about x+1 for example ? You might have opened a pandora box here: you'll need to tech the check about the semantics of all binary operations.

BinaryOperator <line:7:3, col:10> 'int' '+'
      |-ImplicitCastExpr <col:3, col:5> 'int' <IntegralCast>
      | `-ImplicitCastExpr <col:3, col:5> 'unsigned int' <LValueToRValue>
      |   `-MemberExpr <col:3, col:5> 'unsigned int' lvalue bitfield .id 0x55e144e7eaa0
      |     `-DeclRefExpr <col:3> 'SmallBitfield' lvalue Var 0x55e144e7eb18 'x' 'SmallBitfield'
      `-IntegerLiteral <col:10> 'int' 1

Thanks for the details, this explains the motivation well. I think the key point is the combination of:

It turns out doing anything useful with a bitfield will eventually result in an integral promotion, which mandates that the type must be 'int' if it fits
conv.prom/5:

A prvalue for an integral bit-field ([class.bit]) can be converted to a prvalue of type int if int can represent all the values of the bit-field; otherwise, it can be converted to unsigned int if unsigned int can represent all the values of the bit-field. If the bit-field is larger yet, no integral promotion applies to it. If the bit-field has an enumerated type, it is treated as any other value of that type for promotion purposes.

and

shift expression is the only one whose return type depends only on the left operand

So adding that as a comment above ShiftingWidenedBitfieldValue would help.

Maybe also add a note in the comment that we are not interested in whether the shift itself might overflow.

steakhal updated this revision to Diff 388240.Thu, Nov 18, 9:30 AM
steakhal marked 2 inline comments as done.
steakhal edited the summary of this revision. (Show Details)
  • added more tests
  • add a detailed comment about the situation we suppress
  • see through parenExprs
clang-tools-extra/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
105

I'm not sure if this approach is the right approach.
In that sense, we should not act differently on shift operations, rather just ignore member access expressions implicitly promoted to int.
This is what we are actually looking for. I would rather pursue this direction, but it would mean that we suppress a larger set of reports that we currently have.

What's your opinion on this @courbet @aaron.ballman?

steakhal added inline comments.Thu, Nov 18, 9:53 AM
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
62

Outdated comment; I'll skim through and make sure these reflect the current status.

steakhal updated this revision to Diff 389496.Wed, Nov 24, 7:43 AM
steakhal edited the summary of this revision. (Show Details)

Bitshifts have nothing to do with this issue, thus I'm simply matching for bitfield -> int implicit casts due to promotions.

I think it's in pretty good shape, please have a look to help me move this forward.

Yes, I think the new approach is what we want. The comments also make it much clearer.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
2

Can you add a test with:

void takesInt(int);

...

takesInt(x.id);

I think this suffers from the same problem currently.

courbet accepted this revision.Thu, Nov 25, 1:44 AM
This revision is now accepted and ready to land.Thu, Nov 25, 1:44 AM
steakhal updated this revision to Diff 389711.Thu, Nov 25, 3:09 AM
steakhal marked an inline comment as done.

Added the test_parameter_passing() tests, demonstrating the implicit conversion triggered by parameter passing.
It turns out my previous revision suppressed a useful report.
Now, I fixed that and we get a warning for take<int>(x.id) // warn when x is CompleteBitfield with 32 bitwidth.
Aside from this, no behavioral change happened compared to the previous revision.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
2

Sure, great idea! It actually revealed a logical flaw. Thanks.

courbet accepted this revision.Thu, Nov 25, 4:47 AM
courbet added inline comments.
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-bitfields.cpp
63

[nit] newline after take