This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] clarify undef shift result when shift count is negative or exceeds the bit width
ClosedPublic

Authored by danielmarjamaki on Feb 23 2017, 4:53 AM.

Details

Summary

The error messages are confusing when shift result is undefined because the shift count is negative or exceeds the bit width. I have seen that users often draw the conclusion that Clang thinks some operand is uninitialized and determine that Clang shows a false positive.

I also know that some users use negative shift count by intention and don't think that would cause problems.

This patch clarify the error message and refactors the code a little.

Diff Detail

Repository
rL LLVM

Event Timeline

a.sidorin edited edge metadata.Mar 15 2017, 2:05 AM

Hello Daniel,
Your patch looks mostly good to me. I have only minor naming comments.

lib/StaticAnalyzer/Core/CheckerHelpers.cpp
99 ↗(On Diff #89498)

CompRule?

100 ↗(On Diff #89498)

I think we should rename these variables to LHSExpr, RHSVal, LHSVal. I don't like LVal/RVal because they may be associated with rvalue/lvalue types which is not what we want.

Fix review comments

danielmarjamaki marked 2 inline comments as done.Mar 17 2017, 8:34 AM
danielmarjamaki added inline comments.
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
100 ↗(On Diff #89498)

I agree. Good point.

a.sidorin added inline comments.Mar 17 2017, 8:44 AM
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
99 ↗(On Diff #89498)

Oops. I meant renaming of the BOK argument, not the method :) Sorry for misleading.

danielmarjamaki marked an inline comment as done.

Fix review comment. Made isShiftOverflow() static.

danielmarjamaki marked an inline comment as done.Mar 20 2017, 5:46 AM
dkrupp added a subscriber: dkrupp.Mar 22 2017, 2:45 AM
NoQ edited edge metadata.Apr 4 2017, 8:27 AM

Hello, sorry for your having to wait on me. The implementation looks good, i'm only having a couple of public API concerns.

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
46–49 ↗(On Diff #92315)

Because you are making these functions public, i think it's worth it to make it obvious what they do without looking at the particular checker code. Comments are definitely worth it. I think function names could be worded better; i suggest exprComparesTo(const Expr *LHSExpr, BinaryOperatorKind ComparisonOp, SVal RHSVal, CheckerContext &C); and isGreaterOrEqual(...); i'm personally fond of having CheckerContext at the back because that's where we have it in checker callbacks, but that's not important.

lib/StaticAnalyzer/Core/CheckerHelpers.cpp
101 ↗(On Diff #92315)

I believe it is enough to pass only State to this function. SValBuilder and ConstraintManager objects can be obtained from the state's ProgramStateManager. The good thing about requiring only State is that we'd be able to use these functions in checker callbacks that don't provide the CheckerContext object, eg. checkEndAnalysis or checkRegionChanges.

zaks.anna added inline comments.Apr 7 2017, 1:16 PM
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
46 ↗(On Diff #92315)

+ 1 on Artem's comment of making the names more clear and providing documentation. Also, should these be part of CheckerContext?

Fix review comments

  • renamed
  • reorder function arguments (CheckerContext last)
danielmarjamaki marked 4 inline comments as done.Apr 19 2017, 7:23 AM
danielmarjamaki added inline comments.
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
46 ↗(On Diff #92315)

Also, should these be part of CheckerContext?

I chose not to. Then as NoQ suggested it's not possible to use this when CheckerContext is not available:

"The good thing about requiring only State is that we'd be able to use these functions in checker callbacks that don't provide the CheckerContext object, eg. checkEndAnalysis or checkRegionChanges."

danielmarjamaki marked an inline comment as done.Apr 25 2017, 6:34 AM

Ping.

I only found two nits otherwise looks good to me.

include/clang/StaticAnalyzer/Core/PathSensitive/CheckerHelpers.h
46 ↗(On Diff #95740)

Right now the name of this function is exprComparesTo, but none of its arguments are expressions. You should either rename it to svalComparesTo, or use expr as its arguments.

lib/StaticAnalyzer/Core/CheckerHelpers.cpp
116 ↗(On Diff #95740)

Any reason why do you get the constraint manager and not using ProgramState::assume?

lib/StaticAnalyzer/Core/CheckerHelpers.cpp
116 ↗(On Diff #95740)

Mostly that it's just 1 call instead of 2. assumeDual() has some extra logic (early return , assertion). are there some special reason to use assume()?

xazax.hun added inline comments.May 2 2017, 4:59 AM
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
116 ↗(On Diff #95740)

Basically it would be a bit shorter, it also calls AssumeDual in the background.
See https://clang.llvm.org/doxygen/ProgramState_8h_source.html#l00652

danielmarjamaki marked 3 inline comments as done.May 15 2017, 3:27 AM

renamed exprComparesTo to svalComparesTo

danielmarjamaki marked an inline comment as done.May 15 2017, 10:47 AM
NoQ added a comment.Jun 1 2017, 4:05 AM

I have just one comment and i think it'd be good to land.

lib/StaticAnalyzer/Core/CheckerHelpers.cpp
104 ↗(On Diff #99022)

Every SVal is either Unknown or Undefined or NonLoc or Loc, so the check after && is unnecessary.

Also, i believe that it'd be more correct to look at the AST's Expr::isLValue() (in the caller function, where the expression is still available) instead of looking at the SVal type here. These approaches are significantly different: you need to discriminate between pointer-type rvalues and integer-type lvalues, both of which are represented as Loc values; in the former case, we shouldn't blindly get the binding. I've seen these incorrectly discriminated-between in multiple places in the analyzer, and i believe we should fix this eventually.

zaks.anna edited edge metadata.Jun 1 2017, 11:02 PM

These are great additions!

Going back to my comment about adding these to CheckerContext, I think we should be adding helper functions as methods on CheckerContext as it is the primary place where checker writers look for helpers. Two of the three methods added take CheckerContext as an argument, so what is the reason for adding them elsewhere?

As for the svalComparesTo method, if we want to make it available to the two callbacks that do not have checker context, we can include a method on checker context that calls into a helper in CheckerHelpers.h. However, given that even this patch is not using the function, I would argue for leaving it as a helper function internal to CheckerContext.cpp.

lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
132

"right side of operand" does not sound right..

How about:
"The result of the '<<' expression is undefined due to negative value on the right side of operand"
->
"The result of the left shift is undefined because the right operand is negative"
or
"The result of the '<<' expression is undefined because the right operand is negative"

139

It's best not to use ">=" in diagnostic messages.
Suggestions: "due to shift count >= width of type" ->

  • "due to shifting by a value larger than the width of type"
  • "due to shifting by 5, which is larger than the width of type 'int'" // Providing the exact value and the type would be very useful and this information is readily available to us. Note that the users might not see the type or the value because of macros and such.
lib/StaticAnalyzer/Core/CheckerHelpers.cpp
98 ↗(On Diff #99022)

How about evalComparison as a name for this?

121 ↗(On Diff #99022)

Please, use doxygen comment style here and below.

Fix review comments

danielmarjamaki marked 4 inline comments as done.Jun 22 2017, 9:40 AM
danielmarjamaki added inline comments.
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
139

I used "due to shifting by 5, which is larger than the width of type 'int'"

However I did not see an easy way to show the exact value. So I added getConcreteValue(). Maybe you have a better suggestion. If it's a ConcreteInt I show the exact value, but if it's some range etc then I write "due to shifting by a value that is larger..." instead.

The message "due to shifting by 64, which is larger than the width of type 'unsigned long long'" is a bit weird imho. Because 64 is not larger than the width. Not sure how this can be rephrazed better though.

danielmarjamaki marked an inline comment as done.Jun 22 2017, 9:41 AM
xazax.hun added inline comments.Aug 1 2017, 6:38 AM
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
139

SValBuilder has a getKnownValue, does that help?

Cleaned up the patch a little. Thanks Gabor for telling me about SValBuilder::getKnownValue()

xazax.hun accepted this revision.Aug 8 2017, 1:25 AM

It looks good to me but let's wait for Anna, NoQ, or Devin for the final word.

This revision is now accepted and ready to land.Aug 8 2017, 1:25 AM
zaks.anna added inline comments.Sep 25 2017, 12:50 PM
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
149

Please print single quotes around the value.

153

"equal with the width" -> "equal to the width"

Sorry for the wait!

fixed review comments

danielmarjamaki marked 2 inline comments as done.Sep 27 2017, 12:54 PM
danielmarjamaki marked an inline comment as done.Sep 29 2017, 12:33 PM
paquette added inline comments.
include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h
201

Maybe something like

/// Returns true if the value of \p E is greater than or equal to \p Val under unsigned comparison.
203

This shouldn't need a \brief, since it's a single line comment.

It could also be something like

/// Returns true if the value of \p E is negative.
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
153

Maybe "greater than or equal to" instead of "larger or equal to" just for convention? I hear/read that more often, so seeing "larger" is a little weird.

Minor point though, so if it makes the message too long it doesn't matter.

zaks.anna accepted this revision.Oct 10 2017, 11:20 PM

Once the comments by @paquette are addressed, LGTM. Thanks!

lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
153

I agree that "greater than or equal to" is better, so let's change to that.

This revision was automatically updated to reflect the committed changes.