Page MenuHomePhabricator

Add new tautological compare warning for bitwise-or with a non-zero constant
ClosedPublic

Authored by rtrieu on Aug 9 2019, 6:54 PM.

Details

Summary

Taking a value and the bitwise-or it with a non-zero constant will always result in a non-zero value. In a boolean context, this is always true.

if (x | 0x4) {} // always true, intended '&'

This patch creates a new warning group -Wtautological-bitwise-compare for this warning. It also moves in the existing tautological bitwise comparisons into this group. A few other changes were needed to the CFGBuilder so that all bool contexts would be checked. The warnings in -Wtautological-bitwise-compare will be off by default due to using the CFG.

https://bugs.llvm.org/show_bug.cgi?id=42666

Diff Detail

Event Timeline

rtrieu created this revision.Aug 9 2019, 6:54 PM
NoQ added a reviewer: NoQ.Aug 9 2019, 8:18 PM
jfb added a subscriber: jfb.Aug 10 2019, 8:52 AM
jfb added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8161 ↗(On Diff #214484)

Why default ignore?

rtrieu marked an inline comment as done.Aug 12 2019, 2:50 PM
rtrieu added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8161 ↗(On Diff #214484)

This warning, like the tautological overlap warning, uses the CFG. CFG-based analysis are typically excluded from being default warnings due to the extra work of construction the CFG.

jfb added inline comments.Aug 12 2019, 2:53 PM
include/clang/Basic/DiagnosticSemaKinds.td
8161 ↗(On Diff #214484)

Can you say so in the commit message?

rtrieu edited the summary of this revision. (Show Details)Aug 12 2019, 3:29 PM
NoQ added inline comments.Aug 12 2019, 3:31 PM
lib/Analysis/CFG.cpp
1118 ↗(On Diff #214484)

It's kinda strange to me that we first confirm that it's a constant by doing tryTransformToIntOrEnumConstant, but then fire up the heavy machinery of EvaluateAsInt anyway. Did you consider using only EvaluateAsInt() to begin with? I guess you're trying to make sure that "the user's intent is clear" as other similar warnings do, right? Could you comment on that?

rtrieu updated this revision to Diff 214733.Aug 12 2019, 4:18 PM

Create new function isIntOrEnumConstantZero to use instead of EvaluateAsInt

rtrieu marked 2 inline comments as done.Aug 12 2019, 4:21 PM
rtrieu added inline comments.
include/clang/Basic/DiagnosticSemaKinds.td
8161 ↗(On Diff #214484)

Updated patch description.

lib/Analysis/CFG.cpp
1118 ↗(On Diff #214484)

A new function has been created to check the zero-ness of the Expr. Since we know it the Expr comes from tryTransformToIntOrEnumConstant, this function doesn't have to handle the full Expr sub-classes.

NoQ accepted this revision.Aug 16 2019, 4:32 PM

Looks great, thank you!

lib/Analysis/CFG.cpp
1118 ↗(On Diff #214484)

Whoa!

This revision is now accepted and ready to land.Aug 16 2019, 4:32 PM

You should also probably add a note in the release notes (maybe for the others changes too)
thanks for the work btw!

You should also probably add a note in the release notes (maybe for the others changes too)
thanks for the work btw!

+1

No, but I left some notes on the bug on why negative values are hard and where to fix it.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 18 2019, 5:57 PM

Any reason why this is off by default?

Probably it is not very cheap in terms of compile time.

Ah, that's a good reason. Are they in -Wall? We've sometimes reconned "-Wall == needs CFG" :)

xbolva00 added a subscriber: aaron.ballman.EditedOct 21 2019, 12:03 PM

This needs explicit -Wtautological-compare - which makes me sad (useful warning but hidden for users).

cc @aaron.ballman maybe atleast put it under -Wextra?

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

jfb added a comment.Oct 21 2019, 2:01 PM

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

It's addressed in the patch description: https://reviews.llvm.org/D66046?id=214484#inline-591988

xbolva00 added a comment.EditedOct 21 2019, 2:09 PM

That comment explains why this is not on by default (= without any -W flag) but does not explain why this is not part of -Wall (if user enables -Wall, he/she cares about warnings) [you asked about default ignore].

We have many warnings flagged as DefaultIgnore but they are part of -Wall.

thakis added inline comments.Oct 21 2019, 2:44 PM
include/clang/Basic/DiagnosticSemaKinds.td
8161 ↗(On Diff #214484)

It addresses why it's not on-by-default, but not why it's not in -Wall. As I said 2 comments ago, we've kind of retconned -Wall to mean "needs CFG" at times.

Mr Trieu, what do you think about adding some or all of the Wtautological-compare warnings to Wall

I thought -Wtautological-compare was already part of -Wall, so I'm all for adding it. I made a patch https://reviews.llvm.org/D69292 with the proposed move to -Wall so it will have some more visibility. I'll be out for the conference and can commit it afterwards if no issues are brought up.