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

[John McCall]

On Jun 19, 2013, at 11:45 AM, Reid Kleckner <rnk at google.com> wrote:
> 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?

Around the full call.

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

If the cleanup doesn't actually get popped off the cleanup stack, it will still be around for later code in the full-expression.

Here, I think this test case should work:

  struct A { A(); ~A(); };
  int foo(A a, const A &a);
  int bar();
  ...
  int baz(bool cond) {
    return (cond ? foo(A(), A()) : bar());
  }

Also, you should pop your cleanups in reverse order to make it more likely that they actually pop off.

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

DominatingValue is for something different.  Check out the implementation and uses of setBeforeOutermostConditional.

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

Contrariwise, MSVC presumably accepts this closely-related test case, which we would reject:
  class A {
    ~A();
    friend void bar(A *);
  };
  void bar(A a) { }
  void foo(A *a) { bar(*a); }

This is not just an ABI difference;  it is a significant change to the language, and we should implement its rules correctly.

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