NoQ added a comment. 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.
================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051 + + const Expr *OriginExpr; + unsigned Index; ---------------- 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. ================ 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) {} + ---------------- 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. ================ 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) ---------------- Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter. ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:164 + const auto *SSR = dyn_cast<StackSpaceRegion>(getMemorySpace()); + return SSR ? SSR->getStackFrame() : nullptr; +} ---------------- `SSR` shouldn't ever be null. Any parameter must be on the stack. ================ 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); ---------------- 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`. ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1732 + const LocationContext *LC) { + unsigned Index = PVD->getFunctionScopeIndex(); + const StackFrameContext *SFC = LC->getStackFrame(); ---------------- Does this method also suffer from the parameter vs. argument off-by-one problem with operators? ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736 + const Stmt *CallSite = SFC->getCallSite(); + if (!CallSite) + return std::make_pair(nullptr, UINT_MAX); + ---------------- Does this actually ever happen? ================ 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); ---------------- Why would that ever happen? If it's just a sanity check, it should be an assertion. ================ Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2031 + + assert (isa<StackArgumentsSpaceRegion>(MS)); + ---------------- Let's simply make a blanket assertion in `ParamRegion`'s constructor. 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