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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:293-296
+    SymbolRef Sym = Operand;
+    while (isa<SymbolCast>(Sym))
+      Sym = cast<SymbolCast>(Sym)->Operand;
+    return Sym;
----------------
vsavchenko wrote:
> ASDenysPetrov wrote:
> > vsavchenko wrote:
> > > 
> > Do you think the recursive call is better than the loop? But, I guess, I see your point. You option could be safer if we had another implementation of the virtual method. Or you think such alike cast symbol is possible in the future? Well, for now `ignoreCasts` doesn't make sense to any other `Expr` successors.
> Oh, wait, why is it even virtual?  I don't think that it should be virtual.
> Are similar functions in `Expr` virtual?
> And I think that this implementation should live in `SymExpr` directly.  Then it would look like:
> ```
> if (const SymbolCast *ThisAsCast = dyn_cast<SymbolCast>(this)) {
>   return ThisAsCast->ignoreCasts();
> }
> return this;
> ```
Yes, `SymExpr` is an abstract class. And because of limitations and dependency of inheritance we are not able to know the implementaion of `SymbolCast`. Unfortunately, this is not a CRTP.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+    if (!Opts.ShouldSupportSymbolicIntegerCasts)
+      return VisitSymExpr(Sym);
+
----------------
vsavchenko wrote:
> 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?
OK. I reject this idea before. If we call `Visit` inside `VisitSymbolCast`, we will go into recursive loop, because it will return us back to `VisitSymbolCast` as we have passed `Sym` as is. (This is theoretically, I didn't check in practice.) Or I'm missing smth?
I choosed `VisitSymExpr` here because all kinds of `SymbolCast` were previously handled here. So I decided to pass all unsupproted forms of casts there.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1241
+      // Find the first constraint and exit the loop.
+      RSPtr = getConstraint(State, S);
+    }
----------------
vsavchenko wrote:
> 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
Thank you for useful notes!  I'll take them into account.


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

https://reviews.llvm.org/D103096