[cfe-dev] Inlining temporary destructors in the analyzer

[Manuel Klimek]

On Thu, Jul 31, 2014 at 9:36 PM, Jordan Rose <jordan_rose at apple.com> wrote:

>
> On Jul 31, 2014, at 1:20 , Manuel Klimek <klimek at google.com> wrote:
>
> On Thu, Jul 31, 2014 at 5:19 AM, Jordan Rose <jordan_rose at apple.com>
> wrote:
>
>>
>> On Jul 30, 2014, at 8:14 , Manuel Klimek <klimek at google.com> wrote:
>>
>> Hi Jordan,
>>
>> I made some progress on inlining temporary destructors. My main problem
>> currently is that somehow the initializer values in my constructor get lost
>> if I don't also reference 'this' via some other way.
>>
>> An examples shows that best. Consider:
>> struct A {
>>   A(int* y) : z(y) {}
>>   ~A() { *z = 23; }
>>   int *z;
>> };
>> void f() {
>>   int x = 0;
>>   {(A(&x));}
>> }
>>
>> This leads to a "Dereference of undefined pointer value" warning on *z =
>> 23.
>> Funnily enough this warning goes away (and all values get correctly
>> propagated) if change the constructor to be:
>> A(int* y) : z(y) { (void)this; }
>>
>> Obviously this also works fine if I use a local variable instead of a
>> temporary.
>>
>> Any ideas what might be going wrong here would be greatly appreciated.
>>
>>
>> My guess is that the temporary is an rvalue, and in the analyzer a
>> NonLoc, and that the only reason this even appears to work is because we
>> needed to be able to call *methods* on temporaries and so will create a
>> temporary region for them on demand to serve as the 'this' pointer.
>> However, I don't think we expect that to happen for *constructors,* and
>> so we don't keep the region around after the constructor has finished
>> executing. (That is, when it's just a method being called—and neither the
>> constructor nor destructor are inlined—it doesn't actually matter what you
>> do to the contents of the region, because nothing else can access it.)
>>
>>
> So, funnily enough if I remove this code:
>   if (isTemporaryPRValue(CCE, ThisV))
>   ThisV = state->getSVal(ThisV.castAs<Loc>());
> which puts the temp region into a lazycompoundregion, the memory seems to
> be correctly handled. I have not yet looked what else this breaks though.
>
>
> Okay, analyzer design time. :-)
>
> C++ has a distinction between lvalues and rvalues (actually glvalues and
> prvalues). There are a number of things that go into this distinction, but
> the part the analyzer cares about is that glvalues refer to true objects
> and prvalues do not. (In C++ parlance, "object" refers to anything with
> storage, a type, and lifetime, not just class instances. In the analyzer,
> we call such things "regions".)
>
> If you were to build a very literal C++ compiler, every glvalue would be
> represented by an address. PRValues*, on the other hand, don't really need
> addresses; they only exist to be used transiently.
>

I'm not sure how "exists transiently" equals "doesn't need an address". At
least in C++, this seems incorrect (?) for objects.


> ** How do you capitalize "prvalue"?*
>
> The analyzer pretty much always believes the compiler when it comes to
> glvalues vs. prvalues. That means that the value of a glvalue expression is
> *always* a Loc. A prvalue expression, on the other hand, will only be a
> Loc if it's a pointer type*. If you're talking about a struct prvalue, what
> you're really talking about is the bindings in its memory region—it has no
> identity. This is represented by a CompoundVal or LazyCompoundVal. This
> model worked great for C, but it's starting to show strain in C++.
>
> ** or one of the other types mentioned in Loc::isLocType. But not all of
> those can be expression values.*
>
> Besides "regular" temporaries, this is also a problem for prvalue function
> arguments. When the analyzer inlines a function, it just blithely binds the
> arguments to the proper VarRegions created for each ParmVarDecl in the
> current stack frame. That doesn't really fly if we're supposed to be
> calling the copy constructor. Worse, the copy constructor call happens
> *outside* of the function call, which means it happens before we've even
> decided whether or not to inline, which means there are no VarRegions yet
> to construct into. We've/I've been putting off dealing with this one for a
> while; we get around it mostly because we *don't inline constructors for
> temporaries.*
>
> The general temporary problem goes to what Richard was saying: a temporary
> expression is an rvalue, but if we have to run a constructor then clearly
> there *was* a region at some point. The C++11 standard (at least the
> version I have) is not wonderful about classifying temporaries
> ([basic.lval]p1):
>
> - An *lvalue*...designates a function or an object.
>
> - An *xvalue* (an “eXpiring” value) also refers to an object, usually
> near the end of its lifetime.
>
> - A *glvalue* (“generalized” lvalue) is an lvalue or an xvalue.
>
> - An *rvalue...*is an xvalue, a temporary object (12.2) or subobject
> thereof, or a value that is not associated with an object.
>
> - A *prvalue* (“pure” rvalue) is an rvalue that is not an xvalue.
>
>
> So temporary objects are prvalues. Even though they are modifiable
> (defined later in the same section). Even though with construction and
> destruction they end up needing a fixed region, or at least consistency in
> region contents.
>
> I don't have a great answer here. My guess is we'll probably end up
> turning on *some* inlining for temporaries to start, but not all.
>
> Oh, right, so what's that 'isTemporaryPRValue' doing? That's taking the
> few cases where the AST wants a prvalue but the analyzer really needs a
> valid Loc, and conjuring a region on the fly. That's important for things
> that need to have referential identity, like the 'this' pointer in a method
> called on a temporary, or (say) a temporary that's been lifetime-extended.
>

Since this is right at the end of the constructor call, this always creates
and binds the value if we hit the constructor. See my patch - it seems to
work fine :) What am I missing?


>
> Yuck, right? Just making all temporaries into xvalues would most likely
> make 60% of these problems go away.
> Jordan
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20140731/3012baeb/attachment.html>