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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

> Currently, parameters of functions without their definition present cannot be represented as regions because it would be difficult to ensure that the same declaration is used in every case. To overcome this, we introduce a new kind of region called ParamRegion which does not store the Decl of the parameter variable. Instead it stores the index of the parameter which enables retrieving the actual Decl every time using the function declaration of the stack frame.

I know vaguely of what you're talking about because I've been trying to follow your works so far, but I'd prefer to include a bit more context for those not so familiar with MemRegions. Such as, "Usually, we identify a MemRegion with ..., but for parameters, this is difficult because ..., as seen on this example ... . So, this patch introduces a new region, etcetc."

Some immediate questions:

- 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?
- Why is it an issue that we don't always use the same declaration? Does this result in a crash, incorrect modeling?
- Why are we making `ParamRegion` **not** a subclass of `VarRegion`?
- The patch includes some code duplication, which may be okay, but do we need it?

I haven't read every line thoroughly just yet, so I'll catch up with this patch later! In any case, thank you so much for your investment in the health of the codebase!



================
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());
----------------
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?


================
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();
----------------
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`?


Repository:
  rC Clang

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

https://reviews.llvm.org/D79704