Merge pull request #73092 from jckarter/silgen-loadable-error-cleanup-order

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

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()
+  }
+}