From ae04531b5f52fac103683e9b0d8b50593f278300 Mon Sep 17 00:00:00 2001
From: Andrew Trick <atrick@apple.com>
Date: Thu, 23 Jan 2020 14:59:32 -0800
Subject: [PATCH] Fix EscapeAnalysis value_to_bridge_object and
 strong_copy_unowned_value.

Noticed via code inspection. This could potentially miscompile, but we
haven't seen that happen to my knowledge.

Both value_to_bridge_object and strong_copy_XXX need to escape their
resulting value.

The implementation seemed to assume that it is conservatively correct
simply to avoid building a connection graph node for an value. This is
*not* true. Any value that has a pointer type requires a connection
graph node. The only way to be conservative is to create the value
node *and* point it to an escaping content node.

We can always declare that certain special types are not considered
pointer types, but then we need to handle all conversions from those
types to pointer types by escaping the resulting
pointer. BridgeObjects are often on the performance-critical path.
---
 lib/SILOptimizer/Analysis/EscapeAnalysis.cpp | 12 +++---
 test/SILOptimizer/escape_analysis.sil        | 44 ++++++++++++++++++++
 2 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp
index 9a7c9ff814832..e7e8636fd30f6 100644
--- a/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp
+++ b/lib/SILOptimizer/Analysis/EscapeAnalysis.cpp
@@ -2020,12 +2020,9 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
       ConGraph->getNode(cast<SingleValueInstruction>(I));
       return;
 
-#define UNCHECKED_REF_STORAGE(Name, ...)                                       \
-  case SILInstructionKind::StrongCopy##Name##ValueInst:
 #define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...)            \
   case SILInstructionKind::Name##RetainInst:                                   \
-  case SILInstructionKind::StrongRetain##Name##Inst:                           \
-  case SILInstructionKind::StrongCopy##Name##ValueInst:
+  case SILInstructionKind::StrongRetain##Name##Inst:
 #include "swift/AST/ReferenceStorage.def"
     case SILInstructionKind::DeallocStackInst:
     case SILInstructionKind::StrongRetainInst:
@@ -2043,8 +2040,11 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
     case SILInstructionKind::SetDeallocatingInst:
     case SILInstructionKind::FixLifetimeInst:
     case SILInstructionKind::ClassifyBridgeObjectInst:
-    case SILInstructionKind::ValueToBridgeObjectInst:
-      // These instructions don't have any effect on escaping.
+      // Early bailout: These instructions never produce a pointer value and
+      // have no escaping effect on their operands.
+      assert(!llvm::any_of(I->getResults(), [this](SILValue result) {
+        return isPointer(result);
+      }));
       return;
 
 #define ALWAYS_OR_SOMETIMES_LOADABLE_CHECKED_REF_STORAGE(Name, ...) \
diff --git a/test/SILOptimizer/escape_analysis.sil b/test/SILOptimizer/escape_analysis.sil
index 46af879e675bd..eddfac11b7f09 100644
--- a/test/SILOptimizer/escape_analysis.sil
+++ b/test/SILOptimizer/escape_analysis.sil
@@ -1844,3 +1844,47 @@ bb3(%4 : $AnyObject):
   %5 = tuple ()
   return %5 : $()
 }
+
+// Test value_to_bridge_object merged with a local reference. A graph node
+// must be created for all values involved, and they must point to an
+// escaping content node.
+// CHECK-LABEL: CG of testValueToBridgeObject
+// CHECK-NEXT:   Val [ref] %1 Esc: , Succ: (%5.1)
+// CHECK-NEXT:   Val [ref] %5 Esc: , Succ: (%5.1)
+// CHECK-NEXT:   Con [int] %5.1 Esc: G, Succ: (%5.2)
+// CHECK-NEXT:   Con %5.2 Esc: G, Succ:
+// CHECK-NEXT:   Val [ref] %7 Esc: , Succ: (%5.1), %1, %5
+// CHECK-NEXT:   Ret [ref] return Esc: , Succ: %7
+// CHECK-LABEL: End
+sil @testValueToBridgeObject : $@convention(thin) (Builtin.Word) -> C {
+bb0(%0 : $Builtin.Word):
+  %1 = alloc_ref $C
+  cond_br undef, bb1, bb2
+
+bb1:
+  %derived = ref_to_bridge_object %1 : $C, %0 : $Builtin.Word
+  br bb3(%derived : $Builtin.BridgeObject)
+
+bb2:
+  %5 = value_to_bridge_object %0 : $Builtin.Word
+  br bb3(%5 : $Builtin.BridgeObject)
+
+bb3(%7 : $Builtin.BridgeObject):
+  %result = bridge_object_to_ref %7 : $Builtin.BridgeObject to $C
+  return %result : $C
+}
+
+// Test strong_copy_unowned_value returned. It must be marked escaping,
+// not simply "returned", because it's reference is formed from a
+// function argument.
+// CHECK-LABEL: CG of testStrongCopyUnowned
+// CHECK-NEXT:   Val [ref] %1 Esc: , Succ: (%1.1)
+// CHECK-NEXT:   Con [int] %1.1 Esc: G, Succ: (%1.2)
+// CHECK-NEXT:   Con %1.2 Esc: G, Succ:
+// CHECK-NEXT:   Ret [ref] return Esc: , Succ: %1
+// CHECK-LABEL: End
+sil [ossa] @testStrongCopyUnowned : $@convention(thin) (@guaranteed @sil_unowned Builtin.NativeObject) -> @owned Builtin.NativeObject {
+bb0(%0 : @guaranteed $@sil_unowned Builtin.NativeObject):
+  %1 = strong_copy_unowned_value %0 : $@sil_unowned Builtin.NativeObject
+  return %1 : $Builtin.NativeObject
+}