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

[John McCall]

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.

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

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

-RValue CodeGenFunction::EmitAnyExprToTemp(const Expr *E) {
+RValue CodeGenFunction::EmitAnyExprToTemp(const Expr *E, bool ExternallyDestructed) {

There's already an IsDestructed_t;  please use it.

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.

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.

John.