Fix SILGen of borrow accessors returning guaranteed values from within a local borrow

Borrow accessor result can sometimes be generated from a local borrow and
returning the result produced within the local borrow scope will cause ownership errors.

We have to introduce new SIL semantics to make this possible.
Until then use a pair of unchecked_ownership_conversion instructions to silence the ownership errors.
We encounter this when we have:

Address-only self and @guaranteed result
Loadable self and @guaranteed result derived from an unsafe pointer stored property

This change also updates the result convention of borrow accessors with loadable result types
and indirect self argument type.

Author: meg-gupta

lib/SIL/IR/SILFunctionType.cpp

@@ -1387,18 +1387,15 @@ class DestructureResults {
   TypeExpansionContext context;
   bool hasSendingResult;
   bool isBorrowOrMutateAccessor;
-  bool hasSelfWithAddressType;
 
 public:
   DestructureResults(TypeExpansionContext context, TypeConverter &TC,
                      const Conventions &conventions,
                      SmallVectorImpl<SILResultInfo> &results,
-                     bool hasSendingResult, bool isBorrowOrMutateAccessor,
-                     bool hasSelfWithAddressType)
+                     bool hasSendingResult, bool isBorrowOrMutateAccessor)
       : TC(TC), Convs(conventions), Results(results), context(context),
         hasSendingResult(hasSendingResult),
