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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051
+
+  const Expr *OriginExpr;
+  unsigned Index;
----------------
baloghadamsoftware wrote:
> 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.
Wait, how is it not used? You can't get the region type without `OriginExpr`.


================
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) {}
+
----------------
baloghadamsoftware wrote:
> 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?
Yes, you did what i meant. The first assertion is redundant because `isa` is implied by `cast`. Let's also assert that `OE` is non-null.


================
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)
----------------
baloghadamsoftware wrote:
> NoQ wrote:
> > Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter.
> So I can remove the inner branch?
In fact you can go even further and change the type of the overridden `getMemorySpace()` function to return `const StackArgumentsSpaceRegion *` ("covariant return types") so that to drop the `cast` as well.


================
Comment at: clang/lib/StaticAnalyzer/Core/CallEvent.cpp:255
   // correspond to the stack frame's function declaration.
   assert(VR->getStackFrame() == SFC);
 
----------------
balazske wrote:
> Is it correct to create `VR` only to have this assert?
It's a valid pattern but it should be followed by `(void)VR;` in order to avoid unused variable warning in no-assert builds.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:187
+QualType ParamRegion::getValueType() const {
+  return getDecl()->getType();
+}
----------------
This doesn't work when the callee is unknown. You need to consult the origin expr here.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+
----------------
This doesn't work when the callee is unknown.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:579
+  const ParmVarDecl *PVD = getDecl();
+  if (const IdentifierInfo *ID = PVD->getIdentifier()) {
+    os << ID->getName();
----------------
`PVD` may be null.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1236
+  const Stmt *CallSite = SFC->getCallSite();
+  if (!CallSite)
+    return getVarRegion(PVD, LC);
----------------
Let's make a stronger assertion:

```lang=c++
if (SFC->inTopFrame())
  return getVarRegion(PVD, LC);
assert(CallSite && "Destructors with parameters?");
```


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

https://reviews.llvm.org/D79704