This addresses a few simplified crashes similar to the one in in https://bugs.llvm.org/show_bug.cgi?id=41142 but there's more bug synergy there, so more fixes would be necessary.
Here's the crash:
class A {}; class B: A {}; B boo(); void foo() { B b { boo() }; }
The analyzer evaluates { boo() } (an InitListExpr) to a compoundVal{X}, where X is whatever boo() evaluates to. The new aggregate initialization facility introduced in D59054 would now be triggered by the presence of the compoundVal{}, even though the object is not an aggregate (base classes are allowed in aggregates in C++17, but they aren't allowed to be private or virtual).
Note that:
- If we remove the assertion and proceed, what would happen is it'll try to bind the whole object b to the region of its base class A within variable b, which is incorrect. RegionStore would probably not care in this specific case, but with multiple inheritance it'll probably also lead to actual incorrect bindings.
- It is in fact irrelevant whether the object is an aggregate - the incorrect behavior would happen anyway. The real problem is that the compoundVal{} doesn't mean what we think: we expected it to describe a big object that is composed of (0 or more) smaller objects, but in fact it's transparently describing the single object within it, much like the InitListExpr it came from. I'd prefer to declare such compoundVal{}s as ill-formed.
- The general problem with compound values is that it's unobvious what sort of object does it describe. I.e., it's impossible to figure out whether compoundVal{X} corresponds to an object of type A (probably not) or an object of type B (that's what we've got) or of some other type that contains a B inside it (that's what we've expected).
This patch fixes neither of these three problems. Instead, what it does is makes sure that there's a ConstructionContext to the CFG for the return-by-value function call boo(). As a side effect for this change, even though we still produce an ill-formed compoundVal{} as the value of expression { boo() }, we never experience a need to make a Store binding out of it. Instead, the call is correctly treated as a constructor that has already initialized its target region by the time it ends, so the crashing code path is no longer triggered. Another side effect of this change is that the value no longer looks like compoundVal{conj_$2<B>}, but more like compoundVal{lazyCompoundVal{b}} - but both are ill-formed anyway.
Now, the original test case in PR41142 is still not fixed. With the same definition of B, the original test case can be written as follows:
B recursive() { B b { recursive() }; }
The problem here is that we inevitably hit the recursion limit, and then...
- Well, something weird happens. I'll have to have a closer look, but one weird thing i noticed is that the value for { boo() } ends up being compoundVal{Unknown}, as if bindReturnValue() never gets triggered.
Also,
- Regardless of the recursion, we're still in trouble every time we don't find a construction context, and for now there is a more or less indefinite number of situations in which this can happen. We should try to be more defensive somehow.
I'll think a bit more to see if fixing these ill-formed compoundVal{}s fixes the problem. In the worst case, i guess we can remove the assertion: we'd behave incorrectly incorrect and crashing from time to time (due to multiple inheritance), but it'll hopefully happen less often than before we had D59054.