Merge pull request #61393 from atrick/fix-closure-release

atrick · web-flow · commit 18143e27971d · 2022-09-30T19:38:50.000-07:00
Fix unchecked_bitwise_cast to "escape" its operand's ownership
diff --git a/lib/SIL/IR/OperandOwnership.cpp b/lib/SIL/IR/OperandOwnership.cpp
@@ -238,9 +238,31 @@ OPERAND_OWNERSHIP(PointerEscape, ProjectExistentialBox)
 OPERAND_OWNERSHIP(PointerEscape, UncheckedOwnershipConversion)
 OPERAND_OWNERSHIP(PointerEscape, ConvertEscapeToNoEscape)
 
+// UncheckedBitwiseCast ownership behaves like RefToUnowned. It produces an
+// Unowned values from a non-trivial value, without consuming or borrowing the
+// non-trivial value. Unlike RefToUnowned, a bitwise cast works on a compound
+// value and may truncate the value. The resulting value is still Unowned and
+// should be immediately copied to produce an owned value. These happen for two
+// reasons:
+//
+// (1) A Builtin.reinterpretCast is used on a nontrivial type. If the result is
+// non-trivial, then, as part of emitting the cast, SILGen emits a copy_value
+// immediately after the unchecked_bitwise_cast for all uses of the cast.
+//
+// (2) SILGen emits special conversions using SILGenBuilder's
+// createUncheckedBitCast utility. For non-trivial types, this emits an
+// unchecked_bitwise_cast immediately followed by a copy.
+//
+// The only thing protecting the lifetime of the Unowned value is the cast
+// operand's PointerEscape ownership, which prevents OSSA analysis and blocks
+// most optimization of the incoming value.
+//
+// TODO: Verify that Unowned values are only used by copies and that the cast
+// operand's lifetime exceeds the copies.
+OPERAND_OWNERSHIP(PointerEscape, UncheckedBitwiseCast)
+
 // Instructions that escape reference bits with unenforced lifetime.
 // TODO: verify that BitwiseEscape results always have a trivial type.
-OPERAND_OWNERSHIP(BitwiseEscape, UncheckedBitwiseCast)
 OPERAND_OWNERSHIP(BitwiseEscape, ValueToBridgeObject)
 OPERAND_OWNERSHIP(BitwiseEscape, RefToRawPointer)
 OPERAND_OWNERSHIP(BitwiseEscape, UncheckedTrivialBitCast)
diff --git a/lib/SIL/Utils/OwnershipUtils.cpp b/lib/SIL/Utils/OwnershipUtils.cpp
@@ -377,19 +377,26 @@ bool swift::findExtendedUsesOfSimpleBorrowedValue(
   return true;
 }
 
+// TODO: refactor this with SSAPrunedLiveness::computeLiveness.
 bool swift::findUsesOfSimpleValue(SILValue value,
                                   SmallVectorImpl<Operand *> *usePoints) {
   for (auto *use : value->getUses()) {
-    if (use->getOperandOwnership() == OperandOwnership::Borrow) {
+    switch (use->getOperandOwnership()) {
+    case OperandOwnership::PointerEscape:
+      return false;
+    case OperandOwnership::Borrow:
       if (!BorrowingOperand(use).visitScopeEndingUses([&](Operand *end) {
-            if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
-              return false;
-            }
-            usePoints->push_back(end);
-            return true;
-          })) {
+        if (end->getOperandOwnership() == OperandOwnership::Reborrow) {
+          return false;
+        }
+        usePoints->push_back(end);
+        return true;
+      })) {
         return false;
       }
+      break;
+    default:
+      break;
     }
     usePoints->push_back(use);
   }
