[PATCH] D126481: [analyzer] Handle SymbolCast in SValBuilder

[Gabor Marton via Phabricator via cfe-commits]

martong added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:1359
+      return cache(S,
+                   SVB.evalCast(Op, S->getType(), S->getOperand()->getType()));
+    }
----------------
steakhal wrote:
> Hoist this common subexpression.
Ok.


================
Comment at: clang/test/Analysis/svalbuilder-casts.cpp:9-10
+
+// Test that the SValBuilder is able to look up and use a constraint for an
+// operand of a SymbolCast, when the operand is constrained to a const value.
+
----------------
steakhal wrote:
> 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.
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).


================
Comment at: clang/test/Analysis/svalbuilder-casts.cpp:44-45
+  // which is not truncated to short as zero. Thus the branch is infisible.
+  short s = x;
+  if (!s) {
+    if (x == 65537 || x == 131073)
----------------
steakhal wrote:
> 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.
Ok, I added a hunk for the implicit cast.


================
Comment at: clang/test/Analysis/svalbuilder-casts.cpp:46-49
+    if (x == 65537 || x == 131073)
+      clang_analyzer_warnIfReached(); // no-warning
+    else
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
----------------
steakhal wrote:
> ```lang=C++
> 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}}
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126481/new/

https://reviews.llvm.org/D126481