[ownership] Do not lower copy_unowned_value to strong_retain_unowned.

gottesmm · gottesmm · commit f0ed0ead579e · 2018-06-28T11:14:15.000-07:00
The major important thing here is that by using copy_unowned_value we can guarantee that the non-ownership SIL ARC optimizer will treat the release associated with the strong_retain_unowned as on a distinc rc-identity from its argument. As an example of this problem consider the following SILGen like output: ---- %1 = copy_value %0 : $Builtin.NativeObject %2 = ref_to_unowned %1 %3 = copy_unowned_value %2 destroy_value %1 ... destroy_value %3 ---- In this case, we are converting a strong reference to an unowned value and then lifetime extending the value past the original value. After eliminating ownership this lowers to: ---- strong_retain %0 : $Builtin.NativeObject %1 = ref_to_unowned %0 strong_retain_unowned %1 strong_release %0 ... strong_release %0 ---- From an RC identity perspective, we have now blurred the lines in between %3 and %1 in the previous example. This can then result in the following miscompile: ---- %1 = ref_to_unowned %0 strong_retain_unowned %1 ... strong_release %0 ---- In this case, it is possible that we created a lifetime gap that will then cause strong_retain_unowned to assert. By not lowering copy_unowned_value throughout the SIL pipeline, we instead get this after lowering: ---- strong_retain %0 : $Builtin.NativeObject %1 = ref_to_unowned %0 %2 = copy_unowned_value %1 strong_release %0 ... strong_release %2 ---- And we do not miscompile since we preserved the high level rc identity pairing. There shouldn't be any performance impact since we do not really optimize strong_retain_unowned at the SIL level. I went through all of the places that strong_retain_unowned was referenced and added appropriate handling for copy_unowned_value. rdar://41328987 **NOTE** I am going to remove strong_retain_unowned in a forthcoming commit. I just want something more minimal for cherry-picking purposes. (cherry picked from commit 97367d3)
diff --git a/include/swift/Serialization/ModuleFormat.h b/include/swift/Serialization/ModuleFormat.h
@@ -55,7 +55,7 @@ const uint16_t VERSION_MAJOR = 0;
 /// describe what change you made. The content of this comment isn't important;
 /// it just ensures a conflict if two people change the module format.
 /// Don't worry about adhering to the 80-column limit for this line.
-const uint16_t VERSION_MINOR = 414; // Last change: track whether xrefs come from Clang
+const uint16_t VERSION_MINOR = 415; // Last change: {strong_retain,copy}_unowned
 
 using DeclIDField = BCFixed<31>;
 
diff --git a/lib/IRGen/IRGenSIL.cpp b/lib/IRGen/IRGenSIL.cpp
@@ -963,9 +963,7 @@ class IRGenSILFunction :
   void visitRetainValueInst(RetainValueInst *i);
   void visitRetainValueAddrInst(RetainValueAddrInst *i);
   void visitCopyValueInst(CopyValueInst *i);
-  void visitCopyUnownedValueInst(CopyUnownedValueInst *i) {
-    llvm_unreachable("unimplemented");
-  }
+  void visitCopyUnownedValueInst(CopyUnownedValueInst *i);
   void visitReleaseValueInst(ReleaseValueInst *i);
   void visitReleaseValueAddrInst(ReleaseValueAddrInst *i);
   void visitDestroyValueInst(DestroyValueInst *i);
@@ -3844,6 +3842,18 @@ visitStrongRetainUnownedInst(swift::StrongRetainUnownedInst *i) {
                                                        : irgen::Atomicity::NonAtomic);
 }
 
+void IRGenSILFunction::visitCopyUnownedValueInst(
+    swift::CopyUnownedValueInst *i) {
+  Explosion in = getLoweredExplosion(i->getOperand());
+  auto &ti = getReferentTypeInfo(*this, i->getOperand()->getType());
+  ti.strongRetainUnowned(*this, in, irgen::Atomicity::Atomic);
+  // Semantically we are just passing through the input parameter but as a
+  // strong reference... at LLVM IR level these type differences don't
+  // matter. So just set the lowered explosion appropriately.
+  Explosion output = getLoweredExplosion(i->getOperand());
+  setLoweredExplosion(i, output);
+}
+
 void IRGenSILFunction::visitUnownedRetainInst(swift::UnownedRetainInst *i) {
   Explosion lowered = getLoweredExplosion(i->getOperand());
   auto &ti = getReferentTypeInfo(*this, i->getOperand()->getType());
diff --git a/lib/SIL/SILBuilder.cpp b/lib/SIL/SILBuilder.cpp
@@ -264,7 +264,8 @@ static bool couldReduceStrongRefcount(SILInstruction *Inst) {
       isa<RetainValueInst>(Inst) || isa<UnownedRetainInst>(Inst) ||
       isa<UnownedReleaseInst>(Inst) || isa<StrongRetainUnownedInst>(Inst) ||
       isa<StoreWeakInst>(Inst) || isa<StrongRetainInst>(Inst) ||
-      isa<AllocStackInst>(Inst) || isa<DeallocStackInst>(Inst))
+      isa<AllocStackInst>(Inst) || isa<DeallocStackInst>(Inst) ||
+      isa<CopyUnownedValueInst>(Inst))
     return false;
 
   // Assign and copyaddr of trivial types cannot drop refcounts, and 'inits'
