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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D79704#2036581 <https://reviews.llvm.org/D79704#2036581>, @Szelethus wrote:

> If something tricky like this came up, the `getDecl` function should just return with a nullptr, shouldn't it?


How far are you willing to push it? For instance, are you ok with, say, `TypedValueRegion::getValueType()` return a null `QualType` in this single rare cornercase? Because that's what we'll also have to do if a `Decl` isn't available in `VarRegion`. Unless we add more stuff into `VarRegion`, which is basically what we're doing (and it just suddenly turns out that we don't need the `Decl` anymore).

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

> The gain, which is I guess correctness, seems negligible, and I'm not sure this is actually correct, as I mentioned in D79704#inline-730731 <https://reviews.llvm.org/D79704#inline-730731>.

The gain is that we finally have a way to express some regions that we previously could not express at all. This gain isn't huge; i agree that @baloghadamsoftware could most likely get away without it. Apart from the example with no decl at all, i'm pretty excited about eliminating a lot of annoying ambiguities with respect to virtual method calls and function redeclarations that i've been struggling in D49443 <https://reviews.llvm.org/D49443>.

More importantly, this change is correct because that's how programs actually behave. You don't need to know everything (and a `Decl` is literally everything) about the function you're calling in order to call it. The calling convention only requires you to put arguments into certain places in memory and these places are entirely determined by the indices and types of the arguments. And that's exactly what we're trying to mimic. We technically don't even need the `OriginExpr` itself but it's a convenient (and so far seemingly harmless) way of capturing all the information that we actually need (which is, well, indices and types of arguments).



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


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

https://reviews.llvm.org/D79704