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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
