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

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

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

Thank you for quickly looking into this.

In D79704#2029230 <https://reviews.llvm.org/D79704#2029230>, @Szelethus wrote:

> - What identifies a `MemRegion`, `TypedValueRegion`s in particular? Why are parameters so special that they deserve their own region? Do you have a concrete, problematic example?


`ParamRegion` contains different data: mainly the index of the parameter. This makes it independent of the actual `Decl` of the parameter.

> - Why is it an issue that we don't always use the same declaration? Does this result in a crash, incorrect modeling?

See D49443#1193290 <https://reviews.llvm.org/D49443#1193290>.

> - Why are we making `ParamRegion` **not** a subclass of `VarRegion`?

Because `VarRegion` is a `DeclRegion` which stores the `Decl` of the variable. Here the main purpose is to not to store it.

> - The patch includes some code duplication, which may be okay, but do we need it?

Yes, and this comes from my previous answers.



================
Comment at: clang/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountDiagnostics.cpp:435-442
+  const VarDecl *VD;
+  if (const auto *VR =
+      dyn_cast<VarRegion>(cast<SymbolRegionValue>(Sym)->getRegion())) {
+    VD = cast<VarDecl>(VR->getDecl());
+  } else if (const auto *PR =
+             dyn_cast<ParamRegion>(cast<SymbolRegionValue>(Sym)->getRegion())) {
+    VD = cast<ParmVarDecl>(PR->getDecl());
----------------
Szelethus wrote:
> Hmm. So, the surrounding code definitely suggests that we're definitely finishing for a parameter here, but it isn't always a parameter region? I know you struggled with this, have you gained any insight as to why?
Not all regions for `ParmVarDecl` are `ParamRegions`. Exceptions are parameters captured by a lambda or a block as well as parameters of functions analyzed top-level.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs<DeclRegion>()) {
     if (isa<ParmVarDecl>(DeclReg->getDecl()))
       Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs<ParamRegion>()) {
+    Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion();
----------------
Szelethus wrote:
> This is interesting. I looked up `DeclRegion`, and it seems to be the region that is tied to a `ValueDecl`. `VarDecl` is a subtype of `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it make sense for `ParamRegion` to be a subtype of `VarRegion`?
`DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the `Index` it stores. There is no is-a relation between them.


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

https://reviews.llvm.org/D79704