Page MenuHomePhabricator

[analyzer] Model and check unrepresentable left shifts
ClosedPublic

Authored by rnkovacs on Jan 8 2018, 5:24 AM.

Details

Summary

Left shifting a signed positive value is undefined if the result is not representable in the unsigned version of the return type.

The analyzer now returns an UndefVal in this case and UndefResultChecker is updated to warn about it.

Diff Detail

Repository
rC Clang

Event Timeline

rnkovacs created this revision.Jan 8 2018, 5:24 AM

Overall looks good to me, one comment inline. I think it is good to have these checks to prevent the analyzer executing undefined behavior. Maybe this would make it more feasible to run the analyzer with ubsan :)
In the future, it would be great to also look for these cases symbolically, but I believe it is perfectly fine to have that in a separate patch.

lib/StaticAnalyzer/Core/BasicValueFactory.cpp
238

Do you get a warning if you remove the cast above? I am not sure that we actually want to have the cast here.

rnkovacs updated this revision to Diff 129071.Jan 9 2018, 6:13 AM
rnkovacs marked an inline comment as done.

Overall looks good to me, one comment inline. I think it is good to have these checks to prevent the analyzer executing undefined behavior. Maybe this would make it more feasible to run the analyzer with ubsan :)
In the future, it would be great to also look for these cases symbolically, but I believe it is perfectly fine to have that in a separate patch.

I agree, but I thought that making these checks complete first might be a good idea.

NoQ accepted this revision.Jan 10 2018, 11:23 AM

Looks great. @dcoughlin: would you approve the warning message text?

Maybe actually we could print out the exact numbers that cause the bit shift to overflow, since we do have them when we check.

This revision is now accepted and ready to land.Jan 10 2018, 11:23 AM
rnkovacs updated this revision to Diff 129448.Jan 11 2018, 7:06 AM

I extended the warning message to include more information. What do you think?

The diagnostic text looks to me, but I do have a comment about the nested 'if' inline.

lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
156

This inner 'if' looks fishy to me because if the 'else' branch is ever taken then OS will be empty.

If the else branch can't be taken then you should turn it into an assert(). If it can be taken, then you should make sure that the fall through goes through the "default" else case at the bottom. One way to do this is to pull out the "is representable logic" into a helper function and call that in the containing 'else if'.

Something like:

if (B->getOpcode() == BinaryOperatorKind::BO_Shl && isLeftShiftResultRepresentable(LHS, RHS)) {
  OS << "The result of the left shift ..."
}
rnkovacs updated this revision to Diff 129905.Jan 15 2018, 1:51 PM
rnkovacs marked an inline comment as done.
rnkovacs added inline comments.
lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
156

I overlooked this issue, thanks for pointing out. I pulled the logic out into a helper function.

dcoughlin accepted this revision.Jan 21 2018, 1:48 PM

Looks good to me, thanks!

This revision was automatically updated to reflect the committed changes.