[PATCH] D77229: [Analyzer][WIP] Avoid handling of LazyCompundVals in IteratorModeling

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D77229#1995079 <https://reviews.llvm.org/D77229#1995079>, @baloghadamsoftware wrote:

> My first attempt for a new special kind of region for parameters of functions without definitions. Currently it causes more failed tests and assertions than ignoring the problem of different `Decl`s. I will investigate these problems tomorrow, but this path seems difficult.


Thank you!! I recommend a separate patch for this :) Yes it's a bit intrusive because you'll need to add support for the new region in the `RegionStore` but there's nothing extraordinary here as most of the time it'll be treated the same way as the old parameter region.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h:438-442
+  Optional<SVal> getReturnValueUnderConstruction(unsigned BlockCount) const;
+
+  Optional<SVal> getArgObject(unsigned Index, unsigned BlockCount) const;
+
+  Optional<SVal> getReturnObject(unsigned BlockCount) const;
----------------
I believe these functions don't need to be optional. There's always a parameter location and a location to return to (even if the latter is incorrect due to unimplemented construction context, it'll still be some dummy temporary region that you can use).


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1044
 
+class ParamWithoutVarRegion : public TypedValueRegion {
+  friend class MemRegionManager;
----------------
There should be only one way to express the parameter region. Let's call this one simply `ParamRegion` or something like that, and `assert(!isa<ParmVarDecl>(D))` in the constructor of `VarRegion`.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1066
+  QualType getValueType() const override {
+    return CallE->getType();
+  }
----------------
This should be the type of the parameter, not the return type of the call.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/IteratorModeling.cpp:235
 
 void IteratorModeling::checkBind(SVal Loc, SVal Val, const Stmt *S,
                                  CheckerContext &C) const {
----------------
Do you still need `checkBind` at all?


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:278-279
+
+const ConstructionContext
+*CallEvent::getConstructionContext(unsigned BlockCount) const {
+  const CFGBlock *Block;
----------------
This function obviously does not depend on `BlockCount`. You might as well always use `0` as `BlockCount`.

The ideal solution, of course, is for `CallEvent` to keep a `CFGElementRef` from the start.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:559
+  } else {
+    os << "ParamWithoutVarRegion";
+  }
----------------
We could still dump `CallE->getID()` and `Index`.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1208
+  return getSubRegion<ParamWithoutVarRegion>(CE, Index,
+                                             getStackLocalsRegion(STC));
+}
----------------
StackArguments.


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

https://reviews.llvm.org/D77229