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

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

Wow, this is some nice work and huge effort from all the participants, it was pretty informative to read through. Thanks @baloghadamsoftware and @NoQ for working on this!

> 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.

Can we find the FunctionDecl if the call happens through a function pointer? Or in that case do we actually find the VarDecl of the function pointer?

> 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.

So, in this patch, we are trying to solve only that problem when the callee `FunctionDecl` does not have a definition?

> If Decl is missing, then we cannot get its AnalysisDeclContext either, so how can we get a stack frame? However, currently we need a solution where we can get stack frame for callees with Decl but without definition.

This happens in case of top-level params and in case of function pointers, plus the special argument construction, am I right?

Based on the answers to the above questions, possibly we are trying to solve two problems here. Could we split then this patch into two?

1. callee FunctionDecl does not have a definition. This could assert on having an accessable Decl in ParamVarRegion. Here we could have the foundations of the next patch, i.e. we could have ParamVarRegion and NonParamVarRegion.
2. function pointers, plus the special argument construction: ParamVarRegion may not have a Decl and we'd remove the related lines from getCalleeAnalysisDeclContext. This obviously requires more tests. And more brainstorming to get the AnalysisDeclContext when there is no Decl available for the function in getCalleeAnalysisDeclContext.

What do you think?



================
Comment at: clang/lib/StaticAnalyzer/Checkers/ValistChecker.cpp:176-180
   if (const auto *DeclReg = Reg->getAs<DeclRegion>()) {
     if (isa<ParmVarDecl>(DeclReg->getDecl()))
       Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion();
+  } else if (const auto *ParamReg = Reg->getAs<ParamRegion>()) {
+    Reg = C.getState()->getSVal(SV.castAs<Loc>()).getAsRegion();
----------------
balazske wrote:
> baloghadamsoftware wrote:
> > balazske wrote:
> > > baloghadamsoftware wrote:
> > > > Szelethus wrote:
> > > > > This is interesting. I looked up `DeclRegion`, and it seems to be the region that is tied to a `ValueDecl`. `VarDecl` is a subtype of `ValueDecl`, and `ParmVarDecl` is a subtype of `VarDecl`, so wouldn't it make sense for `ParamRegion` to be a subtype of `VarRegion`?
> > > > `DeclRegion` stores the `Decl`, `ParamRegion` retrieves it based on the `Index` it stores. There is no is-a relation between them.
> > > During the lifetime of a `ParamRegion` is it possible that it will return different `Decl` objects?
> > @NoQ?
> Purpose of the question is that if the answer is "no", the decl could be stored at construction of a `ParamRegion` and it could be a subclass of `DeclRegion`. From the code I do not see that the belonging `Decl` can change, probably yes if the stack frame changes somehow. So the reason to have the `ParamRegion` seems to be to have the expression and index available, maybe use these for profiling (instead of a `Decl`). Still it could be made a subclass of `DeclRegion` that stores additionally the expr and index and profiling works with these values.
> During the lifetime of a ParamRegion is it possible that it will return different Decl objects?

AFAIU, Yes. The `CallExpr` may refer to a `FunctionDecl` (the callee), but that Decl may not be the Canonical Decl of the redeclaration chain.


================
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:
> > > > > > 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.
I think we should run this patch against a test suite of open source projects. That could be tmux, curl, redis, xerces, bitcoin, protobuf (those are that we use in CTU at least). We should not have any crashes on theses projects because they exercise widely used language constructs. Besides, they provide a relatively broad set of use of the C and C++ languages, so that's quite a good coverage. In case of any assertion we can use creduce to nail the actual problem.


================
Comment at: clang/unittests/StaticAnalyzer/ParamRegionTest.cpp:72
+TEST(ParamRegion, ParamRegionTest) {
+  EXPECT_TRUE(tooling::runToolOnCode(
+                  std::make_unique<ParamRegionTestAction>(),
----------------
I think the test code would be more valuable if we could use the `ASTMatcher` to match for a given Decl and then for that Decl we could find the corresponding Region. Then we should have different test cases for the different regions (with expectations formulated on those regions). Perhaps a Decl* -> Region mapping is needed in the test Fixture.


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

https://reviews.llvm.org/D79704