baloghadamsoftware marked 9 inline comments as done. baloghadamsoftware added a comment.
Than you for your reviews, @NoQ and @Szelethus. In D79704#2032271 <https://reviews.llvm.org/D79704#2032271>, @NoQ wrote: > 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. Hmm, I removed the part that tries to retrieve the parameter from the arguments of the `CallExpr`-like `Expr` because it was never invoked (I used an `assert(false)` there and executed all the tests and they passed). This means that we always found a `Decl`. However, this `Decl` is //not stored// but retrieved always based on the actual stack frame: we get the `Decl` of the function/method/block/etc. from the stack frame and find its parameter using the `Index`. This is always successful, at least in all the analyzer tests. ================ Comment at: clang/include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:1051 + + const Expr *OriginExpr; + unsigned Index; ---------------- 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. ================ 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) {} + ---------------- 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? ================ 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) ---------------- NoQ wrote: > Yes it is a `StackArgumentsSpaceRegion`! Because we're a parameter. So I can remove the inner branch? ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:164 + const auto *SSR = dyn_cast<StackSpaceRegion>(getMemorySpace()); + return SSR ? SSR->getStackFrame() : nullptr; +} ---------------- NoQ wrote: > `SSR` shouldn't ever be null. Any parameter must be on the stack. OK, I will remove this check. ================ 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); ---------------- NoQ wrote: > 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`. Good idea! ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1732 + const LocationContext *LC) { + unsigned Index = PVD->getFunctionScopeIndex(); + const StackFrameContext *SFC = LC->getStackFrame(); ---------------- NoQ wrote: > Does this method also suffer from the parameter vs. argument off-by-one > problem with operators? Actually I removed all usage of the expression thus I got rid of the problem. ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:1735-1736 + const Stmt *CallSite = SFC->getCallSite(); + if (!CallSite) + return std::make_pair(nullptr, UINT_MAX); + ---------------- NoQ wrote: > Does this actually ever happen? I will check it. ================ 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); ---------------- NoQ wrote: > Why would that ever happen? If it's just a sanity check, it should be an > assertion. This happens all the time a lambda or a block captures a parameter of the enclosing function. I decided to keep the regions of captured variables `VarRegions` regardless whether they are parameters or plain variables. This was the only way I could found. This must not cause any problem. ================ Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:2031 + + assert (isa<StackArgumentsSpaceRegion>(MS)); + ---------------- NoQ wrote: > Let's simply make a blanket assertion in `ParamRegion`'s constructor. OK. 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
