[PATCH] D103096: [analyzer] Implement cast for ranges of symbolic integers.

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

I'll allocate some time to get into your summary, but for now here are my concerns about `SymbolRangeInferrer` and `VisitSymbolCast`.



================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+    if (!Opts.ShouldSupportSymbolicIntegerCasts)
+      return VisitSymExpr(Sym);
+
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > Why do you use `VisitSymExpr` here?  You want to interrupt all `Visits or... I'm not sure I fully understand.
> Here we want to delegate the reasoning to another handler as we don't support non-integral cast yet.
You are not delegating it here.  `Visit` includes a runtime dispatch that calls a correct `VisitTYPE` method.  Here you call `VisitSymExpr` directly, which is one of the `VisitTYPE` methods.  No dispatch will happen, and since we use `VisitSymExpr` as the last resort (it's the most base class, if we got there, we don't actually support the expression), you interrupt the `Visit` and go directly to "the last resort".

See the problem now?


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+      // Find the first constraint and exit the loop.
+      RSPtr = getConstraint(State, S);
+    }
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > Why do you get associated constraint directly without consulting with what `SymbolRangeInferrer` can tell you about it?
> What do you mean? I didn't get. Could you give an example?
`getConstraint` returns whatever constraint we have stored directly in the constraint map.  That's the main source of information for ranges, but not the only one.

Here is the  of things that you skip, when you do `getConstraint` here:
  * we can understand that something is equality/disequality check and find the corresponding info in Equivalence Classes data structure
  * we can see that the expression has the form `A - B` and we can find constraint for `B - A`
  * we can see that the expression is comparison `A op B` and check what other comparison info we have on `A` and `B` (your own change)
  * we can see that the expression is of form `A op B` and check if we know something about `A` and `B`, and produce a reasonable constraint out of this information

In order to use the right information, you should use `infer` that will actually do all other things as well.  That's how `SymbolRangeInferrer` is designed, to be **recursive**.

Speaking of **recursiveness**.  All these loops and manually checking for types of the cast's operand is against this pattern.  Recursive visitors should call `Visit` for children nodes (like `RecursiveASTVisitor`).  In other words, if `f(x)` is a visit function, it should be defined like this:
```
f(x) = g(f(x->operand_1), f(x->operand_2), ... , f(x->operand_N))
```
or if we talk about your case specifically:
```
f(x: SymbolCast) = h(f(x->Operand))
```
and the `h` function should transform the range set returned by `f(x->Operand)` into a range set appropriate for `x`.

NOTE: `h` can also intersect different ranges


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

https://reviews.llvm.org/D103096