[PATCH] D104844: [Analyzer][solver] Fix crashes during symbol simplification
Gabor Marton via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Jun 24 09:01:46 PDT 2021
martong marked 2 inline comments as done.
martong added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:2314-2315
+ if (SymbolRef SimplifiedSym = simplify(St, Sym))
+ Sym = SimplifiedSym;
+
----------------
vsavchenko wrote:
> martong wrote:
> > vsavchenko wrote:
> > > I don't like the idea of duplicating it into every `assume` method. This way we drastically increase our chances to forget it (like you did with `assumeSymGE` and `assumeSymLE`).
> > > I think the better place for it is in `RangedConstraintManager::assumeSymRel` and neighboring methods, though still not perfect.
> > > I don't really get why we get not simplified symbol to begin with.
> > >
> > `assumeSymRel` is not enough, because e.g. `assumeSymGE` is called also e.g. from `assumeSymUnsupported`. Perhaps we could change the signature of `assumeSymEQ/NE/GT/GE/LT/LE` to take an auxiliary `Simplifier` wrapper object instead of `SymbolRef`?
> >
> > ```
> > ProgramStateRef assumeSymNE(ProgramStateRef State, Simplifier S,
> > const llvm::APSInt &V,
> > const llvm::APSInt &Adjustment);
> >
> > ```
> > And for the Simplifier something like:
> > ```
> > struct Simplifier {
> > SymbolRef SimplifiedSym = nullptr;
> > Simplifier(SymbolRef Sym) : SimplifiedSym(simplify(Sym)) {}
> >
> > };
> > ```
> >
> > assumeSymRel is not enough, because e.g. assumeSymGE is called also e.g. from assumeSymUnsupported.
> Yep, that's why I suggested `assumeSymRel` and its neighbors. I actually think that three top-level public methods from `RangedConstraintManager` will do: `assumeSym`, `assumeSymInclusiveRange`, and `assumeSymUnsupported`.
>
>
> We can't really change the signatures of those methods because we'll be introducing this functionality into solvers that didn't sign up for this (and don't need it).
>
> Also we can least put this `if` statement inside of `simplify`, so we can use it like this: `Sym = simplify(St, Sym);`.
Okay, I've updated like so.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104844/new/
https://reviews.llvm.org/D104844
More information about the cfe-commits
mailing list