Skip to content

[sil] Ban casting AnyObject with a guaranteed ownership forwarding checked_cast_br and fix up semantic-arc-opts given that. #40287

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
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
44 changes: 39 additions & 5 deletions include/swift/SIL/SILInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1057,16 +1057,40 @@ class CopyLikeInstruction {
/// initializes the kind field on our object is run before our constructor runs.
class OwnershipForwardingMixin {
ValueOwnershipKind ownershipKind;
bool directlyForwards;

protected:
OwnershipForwardingMixin(SILInstructionKind kind,
ValueOwnershipKind ownershipKind)
: ownershipKind(ownershipKind) {
ValueOwnershipKind ownershipKind,
bool isDirectlyForwarding = true)
: ownershipKind(ownershipKind), directlyForwards(isDirectlyForwarding) {
assert(isa(kind) && "Invalid subclass?!");
assert(ownershipKind && "invalid forwarding ownership");
assert((directlyForwards || ownershipKind != OwnershipKind::Guaranteed) &&
"Non directly forwarding instructions can not forward guaranteed "
"ownership");
}

public:
/// If an instruction is directly forwarding, then any operand op whose
/// ownership it forwards into a result r must have the property that op and r
/// are "rc identical". This means that they are representing the same set of
/// underlying lifetimes (plural b/c of aggregates).
///
/// An instruction that is not directly forwarding, can not have guaranteed
/// ownership since without direct forwarding, there isn't necessarily any
/// connection in between the operand's lifetime and the value's lifetime.
///
/// An example of this is checked_cast_br where when performing the following:
///
/// __SwiftValue(AnyHashable(Klass())) to OtherKlass()
///
/// we will look through the __SwiftValue(AnyHashable(X)) any just cast Klass
/// to OtherKlass. This means that the result argument would no longer be
/// rc-identical to the operand and default case and thus we can not propagate
/// forward any form of guaranteed ownership.
bool isDirectlyForwarding() const { return directlyForwards; }

/// Forwarding ownership is determined by the forwarding instruction's
/// constant ownership attribute. If forwarding ownership is owned, then the
/// instruction moves an owned operand to its result, ending its lifetime. If
Expand All @@ -1081,6 +1105,9 @@ class OwnershipForwardingMixin {
return ownershipKind;
}
void setForwardingOwnershipKind(ValueOwnershipKind newKind) {
assert((isDirectlyForwarding() || newKind != OwnershipKind::Guaranteed) &&
"Non directly forwarding instructions can not forward guaranteed "
"ownership");
ownershipKind = newKind;
}

Expand Down Expand Up @@ -7927,9 +7954,10 @@ class OwnershipForwardingTermInst : public TermInst,
protected:
OwnershipForwardingTermInst(SILInstructionKind kind,
SILDebugLocation debugLoc,
ValueOwnershipKind ownershipKind)
ValueOwnershipKind ownershipKind,
bool isDirectlyForwarding = true)
: TermInst(kind, debugLoc),
OwnershipForwardingMixin(kind, ownershipKind) {
OwnershipForwardingMixin(kind, ownershipKind, isDirectlyForwarding) {
assert(classof(kind));
}

Expand Down Expand Up @@ -8909,7 +8937,13 @@ class CheckedCastBranchInst final
ValueOwnershipKind forwardingOwnershipKind)
: UnaryInstructionWithTypeDependentOperandsBase(
DebugLoc, Operand, TypeDependentOperands, SuccessBB, FailureBB,
Target1Count, Target2Count, forwardingOwnershipKind),
Target1Count, Target2Count, forwardingOwnershipKind,
// We are always directly forwarding unless we are casting an
// AnyObject. This is b/c an AnyObject could contain a boxed
// AnyObject(Class()) that we unwrap as part of the cast. In such a
// case, we would return a different value and potentially end the
// lifetime of the operand value.
!Operand->getType().isAnyObject()),
DestLoweredTy(DestLoweredTy), DestFormalTy(DestFormalTy),
IsExact(IsExact) {}

