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

[Reid Kleckner]

On Tue, Jun 18, 2013 at 10:15 PM, John McCall <rjmccall at apple.com> wrote:

> On Jun 10, 2013, at 1:08 PM, Reid Kleckner <rnk at google.com> wrote:
> >    - Check for dtors of byval params and mark them as used
>
> +  // In the Microsoft C++ ABI, aggregate arguments are destructed by the
> callee.
> +  // However, we still have to push an EH-only cleanup in case we unwind
> before
> +  // we make it to the call.
> +  if (CGM.getTarget().getCXXABI().isTemporaryDestroyedByCallee() &&
> +      hasAggregateEvaluationKind(type)) {
>
> You should move this block after the isGLValue() check, which is
> significantly
> cheaper than hasAggregateEvaluationKind.
>
> You should also hoist the evaluation of hasAggregateEvaluationKind(type)
> out of the if-condition on the following block and then check it first
> here.
>

Oops, getEvaluationKind is a big switch.  Done.


> +    const CXXRecordDecl *RD = type->getAsCXXRecordDecl();
> +    if (RD && RD->hasNonTrivialDestructor()) {
> +      AggValueSlot Slot = CreateAggTemp(type, "agg.arg.tmp");
> +      Slot.setExternallyDestructed();
> +      RValue RV = EmitAnyExpr(E, Slot);
>
> EmitAggExpr.
>

Done.


> +      args.add(RV, type);
> +
> +      pushDestroy(EHCleanup, RV.getAggregateAddr(), type,
> destroyCXXObject,
> +                  /*useEHCleanupForArray*/ true);
> +      // This unreachable is a temporary marker which will be removed
> later.
> +      llvm::Instruction *IsActive = Builder.CreateUnreachable();
>
> Unfortunately, this is not necessarily dominating, because you might be
> inside a conditional expression.  The test case here would be:
>   - in the true branch of a conditional expression:
>     - evaluate a by-val argument with one of these cleanups
>     - evaluate an expression which introduces another cleanup (which will
> prevent the EH cleanup from just getting popped)
>   - in the false branch:
>     - perform a call that can throw
>
> Along the false path, you will not have initialized the activation variable
> for the cleanup.
>

Does the conditional go around the full call, or around an argument?

If the conditional is around the argument, I don't push the cleanup until
after evaluating the argument, so if the false path unwinds, it will not
try to cleanup the argument nor access the activation variable.

I'm trying to think of a way to evaluate a by-val argument and insert a
call that can unwind between the arg construction and the cleanup
activation.  Nothing comes to mind.


> Fortunately, in such cases CGF does track an IP which dominates the first
> condition.


I'm looking at the DominatingValue stuff, but I don't see how to recover
the dominating IP from CGF.  All the other uses of DeactivateCleanup insert
their own placeholders.

+
> +    // In the MSVC++ ABI, make sure arguments have valid destructors.
> +    if
> (Context.getTargetInfo().getCXXABI().isTemporaryDestroyedByCallee())
> +      CheckByValParamsForDtors(NewFD);
> +
>
> Please just do this in CheckParameter.  Check for C++ first, then the ABI
> flag,
> then a record type, then call FinalizeVarWithDestructor. :)
>

FinalizeVarWithDestructor checks access, which fails the test that Richard
provided:

class A {
  ~A();
  friend void foo(A *);
};
void bar(A a) { }
void foo(A *a) { bar(*a); }

MSVC rejects this code, but clang currently accepts it.  It's valid, so I'd
like to accept it even when using the MSVC ABI.

I'll parameterize the access check, but let me know if you'd rather inline
a specialized version of that code.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130619/5c81a0cc/attachment.html>