Skip to content

[ownership] Change guaranteed args from transformation terminators to not require end_borrows. #29600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/swift/SIL/SILBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,12 @@ class SILBuilder {
}

EndBorrowInst *createEndBorrow(SILLocation loc, SILValue borrowedValue) {
if (auto *arg = dyn_cast<SILPhiArgument>(borrowedValue)) {
if (auto *ti = arg->getSingleTerminator()) {
assert(!ti->isTransformationTerminator() &&
"Transforming terminators do not have end_borrow");
}
}
return insert(new (getModule())
EndBorrowInst(getSILDebugLocation(loc), borrowedValue));
}
Expand Down
24 changes: 24 additions & 0 deletions include/swift/SIL/SILInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -7042,6 +7042,30 @@ class TermInst : public NonValueInstruction {
bool isProgramTerminating() const;

TermKind getTermKind() const { return TermKind(getKind()); }

/// Returns true if this is a transformation terminator.
bool isTransformationTerminator() const {
switch (getTermKind()) {
case TermKind::UnwindInst:
case TermKind::UnreachableInst:
case TermKind::ReturnInst:
case TermKind::ThrowInst:
case TermKind::YieldInst:
case TermKind::TryApplyInst:
case TermKind::BranchInst:
case TermKind::CondBranchInst:
case TermKind::SwitchValueInst:
case TermKind::SwitchEnumAddrInst:
case TermKind::DynamicMethodBranchInst:
case TermKind::CheckedCastAddrBranchInst:
case TermKind::CheckedCastValueBranchInst:
return false;
case TermKind::SwitchEnumInst:
case TermKind::CheckedCastBranchInst:
return true;
}
llvm_unreachable("Covered switch isn't covered.");
}
};

