SILCombine: fix a miscompile caused by dead-apply elimination

include/swift/SILOptimizer/Utils/Local.h

@@ -282,6 +282,9 @@ class ValueLifetimeAnalysis {
     return LiveBlocks.count(BB) && BB != DefValue->getParent();
   }
 
+  /// Checks if there is a dealloc_ref inside the value's live range.
+  bool containsDeallocRef(const Frontier &Frontier);
+
   /// For debug dumping.
   void dump() const;
 

lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

@@ -547,6 +547,13 @@ bool SILCombiner::eraseApply(FullApplySite FAS, const UserListTy &Users) {
   // Compute the places where we have to insert release-instructions for the
   // owned arguments. This must not be done before the result of the
   // apply is destroyed. Therefore we compute the lifetime of the apply-result.
+
+  // TODO: this is not required anymore when we have ownership SIL. But with
+  // the current SIL it can happen that the retain of a parameter is moved
+  // _after_ the apply.
+  // When we have ownership SIL we can just destroy the parameters at the apply
+  // location.
+
   ValueLifetimeAnalysis VLA(FAS.getInstruction(), Users);
   ValueLifetimeAnalysis::Frontier Frontier;
   if (Users.empty()) {
@@ -556,6 +563,12 @@ bool SILCombiner::eraseApply(FullApplySite FAS, const UserListTy &Users) {
   } else {
     if (!VLA.computeFrontier(Frontier, ValueLifetimeAnalysis::DontModifyCFG))
       return false;
+    // As we are extending the lifetimes of owned parameters, we have to make
+    // sure that no dealloc_ref instructions are within this extended liferange.
+    // It could be that the dealloc_ref is deallocating a parameter and then
+    // we would have a release after the dealloc.
+    if (VLA.containsDeallocRef(Frontier))
+      return false;
   }
 
   // Release and destroy any owned or in-arguments.

lib/SILOptimizer/Utils/Local.cpp

@@ -1369,6 +1369,44 @@ bool ValueLifetimeAnalysis::isWithinLifetime(SILInstruction *Inst) {
   llvm_unreachable("Expected to find use of value in block!");
 }
 
+// Searches \p BB backwards from the instruction before \p FrontierInst
+// to the beginning of the list and returns true if we find a dealloc_ref
+// /before/ we find \p DefValue (the instruction that defines our target value).
+static bool blockContainsDeallocRef(SILBasicBlock *BB, SILInstruction *DefValue,
+                                    SILInstruction *FrontierInst) {
+  SILBasicBlock::reverse_iterator End = BB->rend();
+  SILBasicBlock::reverse_iterator Iter = FrontierInst->getReverseIterator();
+  for (++Iter; Iter != End; ++Iter) {
+    SILInstruction *I = &*Iter;
+    if (isa<DeallocRefInst>(I))
+      return true;
+    if (I == DefValue)
+      return false;
+  }
+  return false;
+}
+
+bool ValueLifetimeAnalysis::containsDeallocRef(const Frontier &Frontier) {
+  SmallPtrSet<SILBasicBlock *, 8> FrontierBlocks;
+  // Search in live blocks where the value is not alive until the end of the
+  // block, i.e. the live range is terminated by a frontier instruction.
+  for (SILInstruction *FrontierInst : Frontier) {
+    SILBasicBlock *BB = FrontierInst->getParent();
+    if (blockContainsDeallocRef(BB, DefValue, FrontierInst))
+      return true;
+    FrontierBlocks.insert(BB);
+  }
+  // Search in all other live blocks where the value is alive until the end of
+  // the block.
+  for (SILBasicBlock *BB : LiveBlocks) {
+    if (FrontierBlocks.count(BB) == 0) {
+      if (blockContainsDeallocRef(BB, DefValue, BB->getTerminator()))
+        return true;
+    }
+  }
+  return false;
+}
+
 void ValueLifetimeAnalysis::dump() const {
   llvm::errs() << "lifetime of def: " << *DefValue;
   for (SILInstruction *Use : UserSet) {

test/SILOptimizer/sil_combine.sil

@@ -1980,6 +1980,89 @@ bb3:
   return %0 : $B
 }
 
+// CHECK-LABEL: sil @dont_delete_readonly_function_dealloc_ref_simple
+// CHECK:      apply
+// CHECK-NEXT: dealloc_ref
+// CHECK:      return
+sil @dont_delete_readonly_function_dealloc_ref_simple : $@convention(thin) () -> () {
+bb0:
+  %3 = alloc_ref [stack] $B
+  %f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
+  %r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
+  dealloc_ref [stack] %3 : $B
+  strong_release %r : $B
+  %10 = tuple ()
+  return %10 : $()
+}
+
+// CHECK-LABEL: sil @dont_delete_readonly_function_dealloc_ref_multibb
+// CHECK:      apply
+// CHECK:      dealloc_ref
+// CHECK-NEXT: strong_release
+// CHECK:      dealloc_ref
+// CHECK-NEXT: strong_release
+// CHECK:      return
+sil @dont_delete_readonly_function_dealloc_ref_multibb : $@convention(thin) () -> () {
+bb0:
+  %3 = alloc_ref [stack] $B
+  %f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
+  %r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
+  cond_br undef, bb1, bb2
+
+bb1:
+  dealloc_ref [stack] %3 : $B
+  strong_release %r : $B
+  br bb3
+
+bb2:
+  dealloc_ref [stack] %3 : $B
+  strong_release %r : $B
+  br bb3
+
+bb3:
+  %10 = tuple ()
+  return %10 : $()
+}
+
+// CHECK-LABEL: sil @delete_readonly_function_dealloc_ref_simple
+// CHECK-NOT:  apply
+// CHECK:      return
+sil @delete_readonly_function_dealloc_ref_simple : $@convention(thin) () -> () {
+bb0:
+  %3 = alloc_ref [stack] $B
+  %f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
+  %r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
+  strong_release %r : $B
+  dealloc_ref [stack] %3 : $B
+  %10 = tuple ()
+  return %10 : $()
+}
+
+// CHECK-LABEL: sil @delete_readonly_function_dealloc_ref_multibb
+// CHECK-NOT:  apply
+// CHECK:      return
+sil @delete_readonly_function_dealloc_ref_multibb : $@convention(thin) () -> () {
+bb0:
+  %3 = alloc_ref [stack] $B
+  %f = function_ref @readonly_owned : $@convention(thin) (@owned B) -> @owned B
+  %r = apply %f(%3) : $@convention(thin) (@owned B) -> @owned B
+  cond_br undef, bb1, bb2
+
+bb1:
+  strong_release %r : $B
+  dealloc_ref [stack] %3 : $B
+  br bb3
+
+bb2:
+  strong_release %r : $B
+  dealloc_ref [stack] %3 : $B
+  br bb3
+
+bb3:
+  %10 = tuple ()
+  return %10 : $()
+}
+
 //CHECK-LABEL: sil @test_delete_readonly_try_apply
 //CHECK:      bb0(%0 : $ZZZ):
 //CHECK-NEXT:   [[IL:%[0-9]+]] = integer_literal $Builtin.Int1, 0

test/SILOptimizer/silcombine_runtime_crash.swift

@@ -0,0 +1,21 @@
+// RUN: %empty-directory(%t) 
+// RUN: %target-build-swift -O %s -o %t/a.out
+// RUN: %target-run %t/a.out | %FileCheck %s
+
+// REQUIRES: executable_test
+
+// This is an end-to-end test for SR-9627.
+
+@inline(never)
+func save(value: Double?) {
+   var params: [[String : Any]] = [["a": 0]]
+   params = [[
+     "b": 0,
+     "c": value.map({ String.init($0) }) as Any
+   ]]
+}
+
+save(value: 0)
+
+// CHECK: ok
+print("ok")