Skip to content

Commit e4d1123

Browse files
meg-guptameg-gupta
committed
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.
1 parent 19ab4cc commit e4d1123

File tree

8 files changed

+556
-38
lines changed

8 files changed

+556
-38
lines changed

‎lib/SIL/IR/SILFunctionType.cpp‎

Lines changed: 6 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,18 +1387,15 @@ class DestructureResults {
13871387
TypeExpansionContext context;
13881388
bool hasSendingResult;
13891389
bool isBorrowOrMutateAccessor;
1390-
bool hasSelfWithAddressType;
13911390

13921391
public:
13931392
DestructureResults(TypeExpansionContext context, TypeConverter &TC,
13941393
const Conventions &conventions,
13951394
SmallVectorImpl<SILResultInfo> &results,
1396-
bool hasSendingResult, bool isBorrowOrMutateAccessor,
1397-
bool hasSelfWithAddressType)
1395+
bool hasSendingResult, bool isBorrowOrMutateAccessor)
13981396
: TC(TC), Convs(conventions), Results(results), context(context),
13991397
hasSendingResult(hasSendingResult),
1400-
isBorrowOrMutateAccessor(isBorrowOrMutateAccessor),
1401-
hasSelfWithAddressType(hasSelfWithAddressType) {}
1398+
isBorrowOrMutateAccessor(isBorrowOrMutateAccessor) {}
14021399

