[PATCH] D79704: [Analyzer] [NFC] Parameter Regions

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Blanket reply! `ParamRegion` is not a `DeclRegion` because it does not necessarily have a corresponding `Decl`. For instance, when calling a function through an unknown function pointer, you don't have the declaration of a function, let alone its parameters. But for parameters of C++ object type you still need your `ParamRegion` because arguments are constructed into it.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;
----------------
baloghadamsoftware wrote:
> We do not use this at all. However, if I remove it then tests begin to fail with strange reasons, some of them even crashes at strange points. It seems that we need this for profiling the subclass.
`Profile` is completely essential. Without it different regions will be treated as if it's the same region.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1054-1055
+
+  ParamRegion(const Expr *OE, unsigned Idx, const MemRegion *SReg)
+      : TypedValueRegion(SReg, ParamRegionKind), OriginExpr(OE), Index(Idx) {}
+
----------------
It looks like you decided to keep `VarRegion` for the top frame.

I want that asserted here, in the constructor, so that never to make a mistake on this front.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/NSErrorChecker.cpp:197-198
+    if (const ParamRegion *PR = R->getAs<ParamRegion>())
+      if (const StackArgumentsSpaceRegion *
+          stackReg = dyn_cast<StackArgumentsSpaceRegion>(PR->getMemorySpace()))
+        if (stackReg->getStackFrame() == SFC)
----------------
Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:164
+  const auto *SSR = dyn_cast<StackSpaceRegion>(getMemorySpace());
+  return SSR ? SSR->getStackFrame() : nullptr;
+}
----------------
`SSR` shouldn't ever be null. Any parameter must be on the stack.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1579-1584
+      std::tie(Call, Index) = ParamRegion::getExprAndIndexForParam(PVD, LC);
+
+      if (Call)
+        OriginalVR = MemMgr.getParamRegion(Call, Index, LC);
+      else
+        OriginalVR = MemMgr.getVarRegion(VD, LC);
----------------
I suggest making a function in `MemRegionManager` that takes a `Decl` and returns a region. There should only be one place in the code that decides when exactly do we produce a `ParamRegion` instead of a `VarRegion`.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1732
+                                     const LocationContext *LC) {
+  unsigned Index = PVD->getFunctionScopeIndex();
+  const StackFrameContext *SFC = LC->getStackFrame();
----------------
Does this method also suffer from the parameter vs. argument off-by-one problem with operators?


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736
+  const Stmt *CallSite = SFC->getCallSite();
+  if (!CallSite)
+    return std::make_pair(nullptr, UINT_MAX);
+
----------------
Does this actually ever happen?


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1740
+  if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
+    if (Index >= FD->param_size() || FD->parameters()[Index] != PVD)
+      return std::make_pair(nullptr, UINT_MAX);
----------------
Why would that ever happen? If it's just a sanity check, it should be an assertion.


================
Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2031
+
+  assert (isa<StackArgumentsSpaceRegion>(MS));
+
----------------
Let's simply make a blanket assertion in `ParamRegion`'s constructor.


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

https://reviews.llvm.org/D79704