[6.2][cxx-interop] Delay lowering unowned convention until ownership elimination

Explanation: Unowned result conventions do not work well with OSSA. Retain the
result right after the call when we come out of OSSA so we can treat the
returned value as if it was owned when we do optimizations.

This fix a miscompilation due to the DestroyAddrHoisting pass hoisting destroys
above copies with unowned sources. When the destroyed object was the last
reference to the pointed memory the copy is happening too late resulting in
a use after free.

Issues: rdar://160462854
Original PRs: #84612
Risk: We change where retaining of the unowned return values
happen in the optimization pipeline. It is hard to anticipate all the
possible effects but it should make the optimizer more correct.
Testing: Added a compiler test.
Reviewers: @eeckstein, @atrick

Author: Xazax-hun

lib/SIL/IR/SILType.cpp

@@ -656,6 +656,11 @@ SILResultInfo::getOwnershipKind(SILFunction &F,
   case ResultConvention::Owned:
     return OwnershipKind::Owned;
   case ResultConvention::Unowned:
+    // We insert a retain right after the call returning an unowned value in
+    // OwnershipModelEliminator. So treat the result as owned.
+    if (IsTrivial)
+      return OwnershipKind::None;
+    return OwnershipKind::Owned;
   case ResultConvention::UnownedInnerPointer:
     if (IsTrivial)
       return OwnershipKind::None;

lib/SILGen/SILGenApply.cpp

@@ -6236,11 +6236,11 @@ RValue SILGenFunction::emitApply(
                                  B.getDefaultAtomicity());
         hasAlreadyLifetimeExtendedSelf = true;
       }
-      LLVM_FALLTHROUGH;
+      result = resultTL.emitCopyValue(B, loc, result);
+      break;
 
     case ResultConvention::Unowned:
-      // Unretained. Retain the value.
-      result = resultTL.emitCopyValue(B, loc, result);
+      // Handled in OwnershipModelEliminator.
       break;
     }
 

lib/SILGen/SILGenPoly.cpp

@@ -5265,6 +5265,7 @@ void ResultPlanner::execute(SmallVectorImpl<SILValue> &innerDirectResultStack,
       LLVM_FALLTHROUGH;
     case ResultConvention::Owned:
     case ResultConvention::Autoreleased:
+    case ResultConvention::Unowned: // Handled in OwnershipModelEliminator.
       return SGF.emitManagedRValueWithCleanup(resultValue, resultTL);
     case ResultConvention::Pack:
       llvm_unreachable("shouldn't have direct result with pack results");
@@ -5274,8 +5275,6 @@ void ResultPlanner::execute(SmallVectorImpl<SILValue> &innerDirectResultStack,
       // originally 'self'.
       SGF.SGM.diagnose(Loc.getSourceLoc(), diag::not_implemented,
                        "reabstraction of returns_inner_pointer function");
-      LLVM_FALLTHROUGH;
-    case ResultConvention::Unowned:
       return SGF.emitManagedCopy(Loc, resultValue, resultTL);
     }
     llvm_unreachable("bad result convention!");

lib/SILOptimizer/Mandatory/OwnershipModelEliminator.cpp

@@ -22,6 +22,8 @@
 ///
 //===----------------------------------------------------------------------===//
 
+#include "swift/AST/Types.h"
+#include "swift/SIL/SILValue.h"
 #define DEBUG_TYPE "sil-ownership-model-eliminator"
 
 #include "swift/Basic/Assertions.h"
@@ -404,6 +406,20 @@ bool OwnershipModelEliminatorVisitor::visitApplyInst(ApplyInst *ai) {
     changed = true;
   }
 
+  // Insert a retain for unowned results.
+  SILBuilderWithScope builder(ai->getNextInstruction(), builderCtx);
+  auto resultIt = fnConv.getDirectSILResults().begin();
+  auto copyValue = [&](unsigned idx, SILValue v) {
+    auto result = *resultIt;
+    if (result.getConvention() == ResultConvention::Unowned)
+      builder.emitCopyValueOperation(ai->getLoc(), v);
+    ++resultIt;
+  };
+  if (fnConv.getNumDirectSILResults() == 1)
+    copyValue(0, ai);
+  else
+    builder.emitDestructureValueOperation(ai->getLoc(), ai, copyValue);
+
   return changed;
 }
 

test/Interop/Cxx/foreign-reference/Inputs/logging-frts.h

