<div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 18, 2013 at 10:15 PM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="im">On Jun 10, 2013, at 1:08 PM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>

>    - Check for dtors of byval params and mark them as used<br>
<br>
</div>+  // In the Microsoft C++ ABI, aggregate arguments are destructed by the callee.<br>
+  // However, we still have to push an EH-only cleanup in case we unwind before<br>
+  // we make it to the call.<br>
+  if (CGM.getTarget().getCXXABI().isTemporaryDestroyedByCallee() &&<br>
+      hasAggregateEvaluationKind(type)) {<br>
<br>
You should move this block after the isGLValue() check, which is significantly<br>
cheaper than hasAggregateEvaluationKind.<br>
<br>
You should also hoist the evaluation of hasAggregateEvaluationKind(type)<br>
out of the if-condition on the following block and then check it first here.<br></blockquote><div><br></div><div style>Oops, getEvaluationKind is a big switch.  Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

+    const CXXRecordDecl *RD = type->getAsCXXRecordDecl();<br>
+    if (RD && RD->hasNonTrivialDestructor()) {<br>
+      AggValueSlot Slot = CreateAggTemp(type, "agg.arg.tmp");<br>
+      Slot.setExternallyDestructed();<br>
+      RValue RV = EmitAnyExpr(E, Slot);<br>
<br>
EmitAggExpr.<br></blockquote><div><br></div><div style>Done.</div><div style> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">

+      args.add(RV, type);<br>
+<br>
+      pushDestroy(EHCleanup, RV.getAggregateAddr(), type, destroyCXXObject,<br>
+                  /*useEHCleanupForArray*/ true);<br>
+      // This unreachable is a temporary marker which will be removed later.<br>
+      llvm::Instruction *IsActive = Builder.CreateUnreachable();<br>
<br>
Unfortunately, this is not necessarily dominating, because you might be<br>
inside a conditional expression.  The test case here would be:<br>
  - in the true branch of a conditional expression:<br>
    - evaluate a by-val argument with one of these cleanups<br>
    - evaluate an expression which introduces another cleanup (which will prevent the EH cleanup from just getting popped)<br>
  - in the false branch:<br>
    - perform a call that can throw<br>
<br>
Along the false path, you will not have initialized the activation variable<br>
for the cleanup.<br></blockquote><div><br></div><div style>Does the conditional go around the full call, or around an argument?</div><div style><br></div><div style>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.</div>
<div style><br></div><div style>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.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Fortunately, in such cases CGF does track an IP which dominates the first<br>
condition.</blockquote><div><br></div><div style>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.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+<br>
+    // In the MSVC++ ABI, make sure arguments have valid destructors.<br>
+    if (Context.getTargetInfo().getCXXABI().isTemporaryDestroyedByCallee())<br>
+      CheckByValParamsForDtors(NewFD);<br>
+<br>
<br>
Please just do this in CheckParameter.  Check for C++ first, then the ABI flag,<br>
then a record type, then call FinalizeVarWithDestructor. :)<br></blockquote><div><br></div><div style>FinalizeVarWithDestructor checks access, which fails the test that Richard provided:</div><div style><br></div><div style>
<div>class A {</div><div>  ~A();</div><div style>  friend void foo(A *);</div><div>};</div><div><div>void bar(A a) { }</div></div><div style>void foo(A *a) { bar(*a); }</div></div><div style><br></div><div style>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.</div>
<div style><br></div><div style>I'll parameterize the access check, but let me know if you'd rather inline a specialized version of that code.</div></div></div></div>