diff --git a/test/SILOptimizer/copy_propagation.sil b/test/SILOptimizer/copy_propagation.sil
@@ -875,6 +875,7 @@ bb0(%instance : @owned $C):
 // CHECK-NOT:     destroy_value
 // CHECK-LABEL: } // end sil function 'dont_extend_beyond_nonoverlapping_endaccess_after_destroyvalue_in_consuming_block'
 sil [ossa] @dont_extend_beyond_nonoverlapping_endaccess_after_destroyvalue_in_consuming_block : $@convention(thin) () -> () {
+bb0:
   %instance = apply undef() : $@convention(thin) () -> @owned C
   %addr = alloc_stack [lexical] $C, var
   %access = begin_access [static] [modify] %addr : $*C
@@ -885,3 +886,52 @@ sil [ossa] @dont_extend_beyond_nonoverlapping_endaccess_after_destroyvalue_in_co
   %retval = tuple()
   return %retval : $()
 }
+
+sil @closureGetter : $@convention(thin) () -> @owned Optional<@Sendable @callee_guaranteed () -> ()>
+
+// This pattern is produced by SILGen doUncheckedConversion.
+// rdar://100527903 (Overrelease of optional closure when using shorthand syntax)
+//
+// Copy propagation cannot handle a PointerEscape.
+//
+// CHECK-LABEL: sil hidden [ossa] @testBitwiseClosureEscape : $@convention(thin) () -> () {
+// CHECK: [[V:%.*]] = apply %0() : $@convention(thin) () -> @owned Optional<@Sendable @callee_guaranteed () -> ()>
+// CHECK: [[CP1:%.*]] = copy_value [[V]] : $Optional<@Sendable @callee_guaranteed () -> ()>
+// CHECK: destroy_value [[V]] : $Optional<@Sendable @callee_guaranteed () -> ()>
+// CHECK: [[CAST:%.*]] = unchecked_bitwise_cast [[CP1]] : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()>
+// CHECK-NEXT: [[CASTCP:%.*]] = copy_value [[CAST]] : $Optional<@callee_guaranteed () -> ()>
+// CHECK:      store [[CASTCP]] to [init]
+// CHECK:   destroy_value [[CP1]] : $Optional<@Sendable @callee_guaranteed () -> ()>
+// CHECK-LABEL: } // end sil function 'testBitwiseClosureEscape'
+sil hidden [ossa] @testBitwiseClosureEscape : $@convention(thin) () -> () {
+bb0:
+  %2 = function_ref @closureGetter : $@convention(thin) () -> @owned Optional<@Sendable @callee_guaranteed () -> ()>
+  %3 = apply %2() : $@convention(thin) () -> @owned Optional<@Sendable @callee_guaranteed () -> ()>
+  %13 = copy_value %3 : $Optional<@Sendable @callee_guaranteed () -> ()>
+  %14 = alloc_stack $Optional<@callee_guaranteed () -> ()>
+  destroy_value %3 : $Optional<@Sendable @callee_guaranteed () -> ()>
+  %17 = unchecked_bitwise_cast %13 : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()>
+  %18 = copy_value %17 : $Optional<@callee_guaranteed () -> ()>
+  store %18 to [init] %14 : $*Optional<@callee_guaranteed () -> ()>
+  destroy_addr %14 : $*Optional<@callee_guaranteed () -> ()>
+  destroy_value %13 : $Optional<@Sendable @callee_guaranteed () -> ()>
+  dealloc_stack %14 : $*Optional<@callee_guaranteed () -> ()>
+  %99 = tuple ()
+  return %99 : $()
+}
+
+// Lexical destroy hoisting cannot handle a PointerEscape.
+//
+// CHECK-LABEL: sil [ossa] @testNoLexicalHoistEscape : $@convention(thin) (@owned Optional<@Sendable @callee_guaranteed () -> ()>) -> @owned Optional<@callee_guaranteed () -> ()> {
+// CHECK: bb0(%0 : @owned $Optional<@Sendable @callee_guaranteed () -> ()>):
+// CHECK:   [[CAST:%.*]] = unchecked_bitwise_cast %0 : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()>
+// CHECK:   [[COPY:%.*]] = copy_value [[CAST]] : $Optional<@callee_guaranteed () -> ()>
+// CHECK:   destroy_value %0 : $Optional<@Sendable @callee_guaranteed () -> ()>
+// CHECK-LABEL: } // end sil function 'testNoLexicalHoistEscape'
+sil [ossa] @testNoLexicalHoistEscape : $@convention(thin) (@owned Optional<@Sendable @callee_guaranteed () -> ()>) -> @owned Optional<@callee_guaranteed () -> ()> {
+bb0(%0 : @owned $Optional<@Sendable @callee_guaranteed () -> ()>):
+  %cast = unchecked_bitwise_cast %0 : $Optional<@Sendable @callee_guaranteed () -> ()> to $Optional<@callee_guaranteed () -> ()>
+  %copy = copy_value %cast : $Optional<@callee_guaranteed () -> ()>
+  destroy_value %0 : $Optional<@Sendable @callee_guaranteed () -> ()>
+  return %copy : $Optional<@callee_guaranteed () -> ()>
+}
diff --git a/test/SILOptimizer/copy_propagation_borrow.sil b/test/SILOptimizer/copy_propagation_borrow.sil
@@ -243,13 +243,12 @@ bb3:
 // CHECK:      bb1:
 // CHECK-NEXT:   [[COPY:%.*]] = copy_value [[OUTERCOPY]] : $C
 // CHECK-NEXT:   apply %{{.*}}([[COPY]]) : $@convention(thin) (@owned C) -> ()
