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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.


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


================
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.
+
----------------
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.


================
Comment at: clang/test/Analysis/svalbuilder-casts.cpp:30
+  // the entire `x` is zero. We are not able to know the exact value of x.
+  // It can be one of  65536 possible values like [0, 65536, 131072, ...]
+  // and so on. To avoid huge range sets we still assume `x` in the range
----------------



================
Comment at: clang/test/Analysis/svalbuilder-casts.cpp:34-37
+    if (!x)
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
+    else
+      clang_analyzer_warnIfReached(); // expected-warning {{REACHABLE}}
----------------
`clang_analyzer_eval(x == 0); // expected-warning {{UNKNOWN}}`


================
Comment at: clang/test/Analysis/svalbuilder-casts.cpp:43
+  // If two lower bytes of `x` equal to zero, and we know x to be 65537,
+  // which is not truncated to short as zero. Thus the branch is infisible.
+  short s = x;
----------------



================
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)
----------------
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.


================
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}}
----------------
```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}}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126481