[silgenpattern] Allow the initial switch value to be at +0 if it is loadable.

lib/SILGen/SILGenExpr.cpp

@@ -2046,9 +2046,8 @@ ManagedValue SILGenFunction::getManagedValue(SILLocation loc,
     if (value.getOwnershipKind() == ValueOwnershipKind::Trivial)
       return ManagedValue::forUnmanaged(value.getValue());
 
-    // Otherwise, retain and enter a release cleanup.
-    valueTL.emitCopyValue(B, loc, value.getValue());
-    return emitManagedRValueWithCleanup(value.getValue(), valueTL);
+    // Otherwise, copy the value and return.
+    return value.getFinalManagedValue().copy(*this, loc);
   }
 
   // Otherwise, produce a temporary and copy into that.

lib/SILGen/SILGenPattern.cpp

@@ -741,22 +741,28 @@ forwardIntoSubtree(SILGenFunction &SGF, SILLocation loc,
 
   auto consumptionKind = outerCMV.getFinalConsumption();
   (void)consumptionKind;
+
+  // If we have an object and it is take always, we need to borrow the value
+  // since we do not own the value at this point.
+  if (outerMV.getType().isObject()) {
+    assert(consumptionKind == CastConsumptionKind::TakeAlways &&
+           "Object without cleanup that is not take_always?!");
+    return {outerMV.borrow(SGF, loc), CastConsumptionKind::BorrowAlways};
+  }
+
+  // Only address only values use TakeOnSuccess.
+  assert(outerMV.getType().isAddressOnly(SGF.getModule()) &&
+         "TakeOnSuccess can only be used with address only values");
+
   assert((consumptionKind == CastConsumptionKind::TakeAlways ||
           consumptionKind == CastConsumptionKind::TakeOnSuccess) &&
          "non-+1 consumption with a cleanup?");
   scope.pushCleanupState(outerMV.getCleanup(),
                          CleanupState::PersistentlyActive);
 
-  // If SILOwnership is enabled and we have an object, borrow instead of take on
-  // success.
-  if (SGF.F.getModule().getOptions().EnableSILOwnership &&
-      outerMV.getType().isObject()) {
-    return {outerMV.borrow(SGF, loc), CastConsumptionKind::BorrowAlways};
-  }
-
   // Success means that we won't end up in the other branch,
   // but failure doesn't.
-  return { outerMV, CastConsumptionKind::TakeOnSuccess };
+  return {outerMV, CastConsumptionKind::TakeOnSuccess};
 }
 
 /// Forward a value down into an irrefutable branch of the decision tree.
