<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Jun 19, 2013, at 11:45 AM, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:</div><blockquote type="cite"><div dir="ltr"><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></div></div></blockquote><div><br></div>Around the full call.</div><div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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></div></blockquote><div><br></div><div>If the cleanup doesn't actually get popped off the cleanup stack, it will still be around for later code in the full-expression.</div><div><br></div><div>Here, I think this test case should work:</div><div><br></div><div> struct A { A(); ~A(); };</div><div> int foo(A a, const A &a);</div><div> int bar();</div><div> ...</div><div> int baz(bool cond) {</div><div> return (cond ? foo(A(), A()) : bar());</div><div> }</div><div><br></div><div>Also, you should pop your cleanups in reverse order to make it more likely that they actually pop off.</div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">
<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; position: static; z-index: auto; ">
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></div></div></blockquote><div><br></div><div>DominatingValue is for something different. Check out the implementation and uses of setBeforeOutermostConditional.</div><div><br></div><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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>void bar(A a) { }</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></div></div></blockquote><div><br></div>Contrariwise, MSVC presumably accepts this closely-related test case, which we would reject:</div><div> class A {</div><div> ~A();</div><div> friend void bar(A *);</div><div> };</div><div> void bar(A a) { }</div><div> void foo(A *a) { bar(*a); }</div><div><br></div><div>This is not just an ABI difference; it is a significant change to the language, and we should implement its rules correctly.</div><div><div><br></div></div><div>John.</div></body></html>