/// UnreachableInst - Position in the code which would be undefined to reach.
Expand Down
3 changes: 0 additions & 3 deletions lib/SIL/DynamicCasts.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1254,9 +1254,7 @@ void swift::emitIndirectConditionalCastWithScalar(
case CastConsumptionKind::TakeOnSuccess:
break;
case CastConsumptionKind::CopyOnSuccess: {
SILValue originalSuccValue = succValue;
succValue = B.emitCopyValueOperation(loc, succValue);
B.emitEndBorrowOperation(loc, originalSuccValue);
B.emitEndBorrowOperation(loc, srcValue);
break;
}
Expand Down Expand Up @@ -1295,7 +1293,6 @@ void swift::emitIndirectConditionalCastWithScalar(
StoreOwnershipQualifier::Init);
break;
case CastConsumptionKind::CopyOnSuccess:
B.emitEndBorrowOperation(loc, failValue);
B.emitEndBorrowOperation(loc, srcValue);
break;
case CastConsumptionKind::BorrowAlways:
Expand Down
24 changes: 21 additions & 3 deletions lib/SIL/OwnershipUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,15 @@ bool swift::isGuaranteedForwardingValueKind(SILNodeKind kind) {
}

bool swift::isGuaranteedForwardingValue(SILValue value) {
// If we have an argument from a transforming terminator, we can forward
// guaranteed.
if (auto *arg = dyn_cast<SILArgument>(value)) {
if (auto *ti = arg->getSingleTerminator()) {
if (ti->isTransformationTerminator()) {
return true;
}
}
}
return isGuaranteedForwardingValueKind(
value->getKindOfRepresentativeSILNodeInObject());
}
Expand Down Expand Up @@ -173,9 +182,18 @@ bool swift::getUnderlyingBorrowIntroducingValues(
// Otherwise if v is an ownership forwarding value, add its defining
// instruction
if (isGuaranteedForwardingValue(v)) {
auto *i = v->getDefiningInstruction();
assert(i);
llvm::transform(i->getAllOperands(), std::back_inserter(worklist),
if (auto *i = v->getDefiningInstruction()) {
llvm::transform(i->getAllOperands(), std::back_inserter(worklist),
[](const Operand &op) -> SILValue { return op.get(); });
continue;
}

// Otherwise, we should have a block argument that is defined by a single
// predecessor terminator.
auto *arg = cast<SILPhiArgument>(v);
auto *termInst = arg->getSingleTerminator();
assert(termInst && termInst->isTransformationTerminator());
llvm::transform(termInst->getAllOperands(), std::back_inserter(worklist),
[](const Operand &op) -> SILValue { return op.get(); });
continue;
}
Expand Down
52 changes: 49 additions & 3 deletions lib/SIL/SILOwnershipVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -334,9 +334,55 @@ bool SILValueOwnershipChecker::gatherUsers(
continue;
}

// Otherwise if we have a terminator, add any as uses any end_borrow to
// ensure that the subscope is completely enclsed within the super scope. We
// require all of our arguments to be either trivial or guaranteed.
// See if our forwarding terminator is a transformation terminator. If so,
// just add all of the uses of its successors to the worklist to visit as
// users.
if (ti->isTransformationTerminator()) {
for (auto &succ : ti->getSuccessors()) {
auto *succBlock = succ.getBB();

// If we do not have any arguments, then continue.
if (succBlock->args_empty())
continue;

// Otherwise, make sure that all arguments are trivial or guaranteed. If
// we fail, emit an error.
//
// TODO: We could ignore this error and emit a more specific error on
// the actual terminator.
for (auto *succArg : succBlock->getSILPhiArguments()) {
// *NOTE* We do not emit an error here since we want to allow for more
// specific errors to be found during use_verification.
//
// TODO: Add a flag that associates the terminator instruction with
// needing to be verified. If it isn't verified appropriately, assert
// when the verifier is destroyed.
auto succArgOwnershipKind = succArg->getOwnershipKind();
if (!succArgOwnershipKind.isCompatibleWith(ownershipKind)) {
// This is where the error would go.
continue;
}

// If we have an any value, just continue.
if (succArgOwnershipKind == ValueOwnershipKind::None)
continue;

// Otherwise add all users of this BBArg to the worklist to visit
// recursively.
llvm::copy(succArg->getUses(), std::back_inserter(users));
}
}
continue;
}

// Otherwise, we are dealing with the merging of true phis. To work with
// this we will eventually need to create a notion of forwarding borrow
// scopes.

// But until then, we validate that the argument has an end_borrow that acts
// as a subscope that is compeltely enclosed within the scopes of all
// incoming values. We require all of our arguments to be either trivial or
// guaranteed.
for (auto &succ : ti->getSuccessors()) {
auto *succBlock = succ.getBB();

Expand Down
7 changes: 7 additions & 0 deletions lib/SILGen/SILGenBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,13 @@ ManagedValue SILGenBuilder::createGuaranteedPhiArgument(SILType type) {
return SGF.emitManagedBorrowedArgumentWithCleanup(arg);
}

ManagedValue
SILGenBuilder::createGuaranteedTransformingTerminatorArgument(SILType type) {
SILPhiArgument *arg =
getInsertionBB()->createPhiArgument(type, ValueOwnershipKind::Guaranteed);
return ManagedValue::forUnmanaged(arg);
}

ManagedValue SILGenBuilder::createAllocRef(
SILLocation loc, SILType refType, bool objc, bool canAllocOnStack,
ArrayRef<SILType> inputElementTypes,
Expand Down
9 changes: 9 additions & 0 deletions lib/SILGen/SILGenBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,15 @@ class SILGenBuilder : public SILBuilder {
ManagedValue createOwnedPhiArgument(SILType type);
ManagedValue createGuaranteedPhiArgument(SILType type);

/// For arguments from terminators that are "transforming terminators". These
/// types of guaranteed arguments are validated as part of the operand of the
/// transforming terminator since transforming terminators are guaranteed to
/// be the only predecessor of our parent block.
///
/// NOTE: Two examples of transforming terminators are switch_enum,
/// checked_cast_br.
ManagedValue createGuaranteedTransformingTerminatorArgument(SILType type);

using SILBuilder::createMarkUninitialized;
ManagedValue createMarkUninitialized(ValueDecl *decl, ManagedValue operand,
MarkUninitializedInst::Kind muKind);
Expand Down
5 changes: 3 additions & 2 deletions lib/SILGen/SILGenDynamicCast.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ namespace {
// argument.
ManagedValue argument;
if (!shouldTakeOnSuccess(consumption)) {
argument = SGF.B.createGuaranteedPhiArgument(
argument = SGF.B.createGuaranteedTransformingTerminatorArgument(
origTargetTL.getLoweredType());
} else {
argument =
Expand Down Expand Up @@ -236,7 +236,8 @@ namespace {
switch (consumption) {
case CastConsumptionKind::BorrowAlways:
case CastConsumptionKind::CopyOnSuccess:
SGF.B.createGuaranteedPhiArgument(operandValue.getType());
SGF.B.createGuaranteedTransformingTerminatorArgument(
operandValue.getType());
handleFalse(None);
break;
case CastConsumptionKind::TakeAlways:
Expand Down
10 changes: 8 additions & 2 deletions lib/SILGen/SILGenExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,14 @@ namespace {
struct EndBorrowCleanup : Cleanup {
SILValue borrowedValue;

EndBorrowCleanup(SILValue borrowedValue)
: borrowedValue(borrowedValue) {}
EndBorrowCleanup(SILValue borrowedValue) : borrowedValue(borrowedValue) {
if (auto *arg = dyn_cast<SILPhiArgument>(borrowedValue)) {
if (auto *ti = arg->getSingleTerminator()) {
assert(!ti->isTransformationTerminator() &&
"Transforming terminators do not have end_borrow");
}
}
}

void emit(SILGenFunction &SGF, CleanupLocation l,
ForUnwind_t forUnwind) override {
Expand Down
4 changes: 2 additions & 2 deletions lib/SILGen/SILGenPattern.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1921,7 +1921,7 @@ void PatternMatchEmission::emitEnumElementObjectDispatch(

SILValue eltValue;
if (isPlusZero) {
origCMV = {SGF.B.createGuaranteedPhiArgument(eltTy),
origCMV = {SGF.B.createGuaranteedTransformingTerminatorArgument(eltTy),
CastConsumptionKind::BorrowAlways};
} else {
origCMV = {SGF.B.createOwnedPhiArgument(eltTy),
Expand Down Expand Up @@ -1971,7 +1971,7 @@ void PatternMatchEmission::emitEnumElementObjectDispatch(
if (SILBasicBlock *defaultBB = blocks.getDefaultBlock()) {
SGF.B.setInsertionPoint(defaultBB);
if (isPlusZero) {
SGF.B.createGuaranteedPhiArgument(src.getType());
SGF.B.createGuaranteedTransformingTerminatorArgument(src.getType());
} else {
SGF.B.createOwnedPhiArgument(src.getType());
}
Expand Down
17 changes: 5 additions & 12 deletions test/SIL/ownership-verifier/over_consume.sil
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,6 @@ bb0(%0 : @owned $Optional<Builtin.NativeObject>):
switch_enum %0 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2

bb1(%1 : @guaranteed $Builtin.NativeObject):
end_borrow %1 : $Builtin.NativeObject
br bb3

bb2:
Expand All @@ -251,7 +250,7 @@ bb3:
// CHECK: Found use after free?!
// CHECK: Value: %1 = begin_borrow %0 : $Optional<Builtin.NativeObject>
// CHECK: Consuming User: end_borrow %1 : $Optional<Builtin.NativeObject>
// CHECK: Non Consuming User: end_borrow %3 : $Builtin.NativeObject
// CHECK: Non Consuming User: %5 = unchecked_ref_cast %3 : $Builtin.NativeObject to $Builtin.NativeObject
// CHECK: Block: bb1
sil [ossa] @switch_enum_guaranteed_arg_outlives_original_value : $@convention(thin) (@owned Optional<Builtin.NativeObject>) -> () {
bb0(%0 : @owned $Optional<Builtin.NativeObject>):
Expand All @@ -260,7 +259,7 @@ bb0(%0 : @owned $Optional<Builtin.NativeObject>):

bb1(%2 : @guaranteed $Builtin.NativeObject):
end_borrow %1 : $Optional<Builtin.NativeObject>
end_borrow %2 : $Builtin.NativeObject
%2a = unchecked_ref_cast %2 : $Builtin.NativeObject to $Builtin.NativeObject
br bb3

bb2:
Expand Down Expand Up @@ -312,7 +311,6 @@ bb0(%0 : @guaranteed $Builtin.NativeObject):
checked_cast_br %0 : $Builtin.NativeObject to SuperKlass, bb1, bb2

bb1(%1 : @guaranteed $SuperKlass):
end_borrow %1 : $SuperKlass
br bb3

bb2(%2 : @owned $Builtin.NativeObject):
Expand All @@ -339,7 +337,6 @@ bb1(%1 : @owned $SuperKlass):
br bb3

bb2(%2 : @guaranteed $Builtin.NativeObject):
end_borrow %2 : $Builtin.NativeObject
br bb3

bb3:
Expand All @@ -363,11 +360,9 @@ bb0(%0 : @owned $Builtin.NativeObject):
checked_cast_br %0 : $Builtin.NativeObject to SuperKlass, bb1, bb2

bb1(%1 : @guaranteed $SuperKlass):
end_borrow %1 : $SuperKlass
br bb3

bb2(%2 : @guaranteed $Builtin.NativeObject):
end_borrow %2 : $Builtin.NativeObject
br bb3

bb3:
Expand All @@ -387,7 +382,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
checked_cast_br %0 : $Builtin.NativeObject to SuperKlass, bb1, bb2

bb1(%1 : @guaranteed $SuperKlass):
end_borrow %1 : $SuperKlass
br bb3

bb2(%2 : @owned $Builtin.NativeObject):
Expand All @@ -414,7 +408,6 @@ bb1(%1 : @owned $SuperKlass):
br bb3

bb2(%2 : @guaranteed $Builtin.NativeObject):
end_borrow %2 : $Builtin.NativeObject
br bb3

bb3:
Expand All @@ -426,21 +419,21 @@ bb3:
// CHECK: Found use after free?!
// CHECK: Value: %1 = begin_borrow %0 : $Builtin.NativeObject
// CHECK: Consuming User: end_borrow %1 : $Builtin.NativeObject
// CHECK: Non Consuming User: end_borrow %7 : $Builtin.NativeObject
// CHECK: Non Consuming User: %9 = unchecked_ref_cast %7 : $Builtin.NativeObject to $Builtin.NativeObject
// CHECK: Block: bb2
sil [ossa] @checked_cast_br_guaranteed_arg_outlives_original_value : $@convention(thin) (@owned Builtin.NativeObject) -> () {
bb0(%0 : @owned $Builtin.NativeObject):
%1 = begin_borrow %0 : $Builtin.NativeObject
checked_cast_br %1 : $Builtin.NativeObject to SuperKlass, bb1, bb2

bb1(%2 : @guaranteed $SuperKlass):
end_borrow %2 : $SuperKlass
end_borrow %1 : $Builtin.NativeObject
%2a = unchecked_ref_cast %2 : $SuperKlass to $SuperKlass
br bb3

bb2(%3 : @guaranteed $Builtin.NativeObject):
end_borrow %1 : $Builtin.NativeObject
end_borrow %3 : $Builtin.NativeObject
%3a = unchecked_ref_cast %3 : $Builtin.NativeObject to $Builtin.NativeObject
br bb3

bb3:
Expand Down
7 changes: 0 additions & 7 deletions test/SIL/ownership-verifier/use_verifier.sil
Original file line number Diff line number Diff line change
Expand Up @@ -518,7 +518,6 @@ bb0(%0 : @guaranteed $Builtin.NativeObject):
switch_enum %1 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2

bb1(%2 : @guaranteed $Builtin.NativeObject):
end_borrow %2 : $Builtin.NativeObject
br bb3

bb2:
Expand All @@ -536,7 +535,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2

bb1(%3 : @guaranteed $Builtin.NativeObject):
end_borrow %3 : $Builtin.NativeObject
end_borrow %1 : $Builtin.NativeObject
br bb3

Expand All @@ -557,7 +555,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2

bb1(%3 : @guaranteed $Builtin.NativeObject):
end_borrow %3 : $Builtin.NativeObject
br bb3

bb2:
Expand All @@ -577,7 +574,6 @@ bb0(%0 : @owned $Builtin.NativeObject):
switch_enum %2 : $Optional<Builtin.NativeObject>, case #Optional.some!enumelt.1: bb1, case #Optional.none!enumelt: bb2

bb1(%3 : @guaranteed $Builtin.NativeObject):
end_borrow %3 : $Builtin.NativeObject
br bb3

bb2:
Expand Down Expand Up @@ -784,11 +780,9 @@ bb6:
checked_cast_br %6 : $Builtin.NativeObject to SuperKlass, bb7, bb8

bb7(%7 : @guaranteed $SuperKlass):
end_borrow %7 : $SuperKlass
br bb9

bb8(%8 : @guaranteed $Builtin.NativeObject):
end_borrow %8 : $Builtin.NativeObject
br bb9

bb9:
Expand Down Expand Up @@ -1195,4 +1189,3 @@ bb0(%0 : @guaranteed $Builtin.NativeObject, %1 : @owned $Builtin.NativeObject):
%9999 = tuple()
return %9999 : $()
}

Loading