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

[Balogh, Ádám via Phabricator via cfe-commits]

baloghadamsoftware marked an inline comment as done.
baloghadamsoftware added a comment.

In D79704#2040798 <https://reviews.llvm.org/D79704#2040798>, @NoQ wrote:

> > It turns out that parameter regions are never created for functions without `Decl` because of the first lines in `CallEvent::getCalleeAnalysisDeclContext()`.
>
> Removing these lines is more or less the whole point of your work.


Is it? The point of my work is to avoid the crashes you mentioned in D49443#1193290 <https://reviews.llvm.org/D49443#1193290> by making the regions of the parameters independent of their declarations so we can remove the limitation that callee's stack frame is only created for functions with definition. (Can you provide me with examples which the crashes you mentioned with `VarRegions`? I need them for testing my solution.) It could be a future step to also remove the limitation that also allows getting the region for parameters without declaration. Even removing the original limitation (creation of stack frame without definition) is part of my planned subsequent patch, not this one. I am trying to follow incremental development to avoid too huge patches.



================
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:
> > > > > 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.
It dumps a temporary region as until now. So this patch does not change the behavior of the analyzer at this point.


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

https://reviews.llvm.org/D79704