[PATCH] [ms-cxxabi] Destroy temporary record arguments in the callee

[John McCall]

On Jun 6, 2013, at 1:28 PM, Reid Kleckner <rnk at google.com> wrote:
> On Thu, Jun 6, 2013 at 4:06 PM, John McCall <rjmccall at apple.com> wrote:
> On Jun 6, 2013, at 9:32 AM, Reid Kleckner <rnk at google.com> wrote:
> --- lib/CodeGen/CGDecl.cpp
> +++ lib/CodeGen/CGDecl.cpp
> @@ -1621,6 +1621,14 @@
>    if (!Ty->isConstantSizeType() ||
>        !CodeGenFunction::hasScalarEvaluationKind(Ty)) {
>      DeclPtr = Arg;
> +    // Push a destructor cleanup for this parameter if the ABI requires it.
> +    if (!hasScalarEvaluationKind(Ty) &&
> 
> You are immediately dominated by this check already.
> 
> It's an ||, not an &&?

Ah, yes, you are right.  So you should cache this result;  it's not exactly
*expensive* to compute, but it's not free either.
 
> -RValue CodeGenFunction::EmitAnyExprToTemp(const Expr *E) {
> +RValue CodeGenFunction::EmitAnyExprToTemp(const Expr *E, bool ExternallyDestructed) {
> 
> There's already an IsDestructed_t;  please use it.
> 
> OK.  It seemed like an internal detail of AggValueSlot.

It's a type-safe boolean idiom.

> These tricks with externally-destructed aren't really suffiicient;  you need to
> push an EH-only cleanup for the object and then disable it when the call
> occurs.
> 
> So, something like, take a stable iterator to the cleanup stack, emit the args, then delete any unnecessary cleanups?  I can't think of an easy way to identify which cleanups were passed to args and which were not.

(I did see your response.)

The idea is that the CallArgList would collect a list of cleanups to
deactivate (in reverse order by preference) before making the call.
 
> The appropriate solution here is not to propagate IsExternallyDestructed
> through umpteen layers of API; you'll need to check for this case specifically
> when emitting an argument and do all the appropriate management.
> 
> I am doing the check in CodeGenFunction::EmitCallArg().  Maybe I should be propagating a bool InArgumentContext or something instead?

You need to inline a specialized path into EmitCallArg, much like several
of the other specialized paths there, so that you can handle the cleanup
semantics specially.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130606/6eced3ac/attachment.html>