[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#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