diff --git a/lib/SIL/SILVerifier.cpp b/lib/SIL/SILVerifier.cpp
@@ -1866,9 +1866,8 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
                                          "Operand of unowned_retain");
     require(unownedType->isLoadable(ResilienceExpansion::Maximal),
             "unowned_retain requires unowned type to be loadable");
-    require(F.hasQualifiedOwnership(),
-            "copy_unowned_value is only valid in functions with qualified "
-            "ownership");
+    // *NOTE* We allow copy_unowned_value to be used throughout the entire
+    // pipeline even though it is a higher level instruction.
   }
 
   void checkDestroyValueInst(DestroyValueInst *I) {
diff --git a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp
@@ -1372,6 +1372,7 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
     case SILInstructionKind::DeallocStackInst:
     case SILInstructionKind::StrongRetainInst:
     case SILInstructionKind::StrongRetainUnownedInst:
+    case SILInstructionKind::CopyUnownedValueInst:
     case SILInstructionKind::RetainValueInst:
     case SILInstructionKind::UnownedRetainInst:
     case SILInstructionKind::BranchInst:
diff --git a/lib/SILOptimizer/Analysis/MemoryBehavior.cpp b/lib/SILOptimizer/Analysis/MemoryBehavior.cpp
@@ -152,6 +152,7 @@ class MemoryBehaviorVisitor
   }
   REFCOUNTINC_MEMBEHAVIOR_INST(StrongRetainInst)
   REFCOUNTINC_MEMBEHAVIOR_INST(StrongRetainUnownedInst)
+  REFCOUNTINC_MEMBEHAVIOR_INST(CopyUnownedValueInst)
   REFCOUNTINC_MEMBEHAVIOR_INST(UnownedRetainInst)
   REFCOUNTINC_MEMBEHAVIOR_INST(RetainValueInst)
 #undef REFCOUNTINC_MEMBEHAVIOR_INST
