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

[John McCall]

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.

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

EmitAggExpr.

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

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

+
+    // 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. :)

John.