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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
