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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/MemRegion.cpp:191
+const ParmVarDecl *ParamRegion::getDecl() const {
+  const Decl *D = getStackFrame()->getDecl();
+
----------------
baloghadamsoftware wrote:
> 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.
> neither `clang_analyzer_dump()` helps

So, does it dump a parameter region? Because it obviously should.


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

https://reviews.llvm.org/D79704