[PATCH] D125318: [analyzer] Add UnarySymExpr

[Gabor Marton via Phabricator via cfe-commits]

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