Page MenuHomePhabricator

Extend -Wtautological-bitwise-compare "bitwise or with non-zero value" warnings
Needs ReviewPublic

Authored by sberg on Aug 5 2020, 2:28 AM.

Details

Summary

...to C++ constant expression operands.

(I had been puzzled why Clang had not found <https://git.libreoffice.org/core/+/
55f3a4595891a8cc22272225d1c82419f28d4ef9%5E!/> "cid#1465512 Wrong operator used"
in the LibreOffice source code, only found by Coverity Scan. At least building
LibreOffice with this change caused no false positives.)

In C, values of const-qualified variables are never constant expressions, so
nothing should change for C code.

To avoid potential further false positives, restrict this change only to the
"bitwise or with non-zero value" warnings while keeping all other
-Wtautological-bitwise-compare warnings as-is, at least for now.

Diff Detail

Event Timeline

sberg created this revision.Aug 5 2020, 2:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2020, 2:28 AM
sberg requested review of this revision.Aug 5 2020, 2:28 AM
sberg updated this revision to Diff 283204.Aug 5 2020, 5:24 AM

Nothing seems too mysterious here, but why limit to constexpr instead of anything where Clang can see a constant value, which would make it work in C as well? (If you're allowing constexpr, you're already allowing arbitrary work.)

sberg added a comment.Aug 5 2020, 1:38 PM

My naive assumption was that this warning had initially been restricted to integer literals and enumerators to avoid false positives. Hence my conservative approach of extending merely to constant integer expressions for now. Maybe Richard as the original author of the warning (IIUC) can shed some light on that original restriction?

I looked back on the commits and while I did commit, I did it on the behalf of Anders Rönnholm, who did not have commit access at the time. I haven't seen activity from Anders in a while, but added to subscribers just in case.

Way back then, the warning only did operands of a DeclRefExpr and an IntegerLiteral. Over time, that has been extended, case by case, to include whatever new cases people can think up. I don't mind extending the warnings, but we need to be mindful of how the warnings appear. If the sub-expression becomes too large, it will be difficult for the user to understand where the problem is and which constants the compiler is talking about. We may already be at that point. The example could have a more complex initializer for the constant variables, and the warning would be harder to follow. Maybe we also look at the variable initializers and only allow for simple ones. I need to give this some more thought.

To avoid potential further false positives, restrict this change only to the
"bitwise or with non-zero value" warnings while keeping all other
-Wtautological-bitwise-compare warnings as-is, at least for now.

Are you planning to allow this change to other warnings that use the same helper functions?
Also, have you tried running warning over a codebase?

clang/lib/Analysis/CFG.cpp
96–97

The original comment specifies the allowed Expr's by the specific AST nodes they represent. Please use that. I think "IntegerLiteral constant expression, EnumConstantDecl, or constant value VarDecl" would work.

110

IntergerLiteral and EnumConstantDecl are known to have integer types. However, it is possible for a VarDecl to have other types. There should be a check for integer types here.

175

Need a comment here about how tryTransformToIntOrEnumConstant also allows a DeclRefExpr to have a constant VarDecl, but this case is currently excluded for this warning.

sberg added a comment.Aug 6 2020, 5:05 AM

Are you planning to allow this change to other warnings that use the same helper functions?

No, I didn't plan to work on this further. Just scratching that itch of why Clang didn't emit that one warning it "obviously" should have emitted.

Also, have you tried running warning over a codebase?

As I wrote: "At least building LibreOffice with this change caused no false positives." (Or maybe I misunderstand your question.)

rtrieu added a comment.Aug 6 2020, 7:10 PM

Also, have you tried running warning over a codebase?

As I wrote: "At least building LibreOffice with this change caused no false positives." (Or maybe I misunderstand your question.)

My bad. I read too fast and thought you said you ran the scan over LibreOffice instead of your warning.

xbolva00 added a subscriber: xbolva00.

Looks useful, +1