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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko added a comment.

>   // 1. `VisitSymbolCast`.
>   // Get a range for main `reg_$0<int x>` - [-2147483648, 2147483647]
>   // Cast main range to `short` - [-2147483648, 2147483647] -> [-32768, 32767].
>   // Now we get a valid range for further bifurcation - [-32768, 32767].

That's a great example, thanks for putting it together.  I can see your point now!

Please, rebase your change and make use of `ConstraintAssignor`, and rework `VisitSymbolCast`.



================
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;
----------------
ASDenysPetrov wrote:
> 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.
Re-read my comment, please.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+    if (!Opts.ShouldSupportSymbolicIntegerCasts)
+      return VisitSymExpr(Sym);
+
----------------
ASDenysPetrov wrote:
> vsavchenko wrote:
> > ASDenysPetrov wrote:
> > > 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.
> > Did I suggest to `Visit(Sym)`?  Of course it is going to end up in a loop!  
> > Why isn't it `Visit(Sym->getOperand())` here?  Before we started producing casts, casts were transparent.  This logic would fit perfectly with that.
> > were transparent.
> Not exactly. There are still some cases when symbols are not equal to there roots(aka Operands). Such cases were handled by `VisitSymExpr` which uses `infer(Sym->getType());` instead of getOperand`. So this needs a sort of think twice. Also I see a problem with `EquivalenceClass`'es. Consider next:
> ```
> int x, y;
> if(x == y)
>   if ((char)x == 2)
>     if(y == 259)
>       // Here we shall update `(char)x` and find this branch infeasible.
> ```
> Also such cases like:
> ```
> if(x == (short)y)
>   // What we should do(store) with(in) `EquivalenceClass`es.
> ```
> Currently, I have an obscure vision of the solution.
> There are still some cases when symbols are not equal to there roots(aka Operands)
Right now we don't have casts, this is what we do currently.  However faulty it is, it is the existing solution and we should respect that.

>  Also I see a problem with EquivalenceClass'es.
Because of the current situation with casts (or more precisely with their lack), `EquivalenceClass`es do not get merged for symbols with different types.  It is as simple as that.
You can find similar tests in `equality_tracking.c`.


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

https://reviews.llvm.org/D103096