[PATCH] D125318: [analyzer] Add UnarySymExpr

[Balázs Benics via Phabricator via cfe-commits]

steakhal requested changes to this revision.
steakhal added a comment.
This revision now requires changes to proceed.

Looks great. It'll be a nice addition.

It feels like `SymbolCast` is a `UnarySymExpr`, but I like to have a distinct entity for this.

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.



================
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
----------------
This is always true, right?


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:325
+    // sub-regions to regions.
+    assert(isValidTypeForSymbol(T) && !Loc::isLocType(T));
+  }
----------------
I would prefer distinct assertions. This way if it triggers we can know which condition is violated.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h:352
+
+  // Implement isa<T> support.
+  static bool classof(const SymExpr *SE) {
----------------
I see that all the rest of the classes have this comment, but I found them not too useful.


================
Comment at: clang/lib/StaticAnalyzer/Core/SValBuilder.cpp:104
+                               QualType type) {
+  assert(operand);
+  assert(!Loc::isLocType(type));
----------------
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.


================
Comment at: clang/lib/StaticAnalyzer/Core/SimpleSValBuilder.cpp:105-106
     return X.castAs<nonloc::ConcreteInt>().evalComplement(*this);
   default:
     return UnknownVal();
   }
----------------
Also handle it here.


================
Comment at: clang/lib/StaticAnalyzer/Core/SymbolManager.cpp:73-76
+void UnarySymExpr::dumpToStream(raw_ostream &os) const {
+  os << UnaryOperator::getOpcodeStr(Op);
+  Operand->dumpToStream(os);
+}
----------------
Have a test for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125318