baloghadamsoftware marked an inline comment as done. baloghadamsoftware added a comment.
In D79704#2038280 <https://reviews.llvm.org/D79704#2038280>, @NoQ wrote: > In D79704#2038257 <https://reviews.llvm.org/D79704#2038257>, @Szelethus wrote: > > > In D79704#2037100 <https://reviews.llvm.org/D79704#2037100>, @NoQ wrote: > > > > > > The code changes make me feel like we're doing a lot of chore (and make > > > > it super easy to forget checking for parameters explicitly). > > > > > > I wouldn't mind having a common base class for these regions. > > > `DeclRegion` should probably be dropped in this case, in order to avoid > > > dealing with multiple inheritance. And it's definitely the one that needs > > > to be dropped because it currently does nothing except "inheritance for > > > code reuse": there are basically no other common properties between its > > > sub-classes than the accidental code reuse in its methods. > > > > > > Totally. Something like a `VarRegionBase` but with a more clever name. > > > We can even call it `VarRegion` and have sub-classes be `ParamVarRegion` and > `NonParamVarRegion` or something like that. Do you mean `getDecl()` should be pure virtual with different implementation for `ParamVarRegion` (retrieves dynamically based on its `Index`) and `NonParamVarRegion` (stores). However, there //are// some places in the code that check for `DeclRegion` and use its `getDecl()` so to avoid code duplication we can keep `DeclRegion`, but remove storing of `Decl` from it and make `getDecl()` pure virtual already at that level. ================ Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191 +const ParmVarDecl *ParamRegion::getDecl() const { + const Decl *D = getStackFrame()->getDecl(); + ---------------- NoQ wrote: > baloghadamsoftware wrote: > > NoQ wrote: > > > baloghadamsoftware wrote: > > > > NoQ wrote: > > > > > This doesn't work when the callee is unknown. > > > > Please give me an example where the callee is unknown. As I wrote, > > > > originally I put a `getExpr()` method here as well and `getType()` fell > > > > back to it if it could not find the `Decl()`. However, it was never > > > > invoked on the whole test suite. (I put an `assert(false)` into it, and > > > > did not get a crash. > > > > Please give me an example where the callee is unknown. > > > > > > >>! In D79704#2034571, @NoQ wrote: > > > >>>! In D79704#2032947, @Szelethus wrote: > > > >> Could you give a specific code example? > > > > > > > > ```lang=c++ > > > > struct S { > > > > S() { > > > > this; // What region does 'this' point to... > > > > } > > > > }; > > > > > > > > void foo(void (*bar)(S)) { > > > > bar(S()); // ...in this invocation? > > > > } > > > > ``` > > OK, but it still does not crash the analyzer, even if I enable creation of > > stack frames for all the callees, even for those without definition. What > > else should I do to enforce the crash (null pointer dereference)? Try to > > use `getParameterLocation()` in a unit test? > Yes. I demonstrated how you can get the region without a decl but you should > turn this into a test that actually calls one of the problematic functions. > Like, `clang_analyzer_dump()` it or something. Of course I tried `clang_analyzer_explain()`, but neither `clang_analyzer_dump()` helps. I also tried a unit test now where I call `getParameterLocation()` explicitly. It turns out that parameter regions are never created for functions without `Decl` because of the first lines in `CallEvent::getCalleeAnalysisDeclContext()`. This function //needs// the `Decl` to retrieve the `AnalysisDeclContext` of the callee, which is needed to retrieve its stack frame by `getCalleeStackFrame()`. Without stack frame we do not create `ParamRegion`. The other two functions creating `ParamRegion` (`CallEvent::addParameterValuesToBindings` and the newly created `MemRegionManager::getRegionForParam`) start from `ParmVarDecl` which always belongs to a `Decl`. So we can safely assume that currently all parameter regions have a `Decl`. Of course, this can be changed in the future, but I must not include dead code in a patch that cannot even be tested in the current phase. Even creation of callee stack frame for functions without //definition// is not part of this patch, but of the subsequent one. 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