-// CHECK-NEXT:   destroy_value %0 : $C
 // CHECK-NEXT:   br bb3
 // CHECK:      bb2:
-// CHECK-NEXT:   destroy_value %0 : $C
 // CHECK-NEXT:   br bb3
 // CHECK:      bb3:
 // CHECK-NEXT:   destroy_value [[OUTERCOPY]] : $C
+// CHECK-NEXT:   destroy_value %0 : $C
 // CHECK-LABEL: } // end sil function 'testLocalBorrowPostDomDestroy'
 sil [ossa] @testLocalBorrowPostDomDestroy : $@convention(thin) (@owned C) -> () {
 bb0(%0 : @owned $C):
@@ -285,11 +284,9 @@ bb3:
 // CHECK:      bb1:
 // CHECK-NEXT:   apply %{{.*}}([[OUTERCOPY]]) : $@convention(thin) (@guaranteed C) -> ()
 // CHECK-NEXT:   apply %{{.*}}([[OUTERCOPY]]) : $@convention(thin) (@owned C) -> ()
-// CHECK-NEXT:   destroy_value %0 : $C
 // CHECK-NEXT:   br bb3
 // CHECK:      bb2:
 // CHECK-NEXT:   apply %{{.*}}([[OUTERCOPY]]) : $@convention(thin) (@guaranteed C) -> ()
-// CHECK-NEXT:   destroy_value %0 : $C
 // CHECK-NEXT:   destroy_value [[OUTERCOPY]] : $C
 // CHECK-NEXT:   br bb3
 // CHECK:      bb3:
@@ -328,13 +325,11 @@ bb3:
 // CHECK-NEXT:  apply %{{.*}}([[OUTERCOPY]]) : $@convention(thin) (@guaranteed C) -> ()
 // CHECK-NEXT:  [[ARGCOPY:%.*]] = copy_value [[OUTERCOPY]] : $C
 // CHECK-NEXT:  apply %2([[OUTERCOPY]], [[ARGCOPY:%.*]]) : $@convention(thin) (@owned C, @owned C) -> ()
-// CHECK-NEXT:  destroy_value %0 : $C
 // CHECK-NEXT:  br bb3
 // CHECK:     bb2:
 // CHECK-NEXT:  apply %{{.*}}([[OUTERCOPY]]) : $@convention(thin) (@guaranteed C) -> ()
 //
 // This copy would be eliminated if the outer lifetime were also canonicalized (no unchecked_ownership_conversion)
-// CHECK-NEXT:  destroy_value %0 : $C
 // CHECK-NEXT:  [[COPY2:%.*]] = copy_value [[OUTERCOPY]] : $C
 // CHECK-NEXT:  destroy_value [[COPY2]] : $C
 // CHECK-NEXT:  destroy_value %4 : $C
