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

[Reid Kleckner]

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:
> > Itanium destroys them in the caller at the end of the full expression,
>
> Um.  This is standard-dictated behavior.  If the MS ABI really gets this
> wrong, that is a very serious divergence.
>

Yep.


> > but MSVC destroys them in the callee..
>
>
> > This should help clang compile MSVC's debug iterators more correctly.
> > There is still an outstanding issue of a memcpy emitted by the LLVM
> > backend, which is not correct for C++ records.
>
> +  // In the Microsoft C++ ABI, all aggregate arguments are destructed by
> the
> +  // callee, no matter how they are passed at the machine level.
> +  bool calleeDestructed =
> +    getTarget().getCXXABI().isTemporaryDestroyedByCallee();
> +
>    if (E->isGLValue()) {
>      assert(E->getObjectKind() == OK_Ordinary);
> -    return args.add(EmitReferenceBindingToExpr(E, /*InitializedDecl=*/0),
> -                    type);
> +    return args.add(EmitReferenceBindingToExpr(E, /*InitializedDecl=*/0,
> +                                               calleeDestructed), type);
>
> This is not correct.  There is no way that the Microsoft ABI causes the
> callee to destroy arbitrary objects that were passed to it by reference.
>

This is just poorly written.  I have tests to ensure that references aren't
destroyed by the callee.  This bool only kicks in if the argument has an
aggregate evaluation kind, which references don't have.


> --- 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 &&?


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


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


> 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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130606/e5d3ae09/attachment.html>