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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked 9 inline comments as done.
baloghadamsoftware added a comment.

Than you for your reviews, @NoQ and @Szelethus.

In D79704#2032271 <https://reviews.llvm.org/D79704#2032271>, @NoQ wrote:

> 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.


Hmm, I removed the part that tries to retrieve the parameter from the arguments of the `CallExpr`-like `Expr` because it was never invoked (I used an `assert(false)` there and executed all the tests and they passed). This means that we always found a `Decl`. However, this `Decl` is //not stored// but retrieved always based on the actual stack frame: we get the `Decl` of the function/method/block/etc. from the stack frame and find its parameter using the `Index`. This is always successful, at least in all the analyzer tests.



================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;
----------------
NoQ wrote:
> 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.
I know, that was not the question. It is the `OriginExpr` which I tried to remove because I do not use it at all, except for profiling. It seems that without this field the `Profile` does not work correctly.


================
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) {}
+
----------------
NoQ wrote:
> 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.
Yes, because in the top frame we neither have `CallExpr` nor `Decl`. So we should assert here that we are not in the top frame, right? How do we check this based on these parameters?


================
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)
----------------
NoQ wrote:
> Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.
So I can remove the inner branch?


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


================
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);
----------------
NoQ wrote:
> 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`.
Good idea!


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


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


================
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);
----------------
NoQ wrote:
> Why would that ever happen? If it's just a sanity check, it should be an assertion.
This happens all the time a lambda or a block captures a parameter of the enclosing function. I decided to keep the regions of captured variables `VarRegions` regardless whether they are parameters or plain variables. This was the only way I could found. This must not cause any problem.


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


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

https://reviews.llvm.org/D79704