SILGen: Move error values into indirect error returns in proper order with cleanups.

There's an unfortunate layering difference in the cleanup order between address-only
and loadable error values during `catch` pattern matching: for address-only values,
the value is copied into a temporary stack slot, and the stack slot is cleaned up
on exit from the pattern match, meaning the value must be moved into the error return
slot on the "no catch" case before cleanups run. But if it's a loadable value, then
we borrow it for the duration of the switch, and the borrow is released during cleanup
on exit from the pattern match, so the value must be forwarded after running cleanups.

The way the code is structured, it handles these cases properly when the convention of
the function being emitted is in sync with the fundamental properties of the error type
(when the error type is loadable and the error return is by value, or when the error
type is address-only and the error return is indirect, in other words). But when
a closure literal with a loadable error type is emitted in an argument context that
expects a function with an indirect error return, we would try to forward the loadable
error value into the error return slot while a borrow is still active on it, leading
to verifier errors. Defer forwarding the value into memory until after cleanups are
popped, fixing rdar://126576356.

A tidier solution might be to always emit the function body to use a bbarg on the
throw block to pass the error value from the body emission to the epilog when the
type is loadable, deferring the move into memory to the epilog block. This would
make the right behavior fall out of the existing implementation, but would require
a bit more invasive changes (pretty much everywhere that checks IndirectErrorReturn
would need to check a different-tracked AddressOnlyErrorType bit instead or in
addition). This change is more localized.

Author: jckarter

lib/SILGen/Cleanup.cpp

@@ -162,16 +162,28 @@ bool CleanupManager::hasAnyActiveCleanups(CleanupsDepth from) {
 /// emitBranchAndCleanups - Emit a branch to the given jump destination,
 /// threading out through any cleanups we might need to run.  This does not
 /// pop the cleanup stack.
-void CleanupManager::emitBranchAndCleanups(JumpDest dest, SILLocation branchLoc,
+void CleanupManager::emitBranchAndCleanups(JumpDest dest,
+                                           SILLocation branchLoc,
+                                           ArrayRef<SILValue> args,
+                                           ForUnwind_t forUnwind) {
+  emitCleanupsForBranch(dest, branchLoc, args, forUnwind);
+  SGF.getBuilder().createBranch(branchLoc, dest.getBlock(), args);
+}
+
+/// emitBranchAndCleanups - Emit the cleanups necessary before branching to
+/// the given jump destination. This does not pop the cleanup stack, nor does
+/// it emit the actual branch.
+void CleanupManager::emitCleanupsForBranch(JumpDest dest,
+                                           SILLocation branchLoc,
                                            ArrayRef<SILValue> args,
                                            ForUnwind_t forUnwind) {
   SILGenBuilder &builder = SGF.getBuilder();
   assert(builder.hasValidInsertionPoint() && "Emitting branch in invalid spot");
   emitCleanups(dest.getDepth(), dest.getCleanupLocation(),
                forUnwind, /*popCleanups=*/false);
-  builder.createBranch(branchLoc, dest.getBlock(), args);
 }
 
+
 void CleanupManager::emitCleanupsForReturn(CleanupLocation loc,
                                            ForUnwind_t forUnwind) {
   SILGenBuilder &builder = SGF.getBuilder();

lib/SILGen/Cleanup.h

@@ -221,6 +221,17 @@ class LLVM_LIBRARY_VISIBILITY CleanupManager {
                              ArrayRef<SILValue> args = {},
                              ForUnwind_t forUnwind = NotForUnwind);
 
+  /// Emit a branch to the given jump destination,
+  /// threading out through any cleanups we need to run. This does not pop the
+  /// cleanup stack.
+  ///
+  /// \param dest       The destination scope and block.
+  /// \param branchLoc  The location of the branch instruction.
+  /// \param args       Arguments to pass to the destination block.
+  void emitCleanupsForBranch(JumpDest dest, SILLocation branchLoc,
+                             ArrayRef<SILValue> args = {},
+                             ForUnwind_t forUnwind = NotForUnwind);
+
   /// emitCleanupsForReturn - Emit the top-level cleanups needed prior to a
   /// return from the function.
   void emitCleanupsForReturn(CleanupLocation loc, ForUnwind_t forUnwind);

lib/SILGen/SILGenStmt.cpp

@@ -1660,11 +1660,12 @@ void SILGenFunction::emitThrow(SILLocation loc, ManagedValue exnMV,
     if (exn->getType().isAddress()) {
       B.createCopyAddr(loc, exn, indirectErrorAddr,
                        IsTake, IsInitialization);
-    } else {
-      // An indirect error is written into the destination error address.
-      emitSemanticStore(loc, exn, indirectErrorAddr,
-                        getTypeLowering(destErrorType), IsInitialization);
     }
+    
+    // If the error is represented as a value, then we should forward it into
+    // the indirect error return slot. We have to wait to do that until after
+    // we pop cleanups, though, since the value may have a borrow active in
+    // scope that won't be released until the cleanups pop.
   } else if (!throwBB.getArguments().empty()) {
     // Load if we need to.
     if (exn->getType().isAddress()) {
@@ -1680,5 +1681,16 @@ void SILGenFunction::emitThrow(SILLocation loc, ManagedValue exnMV,
   }
 
   // Branch to the cleanup destination.
-  Cleanups.emitBranchAndCleanups(ThrowDest, loc, args, IsForUnwind);
+  Cleanups.emitCleanupsForBranch(ThrowDest, loc, args, IsForUnwind);
+  
+  if (indirectErrorAddr && !exn->getType().isAddress()) {
+    // Forward the error value into the return slot now. This has to happen
+    // after emitting cleanups because the active scope may be borrowing the
+    // error value, and we can't forward ownership until those borrows are
+    // released.
+    emitSemanticStore(loc, exn, indirectErrorAddr,
+                      getTypeLowering(destErrorType), IsInitialization);
+  }
+  
+  getBuilder().createBranch(loc, ThrowDest.getBlock(), args);
 }

test/SILGen/typed_throws_reabstraction.swift

@@ -0,0 +1,39 @@
+// RUN: %target-swift-emit-silgen %s | %FileCheck %s
+func test2() throws { // Not OK
+  // The literal closure below is in a generic error context, even though
+  // it statically throws `any Error`, leading it to be emitted with an
+  // indirect `any Error` out parameter.
+  try call { () throws in
+  // CHECK-LABEL: sil{{.*}} @{{.*}}5test2{{.*}}fU_
+    do {
+      try gorble()
+    }
+    // Make sure we stop borrowing the error value before forwarding it into
+    // the error return slot.
+    // CHECK: bb{{.*}}([[ERROR:%.*]] : @owned $any Error):
+    // CHECK:      [[ERROR_BORROW:%.*]] = begin_borrow [[ERROR]]
+    // CHECK: bb{{[0-9]+}}:
+    // CHECK: bb{{[0-9]+}}:
+    // CHECK:      end_borrow [[ERROR_BORROW]]
+    // CHECK-NEXT: store [[ERROR]] to [init]
+    // CHECK-NEXT: throw_addr
+
+    catch is E1 { /* ignore */ }
+  }
+}
+
+func call<E: Error, R>(
+  _ body: () throws(E) -> R
+) throws(E) -> R {
+  try body()
+}
+
+struct E1: Error {}
+
+func gorble() throws {}
+
+func test1() throws { // OK
+  try call { () throws in
+    try gorble()
+  }
+}