diff --git a/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp b/lib/SILOptimizer/Analysis/SideEffectAnalysis.cpp
@@ -481,6 +481,7 @@ void FunctionSideEffects::analyzeInstruction(SILInstruction *I) {
     return;
   case SILInstructionKind::StrongRetainInst:
   case SILInstructionKind::StrongRetainUnownedInst:
+  case SILInstructionKind::CopyUnownedValueInst:
   case SILInstructionKind::RetainValueInst:
   case SILInstructionKind::UnownedRetainInst:
     getEffectsOn(I->getOperand(0))->Retains = true;
diff --git a/lib/SILOptimizer/Mandatory/GuaranteedARCOpts.cpp b/lib/SILOptimizer/Mandatory/GuaranteedARCOpts.cpp
@@ -75,6 +75,7 @@ static bool couldReduceStrongRefcount(SILInstruction *Inst) {
   if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst) ||
       isa<RetainValueInst>(Inst) || isa<UnownedRetainInst>(Inst) ||
       isa<UnownedReleaseInst>(Inst) || isa<StrongRetainUnownedInst>(Inst) ||
+      isa<CopyUnownedValueInst>(Inst) ||
       isa<StoreWeakInst>(Inst) || isa<StrongRetainInst>(Inst) ||
       isa<AllocStackInst>(Inst) || isa<DeallocStackInst>(Inst) ||
       isa<BeginAccessInst>(Inst) || isa<EndAccessInst>(Inst) ||
diff --git a/lib/SILOptimizer/Transforms/DeadStoreElimination.cpp b/lib/SILOptimizer/Transforms/DeadStoreElimination.cpp
@@ -149,6 +149,7 @@ static bool isDeadStoreInertInstruction(SILInstruction *Inst) {
   switch (Inst->getKind()) {
   case SILInstructionKind::StrongRetainInst:
   case SILInstructionKind::StrongRetainUnownedInst:
+  case SILInstructionKind::CopyUnownedValueInst:
   case SILInstructionKind::UnownedRetainInst:
   case SILInstructionKind::RetainValueInst:
   case SILInstructionKind::DeallocStackInst:
diff --git a/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp b/lib/SILOptimizer/Transforms/OwnershipModelEliminator.cpp
@@ -57,7 +57,6 @@ struct OwnershipModelEliminatorVisitor
   bool visitStoreInst(StoreInst *SI);
   bool visitStoreBorrowInst(StoreBorrowInst *SI);
   bool visitCopyValueInst(CopyValueInst *CVI);
-  bool visitCopyUnownedValueInst(CopyUnownedValueInst *CVI);
   bool visitDestroyValueInst(DestroyValueInst *DVI);
   bool visitLoadBorrowInst(LoadBorrowInst *LBI);
   bool visitBeginBorrowInst(BeginBorrowInst *BBI) {
@@ -160,19 +159,6 @@ bool OwnershipModelEliminatorVisitor::visitCopyValueInst(CopyValueInst *CVI) {
   return true;
 }
 
-bool OwnershipModelEliminatorVisitor::visitCopyUnownedValueInst(
-    CopyUnownedValueInst *CVI) {
-  B.createStrongRetainUnowned(CVI->getLoc(), CVI->getOperand(),
-                              B.getDefaultAtomicity());
-  // Users of copy_value_unowned expect an owned value. So we need to convert
-  // our unowned value to a ref.
-  auto *UTRI =
-      B.createUnownedToRef(CVI->getLoc(), CVI->getOperand(), CVI->getType());
-  CVI->replaceAllUsesWith(UTRI);
-  CVI->eraseFromParent();
-  return true;
-}
-
 bool OwnershipModelEliminatorVisitor::visitUnmanagedRetainValueInst(
     UnmanagedRetainValueInst *URVI) {
   // Now that we have set the unqualified ownership flag, destroy value
diff --git a/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp b/lib/SILOptimizer/Transforms/RedundantLoadElimination.cpp
@@ -156,6 +156,7 @@ static bool isRLEInertInstruction(SILInstruction *Inst) {
   case SILInstructionKind::IsUniqueInst:
   case SILInstructionKind::IsUniqueOrPinnedInst:
   case SILInstructionKind::FixLifetimeInst:
+  case SILInstructionKind::CopyUnownedValueInst:
     return true;
   default:
     return false;
diff --git a/test/IRGen/copy_value_destroy_value.sil b/test/IRGen/copy_value_destroy_value.sil
@@ -36,10 +36,22 @@ bb0(%0 : $Builtin.Int32):
 // CHECK: call %swift.refcounted* @swift_retain(%swift.refcounted* returned [[VAL2]])
 // CHECK: call void @swift_release(%swift.refcounted* [[VAL1]])
 // CHECK: call void @swift_release(%swift.refcounted* [[VAL2]])
-sil @non_trivial : $@convention(thin) (Foo) -> () {
+sil @non_trivial : $@convention(thin) (@guaranteed Foo) -> () {
 bb0(%0 : $Foo):
   %1 = copy_value %0 : $Foo
   destroy_value %1 : $Foo
   %2 = tuple()
   return %2 : $()
 }
+
+// CHECK: define{{( protected)?}} swiftcc void @non_trivial_unowned(
+// CHECK: call %swift.refcounted* @swift_unownedRetainStrong(%swift.refcounted* returned %0)
+// CHECK: call void @swift_release(%swift.refcounted* %0)
+sil @non_trivial_unowned : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
+bb0(%0 : $Builtin.NativeObject):
+  %1 = ref_to_unowned %0 : $Builtin.NativeObject to $@sil_unowned Builtin.NativeObject
+  %2 = copy_unowned_value %1 : $@sil_unowned Builtin.NativeObject
+  destroy_value %2 : $Builtin.NativeObject
+  %9999 = tuple()
+  return %9999 : $()
+}
diff --git a/test/Interpreter/strong_retain_unowned_mispairing.swift b/test/Interpreter/strong_retain_unowned_mispairing.swift
@@ -0,0 +1,44 @@
+// RUN: %target-run-simple-opt-O-swift
+// REQUIRES: executable_test
+
+// We were crashing here due to not preserving rc identity. 
+// rdar://41328987
+
+func takeEscaping(closure: @escaping (String) -> Void) {}
+
+public class Helper {
+  weak var o: P?
+
+  @_optimize(none)
+  init(o: P) {
+    self.o = o
+  }
+}
+
+protocol P: class {}
+
+public class Binding: P {
+  private var helper: Helper?
+
+  public init() {
+    helper = Helper(o: self)
+    
+    // Listen to model changes
+    takeEscaping { [unowned self] (value: String) in
+      self.update()
+    }
+
+    takeEscaping { [unowned self] (value: String) in
+      self.update()
+    }
+  }
+
+  func update() {}
+}
+
+@_optimize(none)
+func testCrash() {
+  _ = Binding()
+}
+
+testCrash()
diff --git a/test/SILOptimizer/ownership_model_eliminator.sil b/test/SILOptimizer/ownership_model_eliminator.sil
@@ -114,11 +114,12 @@ bb0(%0 : @owned $Builtin.NativeObject):
   return %9999 : $()
 }
 
+// We no longer lower copy_unowned_value. So make sure that we actually don't.
+//
 // CHECK-LABEL: sil @copy_unowned_value_test : $@convention(thin) (@owned @sil_unowned Builtin.NativeObject) -> () {
 // CHECK: bb0([[ARG:%.*]] : $@sil_unowned Builtin.NativeObject):
-// CHECK-NEXT: strong_retain_unowned [[ARG]] : $@sil_unowned Builtin.NativeObject
-// CHECK-NEXT: [[OWNED_ARG:%.*]] = unowned_to_ref [[ARG]] : $@sil_unowned Builtin.NativeObject to $Builtin.NativeObject
-// CHECK-NEXT: strong_release [[OWNED_ARG]] : $Builtin.NativeObject
+// CHECK-NEXT: [[STRONG:%.*]] = copy_unowned_value [[ARG]] : $@sil_unowned Builtin.NativeObject
+// CHECK-NEXT: strong_release [[STRONG]] : $Builtin.NativeObject
 // CHECK-NEXT: unowned_release [[ARG]] : $@sil_unowned Builtin.NativeObject
 // CHECK-NEXT: tuple ()
 // CHECK-NEXT: return
diff --git a/test/lit.cfg b/test/lit.cfg
@@ -985,6 +985,16 @@ if not getattr(config, 'target_run_simple_swift', None):
         '%s %s %%s -o %%t/a.out -module-name main && '
         '%s %%t/a.out'
         % (config.target_build_swift, mcp_opt, config.target_run))
+    config.target_run_simple_opt_O_swift = (
+        '%%empty-directory(%%t) && '
+        '%s %s -O %%s -o %%t/a.out -module-name main && '
+        '%s %%t/a.out'
+        % (config.target_build_swift, mcp_opt, config.target_run))
+    config.target_run_simple_opt_Osize_swift = (
+        '%%empty-directory(%%t) && '
+        '%s %s -Osize %%s -o %%t/a.out -module-name main && '
+        '%s %%t/a.out'
+        % (config.target_build_swift, mcp_opt, config.target_run))
     config.target_run_stdlib_swift = (
         'rm -rf %%t && mkdir -p %%t && '
         '%s %s %%s -o %%t/a.out -module-name main '
@@ -1065,6 +1075,8 @@ config.substitutions.append(('%target-swift-frontend', config.target_swift_front
 
 config.substitutions.append(('%target-run-simple-swiftgyb', config.target_run_simple_swiftgyb))
 config.substitutions.append(('%target-run-simple-swift', config.target_run_simple_swift))
+config.substitutions.append(('%target-run-simple-opt-O-swift', config.target_run_simple_opt_O_swift))
+config.substitutions.append(('%target-run-simple-opt-Osize-swift', config.target_run_simple_opt_Osize_swift))
 config.substitutions.append(('%target-run-stdlib-swiftgyb', config.target_run_stdlib_swiftgyb))
 config.substitutions.append(('%target-run-stdlib-swift', config.target_run_stdlib_swift))
 config.substitutions.append(('%target-repl-run-simple-swift', subst_target_repl_run_simple_swift))

Original file line number	Diff line number	Diff line change
`@@ -152,6 +152,7 @@ class MemoryBehaviorVisitor`
`152`	`152`	`}`
`153`	`153`	`REFCOUNTINC_MEMBEHAVIOR_INST(StrongRetainInst)`
`154`	`154`	`REFCOUNTINC_MEMBEHAVIOR_INST(StrongRetainUnownedInst)`
	`155`	`+ REFCOUNTINC_MEMBEHAVIOR_INST(CopyUnownedValueInst)`
`155`	`156`	`REFCOUNTINC_MEMBEHAVIOR_INST(UnownedRetainInst)`
`156`	`157`	`REFCOUNTINC_MEMBEHAVIOR_INST(RetainValueInst)`
`157`	`158`	`#undef REFCOUNTINC_MEMBEHAVIOR_INST`