[PATCH] D40841: [analyzer] Fix a crash on C++17 AST for non-trivial construction into a trivial brace initializer.

[Richard Smith via cfe-commits]

On 5 December 2017 at 09:08, Artem Dergachev via Phabricator via
cfe-commits <cfe-commits at lists.llvm.org> wrote:

> NoQ created this revision.
> Herald added subscribers: cfe-commits, rnkovacs.
>
> Yet another situation where we cannot easily guess, by looking at the CFG,
> the target region for C++ constructor call.
>
> Consider the following brace initialization:
>
>   struct A {
>     A();
>    };
>
>   struct B : public A {
>     int x;
>   };
>
> void foo() {
>
>   B b = {};
>
> }
>
> AST in C++14:
>
>   `-VarDecl 0x7ff07884eab8 <col:3, col:10> col:5 b 'struct B' cinit
>     `-CXXConstructExpr 0x7ff07887a940 <col:9, col:10> 'struct B' 'void
> (void) noexcept(false)' list zeroing
>
> AST in C++17:
>
>   `-VarDecl 0x7fb1cf012b18 <col:3, col:10> col:5 b 'struct B' cinit
>     `-InitListExpr 0x7fb1cf012f38 <col:9, col:10> 'struct B'
>       |-CXXConstructExpr 0x7fb1cf012fd0 <col:10> 'struct A' 'void (void)'
> list
>       `-ImplicitValueInitExpr 0x7fb1cf013000 <<invalid sloc>> 'int'
>
> CFG in C++14:
>
>   [B1]
>     1: {} (CXXConstructExpr, struct B)
>     2: B b = {};
>
> CFG in C++17:
>
>   [B1]
>     1: {} (CXXConstructExpr, struct A)
>     2: /*implicit*/(int)0
>     3: {}
>     4: B b = {};
>
> So, essentially, in C++17 we don't have the trivial constructor call for
> `B` present anywhere at all, neither in AST, nor in CFG.
>
> This causes a crash in the analyzer because he tries to find the target
> region for `CK_NonVirtualBase` constructor of `A` by looking at the parent
> stack frame, expecting to find the derived class constructor (for `B`)
> here, and then taking a base region within its this-object.
>
> This fix is a quick-and-dirty suppression for the crash. It follows the
> tradition of "when unsure, make up a temporary and construct into that,
> then discard the result" - which is wrong, but at least it gets some
> invalidation done.
>
> In our example it would be correct to construct into `base{A, b}`. There
> can also be situations when such initializer-list is instead
> lifetime-extended by a `const&` or `&&` reference (see the tests), so in
> this case we'd need to construct into a respective sub-region of the
> temporary (but not into the temporary itself, of course).
>
> Finally, from the perspective of checker development, i believe it would
> be useful to actually revive the constructor call, so that PreCall/PostCall
> event occured.


Note that there is no constructor call here. This is aggregate
initialization. And there's not really any part of this that's new, except
that a class with base classes is now an a aggregate. You'll see the same
kind of AST formed in all C++ language modes with a slightly modified
example:

```
struct A {
  A();
};

struct B {
  A a;
  int x;
};

void foo() {
  B b = {};
}
```

The analyzer should presumably treat the new example the exact same way it
treats that case.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171205/db545d16/attachment.html>