Page MenuHomePhabricator

[analyzer] Handle SymbolCast in SValBuilder
ClosedPublic

Authored by martong on May 26 2022, 8:44 AM.

Details

Summary

Make the SimpleSValBuilder to be able to look up and use a constraint
for an operand of a SymbolCast, when the operand is constrained to a
const value.
This part of the SValBuilder is responsible for constant folding. We
need this constant folding, so the engine can work with less symbols,
this way it can be more efficient. Whenever a symbol is constrained with
a constant then we substitute the symbol with the corresponding integer.
If a symbol is constrained with a range, then the symbol is kept and we
fall-back to use the range based constraint manager, which is not that
efficient. This patch is the natural extension of the existing constant
folding machinery with the support of SymbolCast symbols.

Diff Detail

Event Timeline

martong created this revision.May 26 2022, 8:44 AM
Herald added a project: Restricted Project. · View Herald Transcript
martong requested review of this revision.May 26 2022, 8:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2022, 8:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal added inline comments.May 26 2022, 9:59 AM
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
1359

Hoist this common subexpression.

clang/test/Analysis/svalbuilder-casts.cpp
10–11

Please ass a test case where these events happen in the execution path:

  1. Have an unconstrained variable, such as a fn parameter x.
  2. Produce a Symbolic cast of x, let's call it s.
  3. Constrain x to some value range, let's say it must be positive.
  4. Verify that the value of the Symbolic cast s is also constrained to be positive.
31
35–38

clang_analyzer_eval(x == 0); // expected-warning {{UNKNOWN}}

44
45–46

It took me a while to get that the short variable has nothing to do with the test.
I would recommend extending the previous example with the short variable to demonstrate that the same thing (narrowing cast) can occur by either an explicit cast or some implicit casts like a variable initialization/assignment.

47–50
clang_analyzer_eval(x == 1);       // expected-warning {{UNKNOWN}}
clang_analyzer_eval(x == -1);      // expected-warning {{UNKNOWN}}
clang_analyzer_eval(x == 0);       // expected-warning {{FALSE}}
clang_analyzer_eval(x == 65537);   // expected-warning {{FALSE}}
clang_analyzer_eval(x == -65537);  // expected-warning {{FALSE}}
clang_analyzer_eval(x == 131073);  // expected-warning {{FALSE}}
clang_analyzer_eval(x == -131073); // expected-warning {{FALSE}}
martong updated this revision to Diff 432588.May 27 2022, 9:34 AM
martong marked 7 inline comments as done.
  • Hoist S->getOperand, rework the test file
clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
1359

Ok.

clang/test/Analysis/svalbuilder-casts.cpp
10–11

Here is the test you'd like.

void testA(int x) {
  short s = x;
  assert(x > 0);
  clang_analyzer_eval(s > 0); // expected-warning{{UNKNOWN}} should be TRUE
}

However, this will not pass, because x is constrained with a range, and in the Simplifier we do constant folding, i.e. the range must collapse to a single value.
This test will pass with the child patch (created by Denys).

45–46

Ok, I added a hunk for the implicit cast.

47–50

Thanks, I've added these cases.

clang_analyzer_eval(x == 1); expected-warning {{UNKNOWN}}
clang_analyzer_eval(x == -1);
expected-warning {{UNKNOWN}}

Actually, 1 and -1 does not cast to 0 as short, so these should be FALSE. Updated like so.

@martong As you said, my solution D103096 suppose to pass these and more other tests cases. So how it will help in combination with my solution D103096?
Although, your patch is really simple but it's more like a plug then a real SymbolCast support, isn't it? I don't quite understand the motivation.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
1354

Should this imply to use the root symbol and not the second nested one?
E.g. from (int)(short)(x) do you want (short)(x) or (x)?
getOperand gives you (short)(x) in this case.

martong marked an inline comment as done.May 30 2022, 2:49 AM

@martong As you said, my solution D103096 suppose to pass these and more other tests cases. So how it will help in combination with my solution D103096?
Although, your patch is really simple but it's more like a plug then a real SymbolCast support, isn't it? I don't quite understand the motivation.

This part of the SValBuilder is responsible for constant folding. We need this constant folding, so the engine can work with less symbols, this way it can be more efficient. Whenever a symbol is constrained with a constant then we substitute the symbol with the corresponding integer. If a symbol is constrained with a range, then the symbol is kept and we fall-back to use the range based constraint manager, which is not that efficient. This patch is the natural extension of the existing constant folding machinery with the support of SymbolCast symbols.

clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp
1354

We need the operand, with your words it is the second nested one, not the root. In case of (int)(short)(x) we need the (short)(x).

ASDenysPetrov accepted this revision.May 30 2022, 9:37 AM

This part of the SValBuilder is responsible for constant folding. We need this constant folding, so the engine can work with less symbols, this way it can be more efficient. Whenever a symbol is constrained with a constant then we substitute the symbol with the corresponding integer. If a symbol is constrained with a range, then the symbol is kept and we fall-back to use the range based constraint manager, which is not that efficient. This patch is the natural extension of the existing constant folding machinery with the support of SymbolCast symbols.

Now I see. Thanks. I also wouldn't mind if you add this explanation to the summary.

This revision is now accepted and ready to land.May 30 2022, 9:37 AM
martong edited the summary of this revision. (Show Details)May 31 2022, 11:32 PM
This revision was landed with ongoing or failed builds.May 31 2022, 11:42 PM
This revision was automatically updated to reflect the committed changes.
martong marked an inline comment as done.