martong marked 9 inline comments as done.
martong added a comment.
Thanks for the review guys!
> It feels like `SymbolCast` is a `UnarySymExpr`, but I like to have a distinct
> entity for this.
Yes, their implementation is similar, but there are subtle differences.
> Anyway. There are a lot more places where we visit and handle all SymExpr
> kinds.
> Such as:
> `SValExplainer`, `SymbolExpressor`, `SymbolicRangeInferrer`,
> `SimpleSValBuilder::simplifySValOnce()::Simplifier`, `TypeRetrievingVisitor`.
>
> Out of these, I think `SValExplainer`, `SymbolExpressor`, and
> `TypeRetrievingVisitor` are the crucial ones to have consistent behavior.
> On top of these, the rest of them e.g. the constraint manager and
> simplification just improve the reasoning about these, so they probably can
> be done incrementally.
Okay, I've extended the `SValExplainer`, `SymbolExpressor` and the
`Simplifier`. `TypeRetrievingVisitor` uses the virtual `getType()` function of
`SymExpr` and we override that in `UnarySymExpr`, so that is already handled.
I'd like to handle the constraint manager related things
(`SymbolicRangeInferrer`) in a separate patch.
> I suspect you'll catch a few crashes/assertion failures if you run this on
> real-world code, due to various parts of the static analyzer being startled
> by the new thing, so I really recommend doing this.
Yeah, I am on it.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:321
+ : SymExpr(UnarySymExprKind), Operand(In), Op(Op), T(T) {
+ assert(classof(this));
+ // Unary expressions are results of arithmetic. Pointer arithmetic is not
----------------
steakhal wrote:
> This is always true, right?
Yes. Maybe that is not meaningful anymore (since classof is impelented in the
class properly). So I've deleted it.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:325
+ // sub-regions to regions.
+ assert(isValidTypeForSymbol(T) && !Loc::isLocType(T));
+ }
----------------
steakhal wrote:
> I would prefer distinct assertions. This way if it triggers we can know which
> condition is violated.
Ok, I've split them up.
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:338
+
+ void dumpToStream(raw_ostream &os) const override;
+
----------------
NoQ wrote:
> `LLVM_DUMP_METHOD`?
`SymbolManager.cpp` does that for the base class' function:
```
LLVM_DUMP_METHOD void SymExpr::dump() const { dumpToStream(llvm::errs()); }
```
================
Comment at:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:352
+
+ // Implement isa<T> support.
+ static bool classof(const SymExpr *SE) {
----------------
steakhal wrote:
> I see that all the rest of the classes have this comment, but I found them
> not too useful.
I'd like to keep it consistent with the other classes, besides the comment
might be useful for newcomers.
================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:104
+ QualType type) {
+ assert(operand);
+ assert(!Loc::isLocType(type));
----------------
NoQ wrote:
> steakhal wrote:
> > I think we should assert that we expect only `UO_Minus` (-), `UO_Not` (~).
> > The rest of the `UO_*` doesn't seem to be relevant anyway: PostInc,
> > PostDec, PreInc, PreDec, AddrOf, Deref, Plus, LNot, Real, Imag, Extension,
> > Coawait.
> Actually, let's put such assertion in the constructor. The earlier we catch
> the problem the better.
Ok, I've added the assertion to the ctor.
================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:105-106
return X.castAs<nonloc::ConcreteInt>().evalComplement(*this);
default:
return UnknownVal();
}
----------------
steakhal wrote:
> Also handle it here.
Ok, done.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125318/new/
https://reviews.llvm.org/D125318
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