@@ -2728,9 +2734,39 @@ void SILGenFunction::emitSwitchStmt(SwitchStmt *S) {
   PatternMatchContext switchContext = { emission };
   SwitchStack.push_back(&switchContext);
 
-  // Emit the subject value. Dispatching will consume it.
-  ManagedValue subjectMV = emitRValueAsSingleValue(S->getSubjectExpr());
-  auto subject = ConsumableManagedValue::forOwned(subjectMV);
+  // Emit the subject value. If at +1, dispatching will consume it. If it is at
+  // +0, we just forward down borrows.
+  ManagedValue subjectMV = emitRValueAsSingleValue(
+      S->getSubjectExpr(), SGFContext::AllowGuaranteedPlusZero);
+
+  // Inline constructor for subject.
+  auto subject = ([&]() -> ConsumableManagedValue {
+    // If we have a plus one value...
+    if (subjectMV.isPlusOne(*this)) {
+      // And we have an address that is loadable, perform a load [take].
+      if (subjectMV.getType().isAddress() &&
+          subjectMV.getType().isLoadable(getModule())) {
+        subjectMV = B.createLoadTake(S, subjectMV);
+      }
+      return {subjectMV, CastConsumptionKind::TakeAlways};
+    }
+
+    // If we have a loadable address and +0, perform a load borrow.
+    if (subjectMV.getType().isAddress() &&
+        subjectMV.getType().isLoadable(getModule())) {
+      subjectMV = B.createLoadBorrow(S, subjectMV);
+    }
+
+    // If then we have an object, return it at +0.
+    if (subjectMV.getType().isObject()) {
+      return {subjectMV, CastConsumptionKind::BorrowAlways};
+    }
+
+    // If we have an address only type returned without a cleanup, we
+    // need to do a copy just to be safe. So for efficiency we pass it
+    // down take_always.
+    return {subjectMV.copy(*this, S), CastConsumptionKind::TakeAlways};
+  }());
 
   auto failure = [&](SILLocation location) {
     // If we fail to match anything, we trap. This can happen with a switch
@@ -2855,13 +2891,14 @@ void SILGenFunction::emitCatchDispatch(DoCatchStmt *S, ManagedValue exn,
 
   // Set up an initial clause matrix.
   ClauseMatrix clauseMatrix(clauseRows);
-  ConsumableManagedValue subject;
-  if (F.getModule().getOptions().EnableSILOwnership &&
-      exn.getType().isObject()) {
-    subject = {exn.borrow(*this, S), CastConsumptionKind::BorrowAlways};
-  } else {
-    subject = {exn, CastConsumptionKind::TakeOnSuccess};
-  }
+
+  assert(exn.getType().isObject() &&
+         "Error is special and should always be an object");
+  // Our model is that sub-cases get the exception at +0 and the throw (if we
+  // need to rethrow the exception) gets the exception at +1 since we need to
+  // trampoline it's ownership to our caller.
+  ConsumableManagedValue subject = {exn.borrow(*this, S),
+                                    CastConsumptionKind::BorrowAlways};
 
   auto failure = [&](SILLocation location) {
     // If we fail to match anything, just rethrow the exception.
@@ -2874,7 +2911,10 @@ void SILGenFunction::emitCatchDispatch(DoCatchStmt *S, ManagedValue exn,
       return;
     }
 
-    // Don't actually kill the exception's cleanup.
+    // Since we borrowed exn before sending it to our subcases, we know that it
+    // must be at +1 at this point. That being said, SILGenPattern will
+    // potentially invoke this for each of the catch statements, so we need to
+    // copy before we pass it into the throw.
     CleanupStateRestorationScope scope(Cleanups);
     if (exn.hasCleanup()) {
       scope.pushCleanupState(exn.getCleanup(),

lib/SILGen/SILGenStmt.cpp

@@ -800,6 +800,12 @@ void StmtEmitter::visitDoCatchStmt(DoCatchStmt *S) {
     // Emit all the catch clauses, branching to the end destination if
     // we fall out of one.
     SGF.emitCatchDispatch(S, exn, S->getCatches(), endDest);
+
+    // We assume that exn's cleanup is still valid at this point. To ensure that
+    // we do not re-emit it and do a double consume, we rely on us having
+    // finished emitting code and thus unsetting the insertion point here. This
+    // assert is to make sure this invariant is clear in the code and validated.
+    assert(!SGF.B.hasValidInsertionPoint());
   }
 
   if (hasLabel) {

test/SIL/ownership-verifier/over_consume.sil

@@ -22,7 +22,12 @@ case some(T)
 case none
 }
 
-class SuperKlass {}
+class SuperKlass {
+  func doSomething()
+}
+
+class Klass : SuperKlass {
+}
 
 ///////////
 // Tests //
@@ -434,3 +439,21 @@ bb3:
   %9999 = tuple()
   return %9999 : $()
 }
+
+// CHECK-LABEL: Function: 'consume_with_classmethod'
+// CHECK: Found use after free?!
+// CHECK: Value:   %2 = upcast %0 : $Klass to $SuperKlass
+// CHECK: Consuming User:   store %2 to [init] %1 : $*SuperKlass
+// CHECK: Non Consuming User:   %4 = class_method %2 : $SuperKlass, #SuperKlass.doSomething!1 : (SuperKlass) -> () -> (), $@convention(method) (@guaranteed SuperKlass) -> ()
+// CHECK: Block: bb0
+sil @consume_with_classmethod : $@convention(thin) (@owned Klass) -> () {
+bb0(%0 : @owned $Klass):
+  %1 = alloc_stack $SuperKlass
+  %2 = upcast %0 : $Klass to $SuperKlass
+  store %2 to [init] %1 : $*SuperKlass
+  %3 = class_method %2 : $SuperKlass, #SuperKlass.doSomething!1 : (SuperKlass) -> () -> (), $@convention(method) (@guaranteed SuperKlass) -> ()
+  destroy_addr %1 : $*SuperKlass
+  dealloc_stack %1 : $*SuperKlass
+  %9999 = tuple()
+  return %9999 : $()
+}

test/SILGen/class_bound_protocols.swift

@@ -1,5 +1,5 @@
 
-// RUN: %target-swift-emit-silgen -parse-stdlib -parse-as-library -module-name Swift %s | %FileCheck %s
+// RUN: %target-swift-emit-silgen -enable-sil-ownership -parse-stdlib -parse-as-library -module-name Swift %s | %FileCheck %s
 
 enum Optional<T> {
   case some(T)
@@ -132,8 +132,9 @@ func class_bound_method(x: ClassBound) {
   // CHECK: [[READ:%.*]] = begin_access [read] [unknown] [[XBOX_PB]] : $*ClassBound
   // CHECK: [[X:%.*]] = load [copy] [[READ]] : $*ClassBound
   // CHECK: [[PROJ:%.*]] = open_existential_ref [[X]] : $ClassBound to $[[OPENED:@opened(.*) ClassBound]]
+  // CHECK: [[BORROWED_PROJ:%.*]] = begin_borrow [[PROJ]]
   // CHECK: [[METHOD:%.*]] = witness_method $[[OPENED]], #ClassBound.classBoundMethod!1
-  // CHECK: apply [[METHOD]]<[[OPENED]]>([[PROJ]])
+  // CHECK: apply [[METHOD]]<[[OPENED]]>([[BORROWED_PROJ]])
   // CHECK: destroy_value [[PROJ]]
   // CHECK: destroy_value [[XBOX]]
 }

test/SILGen/errors.swift

@@ -1,17 +1,5 @@
 // RUN: %target-swift-emit-silgen -parse-stdlib -enable-sil-ownership -Xllvm -sil-print-debuginfo -verify -primary-file %s %S/Inputs/errors_other.swift | %FileCheck %s
 
-// TODO: Turn back on ownership verification. I turned off the verification on
-// this file since it shows an ownership error that does not affect
-// codegen. Specifically when we destroy the temporary array we use for the
-// variadic tuple, we try to borrow the temporary array when we pass it to the
-// destroy function. This makes the ownership verifier think that the owned
-// value we are trying to destroy is not cleaned up. But once ownership is
-// stripped out, the destroy array function still does what it needs to do. The
-// actual fix for this would require a bunch of surgery in SILGenApply around
-// how function types are computed and preserving non-canonical function types
-// through SILGenApply. This is something that can be done after +0 is turned
-// on.
-
 import Swift
 
 class Cat {}
@@ -176,6 +164,83 @@ func all_together_now(_ flag: Bool) -> Cat {
   }
 }
 
+// Make sure that if we catch an error in a throwing function we borrow the
+// error and only consume the error in the rethrow block.
+//
+// CHECK-LABEL: sil hidden @$s6errors20all_together_now_twoyAA3CatCSgSbKF : $@convention(thin) (Bool) -> (@owned Optional<Cat>, @error Error) {
+// CHECK: bb0(
+// CHECK-NOT: bb1
+// CHECK:   try_apply {{.*}}, normal [[NORMAL_BB:bb[0-9]+]], error [[ERROR_BB:bb[0-9]+]]
+//
+// CHECK: [[ERROR_BB]]([[ERROR:%.*]] : @owned $Error):
+// CHECK:   [[BORROWED_ERROR:%.*]] = begin_borrow [[ERROR]]
+// CHECK:   [[COPIED_ERROR:%.*]] = copy_value [[BORROWED_ERROR]]
+// CHECK:   store [[COPIED_ERROR]] to [init] [[CAST_INPUT_MEM:%.*]] : $*Error
+// CHECK:   checked_cast_addr_br copy_on_success Error in [[CAST_INPUT_MEM]] : $*Error to HomeworkError in [[CAST_OUTPUT_MEM:%.*]] : $*HomeworkError, [[CAST_YES_BB:bb[0-9]+]], [[CAST_NO_BB:bb[0-9]+]],
+//
+// CHECK: [[CAST_YES_BB]]:
+// CHECK:   [[SUBERROR:%.*]] = load [take] [[CAST_OUTPUT_MEM]]
+// CHECK:   switch_enum [[SUBERROR]] : $HomeworkError, case #HomeworkError.TooHard!enumelt: {{bb[0-9]+}}, default [[SWITCH_MATCH_FAIL_BB:bb[0-9]+]],
+//
+// CHECK: [[SWITCH_MATCH_FAIL_BB]]([[SUBERROR:%.*]] : @owned $HomeworkError):
+// CHECK:   destroy_value [[SUBERROR]]
+// CHECK:   end_borrow [[BORROWED_ERROR]]
+// CHECK:   br [[RETHROW_BB:bb[0-9]+]]([[ERROR]] : $Error)
+//
+// CHECK: [[CAST_NO_BB]]:
+// CHECK:   end_borrow [[BORROWED_ERROR]]
+// CHECK:   br [[RETHROW_BB]]([[ERROR]] : $Error)
+//
+// CHECK: [[RETHROW_BB]]([[ERROR_FOR_RETHROW:%.*]] : @owned $Error):
+// CHECK:   throw [[ERROR_FOR_RETHROW]]
+// CHECK: } // end sil function '$s6errors20all_together_now_twoyAA3CatCSgSbKF'
+func all_together_now_two(_ flag: Bool) throws -> Cat? {
+  do {
+    return try dont_return(Cat())
+  } catch HomeworkError.TooHard {
+    return nil
+  }
+}
+
+// Same as the previous test, but with multiple cases instead of just one.
+//
+// CHECK-LABEL: sil hidden @$s6errors22all_together_now_threeyAA3CatCSgSbKF : $@convention(thin) (Bool) -> (@owned Optional<Cat>, @error Error) {
+// CHECK: bb0(
+// CHECK-NOT: bb1
+// CHECK:   try_apply {{.*}}, normal [[NORMAL_BB:bb[0-9]+]], error [[ERROR_BB:bb[0-9]+]]
+//
+// CHECK: [[ERROR_BB]]([[ERROR:%.*]] : @owned $Error):
+// CHECK:   [[BORROWED_ERROR:%.*]] = begin_borrow [[ERROR]]
+// CHECK:   [[COPIED_ERROR:%.*]] = copy_value [[BORROWED_ERROR]]
+// CHECK:   store [[COPIED_ERROR]] to [init] [[CAST_INPUT_MEM:%.*]] : $*Error
+// CHECK:   checked_cast_addr_br copy_on_success Error in [[CAST_INPUT_MEM]] : $*Error to HomeworkError in [[CAST_OUTPUT_MEM:%.*]] : $*HomeworkError, [[CAST_YES_BB:bb[0-9]+]], [[CAST_NO_BB:bb[0-9]+]],
+//
+// CHECK: [[CAST_YES_BB]]:
+// CHECK:   [[SUBERROR:%.*]] = load [take] [[CAST_OUTPUT_MEM]]
+// CHECK:   switch_enum [[SUBERROR]] : $HomeworkError, case #HomeworkError.TooHard!enumelt: {{bb[0-9]+}}, case #HomeworkError.TooMuch!enumelt: {{bb[0-9]+}}, default [[SWITCH_MATCH_FAIL_BB:bb[0-9]+]],
+//
+// CHECK: [[SWITCH_MATCH_FAIL_BB]]([[SUBERROR:%.*]] : @owned $HomeworkError):
+// CHECK:   destroy_value [[SUBERROR]]
+// CHECK:   end_borrow [[BORROWED_ERROR]]
+// CHECK:   br [[RETHROW_BB:bb[0-9]+]]([[ERROR]] : $Error)
+//
+// CHECK: [[CAST_NO_BB]]:
+// CHECK:   end_borrow [[BORROWED_ERROR]]
+// CHECK:   br [[RETHROW_BB]]([[ERROR]] : $Error)
+//
+// CHECK: [[RETHROW_BB]]([[ERROR_FOR_RETHROW:%.*]] : @owned $Error):
+// CHECK:   throw [[ERROR_FOR_RETHROW]]
+// CHECK: } // end sil function '$s6errors22all_together_now_threeyAA3CatCSgSbKF'
+func all_together_now_three(_ flag: Bool) throws -> Cat? {
+  do {
+    return try dont_return(Cat())
+  } catch HomeworkError.TooHard {
+    return nil
+  } catch HomeworkError.TooMuch {
+    return nil
+  }
+}
+
 //   Catch in non-throwing context.
 // CHECK-LABEL: sil hidden @$s6errors11catch_a_catAA3CatCyF : $@convention(thin) () -> @owned Cat
 // CHECK-NEXT: bb0:

test/SILGen/indirect_enum.swift

@@ -149,10 +149,8 @@ func d() {}
 // CHECK-LABEL: sil hidden @$s13indirect_enum11switchTreeAyyAA0D1AOyxGlF : $@convention(thin) <T> (@guaranteed TreeA<T>) -> () {
 func switchTreeA<T>(_ x: TreeA<T>) {
   // CHECK: bb0([[ARG:%.*]] : @guaranteed $TreeA<T>):
-  // --           x +2
-  // CHECK:       [[ARG_COPY:%.*]] = copy_value [[ARG]]
-  // CHECK:       [[BORROWED_ARG_COPY:%.*]] = begin_borrow [[ARG_COPY]]
-  // CHECK:       switch_enum [[BORROWED_ARG_COPY]] : $TreeA<T>,
+  // --           x +0
+  // CHECK:       switch_enum [[ARG]] : $TreeA<T>,
   // CHECK:          case #TreeA.Nil!enumelt: [[NIL_CASE:bb1]],
   // CHECK:          case #TreeA.Leaf!enumelt.1: [[LEAF_CASE:bb2]],
   // CHECK:          case #TreeA.Branch!enumelt.1: [[BRANCH_CASE:bb3]],
@@ -168,7 +166,7 @@ func switchTreeA<T>(_ x: TreeA<T>) {
   // CHECK:       function_ref @$s13indirect_enum1b{{[_0-9a-zA-Z]*}}F
   // CHECK:       destroy_addr [[X]]
   // CHECK:       dealloc_stack [[X]]
-  // --           x +1
+  // --           x +0
   // CHECK:       br [[OUTER_CONT]]
   case .Leaf(let x):
     b(x)
@@ -204,8 +202,7 @@ func switchTreeA<T>(_ x: TreeA<T>) {
     c(x, y)
 
   // CHECK:     [[DEFAULT]]:
-  // --           x +1
-  // CHECK:       destroy_value [[ARG_COPY]]
+  // --           x +0
   default:
     d()
   }