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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:1203
+    if (!Opts.ShouldSupportSymbolicIntegerCasts)
+      return VisitSymExpr(Sym);
+
----------------
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.


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

https://reviews.llvm.org/D103096