Page MenuHomePhabricator

[Analyzer][Core] Better simplification in SimpleSValBuilder::evalBinOpNN
ClosedPublic

Authored by martong on Fri, Nov 12, 4:06 AM.

Details

Summary

Make the SValBuilder capable to simplify existing
SVals based on a newly added constraints when evaluating a BinOp.

Before this patch, we called simplify only in some edge cases.
However, we can and should investigate the constraints in all cases.

Diff Detail

Event Timeline

martong created this revision.Fri, Nov 12, 4:06 AM
martong requested review of this revision.Fri, Nov 12, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 12, 4:06 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal added inline comments.Fri, Nov 12, 6:06 AM
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
378

It seems like you simplify the rhs as well. Could we have a test for that?

clang/test/Analysis/svalbuilder-simplify-in-evalbinop.cpp
14

Please declare x in this statement.

martong marked 2 inline comments as done.Tue, Nov 23, 6:26 AM

I'm attaching the coverage of the new test file for the related change:

375             :   // Constraints may have changed since the creation of a bound SVal. Check if
376             :   // the values can be simplified based on those new constraints.
377          12 :   SVal simplifiedLhs = simplifySVal(state, lhs);
378          12 :   SVal simplifiedRhs = simplifySVal(state, rhs);
379          12 :   if (auto simplifiedLhsAsNonLoc = simplifiedLhs.getAs<NonLoc>())
380          12 :     lhs = *simplifiedLhsAsNonLoc;
381          12 :   if (auto simplifiedRhsAsNonLoc = simplifiedRhs.getAs<NonLoc>())
382          12 :     rhs = *simplifiedRhsAsNonLoc;
383             :
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
378

Ok, I've added a new test case and reworked the existing a bit.

martong updated this revision to Diff 389186.Tue, Nov 23, 6:26 AM
martong marked an inline comment as done.
  • Add new test case
steakhal accepted this revision.Tue, Nov 23, 7:17 AM

Awesome!

This revision is now accepted and ready to land.Tue, Nov 23, 7:17 AM
This revision was landed with ongoing or failed builds.Tue, Nov 23, 7:45 AM
This revision was automatically updated to reflect the committed changes.

I'm attaching the coverage of the new test file for the related change:

375             :   // Constraints may have changed since the creation of a bound SVal. Check if
376             :   // the values can be simplified based on those new constraints.
377          12 :   SVal simplifiedLhs = simplifySVal(state, lhs);
378          12 :   SVal simplifiedRhs = simplifySVal(state, rhs);
379          12 :   if (auto simplifiedLhsAsNonLoc = simplifiedLhs.getAs<NonLoc>())
380          12 :     lhs = *simplifiedLhsAsNonLoc;
381          12 :   if (auto simplifiedRhsAsNonLoc = simplifiedRhs.getAs<NonLoc>())
382          12 :     rhs = *simplifiedRhsAsNonLoc;
383             :
bjope added a subscriber: bjope.Fri, Nov 26, 10:24 AM

This seem to cause some weird results.

Given this input:

bar(short k) {
  k++;
  for (short f = 0; f < k; f++)
    ;
  (long)k << 16;
}

we get

> clang  --analyze --target=x86_64 'bbi-63538.c'
bbi-63538.c:5:11: warning: The result of the left shift is undefined due to shifting '1' by '16', which is unrepresentable in the unsigned version of the return type 'long' [core.UndefinedBinaryOperatorResult]
  (long)k << 16;
  ~~~~~~~ ^
bbi-63538.c:5:11: warning: The result of the left shift is undefined due to shifting '2' by '16', which is unrepresentable in the unsigned version of the return type 'long' [core.UndefinedBinaryOperatorResult]
  (long)k << 16;
  ~~~~~~~ ^
bbi-63538.c:5:11: warning: The result of the left shift is undefined due to shifting '3' by '16', which is unrepresentable in the unsigned version of the return type 'long' [core.UndefinedBinaryOperatorResult]
  (long)k << 16;
  ~~~~~~~ ^
3 warnings generated.

This seem to cause some weird results.

Given this input:

bar(short k) {
  k++;
  for (short f = 0; f < k; f++)
    ;
  (long)k << 16;
}

we get

> clang  --analyze --target=x86_64 'bbi-63538.c'
bbi-63538.c:5:11: warning: The result of the left shift is undefined due to shifting '1' by '16', which is unrepresentable in the unsigned version of the return type 'long' [core.UndefinedBinaryOperatorResult]
  (long)k << 16;
  ~~~~~~~ ^
bbi-63538.c:5:11: warning: The result of the left shift is undefined due to shifting '2' by '16', which is unrepresentable in the unsigned version of the return type 'long' [core.UndefinedBinaryOperatorResult]
  (long)k << 16;
  ~~~~~~~ ^
bbi-63538.c:5:11: warning: The result of the left shift is undefined due to shifting '3' by '16', which is unrepresentable in the unsigned version of the return type 'long' [core.UndefinedBinaryOperatorResult]
  (long)k << 16;
  ~~~~~~~ ^
3 warnings generated.

Good catch @bjope !
Here is an example without the loop: https://godbolt.org/z/z3rqMz4bs
We'll have a closer look.