Expand Down
8 changes: 5 additions & 3 deletions lib/SIL/Utils/MemAccessUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -877,9 +877,11 @@ SILValue swift::findOwnershipReferenceAggregate(SILValue ref) {
if (auto *arg = dyn_cast<SILArgument>(root)) {
if (auto *term = arg->getSingleTerminator()) {
if (term->isTransformationTerminator()) {
assert(OwnershipForwardingTermInst::isa(term));
root = term->getOperand(0);
continue;
auto *ti = cast<OwnershipForwardingTermInst>(term);
if (ti->isDirectlyForwarding()) {
root = term->getOperand(0);
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not critical in this PR, but it seems that 6 lines above should now have an assert:

  assert(cast<OwnershipForwardingTermInst>(root)->isDirectlyForwarding());
   root = root->getDefiningInstruction()->getOperand(0);

}
}
}
Expand Down
130 changes: 32 additions & 98 deletions lib/SIL/Utils/OwnershipUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,121 +29,51 @@ bool swift::isValueAddressOrTrivial(SILValue v) {
v.getOwnershipKind() == OwnershipKind::None;
}

// These operations forward both owned and guaranteed ownership.
static bool isOwnershipForwardingInstructionKind(SILInstructionKind kind) {
switch (kind) {
case SILInstructionKind::TupleInst:
case SILInstructionKind::StructInst:
case SILInstructionKind::EnumInst:
case SILInstructionKind::DifferentiableFunctionInst:
case SILInstructionKind::LinearFunctionInst:
case SILInstructionKind::OpenExistentialRefInst:
case SILInstructionKind::UpcastInst:
case SILInstructionKind::UncheckedValueCastInst:
case SILInstructionKind::UncheckedRefCastInst:
case SILInstructionKind::ConvertFunctionInst:
case SILInstructionKind::RefToBridgeObjectInst:
case SILInstructionKind::BridgeObjectToRefInst:
case SILInstructionKind::UnconditionalCheckedCastInst:
case SILInstructionKind::UncheckedEnumDataInst:
case SILInstructionKind::SelectEnumInst:
case SILInstructionKind::SwitchEnumInst:
case SILInstructionKind::CheckedCastBranchInst:
case SILInstructionKind::DestructureStructInst:
case SILInstructionKind::DestructureTupleInst:
case SILInstructionKind::MarkDependenceInst:
case SILInstructionKind::InitExistentialRefInst:
return true;
default:
return false;
}
}

// These operations forward guaranteed ownership, but don't necessarily forward
// owned values.
static bool isGuaranteedForwardingInstructionKind(SILInstructionKind kind) {
switch (kind) {
case SILInstructionKind::TupleExtractInst:
case SILInstructionKind::StructExtractInst:
case SILInstructionKind::DifferentiableFunctionExtractInst:
case SILInstructionKind::LinearFunctionExtractInst:
case SILInstructionKind::OpenExistentialValueInst:
case SILInstructionKind::OpenExistentialBoxValueInst:
return true;
default:
return isOwnershipForwardingInstructionKind(kind);
}
}

bool swift::canOpcodeForwardGuaranteedValues(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()) {
assert(OwnershipForwardingMixin::isa(ti));
return true;
}
if (ti->isTransformationTerminator())
return OwnershipForwardingMixin::get(ti)->isDirectlyForwarding();

auto *inst = value->getDefiningInstruction();
if (!inst)
return false;
if (auto *inst = value->getDefiningInstruction())
if (auto *mixin = OwnershipForwardingMixin::get(inst))
return mixin->isDirectlyForwarding() &&
!isa<OwnedFirstArgForwardingSingleValueInst>(inst);

bool result = isGuaranteedForwardingInstructionKind(inst->getKind());
if (result) {
assert(!isa<OwnedFirstArgForwardingSingleValueInst>(inst));
assert(OwnershipForwardingMixin::isa(inst));
}
return result;
return false;
}

bool swift::canOpcodeForwardGuaranteedValues(Operand *use) {
auto *user = use->getUser();
bool result = isOwnershipForwardingInstructionKind(user->getKind());
if (result) {
assert(!isa<GuaranteedFirstArgForwardingSingleValueInst>(user));
assert(OwnershipForwardingMixin::isa(user));
}
return result;
}

static bool isOwnedForwardingValueKind(SILInstructionKind kind) {
switch (kind) {
case SILInstructionKind::MarkUninitializedInst:
return true;
default:
return isOwnershipForwardingInstructionKind(kind);
}
if (auto *mixin = OwnershipForwardingMixin::get(use->getUser()))
return mixin->isDirectlyForwarding() &&
!isa<OwnedFirstArgForwardingSingleValueInst>(use->getUser());
return false;
}

bool swift::canOpcodeForwardOwnedValues(SILValue value) {
// If we have a SILArgument and we are the successor block of a transforming
// terminator, we are fine.
if (auto *arg = dyn_cast<SILPhiArgument>(value))
if (auto *predTerm = arg->getSingleTerminator())
if (predTerm->isTransformationTerminator()) {
assert(OwnershipForwardingMixin::isa(predTerm));
return true;
}
auto *inst = value->getDefiningInstruction();
if (!inst)
return false;
if (predTerm->isTransformationTerminator())
return OwnershipForwardingMixin::get(predTerm)->isDirectlyForwarding();

bool result = isOwnedForwardingValueKind(inst->getKind());
if (result) {
assert(!isa<GuaranteedFirstArgForwardingSingleValueInst>(inst));
assert(OwnershipForwardingMixin::isa(inst));
}
return result;
if (auto *inst = value->getDefiningInstruction())
if (auto *mixin = OwnershipForwardingMixin::get(inst))
return mixin->isDirectlyForwarding() &&
!isa<GuaranteedFirstArgForwardingSingleValueInst>(inst);

return false;
}

bool swift::canOpcodeForwardOwnedValues(Operand *use) {
auto *user = use->getUser();
bool result = isOwnershipForwardingInstructionKind(user->getKind());
if (result) {
assert(OwnershipForwardingMixin::isa(user));
}
return result;
if (auto *mixin = OwnershipForwardingMixin::get(user))
return mixin->isDirectlyForwarding() &&
!isa<GuaranteedFirstArgForwardingSingleValueInst>(user);
return false;
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -980,7 +910,8 @@ bool swift::getAllBorrowIntroducingValues(SILValue inputValue,
// predecessor terminator.
auto *arg = cast<SILPhiArgument>(value);
auto *termInst = arg->getSingleTerminator();
assert(termInst && termInst->isTransformationTerminator());
assert(termInst && termInst->isTransformationTerminator() &&
OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
assert(termInst->getNumOperands() == 1 &&
"Transforming terminators should always have a single operand");
worklist.push_back(termInst->getAllOperands()[0].get());
Expand Down Expand Up @@ -1031,7 +962,8 @@ BorrowedValue swift::getSingleBorrowIntroducingValue(SILValue inputValue) {
// predecessor terminator.
auto *arg = cast<SILPhiArgument>(currentValue);
auto *termInst = arg->getSingleTerminator();
assert(termInst && termInst->isTransformationTerminator());
assert(termInst && termInst->isTransformationTerminator() &&
OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
assert(termInst->getNumOperands() == 1 &&
"Transformation terminators should only have single operands");
currentValue = termInst->getAllOperands()[0].get();
Expand Down Expand Up @@ -1083,7 +1015,8 @@ bool swift::getAllOwnedValueIntroducers(
// predecessor terminator.
auto *arg = cast<SILPhiArgument>(value);
auto *termInst = arg->getSingleTerminator();
assert(termInst && termInst->isTransformationTerminator());
assert(termInst && termInst->isTransformationTerminator() &&
OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
assert(termInst->getNumOperands() == 1 &&
"Transforming terminators should always have a single operand");
worklist.push_back(termInst->getAllOperands()[0].get());
Expand Down Expand Up @@ -1127,10 +1060,11 @@ OwnedValueIntroducer swift::getSingleOwnedValueIntroducer(SILValue inputValue) {
}

// Otherwise, we should have a block argument that is defined by a single
// predecessor terminator.
// predecessor terminator and is directly forwarding.
auto *arg = cast<SILPhiArgument>(currentValue);
auto *termInst = arg->getSingleTerminator();
assert(termInst && termInst->isTransformationTerminator());
assert(termInst && termInst->isTransformationTerminator() &&
OwnershipForwardingMixin::get(termInst)->isDirectlyForwarding());
assert(termInst->getNumOperands()
- termInst->getNumTypeDependentOperands() == 1 &&
"Transformation terminators should only have single operands");
Expand Down
18 changes: 18 additions & 0 deletions lib/SIL/Verifier/SILVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4048,6 +4048,24 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
CBI->getOperand().getOwnershipKind()),
"failure dest block argument must have ownership compatible with "
"the checked_cast_br operand");

// Do not allow for checked_cast_br to forward guaranteed ownership if the
// source type is an AnyObject.
//
// EXPLANATION: A checked_cast_br from an AnyObject may return a different
// object. This occurs for instance if one has an AnyObject that is a
// boxed AnyHashable (ClassType). This breaks with the guarantees of
// checked_cast_br guaranteed, so we ban it.
require(!CBI->getOperand()->getType().isAnyObject() ||
CBI->getOperand().getOwnershipKind() !=
OwnershipKind::Guaranteed,
"checked_cast_br with an AnyObject typed source cannot forward "
"guaranteed ownership");
require(CBI->isDirectlyForwarding() ||
CBI->getOperand().getOwnershipKind() !=
OwnershipKind::Guaranteed,
"If checked_cast_br is not directly forwarding, it can not have "
"guaranteed ownership");
} else {
require(CBI->getFailureBB()->args_empty(),
"Failure dest of checked_cast_br must not take any argument in "
Expand Down
6 changes: 6 additions & 0 deletions lib/SILGen/SILGenBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,12 @@ void SILGenBuilder::createCheckedCastBranch(SILLocation loc, bool isExact,
SILBasicBlock *falseBlock,
ProfileCounter Target1Count,
ProfileCounter Target2Count) {
// Check if our source type is AnyObject. In such a case, we need to ensure
// plus one our operand since SIL does not support guaranteed casts from an
// AnyObject.
if (op.getType().isAnyObject()) {
op = op.ensurePlusOne(SGF, loc);
}
createCheckedCastBranch(loc, isExact, op.forward(SGF),
destLoweredTy, destFormalTy,
trueBlock, falseBlock,
Expand Down
15 changes: 15 additions & 0 deletions lib/SILOptimizer/SemanticARC/OwnershipLiveRange.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "OwnershipPhiOperand.h"

#include "swift/SIL/BasicBlockUtils.h"
#include "swift/SIL/OwnershipUtils.h"

using namespace swift;
using namespace swift::semanticarc;
Expand Down Expand Up @@ -83,6 +84,20 @@ OwnershipLiveRange::OwnershipLiveRange(SILValue value)
continue;
}

// If we have a subclass of OwnershipForwardingMixin that doesnt directly
// forward its operand to the result, treat the use as an unknown consuming
// use.
//
// If we do not directly forward and we have an owned value (which we do
// here), we could get back a different value. Thus we can not transform
// such a thing from owned to guaranteed.
if (auto *i = OwnershipForwardingMixin::get(op->getUser())) {
if (!i->isDirectlyForwarding()) {
tmpUnknownConsumingUses.push_back(op);
continue;
}
}

// Ok, this is a forwarding instruction whose ownership we can flip from
// owned -> guaranteed.
tmpForwardingConsumingUses.push_back(op);
Expand Down
44 changes: 44 additions & 0 deletions test/SILGen/checked_cast_br_anyobject.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// RUN: %target-swift-emit-silgen -module-name checked_cast_br_anyobject -parse-as-library -Xllvm -sil-full-demangle -enforce-exclusivity=checked %s

public struct BridgedSwiftObject {
var swiftMetatype : UnsafeRawPointer
var refCounts : Int64
}

public typealias SwiftObject = UnsafeMutablePointer<BridgedSwiftObject>

extension UnsafeMutablePointer where Pointee == BridgedSwiftObject {
init<T: AnyObject>(_ object: T) {
let ptr = Unmanaged.passUnretained(object).toOpaque()
self = ptr.bindMemory(to: BridgedSwiftObject.self, capacity: 1)
}

func getAs<T: AnyObject>(_ objectType: T.Type) -> T {
return Unmanaged<T>.fromOpaque(self).takeUnretainedValue()
}
}

extension Optional where Wrapped == UnsafeMutablePointer<BridgedSwiftObject> {
func getAs<T: AnyObject>(_ objectType: T.Type) -> T? {
if let pointer = self {
return Unmanaged<T>.fromOpaque(pointer).takeUnretainedValue()
}
return nil
}
}

public class Klass {}
public class Klass2 {}

// Make sure that we do not crash when emitting this code!
public func getValue(_ obj: UnsafeMutablePointer<BridgedSwiftObject>) -> AnyObject {
let v = obj.getAs(AnyObject.self)
switch v {
case let k as Klass:
return k
case let k as Klass2:
return k
default:
fatalError("unknown type")
}
}
Loading