14031400
void destructure(AbstractionPattern origType, CanType substType) {
14041401
// Recur into tuples.
@@ -1454,8 +1451,7 @@ class DestructureResults {
14541451
if (isBorrowOrMutateAccessor) {
14551452
if (substResultTL.isTrivial()) {
14561453
convention = ResultConvention::Unowned;
1457-
} else if (hasSelfWithAddressType ||
1458-
isFormallyReturnedIndirectly(origType, substType,
1454+
} else if (isFormallyReturnedIndirectly(origType, substType,
14591455
substResultTLForConvention)) {
14601456
assert(Convs.getResult(substResultTLForConvention) ==
14611457
ResultConvention::Guaranteed);
@@ -2728,16 +2724,12 @@ static CanSILFunctionType getSILFunctionType(
27282724
coroutineOrigYieldType, coroutineSubstYieldType,
27292725
yields, coroutineKind);
27302726

2731-
bool hasSelfWithAddressType =
2732-
extInfoBuilder.hasSelfParam() &&
2733-
inputs.back().getSILStorageInterfaceType().isAddress();
2734-
27352727
// Destructure the result tuple type.
27362728
SmallVector<SILResultInfo, 8> results;
27372729
{
2738-
DestructureResults destructurer(
2739-
expansionContext, TC, conventions, results, hasSendingResult,
2740-
isBorrowOrMutateAccessor(constant), hasSelfWithAddressType);
2730+
DestructureResults destructurer(expansionContext, TC, conventions, results,
2731+
hasSendingResult,
2732+
isBorrowOrMutateAccessor(constant));
27412733
destructurer.destructure(origResultType, substFormalResultType);
27422734
}
27432735

‎lib/SILGen/SILGenApply.cpp‎

Lines changed: 19 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5471,20 +5471,28 @@ ManagedValue SILGenFunction::applyBorrowAccessor(
54715471
ExecutorBreadcrumb());
54725472
assert(rawResults.size() == 1);
54735473
auto rawResult = rawResults[0];
5474-
if (!rawResult->getType().isMoveOnly()) {
5475-
return ManagedValue::forForwardedRValue(*this, rawResult);
5474+
5475+
if (fn.getFunction()->getConventions().hasGuaranteedResult()) {
5476+
auto selfArg = args.back().getValue();
5477+
if (isa<LoadBorrowInst>(selfArg)) {
5478+
rawResult = emitUncheckedGuaranteedConversion(rawResult);
5479+
}
54765480
}
5477-
if (rawResult->getType().isAddress()) {
5478-
auto result = B.createMarkUnresolvedNonCopyableValueInst(
5479-
loc, rawResult,
5481+
if (rawResult->getType().isMoveOnly()) {
5482+
if (rawResult->getType().isAddress()) {
5483+
auto result = B.createMarkUnresolvedNonCopyableValueInst(
5484+
loc, rawResult,
5485+
MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
5486+
return ManagedValue::forRValueWithoutOwnership(result);
5487+
}
5488+
auto result = emitManagedCopy(loc, rawResult);
5489+
result = B.createMarkUnresolvedNonCopyableValueInst(
5490+
loc, result,
54805491
MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
5481-
return ManagedValue::forRValueWithoutOwnership(result);
5492+
return emitManagedBeginBorrow(loc, result.getValue());
54825493
}
5483-
auto result = emitManagedCopy(loc, rawResult);
5484-
result = B.createMarkUnresolvedNonCopyableValueInst(
5485-
loc, result,
5486-
MarkUnresolvedNonCopyableValueInst::CheckKind::NoConsumeOrAssign);
5487-
return emitManagedBeginBorrow(loc, result.getValue());
5494+
5495+
return ManagedValue::forForwardedRValue(*this, rawResult);
54885496
}
54895497

54905498
RValue CallEmission::applyFirstLevelCallee(SGFContext C) {

‎lib/SILGen/SILGenFunction.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2276,6 +2276,8 @@ class LLVM_LIBRARY_VISIBILITY SILGenFunction
22762276
bool emitGuaranteedReturn(SILLocation loc, Expr *ret,
22772277
SmallVectorImpl<SILValue> &directResults);
22782278

2279+
SILValue emitUncheckedGuaranteedConversion(SILValue value);
2280+
22792281
void emitYield(SILLocation loc, MutableArrayRef<ArgumentSource> yieldValues,
22802282
ArrayRef<AbstractionPattern> origTypes,
22812283
JumpDest unwindDest);

‎lib/SILGen/SILGenStmt.cpp‎

Lines changed: 52 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -708,11 +708,37 @@ static Expr *lookThroughProjections(Expr *expr) {
708708
return lookThroughProjections(lookupExpr->getBase());
709709
}
710710

711+
SILValue SILGenFunction::emitUncheckedGuaranteedConversion(SILValue value) {
712+
assert(value->getType().isObject());
713+
assert(F.getConventions().hasGuaranteedResult());
714+
auto regularLoc = RegularLocation::getAutoGeneratedLocation();
715+
// Introduce a pair of unchecked_ownership_conversion instructions to
716+
// avoid ownership errors from returning a load borrowed value.
717+
// TODO: Introduce new SIL semantics to allow returning borrowed
718+
// values from within a local borrow scope.
719+
auto result = B.createUncheckedOwnershipConversion(regularLoc, value,
720+
OwnershipKind::Unowned);
721+
result = B.createUncheckedOwnershipConversion(regularLoc, result,
722+
OwnershipKind::Guaranteed);
723+
return result;
724+
}
725+
711726
bool SILGenFunction::emitGuaranteedReturn(
712727
SILLocation loc, Expr *ret, SmallVectorImpl<SILValue> &directResults) {
713728
auto *afd = cast<AbstractFunctionDecl>(FunctionDC->getAsDecl());
714729
assert(cast<AccessorDecl>(afd)->isBorrowAccessor());
715730

731+
732+
auto emitLoadBorrowFromGuaranteedAddress =
733+
[&](ManagedValue guaranteedAddress) -> SILValue {
734+
assert(guaranteedAddress.getValue()->getType().isAddress());
735+
assert(F.getConventions().hasGuaranteedResult());
736+
auto regularLoc = RegularLocation::getAutoGeneratedLocation();
737+
auto load =
738+
B.createLoadBorrow(regularLoc, guaranteedAddress).getValue();
739+
return emitUncheckedGuaranteedConversion(load);
740+
};
741+
716742
// If the return expression is a literal, emit as a regular return
717743
// expression.
718744
if (isa<LiteralExpr>(ret)) {
@@ -737,10 +763,10 @@ bool SILGenFunction::emitGuaranteedReturn(
737763
FormalEvaluationScope scope(*this);
738764
LValueOptions options;
739765
auto lvalue = emitLValue(ret,
740-
F.getConventions().hasGuaranteedResult()
766+
F.getSelfArgument()->getType().isObject()
741767
? SGFAccessKind::BorrowedObjectRead
742768
: SGFAccessKind::BorrowedAddressRead,
743-
F.getConventions().hasGuaranteedResult()
769+
F.getSelfArgument()->getType().isObject()
744770
? options.forGuaranteedReturn(true)
745771
: options.forGuaranteedAddressReturn(true));
746772

@@ -766,8 +792,14 @@ bool SILGenFunction::emitGuaranteedReturn(
766792
// }
767793
// }
768794
if (afd->getAttrs().hasAttribute<UnsafeSelfDependentResultAttr>()) {
769-
auto resultValue = emitBorrowedLValue(ret, std::move(lvalue));
770-
directResults.push_back(resultValue.getValue());
795+
auto regularLoc = RegularLocation::getAutoGeneratedLocation();
796+
auto resultMV = emitBorrowedLValue(regularLoc, std::move(lvalue));
797+
SILValue result = resultMV.getValue();
798+
if (resultMV.getType().isAddress() &&
799+
F.getConventions().hasGuaranteedResult()) {
800+
result = emitLoadBorrowFromGuaranteedAddress(resultMV);
801+
}
802+
directResults.push_back(result);
771803
return false;
772804
}
773805

@@ -782,8 +814,9 @@ bool SILGenFunction::emitGuaranteedReturn(
782814
return true;
783815
}
784816

785-
auto result = tryEmitProjectedLValue(ret, std::move(lvalue), TSanKind::None);
786-
if (!result) {
817+
auto resultMV =
818+
tryEmitProjectedLValue(ret, std::move(lvalue), TSanKind::None);
819+
if (!resultMV) {
787820
diagnose(getASTContext(), ret->getStartLoc(),
788821
diag::invalid_borrow_accessor_return);
789822
diagnose(getASTContext(), ret->getStartLoc(),
@@ -800,15 +833,24 @@ bool SILGenFunction::emitGuaranteedReturn(
800833
return true;
801834
}
802835

803-
auto resultValue = result->getValue();
836+
SILValue result = resultMV->getValue();
804837
SILType selfType = F.getSelfArgument()->getType();
805-
if (selfType.isMoveOnly() && F.getConventions().hasGuaranteedResult()) {
838+
839+
if (F.getConventions().hasGuaranteedResult()) {
806840
// If we are returning the result of borrow accessor, strip the
807841
// unnecessary copy_value + mark_unresolved_non_copyable_value
808842
// instructions.
809-
resultValue = lookThroughMoveOnlyCheckerPattern(resultValue);
843+
if (selfType.isMoveOnly()) {
844+
result = lookThroughMoveOnlyCheckerPattern(result);
845+
}
846+
// If the SIL convention is @guaranteed and the generated result is an
847+
// address, emit a load_borrow.
848+
if (result->getType().isAddress()) {
849+
result = emitLoadBorrowFromGuaranteedAddress(*resultMV);
850+
}
810851
}
811-
directResults.push_back(resultValue);
852+
853+
directResults.push_back(result);
812854
return false;
813855
}
814856

‎test/IRGen/borrow_accessor.swift‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -317,7 +317,8 @@ func test() {
317317
// CHECK: [[REG2:%.*]] = getelementptr inbounds i32, ptr %"GenWrapper<T>", i64 8
318318
// CHECK: [[REG3:%.*]] = load i32, ptr [[REG2]], align 8
319319
// CHECK: [[REG4:%.*]] = getelementptr inbounds i8, ptr [[REG0]], i32 [[REG3]]
320-
// CHECK: ret ptr [[REG4]]
320+
// CHECK: [[REG5:%.*]] = load ptr, ptr [[REG4]], align 8
321+
// CHECK: ret ptr [[REG5]]
321322
// CHECK: }
322323

323324
// CHECK: define hidden swiftcc ptr @"$s15borrow_accessor10GenWrapperVyxSicib"(i64 [[REG0:%.*]], ptr %"GenWrapper<T>", ptr noalias swiftself [[REG1:%.*]]) #0 {

0 commit comments

Comments
 (0)