-        isBorrowOrMutateAccessor(isBorrowOrMutateAccessor),
-        hasSelfWithAddressType(hasSelfWithAddressType) {}
+        isBorrowOrMutateAccessor(isBorrowOrMutateAccessor) {}
 
   void destructure(AbstractionPattern origType, CanType substType) {
     // Recur into tuples.
@@ -1454,8 +1451,7 @@ class DestructureResults {
     if (isBorrowOrMutateAccessor) {
       if (substResultTL.isTrivial()) {
         convention = ResultConvention::Unowned;
-      } else if (hasSelfWithAddressType ||
-                 isFormallyReturnedIndirectly(origType, substType,
+      } else if (isFormallyReturnedIndirectly(origType, substType,
                                               substResultTLForConvention)) {
         assert(Convs.getResult(substResultTLForConvention) ==
                ResultConvention::Guaranteed);
@@ -2728,16 +2724,12 @@ static CanSILFunctionType getSILFunctionType(
                                 coroutineOrigYieldType, coroutineSubstYieldType,
                                 yields, coroutineKind);
 
-  bool hasSelfWithAddressType =
-      extInfoBuilder.hasSelfParam() &&
-      inputs.back().getSILStorageInterfaceType().isAddress();
-
   // Destructure the result tuple type.
   SmallVector<SILResultInfo, 8> results;
   {
-    DestructureResults destructurer(
-        expansionContext, TC, conventions, results, hasSendingResult,
-        isBorrowOrMutateAccessor(constant), hasSelfWithAddressType);
+    DestructureResults destructurer(expansionContext, TC, conventions, results,
+                                    hasSendingResult,
+                                    isBorrowOrMutateAccessor(constant));
     destructurer.destructure(origResultType, substFormalResultType);
   }
 

lib/SILGen/SILGenApply.cpp

@@ -5471,20 +5471,28 @@ ManagedValue SILGenFunction::applyBorrowAccessor(
                ExecutorBreadcrumb());
   assert(rawResults.size() == 1);
   auto rawResult = rawResults[0];
-  if (!rawResult->getType().isMoveOnly()) {
-    return ManagedValue::forForwardedRValue(*this, rawResult);
+
+  if (fn.getFunction()->getConventions().hasGuaranteedResult()) {
+    auto selfArg = args.back().getValue();
+    if (isa<LoadBorrowInst>(selfArg)) {
+      rawResult = emitUncheckedGuaranteedConversion(rawResult);
+    }
   }
-  if (rawResult->getType().isAddress()) {
-    auto result = B.createMarkUnresolvedNonCopyableValueInst(
-        loc, rawResult,
+  if (rawResult->getType().isMoveOnly()) {
+    if (rawResult->getType().isAddress()) {
+      auto result = B.createMarkUnresolvedNonCopyableValueInst(
+          loc, rawResult,
+          MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
+      return ManagedValue::forRValueWithoutOwnership(result);
+    }
+    auto result = emitManagedCopy(loc, rawResult);
+    result = B.createMarkUnresolvedNonCopyableValueInst(
+        loc, result,
         MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
-    return ManagedValue::forRValueWithoutOwnership(result);
+    return emitManagedBeginBorrow(loc, result.getValue());
   }
-  auto result = emitManagedCopy(loc, rawResult);
-  result = B.createMarkUnresolvedNonCopyableValueInst(
-      loc, result,
-      MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
-  return emitManagedBeginBorrow(loc, result.getValue());
+
+  return ManagedValue::forForwardedRValue(*this, rawResult);
 }
 
 RValue CallEmission::applyFirstLevelCallee(SGFContext C) {

lib/SILGen/SILGenFunction.h

@@ -2276,6 +2276,8 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
   bool emitGuaranteedReturn(SILLocation loc, Expr *ret,
                             SmallVectorImpl<SILValue> &directResults);
 
+  SILValue emitUncheckedGuaranteedConversion(SILValue value);
+
   void emitYield(SILLocation loc, MutableArrayRef<ArgumentSource> yieldValues,
                  ArrayRef<AbstractionPattern> origTypes,
                  JumpDest unwindDest);

lib/SILGen/SILGenStmt.cpp

@@ -708,11 +708,37 @@ static Expr *lookThroughProjections(Expr *expr) {
   return lookThroughProjections(lookupExpr->getBase());
 }
 
+SILValue SILGenFunction::emitUncheckedGuaranteedConversion(SILValue value) {
+  assert(value->getType().isObject());
+  assert(F.getConventions().hasGuaranteedResult());
+  auto regularLoc = RegularLocation::getAutoGeneratedLocation();
+  // Introduce a pair of unchecked_ownership_conversion instructions to
+  // avoid ownership errors from returning a load borrowed value.
+  // TODO: Introduce new SIL semantics to allow returning borrowed
+  // values from within a local borrow scope.
+  auto result = B.createUncheckedOwnershipConversion(regularLoc, value,
+                                                     OwnershipKind::Unowned);
+  result = B.createUncheckedOwnershipConversion(regularLoc, result,
+                                                OwnershipKind::Guaranteed);
+  return result;
+}
+
 bool SILGenFunction::emitGuaranteedReturn(
     SILLocation loc, Expr *ret, SmallVectorImpl<SILValue> &directResults) {
   auto *afd = cast<AbstractFunctionDecl>(FunctionDC->getAsDecl());
   assert(cast<AccessorDecl>(afd)->isBorrowAccessor());
 
+
+  auto emitLoadBorrowFromGuaranteedAddress =
+      [&](ManagedValue guaranteedAddress) -> SILValue {
+    assert(guaranteedAddress.getValue()->getType().isAddress());
+    assert(F.getConventions().hasGuaranteedResult());
+    auto regularLoc = RegularLocation::getAutoGeneratedLocation();
+    auto load =
+        B.createLoadBorrow(regularLoc, guaranteedAddress).getValue();
+    return emitUncheckedGuaranteedConversion(load);
+  };
+
   // If the return expression is a literal, emit as a regular return
   // expression.
   if (isa<LiteralExpr>(ret)) {
@@ -737,10 +763,10 @@ bool SILGenFunction::emitGuaranteedReturn(
   FormalEvaluationScope scope(*this);
   LValueOptions options;
   auto lvalue = emitLValue(ret,
-                           F.getConventions().hasGuaranteedResult()
+                           F.getSelfArgument()->getType().isObject()
                                ? SGFAccessKind::BorrowedObjectRead
                                : SGFAccessKind::BorrowedAddressRead,
-                           F.getConventions().hasGuaranteedResult()
+                           F.getSelfArgument()->getType().isObject()
                                ? options.forGuaranteedReturn(true)
                                : options.forGuaranteedAddressReturn(true));
 
@@ -766,8 +792,14 @@ bool SILGenFunction::emitGuaranteedReturn(
   //   }
   // }
   if (afd->getAttrs().hasAttribute<UnsafeSelfDependentResultAttr>()) {
-    auto resultValue = emitBorrowedLValue(ret, std::move(lvalue));
-    directResults.push_back(resultValue.getValue());
+    auto regularLoc = RegularLocation::getAutoGeneratedLocation();
+    auto resultMV = emitBorrowedLValue(regularLoc, std::move(lvalue));
+    SILValue result = resultMV.getValue();
+    if (resultMV.getType().isAddress() &&
+        F.getConventions().hasGuaranteedResult()) {
+      result = emitLoadBorrowFromGuaranteedAddress(resultMV);
+    }
+    directResults.push_back(result);
     return false;
   }
 
@@ -782,8 +814,9 @@ bool SILGenFunction::emitGuaranteedReturn(
     return true;
   }
 
-  auto result = tryEmitProjectedLValue(ret, std::move(lvalue), TSanKind::None);
-  if (!result) {
+  auto resultMV =
+      tryEmitProjectedLValue(ret, std::move(lvalue), TSanKind::None);
+  if (!resultMV) {
     diagnose(getASTContext(), ret->getStartLoc(),
              diag::invalid_borrow_accessor_return);
     diagnose(getASTContext(), ret->getStartLoc(),
@@ -800,15 +833,24 @@ bool SILGenFunction::emitGuaranteedReturn(
     return true;
   }
 
-  auto resultValue = result->getValue();
+  SILValue result = resultMV->getValue();
   SILType selfType = F.getSelfArgument()->getType();
-  if (selfType.isMoveOnly() && F.getConventions().hasGuaranteedResult()) {
+ 
+  if (F.getConventions().hasGuaranteedResult()) {
     // If we are returning the result of borrow accessor, strip the
     // unnecessary copy_value + mark_unresolved_non_copyable_value
     // instructions.
-    resultValue = lookThroughMoveOnlyCheckerPattern(resultValue);
+    if (selfType.isMoveOnly()) {
+    result = lookThroughMoveOnlyCheckerPattern(result);
+    }
+    // If the SIL convention is @guaranteed and the generated result is an
+    // address, emit a load_borrow.
+    if (result->getType().isAddress()) {
+      result = emitLoadBorrowFromGuaranteedAddress(*resultMV);
+    }
   }
-  directResults.push_back(resultValue);
+
+  directResults.push_back(result);
   return false;
 }
 

test/IRGen/borrow_accessor.swift

@@ -317,7 +317,8 @@ func test() {
 // CHECK:   [[REG2:%.*]] = getelementptr inbounds i32, ptr %"GenWrapper<T>", i64 8
 // CHECK:   [[REG3:%.*]] = load i32, ptr [[REG2]], align 8
 // CHECK:   [[REG4:%.*]] = getelementptr inbounds i8, ptr [[REG0]], i32 [[REG3]]
-// CHECK:   ret ptr [[REG4]]
+// CHECK:   [[REG5:%.*]] = load ptr, ptr [[REG4]], align 8
+// CHECK:   ret ptr [[REG5]]
 // CHECK: }
 
 // CHECK: define hidden swiftcc ptr @"$s15borrow_accessor10GenWrapperVyxSicib"(i64 [[REG0:%.*]], ptr %"GenWrapper<T>", ptr noalias swiftself [[REG1:%.*]]) #0 {