@@ -7,12 +7,14 @@ class SharedFRT {
 public:
   SharedFRT() : _refCount(1) { logMsg("Ctor"); }
 
+protected:
+  virtual ~SharedFRT() { logMsg("Dtor"); }
+
 private:
   void logMsg(const char *s) const {
     printf("RefCount: %d, message: %s\n", _refCount, s);
   }
 
-  ~SharedFRT() { logMsg("Dtor"); }
   SharedFRT(const SharedFRT &) = delete;
   SharedFRT &operator=(const SharedFRT &) = delete;
   SharedFRT(SharedFRT &&) = delete;
@@ -62,3 +64,24 @@ inline LargeStructWithRefCountedField getStruct() {
 inline LargeStructWithRefCountedFieldNested getNestedStruct() {
   return {0, {0, 0, 0, 0, new SharedFRT()}};
 }
+
+template <class T>
+struct Ref {
+  T *_Nonnull ptr() const { return ref; }
+  T *ref;
+};
+
+class Payload final : public SharedFRT {
+public:
+  static Ref<Payload> create(int value) {
+    Ref<Payload> ref;
+    ref.ref = new Payload(value);
+    return ref;
+  }
+
+  int value() const { return m_value; }
+
+private:
+  explicit Payload(int value) : m_value(value) {}
+  int m_value;
+};

test/Interop/Cxx/foreign-reference/get-frt-from-smart-ptr.swift

@@ -0,0 +1,26 @@
+// RUN: %target-run-simple-swift(-I %swift_src_root/lib/ClangImporter/SwiftBridging -I %S/Inputs -cxx-interoperability-mode=default -Xfrontend -disable-availability-checking -O) | %FileCheck %s
+
+// REQUIRES: executable_test
+
+import LoggingFrts
+
+@inline(never)
+func use(_ x: CInt) {
+    print("Value is \(x).")
+}
+
+func testRefIssues() {
+    var a2 = Optional.some(Payload.create(0));
+    let b2 = a2!.ptr();
+    a2 = Optional.none;
+    let v2 = b2.value();
+    use(v2)
+}
+testRefIssues()
+
+// CHECK:      RefCount: 1, message: Ctor
+// CHECK-NEXT: RefCount: 2, message: retain
+// CHECK-NEXT: RefCount: 1, message: release
+// CHECK-NEXT: Value is 0.
+// CHECK-NEXT: RefCount: 0, message: release
+// CHECK-NEXT: RefCount: 0, message: Dtor

test/SIL/OwnershipVerifier/use_verifier.sil

@@ -828,30 +828,6 @@ bb5(%5 : @owned $ThreeDifferingPayloadEnum):
   return %5 : $ThreeDifferingPayloadEnum
 }
 
-sil [ossa] @enum_cases_with_trivial_unowned_cases_arg_into_phi : $@convention(thin) (Builtin.NativeObject) -> ThreeDifferingPayloadEnum {
-bb0(%0 : @unowned $Builtin.NativeObject):
-  cond_br undef, bb1, bb2
-
-bb1:
-  cond_br undef, bb3, bb4
-
-bb2:
-  %1 = enum $ThreeDifferingPayloadEnum, #ThreeDifferingPayloadEnum.nopayload!enumelt
-  br bb5(%1 : $ThreeDifferingPayloadEnum)
-
-bb3:
-  %2 = enum $ThreeDifferingPayloadEnum, #ThreeDifferingPayloadEnum.nontrivial_payload!enumelt, %0 : $Builtin.NativeObject
-  br bb5(%2 : $ThreeDifferingPayloadEnum)
-
-bb4:
-  %3 = integer_literal $Builtin.Int32, 0
-  %4 = enum $ThreeDifferingPayloadEnum, #ThreeDifferingPayloadEnum.trivial_payload!enumelt, %3 : $Builtin.Int32
-  br bb5(%4 : $ThreeDifferingPayloadEnum)
-
-bb5(%5 : @unowned $ThreeDifferingPayloadEnum):
-  return %5 : $ThreeDifferingPayloadEnum
-}
-
 sil [ossa] @enum_cases_with_trivial_guaranteed_cases_arg_into_phi : $@convention(thin) (@guaranteed Builtin.NativeObject) -> @owned ThreeDifferingPayloadEnum {
 bb0(%0 : @guaranteed $Builtin.NativeObject):
   cond_br undef, bb1, bb2
@@ -1383,20 +1359,6 @@ bb3(%fUnknown : @owned $@callee_owned () -> ()):
   return %9999 : $()
 }
 
-sil [ossa] @unowned_to_ref_is_unowned_instant_use : $@convention(thin) (@guaranteed Builtin.NativeObject) -> Builtin.NativeObject {
-bb0(%0 : @guaranteed $Builtin.NativeObject):
-  %1 = ref_to_unowned %0 : $Builtin.NativeObject to $@sil_unowned Builtin.NativeObject
-  %2 = unowned_to_ref %1 : $@sil_unowned Builtin.NativeObject to $Builtin.NativeObject
-  return %2 : $Builtin.NativeObject
-}
-
-sil [ossa] @unmanaged_to_ref_is_unowned_instant_use : $@convention(thin) (@guaranteed Builtin.NativeObject) -> Builtin.NativeObject {
-bb0(%0 : @guaranteed $Builtin.NativeObject):
-  %1 = ref_to_unmanaged %0 : $Builtin.NativeObject to $@sil_unmanaged Builtin.NativeObject
-  %2 = unmanaged_to_ref %1 : $@sil_unmanaged Builtin.NativeObject to $Builtin.NativeObject
-  return %2 : $Builtin.NativeObject
-}
-
 sil [ossa] @nontrivial_enum_unchecked_enum_data_trivial_payload_owned : $@convention(thin) (@owned ThreeDifferingPayloadEnum) -> Builtin.Int32 {
 bb0(%0 : @owned $ThreeDifferingPayloadEnum):
   // NOTE: It may be surprising that %0 is consumed by this unchecked_enum_data

test/SILOptimizer/cse_objc_ossa.sil

@@ -14,12 +14,13 @@ import Foundation
 // CHECK-NOT: objc_protocol
 // CHECK: tuple (%0 : $Protocol, %0 : $Protocol)
 // CHECK-LABEL: } // end sil function 'cse_objc_protocol'
-sil [ossa] @cse_objc_protocol : $@convention(thin) () -> (Protocol, Protocol) {
+sil [ossa] @cse_objc_protocol : $@convention(thin) () -> @owned (Protocol, Protocol) {
 bb0:
   %0 = objc_protocol #XX : $Protocol
   %1 = objc_protocol #XX : $Protocol
   %2 = tuple (%0: $Protocol, %1: $Protocol)
-  return %2 : $(Protocol, Protocol)
+  %3 = copy_value %2
+  return %3
 }
 
 @objc protocol Walkable {

test/SILOptimizer/cse_ossa.sil

@@ -700,12 +700,13 @@ bb0(%0 : $*Builtin.Int8):
 // CHECK-NOT: raw_pointer_to_ref
 // CHECK: tuple
 // CHECK-LABEL: } // end sil function 'cse_raw_pointer_to_ref'
-sil [ossa] @cse_raw_pointer_to_ref : $@convention(thin) (Builtin.RawPointer) -> (C, C) {
+sil [ossa] @cse_raw_pointer_to_ref : $@convention(thin) (Builtin.RawPointer) -> @owned (C, C) {
 bb0(%0 : $Builtin.RawPointer):
   %1 = raw_pointer_to_ref %0 : $Builtin.RawPointer to $C
   %2 = raw_pointer_to_ref %0 : $Builtin.RawPointer to $C
   %6 = tuple(%1: $C, %2: $C)
-  return %6 : $(C, C)
+  %7 = copy_value %6
+  return %7
 }
 
 // CHECK-LABEL: sil [ossa] @cse_unchecked_addr_cast :

test/SILOptimizer/cse_ossa_nontrivial.sil

@@ -255,19 +255,6 @@ bb0(%0 : @owned $Klass):
   return %6 : $()
 }
 
-// CHECK-LABEL: sil [ossa] @cse_raw_pointer_to_ref :
-// CHECK: raw_pointer_to_ref
-// CHECK-NOT: raw_pointer_to_ref
-// CHECK: tuple
-// CHECK-LABEL: } // end sil function 'cse_raw_pointer_to_ref'
-sil [ossa] @cse_raw_pointer_to_ref : $@convention(thin) (Builtin.RawPointer) -> (Klass, Klass) {
-bb0(%0 : $Builtin.RawPointer):
-  %1 = raw_pointer_to_ref %0 : $Builtin.RawPointer to $Klass
-  %2 = raw_pointer_to_ref %0 : $Builtin.RawPointer to $Klass
-  %6 = tuple(%1: $Klass, %2: $Klass)
-  return %6 : $(Klass, Klass)
-}
-
 enum Enum1 {
     case Case1
     case Case2