From 0b42f35f6a447f40d752a3911e2eea9309339cc0 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Mon, 15 Jul 2024 18:24:56 -0700 Subject: [PATCH 1/6] Add operator<<(llvm::raw_ostream &os, SILIsolationInfo|SILDynamicMergedIsolationInfo. Just slicing off a nice addition. These already had appropriate print methods so this commit just exposes the print API in the raw_ostream format. --- .../SILOptimizer/Utils/SILIsolationInfo.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/include/swift/SILOptimizer/Utils/SILIsolationInfo.h b/include/swift/SILOptimizer/Utils/SILIsolationInfo.h index 06122d4db1ac2..bbfa0ef4f7132 100644 --- a/include/swift/SILOptimizer/Utils/SILIsolationInfo.h +++ b/include/swift/SILOptimizer/Utils/SILIsolationInfo.h @@ -493,4 +493,22 @@ class SILDynamicMergedIsolationInfo { } // namespace swift +namespace llvm { + +inline llvm::raw_ostream & +operator<<(llvm::raw_ostream &os, + const swift::SILIsolationInfo &isolationInfo) { + isolationInfo.printForOneLineLogging(os); + return os; +} + +inline llvm::raw_ostream & +operator<<(llvm::raw_ostream &os, + const swift::SILDynamicMergedIsolationInfo &isolationInfo) { + isolationInfo.printForOneLineLogging(os); + return os; +} + +} // namespace llvm + #endif From 75152e7834b7f828e5b62b491760ef74be6716f2 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Tue, 16 Jul 2024 13:10:22 -0700 Subject: [PATCH 2/6] [region-isolation] Teach region isolation how to handle cases where due to compiler bugs, we have a function isolated to self without an isolated parameter. Closures generally only inherit actor instance isolation if they directly capture state from the actor instance. In this case, for some reason that is not true, so we hit an assert that assumes that we will only see a global actor isolated isolation. Region Isolation should be able to handle code even if the closure isolation invariant is violated by the frontend. So to do this, I am introducing a new singleton actor instance to represent the isolation of a defer or closure created in an actor instance isolated method. The reason why I am using a singleton is that closures and defer are not methods so we do not actually know which parameter is 'self' since it isn't in the abi. But we still need some value to represent the captured values as belonging to. To square this circle, I just did what we have done in a similar situation where we did not have a value: (ActorAccessorInit). In that case, we just use a sentinel to represent the instance (NOTE: This is represented just via a kind so ActorInstances that are operator== equal will not &value equal since we are just using a kind). --- .../SILOptimizer/Utils/SILIsolationInfo.h | 26 ++++++++++++++++--- lib/SILOptimizer/Utils/SILIsolationInfo.cpp | 25 +++++++++++++++--- .../transfernonsendable_crashers_objc.h | 6 +++++ .../transfernonsendable_crashers_objc.swift | 7 +++++ 4 files changed, 58 insertions(+), 6 deletions(-) diff --git a/include/swift/SILOptimizer/Utils/SILIsolationInfo.h b/include/swift/SILOptimizer/Utils/SILIsolationInfo.h index bbfa0ef4f7132..a3ca232914db8 100644 --- a/include/swift/SILOptimizer/Utils/SILIsolationInfo.h +++ b/include/swift/SILOptimizer/Utils/SILIsolationInfo.h @@ -46,11 +46,18 @@ class ActorInstance { /// access to the self value and instead have access indirectly to the /// storage associated with the accessor. ActorAccessorInit = 0x1, + + /// An actor instance that is represented by "self" being captured in a + /// closure of some sort. In such a case, we do not know which of the + /// parameters are the true "self" (since the closure is a thin + /// function)... so we just use an artificial ActorInstance to represent + /// self in this case. + CapturedActorSelf = 0x2, }; - /// Set to (SILValue(), ActorAccessorInit) if we have an ActorAccessorInit. Is - /// null if we have (SILValue(), Kind::Value). - llvm::PointerIntPair value; + /// Set to (SILValue(), $KIND) if we have an ActorAccessorInit|CapturedSelf. + /// Is null if we have (SILValue(), Kind::Value). + llvm::PointerIntPair value; ActorInstance(SILValue value, Kind kind) : value(value, std::underlying_type::type(kind)) {} @@ -68,10 +75,18 @@ class ActorInstance { return ActorInstance(value, Kind::Value); } + /// See Kind::ActorAccessorInit for explanation on what a ActorAccessorInit + /// is. static ActorInstance getForActorAccessorInit() { return ActorInstance(SILValue(), Kind::ActorAccessorInit); } + /// See Kind::CapturedActorSelf for explanation on what a CapturedActorSelf + /// is. + static ActorInstance getForCapturedSelf() { + return ActorInstance(SILValue(), Kind::CapturedActorSelf); + } + explicit operator bool() const { return bool(value.getOpaqueValue()); } Kind getKind() const { return Kind(value.getInt()); } @@ -91,6 +106,10 @@ class ActorInstance { bool isAccessorInit() const { return getKind() == Kind::ActorAccessorInit; } + bool isCapturedActorSelf() const { + return getKind() == Kind::CapturedActorSelf; + } + bool operator==(const ActorInstance &other) const { // If both are null, return true. if (!bool(*this) && !bool(other)) @@ -105,6 +124,7 @@ class ActorInstance { case Kind::Value: return getValue() == other.getValue(); case Kind::ActorAccessorInit: + case Kind::CapturedActorSelf: return true; } } diff --git a/lib/SILOptimizer/Utils/SILIsolationInfo.cpp b/lib/SILOptimizer/Utils/SILIsolationInfo.cpp index 11e37a2fa20db..7a07a98c9a162 100644 --- a/lib/SILOptimizer/Utils/SILIsolationInfo.cpp +++ b/lib/SILOptimizer/Utils/SILIsolationInfo.cpp @@ -879,8 +879,14 @@ SILIsolationInfo SILIsolationInfo::get(SILArgument *arg) { if (auto functionIsolation = fArg->getFunction()->getActorIsolation()) { if (functionIsolation.isActorIsolated()) { assert(functionIsolation.isGlobalActor()); - return SILIsolationInfo::getGlobalActorIsolated( - fArg, functionIsolation.getGlobalActor()); + if (functionIsolation.isGlobalActor()) { + return SILIsolationInfo::getGlobalActorIsolated( + fArg, functionIsolation.getGlobalActor()); + } + + return SILIsolationInfo::getActorInstanceIsolated( + fArg, ActorInstance::getForActorAccessorInit(), + functionIsolation.getActor()); } } @@ -944,6 +950,12 @@ void SILIsolationInfo::print(llvm::raw_ostream &os) const { os << '\n'; os << "instance: actor accessor init\n"; return; + case ActorInstance::Kind::CapturedActorSelf: + os << "'self'-isolated"; + printOptions(os); + os << '\n'; + os << "instance: captured actor instance self\n"; + return; } } @@ -1062,6 +1074,9 @@ void SILIsolationInfo::printForDiagnostics(llvm::raw_ostream &os) const { case ActorInstance::Kind::ActorAccessorInit: os << "'self'-isolated"; return; + case ActorInstance::Kind::CapturedActorSelf: + os << "'self'-isolated"; + return; } } @@ -1102,7 +1117,11 @@ void SILIsolationInfo::printForOneLineLogging(llvm::raw_ostream &os) const { break; } case ActorInstance::Kind::ActorAccessorInit: - os << "'self'-isolated"; + os << "'self'-isolated (actor-accessor-init)"; + printOptions(os); + return; + case ActorInstance::Kind::CapturedActorSelf: + os << "'self'-isolated (captured-actor-self)"; printOptions(os); return; } diff --git a/test/Concurrency/Inputs/transfernonsendable_crashers_objc.h b/test/Concurrency/Inputs/transfernonsendable_crashers_objc.h index f2021d515bad7..56a666df25ce2 100644 --- a/test/Concurrency/Inputs/transfernonsendable_crashers_objc.h +++ b/test/Concurrency/Inputs/transfernonsendable_crashers_objc.h @@ -1,5 +1,11 @@ +@import Foundation; + @interface MyNotificationCenter - (id)init; - (void)post; @end + +@protocol MySession +- (void)endSession; +@end diff --git a/test/Concurrency/transfernonsendable_crashers_objc.swift b/test/Concurrency/transfernonsendable_crashers_objc.swift index fdbca44eeb997..821e7c56be37a 100644 --- a/test/Concurrency/transfernonsendable_crashers_objc.swift +++ b/test/Concurrency/transfernonsendable_crashers_objc.swift @@ -18,3 +18,10 @@ public func handleFile(at location: URL) throws { private func moveTheme(from location: URL) throws -> URL { fatalError() } private func unzipFile(at location: URL) throws -> URL { fatalError() } + +actor MyActor { + func test() { + var session: MySession? + defer { session?.end() } + } +} From ace94b00bab0cfc45a035d73fbb8bd3ae2936c60 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Tue, 16 Jul 2024 22:54:38 -0700 Subject: [PATCH 3/6] [region-isolation] Move RepresentativeValue from RegionAnalysis.h -> PartitionUtils.h and add APIs for mapping an ElementID -> Representative. This is just moving up the declaration in the chain of dependencies so that I can write logic in PartitionUtils.h using it. I also added entrypoints to lookup the ReprensetativeValue for our various emitters. --- .../SILOptimizer/Analysis/RegionAnalysis.h | 86 +----------------- .../swift/SILOptimizer/Utils/PartitionUtils.h | 87 +++++++++++++++++++ lib/SILOptimizer/Analysis/RegionAnalysis.cpp | 9 ++ .../Mandatory/TransferNonSendable.cpp | 4 + 4 files changed, 104 insertions(+), 82 deletions(-) diff --git a/include/swift/SILOptimizer/Analysis/RegionAnalysis.h b/include/swift/SILOptimizer/Analysis/RegionAnalysis.h index 99bc66d531d81..12fd0c2cc42f4 100644 --- a/include/swift/SILOptimizer/Analysis/RegionAnalysis.h +++ b/include/swift/SILOptimizer/Analysis/RegionAnalysis.h @@ -114,7 +114,6 @@ class BlockPartitionState { class TrackableValue; class TrackableValueState; -class RepresentativeValue; enum class TrackableValueFlag { /// Base value that says a value is uniquely represented and is @@ -208,53 +207,6 @@ class regionanalysisimpl::TrackableValueState { } }; -/// The representative value of the equivalence class that makes up a tracked -/// value. -/// -/// We use a wrapper struct here so that we can inject "fake" actor isolated -/// values into the regions of values that become merged into an actor by -/// calling a function without a non-sendable result. -class regionanalysisimpl::RepresentativeValue { - friend llvm::DenseMapInfo; - - using InnerType = PointerUnion; - - /// If this is set to a SILValue then it is the actual represented value. If - /// it is set to a SILInstruction, then this is a "fake" representative value - /// used to inject actor isolatedness. The instruction stored is the - /// instruction that introduced the actor isolated-ness. - InnerType value; - -public: - RepresentativeValue() : value() {} - RepresentativeValue(SILValue value) : value(value) {} - RepresentativeValue(SILInstruction *actorRegionInst) - : value(actorRegionInst) {} - - operator bool() const { return bool(value); } - - void print(llvm::raw_ostream &os) const { - if (auto *inst = value.dyn_cast()) { - os << "ActorRegionIntroducingInst: " << *inst; - return; - } - - os << *value.get(); - } - - SILValue getValue() const { return value.get(); } - SILValue maybeGetValue() const { return value.dyn_cast(); } - bool hasRegionIntroducingInst() const { return value.is(); } - SILInstruction *getActorRegionIntroducingInst() const { - return value.get(); - } - - SWIFT_DEBUG_DUMP { print(llvm::dbgs()); } - -private: - RepresentativeValue(InnerType value) : value(value) {} -}; - /// A tuple consisting of a base value and its value state. /// /// DISCUSSION: We are computing regions among equivalence classes of values @@ -336,7 +288,6 @@ class RegionAnalysisValueMap { using Region = PartitionPrimitives::Region; using TrackableValue = regionanalysisimpl::TrackableValue; using TrackableValueState = regionanalysisimpl::TrackableValueState; - using RepresentativeValue = regionanalysisimpl::RepresentativeValue; private: /// A map from the representative of an equivalence class of values to their @@ -365,6 +316,10 @@ class RegionAnalysisValueMap { /// value" returns an empty SILValue. SILValue maybeGetRepresentative(Element trackableValueID) const; + /// Returns the value for this instruction if it isn't a fake "represenative + /// value" to inject actor isolatedness. Asserts in such a case. + RepresentativeValue getRepresentativeValue(Element trackableValueID) const; + /// Returns the fake "representative value" for this element if it /// exists. Returns nullptr otherwise. SILInstruction *maybeGetActorIntroducingInst(Element trackableValueID) const; @@ -576,37 +531,4 @@ class RegionAnalysis final } // namespace swift -namespace llvm { - -inline llvm::raw_ostream & -operator<<(llvm::raw_ostream &os, - const swift::regionanalysisimpl::RepresentativeValue &value) { - value.print(os); - return os; -} - -template <> -struct DenseMapInfo { - using RepresentativeValue = swift::regionanalysisimpl::RepresentativeValue; - using InnerType = RepresentativeValue::InnerType; - using InnerDenseMapInfo = DenseMapInfo; - - static RepresentativeValue getEmptyKey() { - return RepresentativeValue(InnerDenseMapInfo::getEmptyKey()); - } - static RepresentativeValue getTombstoneKey() { - return RepresentativeValue(InnerDenseMapInfo::getTombstoneKey()); - } - - static unsigned getHashValue(RepresentativeValue value) { - return InnerDenseMapInfo::getHashValue(value.value); - } - - static bool isEqual(RepresentativeValue LHS, RepresentativeValue RHS) { - return InnerDenseMapInfo::isEqual(LHS.value, RHS.value); - } -}; - -} // namespace llvm - #endif diff --git a/include/swift/SILOptimizer/Utils/PartitionUtils.h b/include/swift/SILOptimizer/Utils/PartitionUtils.h index a769fac3937c5..6063f7daa6d77 100644 --- a/include/swift/SILOptimizer/Utils/PartitionUtils.h +++ b/include/swift/SILOptimizer/Utils/PartitionUtils.h @@ -100,6 +100,53 @@ namespace swift { class Partition; class TransferringOperandToStateMap; +/// The representative value of the equivalence class that makes up a tracked +/// value. +/// +/// We use a wrapper struct here so that we can inject "fake" actor isolated +/// values into the regions of values that become merged into an actor by +/// calling a function without a non-sendable result. +class RepresentativeValue { + friend llvm::DenseMapInfo; + + using InnerType = PointerUnion; + + /// If this is set to a SILValue then it is the actual represented value. If + /// it is set to a SILInstruction, then this is a "fake" representative value + /// used to inject actor isolatedness. The instruction stored is the + /// instruction that introduced the actor isolated-ness. + InnerType value; + +public: + RepresentativeValue() : value() {} + RepresentativeValue(SILValue value) : value(value) {} + RepresentativeValue(SILInstruction *actorRegionInst) + : value(actorRegionInst) {} + + operator bool() const { return bool(value); } + + void print(llvm::raw_ostream &os) const { + if (auto *inst = value.dyn_cast()) { + os << "ActorRegionIntroducingInst: " << *inst; + return; + } + + os << *value.get(); + } + + SILValue getValue() const { return value.get(); } + SILValue maybeGetValue() const { return value.dyn_cast(); } + bool hasRegionIntroducingInst() const { return value.is(); } + SILInstruction *getActorRegionIntroducingInst() const { + return value.get(); + } + + SWIFT_DEBUG_DUMP { print(llvm::dbgs()); } + +private: + RepresentativeValue(InnerType value) : value(value) {} +}; + /// A persistent data structure that is used to "rewind" partition history so /// that we can discover when values become part of the same region. /// @@ -1029,6 +1076,10 @@ struct PartitionOpEvaluator { return asImpl().getRepresentative(value); } + RepresentativeValue getRepresentativeValue(Element element) const { + return asImpl().getRepresentativeValue(element); + } + /// Apply \p op to the partition op. void apply(const PartitionOp &op) const { if (shouldEmitVerboseLogging()) { @@ -1416,6 +1467,10 @@ struct PartitionOpEvaluatorBaseImpl : PartitionOpEvaluator { /// have one. SILValue getRepresentative(SILValue value) const { return SILValue(); } + RepresentativeValue getRepresentativeValue(Element element) const { + return RepresentativeValue(); + } + /// Check if the representative value of \p elt is closure captured at \p /// op. /// @@ -1448,4 +1503,36 @@ struct PartitionOpEvaluatorBasic final } // namespace swift +namespace llvm { + +inline llvm::raw_ostream &operator<<(llvm::raw_ostream &os, + const swift::RepresentativeValue &value) { + value.print(os); + return os; +} + +template <> +struct DenseMapInfo { + using RepresentativeValue = swift::RepresentativeValue; + using InnerType = RepresentativeValue::InnerType; + using InnerDenseMapInfo = DenseMapInfo; + + static RepresentativeValue getEmptyKey() { + return RepresentativeValue(InnerDenseMapInfo::getEmptyKey()); + } + static RepresentativeValue getTombstoneKey() { + return RepresentativeValue(InnerDenseMapInfo::getTombstoneKey()); + } + + static unsigned getHashValue(RepresentativeValue value) { + return InnerDenseMapInfo::getHashValue(value.value); + } + + static bool isEqual(RepresentativeValue LHS, RepresentativeValue RHS) { + return InnerDenseMapInfo::isEqual(LHS.value, RHS.value); + } +}; + +} // namespace llvm + #endif // SWIFT_PARTITIONUTILS_H diff --git a/lib/SILOptimizer/Analysis/RegionAnalysis.cpp b/lib/SILOptimizer/Analysis/RegionAnalysis.cpp index 5e936e4cd88ce..cbbe67eb0c135 100644 --- a/lib/SILOptimizer/Analysis/RegionAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/RegionAnalysis.cpp @@ -3267,6 +3267,10 @@ bool BlockPartitionState::recomputeExitFromEntry( .maybeGetValue(); } + RepresentativeValue getRepresentativeValue(Element element) const { + return translator.getValueMap().getRepresentativeValue(element); + } + bool isClosureCaptured(Element elt, Operand *op) const { auto iter = translator.getValueForId(elt); if (!iter) @@ -3505,6 +3509,11 @@ RegionAnalysisValueMap::maybeGetRepresentative(Element trackableValueID) const { return getValueForId(trackableValueID)->getRepresentative().maybeGetValue(); } +RepresentativeValue +RegionAnalysisValueMap::getRepresentativeValue(Element trackableValueID) const { + return getValueForId(trackableValueID)->getRepresentative(); +} + SILIsolationInfo RegionAnalysisValueMap::getIsolationRegion(Element trackableValueID) const { auto iter = getValueForId(trackableValueID); diff --git a/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp b/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp index e8a43b2e7609d..8f6e8db691dc2 100644 --- a/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp +++ b/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp @@ -2030,6 +2030,10 @@ struct DiagnosticEvaluator final .maybeGetValue(); } + RepresentativeValue getRepresentativeValue(Element element) const { + return info->getValueMap().getRepresentativeValue(element); + } + bool isClosureCaptured(Element element, Operand *op) const { auto value = info->getValueMap().maybeGetRepresentative(element); if (!value) From ae797d43e93cbd6af9207ecc197537d7abd687e1 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Wed, 17 Jul 2024 14:17:01 -0700 Subject: [PATCH 4/6] [region-isolation] Propagate through the whole source operand instead of just the representative of the source value when constructing assign and merge. This will let me know the exact source operand used instead of the source value representative. This will ensure that the name associated with the diagnostic is not of the representative value, but the actual value that was the source of the assign. This is an NFCI commit that is an algebraic refactor. --- include/swift/SIL/InstWrappers.h | 19 ++ include/swift/SIL/SILInstruction.h | 24 +++ .../swift/SILOptimizer/Utils/PartitionUtils.h | 21 ++- lib/SILOptimizer/Analysis/RegionAnalysis.cpp | 162 +++++++++++------- 4 files changed, 154 insertions(+), 72 deletions(-) diff --git a/include/swift/SIL/InstWrappers.h b/include/swift/SIL/InstWrappers.h index d080ab7f294f2..a23906009c43b 100644 --- a/include/swift/SIL/InstWrappers.h +++ b/include/swift/SIL/InstWrappers.h @@ -221,6 +221,13 @@ class SelectEnumOperation { return sei->getCase(i); return value.get()->getCase(i); } + + std::pair getCaseOperand(unsigned i) const { + if (auto *sei = value.dyn_cast()) + return sei->getCaseOperand(i); + return value.get()->getCaseOperand(i); + } + /// Return the value that will be used as the result for the specified enum /// case. SILValue getCaseResult(EnumElementDecl *D) { @@ -229,6 +236,12 @@ class SelectEnumOperation { return value.get()->getCaseResult(D); } + Operand *getCaseResultOperand(EnumElementDecl *D) { + if (auto *sei = value.dyn_cast()) + return sei->getCaseResultOperand(D); + return value.get()->getCaseResultOperand(D); + } + /// If the default refers to exactly one case decl, return it. NullablePtr getUniqueCaseForDefault(); @@ -243,6 +256,12 @@ class SelectEnumOperation { return sei->getDefaultResult(); return value.get()->getDefaultResult(); } + + Operand *getDefaultResultOperand() const { + if (auto *sei = value.dyn_cast()) + return sei->getDefaultResultOperand(); + return value.get()->getDefaultResultOperand(); + } }; class ForwardingOperation { diff --git a/include/swift/SIL/SILInstruction.h b/include/swift/SIL/SILInstruction.h index 9a8d3baa38ba0..0c6d89bc12b57 100644 --- a/include/swift/SIL/SILInstruction.h +++ b/include/swift/SIL/SILInstruction.h @@ -7067,6 +7067,12 @@ class SelectEnumInstBase : public BaseTy { getAllOperands()[i+1].get()); } + std::pair getCaseOperand(unsigned i) const { + auto *self = const_cast(this); + return std::make_pair(getEnumElementDeclStorage()[i], + &self->getAllOperands()[i + 1]); + } + /// Return the value that will be used as the result for the specified enum /// case. SILValue getCaseResult(EnumElementDecl *D) { @@ -7079,6 +7085,18 @@ class SelectEnumInstBase : public BaseTy { return getDefaultResult(); } + Operand *getCaseResultOperand(EnumElementDecl *D) { + for (unsigned i = 0, e = getNumCases(); i != e; ++i) { + auto Entry = getCaseOperand(i); + if (Entry.first == D) + return Entry.second; + } + + // select_enum is required to be fully covered, so return the default if we + // didn't find anything. + return getDefaultResultOperand(); + } + bool hasDefault() const { return sharedUInt8().SelectEnumInstBase.hasDefault; } @@ -7088,6 +7106,12 @@ class SelectEnumInstBase : public BaseTy { return getAllOperands().back().get(); } + Operand *getDefaultResultOperand() const { + assert(hasDefault() && "doesn't have a default"); + auto *self = const_cast(this); + return &self->getAllOperands().back(); + } + unsigned getNumCases() const { return getAllOperands().size() - 1 - hasDefault(); } diff --git a/include/swift/SILOptimizer/Utils/PartitionUtils.h b/include/swift/SILOptimizer/Utils/PartitionUtils.h index 6063f7daa6d77..faf7c6fdd86fc 100644 --- a/include/swift/SILOptimizer/Utils/PartitionUtils.h +++ b/include/swift/SILOptimizer/Utils/PartitionUtils.h @@ -510,15 +510,23 @@ class PartitionOp { "Transfer needs a sourceInst"); } + PartitionOp(PartitionOpKind opKind, Element arg1, Element arg2, + Operand *sourceOp = nullptr) + : opKind(opKind), opArgs({arg1, arg2}), source(sourceOp) { + assert((opKind == PartitionOpKind::Assign || + opKind == PartitionOpKind::Merge) && + "Only supported for assign and merge"); + } + PartitionOp(PartitionOpKind opKind, SILInstruction *sourceInst) : opKind(opKind), opArgs(), source(sourceInst) {} friend class Partition; public: - static PartitionOp Assign(Element tgt, Element src, - SILInstruction *sourceInst = nullptr) { - return PartitionOp(PartitionOpKind::Assign, tgt, src, sourceInst); + static PartitionOp Assign(Element destElt, Element srcElt, + Operand *srcOperand = nullptr) { + return PartitionOp(PartitionOpKind::Assign, destElt, srcElt, srcOperand); } static PartitionOp AssignFresh(Element tgt, @@ -535,9 +543,10 @@ class PartitionOp { return PartitionOp(PartitionOpKind::UndoTransfer, tgt, untransferringInst); } - static PartitionOp Merge(Element tgt1, Element tgt2, - SILInstruction *sourceInst = nullptr) { - return PartitionOp(PartitionOpKind::Merge, tgt1, tgt2, sourceInst); + static PartitionOp Merge(Element destElement, Element srcElement, + Operand *sourceOperand = nullptr) { + return PartitionOp(PartitionOpKind::Merge, destElement, srcElement, + sourceOperand); } static PartitionOp Require(Element tgt, diff --git a/lib/SILOptimizer/Analysis/RegionAnalysis.cpp b/lib/SILOptimizer/Analysis/RegionAnalysis.cpp index cbbe67eb0c135..f21118332eae0 100644 --- a/lib/SILOptimizer/Analysis/RegionAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/RegionAnalysis.cpp @@ -66,6 +66,21 @@ static llvm::cl::opt AbortOnUnknownPatternMatchErrorCmdLine( // MARK: Utilities //===----------------------------------------------------------------------===// +namespace { + +using OperandRefRangeTransform = std::function; +using OperandRefRange = + iterator_range::iterator, + OperandRefRangeTransform>>; + +} // anonymous namespace + +static OperandRefRange makeOperandRefRange(MutableArrayRef input) { + auto toOperand = [](Operand &operand) { return &operand; }; + auto baseRange = llvm::make_range(input.begin(), input.end()); + return llvm::map_range(baseRange, OperandRefRangeTransform(toOperand)); +} + std::optional regionanalysisimpl::getApplyIsolationCrossing(SILInstruction *inst) { if (ApplyExpr *apply = inst->getLoc().getAsASTNode()) @@ -418,9 +433,9 @@ SILValue RegionAnalysisFunctionInfo::getUnderlyingTrackedValue(SILValue value) { namespace { struct TermArgSources { - SmallFrozenMultiMap argSources; + SmallFrozenMultiMap argSources; - template > + template > void addValues(ValueRangeTy valueRange, SILBasicBlock *destBlock) { for (auto pair : llvm::enumerate(valueRange)) argSources.insert(destBlock->getArgument(pair.index()), pair.value()); @@ -461,19 +476,22 @@ struct TermArgSources { } private: - void init(BranchInst *bi) { addValues(bi->getArgs(), bi->getDestBB()); } + void init(BranchInst *bi) { + addValues(makeOperandRefRange(bi->getAllOperands()), bi->getDestBB()); + } void init(CondBranchInst *cbi) { - addValues(cbi->getTrueArgs(), cbi->getTrueBB()); - addValues(cbi->getFalseArgs(), cbi->getFalseBB()); + addValues(makeOperandRefRange(cbi->getTrueOperands()), cbi->getTrueBB()); + addValues(makeOperandRefRange(cbi->getFalseOperands()), cbi->getFalseBB()); } void init(DynamicMethodBranchInst *dmBranchInst) { - addValues({dmBranchInst->getOperand()}, dmBranchInst->getHasMethodBB()); + addValues({&dmBranchInst->getAllOperands()[0]}, + dmBranchInst->getHasMethodBB()); } void init(CheckedCastBranchInst *ccbi) { - addValues({ccbi->getOperand()}, ccbi->getSuccessBB()); + addValues({&ccbi->getAllOperands()[0]}, ccbi->getSuccessBB()); } }; @@ -1145,20 +1163,21 @@ struct PartitionOpBuilder { PartitionOp::AssignFresh(lookupValueID(value), currentInst)); } - void addAssign(SILValue tgt, SILValue src) { - assert(valueHasID(src, /*dumpIfHasNoID=*/true) && + void addAssign(SILValue destValue, Operand *srcOperand) { + assert(valueHasID(srcOperand->get(), /*dumpIfHasNoID=*/true) && "source value of assignment should already have been encountered"); - Element srcID = lookupValueID(src); - if (lookupValueID(tgt) == srcID) { + Element srcID = lookupValueID(srcOperand->get()); + if (lookupValueID(destValue) == srcID) { LLVM_DEBUG(llvm::dbgs() << " Skipping assign since tgt and src have " "the same representative.\n"); LLVM_DEBUG(llvm::dbgs() << " Rep ID: %%" << srcID.num << ".\n"); return; } - currentInstPartitionOps.emplace_back(PartitionOp::Assign( - lookupValueID(tgt), lookupValueID(src), currentInst)); + currentInstPartitionOps.emplace_back( + PartitionOp::Assign(lookupValueID(destValue), + lookupValueID(srcOperand->get()), srcOperand)); } void addTransfer(SILValue representative, Operand *op) { @@ -1178,30 +1197,31 @@ struct PartitionOpBuilder { lookupValueID(representative), untransferringInst)); } - void addMerge(SILValue fst, SILValue snd) { - assert(valueHasID(fst, /*dumpIfHasNoID=*/true) && - valueHasID(snd, /*dumpIfHasNoID=*/true) && + void addMerge(SILValue destValue, Operand *srcOperand) { + assert(valueHasID(destValue, /*dumpIfHasNoID=*/true) && + valueHasID(srcOperand->get(), /*dumpIfHasNoID=*/true) && "merged values should already have been encountered"); - if (lookupValueID(fst) == lookupValueID(snd)) + if (lookupValueID(destValue) == lookupValueID(srcOperand->get())) return; - currentInstPartitionOps.emplace_back(PartitionOp::Merge( - lookupValueID(fst), lookupValueID(snd), currentInst)); + currentInstPartitionOps.emplace_back( + PartitionOp::Merge(lookupValueID(destValue), + lookupValueID(srcOperand->get()), srcOperand)); } /// Mark \p value artifically as being part of an actor isolated region by /// introducing a new fake actor introducing representative and merging them. - void addActorIntroducingInst(SILValue value, + void addActorIntroducingInst(SILValue sourceValue, Operand *sourceOperand, SILIsolationInfo actorIsolation) { - assert(valueHasID(value, /*dumpIfHasNoID=*/true) && + assert(valueHasID(sourceValue, /*dumpIfHasNoID=*/true) && "merged values should already have been encountered"); auto elt = getActorIntroducingRepresentative(actorIsolation); currentInstPartitionOps.emplace_back( PartitionOp::AssignFresh(elt, currentInst)); currentInstPartitionOps.emplace_back( - PartitionOp::Merge(lookupValueID(value), elt, currentInst)); + PartitionOp::Merge(lookupValueID(sourceValue), elt, sourceOperand)); } void addRequire(SILValue value) { @@ -1609,7 +1629,7 @@ class PartitionOpTranslator { const SourceRange &sourceValues, SILIsolationInfo resultIsolationInfoOverride = {}, bool requireSrcValues = true) { - SmallVector assignOperands; + SmallVector, 8> assignOperands; SmallVector assignResults; // A helper we use to emit an unknown patten error if our merge is @@ -1623,9 +1643,11 @@ class PartitionOpTranslator { mergedInfo = SILDynamicMergedIsolationInfo::getDisconnected(false); } - for (SILValue src : sourceValues) { + for (Operand *srcOperand : sourceValues) { + auto src = srcOperand->get(); if (auto value = tryToTrackValue(src)) { - assignOperands.push_back(value->getRepresentative().getValue()); + assignOperands.push_back( + {srcOperand, value->getRepresentative().getValue()}); auto originalMergedInfo = mergedInfo; (void)originalMergedInfo; if (mergedInfo) @@ -1684,11 +1706,11 @@ class PartitionOpTranslator { // uses of the store_borrow. if (requireSrcValues) for (auto src : assignOperands) - builder.addRequire(src); + builder.addRequire(src.second); // Merge all srcs. for (unsigned i = 1; i < assignOperands.size(); i++) { - builder.addMerge(assignOperands[i - 1], assignOperands[i]); + builder.addMerge(assignOperands[i - 1].second, assignOperands[i].first); } // If we do not have any non sendable results, return early. @@ -1703,7 +1725,8 @@ class PartitionOpTranslator { // passed in a specific isolation info unlike earlier when processing // actual results. if (assignOperands.size() && resultIsolationInfoOverride) { - builder.addActorIntroducingInst(assignOperands.back(), + builder.addActorIntroducingInst(assignOperands.back().second, + assignOperands.back().first, resultIsolationInfoOverride); } @@ -1718,7 +1741,7 @@ class PartitionOpTranslator { // If no non-sendable srcs, non-sendable tgts get a fresh region. builder.addAssignFresh(front); } else { - builder.addAssign(front, assignOperands.front()); + builder.addAssign(front, assignOperands.front().first); } // Assign all targets to the target region. @@ -1726,7 +1749,7 @@ class PartitionOpTranslator { SILValue next = assignResultsRef.front(); assignResultsRef = assignResultsRef.drop_front(); - builder.addAssign(next, front); + builder.addAssign(next, assignOperands.front().first); } } @@ -1898,7 +1921,8 @@ class PartitionOpTranslator { SmallVector applyResults; getApplyResults(pai, applyResults); - translateSILMultiAssign(applyResults, pai->getOperandValues()); + translateSILMultiAssign(applyResults, + makeOperandRefRange(pai->getAllOperands())); } void translateCreateAsyncTask(BuiltinInst *bi) { @@ -1922,13 +1946,14 @@ class PartitionOpTranslator { // If we do not have a special builtin, just do a multi-assign. Builtins do // not cross async boundaries. - return translateSILMultiAssign(bi->getResults(), bi->getOperandValues()); + return translateSILMultiAssign(bi->getResults(), + makeOperandRefRange(bi->getAllOperands())); } void translateNonIsolationCrossingSILApply(FullApplySite fas) { // For non-self parameters, gather all of the transferring parameters and // gather our non-transferring parameters. - SmallVector nonTransferringParameters; + SmallVector nonTransferringParameters; if (fas.getNumArguments()) { // NOTE: We want to process indirect parameters as if they are // parameters... so we process them in nonTransferringParameters. @@ -1945,7 +1970,7 @@ class PartitionOpTranslator { builder.addTransfer(value->getRepresentative().getValue(), &op); } } else { - nonTransferringParameters.push_back(op.get()); + nonTransferringParameters.push_back(&op); } } } @@ -1963,7 +1988,7 @@ class PartitionOpTranslator { &selfOperand); } } else { - nonTransferringParameters.push_back(selfOperand.get()); + nonTransferringParameters.push_back(&selfOperand); } } @@ -2115,13 +2140,13 @@ class PartitionOpTranslator { } template <> - void translateSILAssign(SILValue dest, SILValue src) { - return translateSILAssign(dest, TinyPtrVector(src)); + void translateSILAssign(SILValue dest, Operand *srcOperand) { + return translateSILAssign(dest, TinyPtrVector(srcOperand)); } void translateSILAssign(SILInstruction *inst) { return translateSILMultiAssign(inst->getResults(), - inst->getOperandValues()); + makeOperandRefRange(inst->getAllOperands())); } /// If the passed SILValue is NonSendable, then create a fresh region for it, @@ -2131,7 +2156,7 @@ class PartitionOpTranslator { /// isolationInfo is set. void translateSILAssignFresh(SILValue val) { return translateSILMultiAssign(TinyPtrVector(val), - TinyPtrVector()); + TinyPtrVector()); } void translateSILAssignFresh(SILValue val, SILIsolationInfo info) { @@ -2146,27 +2171,26 @@ class PartitionOpTranslator { } template - void translateSILMerge(SILValue dest, Collection collection, + void translateSILMerge(SILValue dest, Collection srcCollection, bool requireOperands = true) { auto trackableDest = tryToTrackValue(dest); if (!trackableDest) return; - for (SILValue elt : collection) { - if (auto trackableSrc = tryToTrackValue(elt)) { + for (Operand *op : srcCollection) { + if (auto trackableSrc = tryToTrackValue(op->get())) { if (requireOperands) { builder.addRequire(trackableSrc->getRepresentative().getValue()); builder.addRequire(trackableDest->getRepresentative().getValue()); } - builder.addMerge(trackableDest->getRepresentative().getValue(), - trackableSrc->getRepresentative().getValue()); + builder.addMerge(trackableDest->getRepresentative().getValue(), op); } } } template <> - void translateSILMerge(SILValue dest, SILValue src, - bool requireOperands) { - return translateSILMerge(dest, TinyPtrVector(src), + void translateSILMerge(SILValue dest, Operand *src, + bool requireOperands) { + return translateSILMerge(dest, TinyPtrVector(src), requireOperands); } @@ -2204,7 +2228,6 @@ class PartitionOpTranslator { /// them as merges, to ensure any aliases of \p dest are also updated. void translateSILStore(Operand *dest, Operand *src) { SILValue destValue = dest->get(); - SILValue srcValue = src->get(); if (auto nonSendableDest = tryToTrackValue(destValue)) { // In the following situations, we can perform an assign: @@ -2220,10 +2243,10 @@ class PartitionOpTranslator { // per field basis to allow for us to assign. if (nonSendableDest.value().isNoAlias() && !isProjectedFromAggregate(destValue)) - return translateSILAssign(destValue, srcValue); + return translateSILAssign(destValue, src); // Stores to possibly aliased storage must be treated as merges. - return translateSILMerge(destValue, srcValue); + return translateSILMerge(destValue, src); } // Stores to storage of non-Sendable type can be ignored. @@ -2244,10 +2267,12 @@ class PartitionOpTranslator { // miscompiling. For memory like this, we probably need to track it on a // per field basis to allow for us to assign. if (nonSendableTgt.value().isNoAlias() && !isProjectedFromAggregate(dest)) - return translateSILAssign(dest, inst->getElements()); + return translateSILAssign( + dest, makeOperandRefRange(inst->getElementOperands())); // Stores to possibly aliased storage must be treated as merges. - return translateSILMerge(dest, inst->getElements()); + return translateSILMerge(dest, + makeOperandRefRange(inst->getElementOperands())); } // Stores to storage of non-Sendable type can be ignored. @@ -2260,11 +2285,11 @@ class PartitionOpTranslator { /// An enum select is just a multi assign. void translateSILSelectEnum(SelectEnumOperation selectEnumInst) { - SmallVector enumOperands; + SmallVector enumOperands; for (unsigned i = 0; i < selectEnumInst.getNumCases(); i++) - enumOperands.push_back(selectEnumInst.getCase(i).second); + enumOperands.push_back(selectEnumInst.getCaseOperand(i).second); if (selectEnumInst.hasDefault()) - enumOperands.push_back(selectEnumInst.getDefaultResult()); + enumOperands.push_back(selectEnumInst.getDefaultResultOperand()); return translateSILMultiAssign( TinyPtrVector(selectEnumInst->getResult(0)), enumOperands); } @@ -2278,7 +2303,7 @@ class PartitionOpTranslator { if (dest->getNumArguments() > 0) { assert(dest->getNumArguments() == 1 && "expected at most one bb arg in dest of enum switch"); - argSources.addValues({switchEnumInst->getOperand()}, dest); + argSources.addValues({&switchEnumInst->getOperandRef()}, dest); } } @@ -2378,8 +2403,8 @@ class PartitionOpTranslator { return; case TranslationSemantics::Assign: - return translateSILMultiAssign(inst->getResults(), - inst->getOperandValues()); + return translateSILMultiAssign( + inst->getResults(), makeOperandRefRange(inst->getAllOperands())); case TranslationSemantics::Require: for (auto op : inst->getOperandValues()) @@ -2452,7 +2477,10 @@ Element PartitionOpBuilder::getActorIntroducingRepresentative( } bool PartitionOpBuilder::valueHasID(SILValue value, bool dumpIfHasNoID) { - return translator->valueHasID(value, dumpIfHasNoID); + auto v = translator->valueMap.getTrackableValue(value); + if (auto m = v.getRepresentative().maybeGetValue()) + return translator->valueHasID(m, dumpIfHasNoID); + return true; } void PartitionOpBuilder::print(llvm::raw_ostream &os) const { @@ -2922,7 +2950,6 @@ PartitionOpTranslator::visitStoreBorrowInst(StoreBorrowInst *sbi) { // store_borrow itself to be a require use. This prevents the store_borrow // from causing incorrect diagnostics. SILValue destValue = sbi->getDest(); - SILValue srcValue = sbi->getSrc(); auto nonSendableDest = tryToTrackValue(destValue); if (!nonSendableDest) @@ -2941,11 +2968,13 @@ PartitionOpTranslator::visitStoreBorrowInst(StoreBorrowInst *sbi) { // per field basis to allow for us to assign. if (nonSendableDest.value().isNoAlias() && !isProjectedFromAggregate(destValue)) { - translateSILMultiAssign(sbi->getResults(), sbi->getOperandValues(), + translateSILMultiAssign(sbi->getResults(), + makeOperandRefRange(sbi->getAllOperands()), SILIsolationInfo(), false /*require src*/); } else { // Stores to possibly aliased storage must be treated as merges. - translateSILMerge(destValue, srcValue, false /*require src*/); + translateSILMerge(destValue, &sbi->getAllOperands()[StoreBorrowInst::Src], + false /*require src*/); } return TranslationSemantics::Special; @@ -3033,7 +3062,7 @@ TranslationSemantics PartitionOpTranslator::visitPackElementGetInst(PackElementGetInst *r) { if (!isNonSendableType(r->getType())) return TranslationSemantics::Require; - translateSILAssign(SILValue(r), r->getPack()); + translateSILAssign(SILValue(r), r->getPackOperand()); return TranslationSemantics::Special; } @@ -3042,7 +3071,7 @@ TranslationSemantics PartitionOpTranslator::visitTuplePackElementAddrInst( if (!isNonSendableType(r->getType())) { translateSILRequire(r->getTuple()); } else { - translateSILAssign(SILValue(r), r->getTuple()); + translateSILAssign(SILValue(r), r->getTupleOperand()); } return TranslationSemantics::Special; } @@ -3052,7 +3081,7 @@ PartitionOpTranslator::visitTuplePackExtractInst(TuplePackExtractInst *r) { if (!isNonSendableType(r->getType())) { translateSILRequire(r->getTuple()); } else { - translateSILAssign(SILValue(r), r->getTuple()); + translateSILAssign(SILValue(r), r->getTupleOperand()); } return TranslationSemantics::Special; } @@ -3216,7 +3245,8 @@ TranslationSemantics PartitionOpTranslator::visitCheckedCastAddrBranchInst( // differently depending on what the result of checked_cast_addr_br // is. For now just keep the current behavior. It is more conservative, // but still correct. - translateSILMultiAssign(ArrayRef(), ccabi->getOperandValues()); + translateSILMultiAssign(ArrayRef(), + makeOperandRefRange(ccabi->getAllOperands())); return TranslationSemantics::Special; } From bcbf5c515e6db08ba5d9a978bc3ee9272f334206 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Wed, 17 Jul 2024 11:00:51 -0700 Subject: [PATCH 5/6] [region-isolation] Emit an error when we assign a non-disconnected value into a sending result. rdar://127318392 --- include/swift/AST/DiagnosticsSIL.def | 17 ++ .../swift/SILOptimizer/Utils/PartitionUtils.h | 78 +++++- .../SILOptimizer/Utils/VariableNameUtils.h | 7 + .../Mandatory/TransferNonSendable.cpp | 265 +++++++++++++++++- lib/SILOptimizer/Utils/VariableNameUtils.cpp | 2 +- ...nonsendable_global_actor_nonsendable.swift | 52 ++-- .../transfernonsendable_sending_results.swift | 52 +++- 7 files changed, 450 insertions(+), 23 deletions(-) diff --git a/include/swift/AST/DiagnosticsSIL.def b/include/swift/AST/DiagnosticsSIL.def index 6b42275a56215..20a34b7e6b42c 100644 --- a/include/swift/AST/DiagnosticsSIL.def +++ b/include/swift/AST/DiagnosticsSIL.def @@ -972,6 +972,9 @@ ERROR(regionbasedisolation_arg_passed_to_strongly_transferred_param, none, ERROR(regionbasedisolation_named_transfer_yields_race, none, "sending %0 risks causing data races", (Identifier)) +NOTE(regionbasedisolation_type_is_non_sendable, none, + "%0 is a non-Sendable type", + (Type)) ERROR(regionbasedisolation_inout_sending_cannot_be_actor_isolated, none, "'inout sending' parameter %0 cannot be %1at end of function", @@ -980,6 +983,20 @@ NOTE(regionbasedisolation_inout_sending_cannot_be_actor_isolated_note, none, "%1%0 risks causing races in between %1uses and caller uses since caller assumes value is not actor isolated", (Identifier, StringRef)) +ERROR(regionbasedisolation_out_sending_cannot_be_actor_isolated_type, none, + "returning a %1 %0 value as a 'sending' result risks causing data races", + (Type, StringRef)) +NOTE(regionbasedisolation_out_sending_cannot_be_actor_isolated_note_type, none, + "returning a %1 %0 value risks causing races since the caller assumes the value can be safely sent to other isolation domains", + (Type, StringRef)) + +ERROR(regionbasedisolation_out_sending_cannot_be_actor_isolated_named, none, + "returning %1 %0 as a 'sending' result risks causing data races", + (Identifier, StringRef)) +NOTE(regionbasedisolation_out_sending_cannot_be_actor_isolated_note_named, none, + "returning %1 %0 risks causing data races since the caller assumes that %0 can be safely sent to other isolation domains", + (Identifier, StringRef)) + NOTE(regionbasedisolation_named_info_transfer_yields_race, none, "sending %1%0 to %2 callee risks causing data races between %2 and local %3 uses", (Identifier, StringRef, ActorIsolation, ActorIsolation)) diff --git a/include/swift/SILOptimizer/Utils/PartitionUtils.h b/include/swift/SILOptimizer/Utils/PartitionUtils.h index faf7c6fdd86fc..600ab60e74def 100644 --- a/include/swift/SILOptimizer/Utils/PartitionUtils.h +++ b/include/swift/SILOptimizer/Utils/PartitionUtils.h @@ -996,6 +996,16 @@ struct PartitionOpEvaluator { isolationRegionInfo); } + /// Just call our CRTP subclass. + void handleAssignTransferNonTransferrableIntoSendingResult( + const PartitionOp &op, Element destElement, + SILFunctionArgument *destValue, Element srcElement, SILValue srcValue, + SILDynamicMergedIsolationInfo srcIsolationRegionInfo) const { + return asImpl().handleAssignTransferNonTransferrableIntoSendingResult( + op, destElement, destValue, srcElement, srcValue, + srcIsolationRegionInfo); + } + /// Call our CRTP subclass. void handleInOutSendingNotInitializedAtExitError( const PartitionOp &op, Element elt, Operand *transferringOp) const { @@ -1112,13 +1122,41 @@ struct PartitionOpEvaluator { p.pushHistorySequenceBoundary(loc); switch (op.getKind()) { - case PartitionOpKind::Assign: + case PartitionOpKind::Assign: { assert(op.getOpArgs().size() == 2 && "Assign PartitionOp should be passed 2 arguments"); assert(p.isTrackingElement(op.getOpArgs()[1]) && "Assign PartitionOp's source argument should be already tracked"); + + // See if we are assigning an a non-disconnected value into a 'out + // sending' parameter. In such a case, we emit a diagnostic. + if (op.getSourceInst() + ->getFunction() + ->getLoweredFunctionType() + ->hasSendingResult()) { + if (auto instance = getRepresentativeValue(op.getOpArgs()[0])) { + if (auto value = instance.maybeGetValue()) { + if (auto *fArg = dyn_cast(value)) { + if (fArg->getArgumentConvention().isIndirectOutParameter()) { + Region srcRegion = p.getRegion(op.getOpArgs()[1]); + auto dynamicRegionIsolation = getIsolationRegionInfo(srcRegion); + // We can unconditionally getValue here since we can never + // assign an actor introducing inst. + auto rep = getRepresentativeValue(op.getOpArgs()[1]).getValue(); + if (!dynamicRegionIsolation.isDisconnected()) { + handleAssignTransferNonTransferrableIntoSendingResult( + op, op.getOpArgs()[0], fArg, op.getOpArgs()[1], rep, + dynamicRegionIsolation); + } + } + } + } + } + } + p.assignElement(op.getOpArgs()[0], op.getOpArgs()[1]); return; + } case PartitionOpKind::AssignFresh: assert(op.getOpArgs().size() == 1 && "AssignFresh PartitionOp should be passed 1 argument"); @@ -1199,15 +1237,42 @@ struct PartitionOpEvaluator { p.undoTransfer(op.getOpArgs()[0]); return; } - case PartitionOpKind::Merge: + case PartitionOpKind::Merge: { assert(op.getOpArgs().size() == 2 && "Merge PartitionOp should be passed 2 arguments"); assert(p.isTrackingElement(op.getOpArgs()[0]) && p.isTrackingElement(op.getOpArgs()[1]) && "Merge PartitionOp's arguments should already be tracked"); + // See if we are assigning an a non-disconnected value into a 'out + // sending' parameter. In such a case, we emit a diagnostic. + if (op.getSourceInst() + ->getFunction() + ->getLoweredFunctionType() + ->hasSendingResult()) { + if (auto instance = getRepresentativeValue(op.getOpArgs()[0])) { + if (auto value = instance.maybeGetValue()) { + if (auto *fArg = dyn_cast(value)) { + if (fArg->getArgumentConvention().isIndirectOutParameter()) { + Region srcRegion = p.getRegion(op.getOpArgs()[1]); + auto dynamicRegionIsolation = getIsolationRegionInfo(srcRegion); + // We can unconditionally getValue here since we can never + // assign an actor introducing inst. + auto rep = getRepresentativeValue(op.getOpArgs()[1]).getValue(); + if (!dynamicRegionIsolation.isDisconnected()) { + handleAssignTransferNonTransferrableIntoSendingResult( + op, op.getOpArgs()[0], fArg, op.getOpArgs()[1], rep, + dynamicRegionIsolation); + } + } + } + } + } + } + p.merge(op.getOpArgs()[0], op.getOpArgs()[1]); return; + } case PartitionOpKind::Require: assert(op.getOpArgs().size() == 1 && "Require PartitionOp should be passed 1 argument"); @@ -1431,10 +1496,19 @@ struct PartitionOpEvaluatorBaseImpl : PartitionOpEvaluator { const PartitionOp &op, Element elt, SILDynamicMergedIsolationInfo regionInfo) const {} + /// Please see documentation on the CRTP version of this call for information + /// about this entrypoint. void handleTransferNonTransferrable( const PartitionOp &op, Element elt, Element otherElement, SILDynamicMergedIsolationInfo isolationRegionInfo) const {} + /// Please see documentation on the CRTP version of this call for information + /// about this entrypoint. + void handleAssignTransferNonTransferrableIntoSendingResult( + const PartitionOp &partitionOp, Element destElement, + SILFunctionArgument *destValue, Element srcElement, SILValue srcValue, + SILDynamicMergedIsolationInfo srcIsolationRegionInfo) const {} + /// Used to signify an "unknown code pattern" has occured while performing /// dataflow. /// diff --git a/include/swift/SILOptimizer/Utils/VariableNameUtils.h b/include/swift/SILOptimizer/Utils/VariableNameUtils.h index 265b4dc3ca3bf..f88d9fc4a9c2d 100644 --- a/include/swift/SILOptimizer/Utils/VariableNameUtils.h +++ b/include/swift/SILOptimizer/Utils/VariableNameUtils.h @@ -201,6 +201,13 @@ class VariableNameInferrer { static std::optional> inferNameAndRoot(SILValue value); + /// Given a specific decl \p d, come up with a name for it. + /// + /// This is used internally for translating all decls to names. This is + /// exposed in case someone wants to wrap VariableNameUtils and needs to use + /// the same internal mapping that VariableNameUtils uses. + static StringRef getNameFromDecl(Decl *d); + private: void drainVariableNamePath(); void popSingleVariableName(); diff --git a/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp b/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp index 8f6e8db691dc2..caef5d7bc15d6 100644 --- a/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp +++ b/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp @@ -475,6 +475,33 @@ struct TransferredNonTransferrableInfo { isolationRegionInfo(isolationRegionInfo) {} }; +struct AssignIsolatedIntoOutSendingParameterInfo { + /// The user that actually caused the transfer. + Operand *srcOperand; + + /// The specific out sending result. + SILFunctionArgument *outSendingResult; + + /// The non-transferrable value that is in the same region as \p + /// outSendingResult. + SILValue nonTransferrableValue; + + /// The region info that describes the dynamic dataflow derived isolation + /// region info for the non-transferrable value. + /// + /// This is equal to the merge of the IsolationRegionInfo from all elements in + /// nonTransferrable's region when the error was diagnosed. + SILDynamicMergedIsolationInfo isolatedValueIsolationRegionInfo; + + AssignIsolatedIntoOutSendingParameterInfo( + Operand *transferringOperand, SILFunctionArgument *outSendingResult, + SILValue nonTransferrableValue, + SILDynamicMergedIsolationInfo isolationRegionInfo) + : srcOperand(transferringOperand), outSendingResult(outSendingResult), + nonTransferrableValue(nonTransferrableValue), + isolatedValueIsolationRegionInfo(isolationRegionInfo) {} +}; + /// Wrapper around a SILInstruction that internally specifies whether we are /// dealing with an inout reinitialization needed or if it is just a normal /// use after transfer. @@ -514,6 +541,8 @@ class TransferNonSendableImpl { transferredNonTransferrableInfoList; SmallVector inoutSendingNotDisconnectedInfoList; + SmallVector + assignIsolatedIntoOutSendingParameterInfoList; public: TransferNonSendableImpl(RegionAnalysisFunctionInfo *regionInfo) @@ -527,6 +556,7 @@ class TransferNonSendableImpl { void emitUseAfterTransferDiagnostics(); void emitTransferredNonTransferrableDiagnostics(); void emitInOutSendingNotDisconnectedInfoList(); + void emitAssignIsolatedIntoSendingResultDiagnostics(); }; } // namespace @@ -1819,6 +1849,208 @@ void TransferNonSendableImpl::emitInOutSendingNotDisconnectedInfoList() { } } +//===----------------------------------------------------------------------===// +// MARK: AssignTransferNonTransferrableIntoSendingResult +//===----------------------------------------------------------------------===// + +namespace { + +class AssignIsolatedIntoSendingResultDiagnosticEmitter { + AssignIsolatedIntoOutSendingParameterInfo info; + bool emittedErrorDiagnostic = false; + +public: + AssignIsolatedIntoSendingResultDiagnosticEmitter( + AssignIsolatedIntoOutSendingParameterInfo info) + : info(info) {} + + ~AssignIsolatedIntoSendingResultDiagnosticEmitter() { + // If we were supposed to emit a diagnostic and didn't emit an unknown + // pattern error. + if (!emittedErrorDiagnostic) + emitUnknownPatternError(); + } + + std::optional getBehaviorLimit() const { + return getDiagnosticBehaviorLimitForValue(info.outSendingResult); + } + + void emitUnknownPatternError() { + if (shouldAbortOnUnknownPatternMatchError()) { + llvm::report_fatal_error( + "RegionIsolation: Aborting on unknown pattern match error"); + } + + diagnoseError(info.srcOperand->getUser(), + diag::regionbasedisolation_unknown_pattern) + .limitBehaviorIf(getBehaviorLimit()); + } + + void emit(); + + ASTContext &getASTContext() const { + return info.srcOperand->getFunction()->getASTContext(); + } + + template + InFlightDiagnostic diagnoseError(SourceLoc loc, Diag diag, + U &&...args) { + emittedErrorDiagnostic = true; + return std::move(getASTContext() + .Diags.diagnose(loc, diag, std::forward(args)...) + .warnUntilSwiftVersion(6)); + } + + template + InFlightDiagnostic diagnoseError(SILLocation loc, Diag diag, + U &&...args) { + return diagnoseError(loc.getSourceLoc(), diag, std::forward(args)...); + } + + template + InFlightDiagnostic diagnoseError(SILInstruction *inst, Diag diag, + U &&...args) { + return diagnoseError(inst->getLoc(), diag, std::forward(args)...); + } + + template + InFlightDiagnostic diagnoseError(Operand *op, Diag diag, U &&...args) { + return diagnoseError(op->getUser()->getLoc(), diag, + std::forward(args)...); + } + + template + InFlightDiagnostic diagnoseNote(SourceLoc loc, Diag diag, U &&...args) { + return getASTContext().Diags.diagnose(loc, diag, std::forward(args)...); + } + + template + InFlightDiagnostic diagnoseNote(SILLocation loc, Diag diag, + U &&...args) { + return diagnoseNote(loc.getSourceLoc(), diag, std::forward(args)...); + } + + template + InFlightDiagnostic diagnoseNote(SILInstruction *inst, Diag diag, + U &&...args) { + return diagnoseNote(inst->getLoc(), diag, std::forward(args)...); + } + + template + InFlightDiagnostic diagnoseNote(Operand *op, Diag diag, U &&...args) { + return diagnoseNote(op->getUser()->getLoc(), diag, + std::forward(args)...); + } +}; + +} // namespace + +/// Look through values looking for our out parameter. We want to tightly +/// control this to be conservative... so we handroll this. +static SILValue findOutParameter(SILValue v) { + while (true) { + SILValue temp = v; + if (auto *initOpt = dyn_cast(temp)) { + if (initOpt->getElement()->getParentEnum() == + initOpt->getFunction()->getASTContext().getOptionalDecl()) { + temp = initOpt->getOperand(); + } + } + + if (temp == v) { + return v; + } + + v = temp; + } +} + +void AssignIsolatedIntoSendingResultDiagnosticEmitter::emit() { + // Then emit the note with greater context. + SmallString<64> descriptiveKindStr; + { + llvm::raw_svector_ostream os(descriptiveKindStr); + info.isolatedValueIsolationRegionInfo.printForDiagnostics(os); + } + + // Grab the var name if we can find it. + if (auto varName = VariableNameInferrer::inferName(info.srcOperand->get())) { + // In general, when we do an assignment like this, we assume that srcOperand + // and our outSendingResult have the same type. This doesn't always happen + // though especially if our outSendingResult is used as an out parameter of + // a class_method. Check for such a case and if so, add to the end of our + // string a path component for that class_method. + if (info.srcOperand->get()->getType() != info.outSendingResult->getType()) { + if (auto fas = FullApplySite::isa(info.srcOperand->getUser())) { + if (fas.getSelfArgument() == info.srcOperand->get() && + fas.getNumIndirectSILResults() == 1) { + // First check if our function argument is exactly our out parameter. + bool canEmit = + info.outSendingResult == fas.getIndirectSILResults()[0]; + + // If that fails, see if we are storing into a temporary + // alloc_stack. In such a case, find the root value that the temporary + // is initialized to and see if that is our target function + // argument. In such a case, we also want to add the decl name to our + // type. + if (!canEmit) { + canEmit = info.outSendingResult == + findOutParameter(fas.getIndirectSILResults()[0]); + } + + if (canEmit) { + if (auto *callee = + dyn_cast_or_null(fas.getCalleeOrigin())) { + SmallString<64> resultingString; + resultingString.append(varName->str()); + resultingString += '.'; + resultingString += VariableNameInferrer::getNameFromDecl( + callee->getMember().getDecl()); + varName = fas->getFunction()->getASTContext().getIdentifier( + resultingString); + } + } + } + } + } + + diagnoseError( + info.srcOperand, + diag::regionbasedisolation_out_sending_cannot_be_actor_isolated_named, + *varName, descriptiveKindStr) + .limitBehaviorIf(getBehaviorLimit()); + + diagnoseNote( + info.srcOperand, + diag:: + regionbasedisolation_out_sending_cannot_be_actor_isolated_note_named, + *varName, descriptiveKindStr); + return; + } + + Type type = info.nonTransferrableValue->getType().getASTType(); + + diagnoseError( + info.srcOperand, + diag::regionbasedisolation_out_sending_cannot_be_actor_isolated_type, + type, descriptiveKindStr) + .limitBehaviorIf(getBehaviorLimit()); + + diagnoseNote( + info.srcOperand, + diag::regionbasedisolation_out_sending_cannot_be_actor_isolated_note_type, + type, descriptiveKindStr); + diagnoseNote(info.srcOperand, diag::regionbasedisolation_type_is_non_sendable, + type); +} + +void TransferNonSendableImpl::emitAssignIsolatedIntoSendingResultDiagnostics() { + for (auto &info : assignIsolatedIntoOutSendingParameterInfoList) { + AssignIsolatedIntoSendingResultDiagnosticEmitter emitter(info); + emitter.emit(); + } +} + //===----------------------------------------------------------------------===// // MARK: Diagnostic Evaluator //===----------------------------------------------------------------------===// @@ -1842,6 +2074,12 @@ struct DiagnosticEvaluator final SmallVectorImpl &inoutSendingNotDisconnectedInfoList; + /// A list of state that tracks specific 'inout sending' parameters that were + /// actor isolated on function exit with the necessary state to emit the + /// error. + SmallVectorImpl + &assignIsolatedIntoOutSendingParameterInfoList; + DiagnosticEvaluator(Partition &workingPartition, RegionAnalysisFunctionInfo *info, SmallFrozenMultiMap @@ -1850,6 +2088,8 @@ struct DiagnosticEvaluator final &transferredNonTransferrable, SmallVectorImpl &inoutSendingNotDisconnectedInfoList, + SmallVectorImpl + &assignIsolatedIntoOutSendingParameterInfo, TransferringOperandToStateMap &operandToStateMap) : PartitionOpEvaluatorBaseImpl( workingPartition, info->getOperandSetFactory(), operandToStateMap), @@ -1857,7 +2097,9 @@ struct DiagnosticEvaluator final transferOpToRequireInstMultiMap(transferOpToRequireInstMultiMap), transferredNonTransferrable(transferredNonTransferrable), inoutSendingNotDisconnectedInfoList( - inoutSendingNotDisconnectedInfoList) {} + inoutSendingNotDisconnectedInfoList), + assignIsolatedIntoOutSendingParameterInfoList( + assignIsolatedIntoOutSendingParameterInfo) {} void handleLocalUseAfterTransfer(const PartitionOp &partitionOp, Element transferredVal, @@ -1975,6 +2217,25 @@ struct DiagnosticEvaluator final } } + void handleAssignTransferNonTransferrableIntoSendingResult( + const PartitionOp &partitionOp, Element destElement, + SILFunctionArgument *destValue, Element srcElement, SILValue srcValue, + SILDynamicMergedIsolationInfo srcIsolationRegionInfo) const { + auto srcRep = info->getValueMap().getRepresentativeValue(srcElement); + LLVM_DEBUG( + llvm::dbgs() + << " Emitting Error! Kind: Assign Isolated Into Sending Result!\n" + << " Assign Inst: " << *partitionOp.getSourceInst() + << " Dest Value: " << *destValue + << " Dest Element: " << destElement << '\n' + << " Src Value: " << srcValue + << " Src Element: " << srcElement << '\n' + << " Src Rep: " << srcRep + << " Src Isolation: " << srcIsolationRegionInfo << '\n'); + assignIsolatedIntoOutSendingParameterInfoList.emplace_back( + partitionOp.getSourceOp(), destValue, srcValue, srcIsolationRegionInfo); + } + void handleInOutSendingNotInitializedAtExitError(const PartitionOp &partitionOp, Element inoutSendingVal, @@ -2065,6 +2326,7 @@ void TransferNonSendableImpl::runDiagnosticEvaluator() { transferOpToRequireInstMultiMap, transferredNonTransferrableInfoList, inoutSendingNotDisconnectedInfoList, + assignIsolatedIntoOutSendingParameterInfoList, regionInfo->getTransferringOpToStateMap()); // And then evaluate all of our partition ops on the entry partition. @@ -2098,6 +2360,7 @@ void TransferNonSendableImpl::emitDiagnostics() { emitTransferredNonTransferrableDiagnostics(); emitUseAfterTransferDiagnostics(); emitInOutSendingNotDisconnectedInfoList(); + emitAssignIsolatedIntoSendingResultDiagnostics(); } namespace { diff --git a/lib/SILOptimizer/Utils/VariableNameUtils.cpp b/lib/SILOptimizer/Utils/VariableNameUtils.cpp index 9a4c3575ebd3f..e2a7e9518334b 100644 --- a/lib/SILOptimizer/Utils/VariableNameUtils.cpp +++ b/lib/SILOptimizer/Utils/VariableNameUtils.cpp @@ -639,7 +639,7 @@ SILValue VariableNameInferrer::findDebugInfoProvidingValueHelper( } } -static StringRef getNameFromDecl(Decl *d) { +StringRef VariableNameInferrer::getNameFromDecl(Decl *d) { if (d) { if (auto accessor = dyn_cast(d)) { return accessor->getStorage()->getBaseName().userFacingName(); diff --git a/test/Concurrency/transfernonsendable_global_actor_nonsendable.swift b/test/Concurrency/transfernonsendable_global_actor_nonsendable.swift index cf56c25c9f043..3805b1dcba155 100644 --- a/test/Concurrency/transfernonsendable_global_actor_nonsendable.swift +++ b/test/Concurrency/transfernonsendable_global_actor_nonsendable.swift @@ -108,18 +108,18 @@ extension NonSendableGlobalActorIsolatedStruct { self.p } - // TODO: Should error here. mutating func test5() -> sending (any GlobalActorIsolatedProtocol)? { - self.p + self.p // expected-error {{returning main actor-isolated 'self.p' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p' risks causing data races since the caller assumes that 'self.p' can be safely sent to other isolation domains}} } mutating func test6() -> (any OtherProtocol)? { self.p2 } - // TODO: Should error here. mutating func test7() -> sending (any OtherProtocol)? { - self.p2 + self.p2 // expected-error {{returning main actor-isolated 'self.p2' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p2' risks causing data races since the caller assumes that 'self.p2' can be safely sent to other isolation domains}} } } @@ -153,12 +153,20 @@ extension NonSendableGlobalActorIsolatedEnum { return x } - // TODO: This should error. mutating func test3() -> sending (any GlobalActorIsolatedProtocol)? { guard case let .fourth(x) = self else { return nil } - return x + return x // expected-error {{returning main actor-isolated 'x' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'x' risks causing data races since the caller assumes that 'x' can be safely sent to other isolation domains}} + } + + mutating func test3a() -> sending (any GlobalActorIsolatedProtocol)? { + if case let .fourth(x) = self { + return x // expected-error {{returning main actor-isolated 'x' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'x' risks causing data races since the caller assumes that 'x' can be safely sent to other isolation domains}} + } + return nil } mutating func test3() -> sending NonSendableKlass? { @@ -168,6 +176,14 @@ extension NonSendableGlobalActorIsolatedEnum { return x } // expected-error {{sending 'x.some' risks causing data races}} // expected-note @-1 {{main actor-isolated 'x.some' cannot be a 'sending' result. main actor-isolated uses may race with caller uses}} + + mutating func test3a() -> sending NonSendableKlass? { + if case let .second(x) = self { + return x + } + return nil + } // expected-error {{sending 'x.some' risks causing data races}} + // expected-note @-1 {{main actor-isolated 'x.some' cannot be a 'sending' result. main actor-isolated uses may race with caller uses}} } extension NonSendableGlobalActorIsolatedKlass { @@ -188,18 +204,18 @@ extension NonSendableGlobalActorIsolatedKlass { self.p } - // TODO: Should error here. func test5() -> sending (any GlobalActorIsolatedProtocol)? { - self.p + self.p // expected-error {{returning main actor-isolated 'self.p' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p' risks causing data races since the caller assumes that 'self.p' can be safely sent to other isolation domains}} } func test6() -> (any OtherProtocol)? { self.p2 } - // TODO: Should error here. func test7() -> sending (any OtherProtocol)? { - self.p2 + self.p2 // expected-error {{returning main actor-isolated 'self.p2' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p2' risks causing data races since the caller assumes that 'self.p2' can be safely sent to other isolation domains}} } } @@ -221,18 +237,18 @@ extension FinalNonSendableGlobalActorIsolatedKlass { self.p } - // TODO: Should error here. func test5() -> sending (any GlobalActorIsolatedProtocol)? { - self.p + self.p // expected-error {{returning main actor-isolated 'self.p' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p' risks causing data races since the caller assumes that 'self.p' can be safely sent to other isolation domains}} } func test6() -> (any OtherProtocol)? { self.p2 } - // TODO: Should error here. func test7() -> sending (any OtherProtocol)? { - self.p2 + self.p2 // expected-error {{returning main actor-isolated 'self.p2' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p2' risks causing data races since the caller assumes that 'self.p2' can be safely sent to other isolation domains}} } } @@ -254,17 +270,17 @@ extension GlobalActorIsolatedProtocol { self.p } - // TODO: Should error here. mutating func test5() -> sending (any GlobalActorIsolatedProtocol)? { - self.p + self.p // expected-error {{returning main actor-isolated 'self.p' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p' risks causing data races since the caller assumes that 'self.p' can be safely sent to other isolation domains}} } mutating func test6() -> (any OtherProtocol)? { self.p2 } - // TODO: Should error here. mutating func test7() -> sending (any OtherProtocol)? { - self.p2 + self.p2 // expected-error {{returning main actor-isolated 'self.p2' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning main actor-isolated 'self.p2' risks causing data races since the caller assumes that 'self.p2' can be safely sent to other isolation domains}} } } diff --git a/test/Concurrency/transfernonsendable_sending_results.swift b/test/Concurrency/transfernonsendable_sending_results.swift index b0a3e5a9f242f..f7f7b689d02ad 100644 --- a/test/Concurrency/transfernonsendable_sending_results.swift +++ b/test/Concurrency/transfernonsendable_sending_results.swift @@ -70,6 +70,26 @@ enum MainActorIsolatedEnum { case second(NonSendableKlass) } +struct GenericNonSendableStruct { + var t: T + var t2: T? + var x: NonSendableKlass +} + +class GenericNonSendableKlass { + var t: T + var t2: T? + var x: NonSendableKlass? + + init(_ inputT: T) { + t = inputT + t2 = nil + x = NonSendableKlass() + } +} + +func sendParameter(_ t: sending T) {} + ///////////////// // MARK: Tests // ///////////////// @@ -177,7 +197,7 @@ extension MainActorIsolatedEnum { } return nil } // expected-warning {{sending 'ns.some' risks causing data races}} - // expected-note @-1 {{main actor-isolated 'ns.some' cannot be a 'sending' result. main actor-isolated uses may race with caller uses}} + // expected-note @-1 {{main actor-isolated 'ns.some' cannot be a 'sending' result. main actor-isolated uses may race with caller uses}} func testIfLetReturnNoTransfer() -> NonSendableKlass? { if case .second(let ns) = self { @@ -245,3 +265,33 @@ func asyncLetReabstractionThunkTest2() async { async let newValue2: NonSendableKlass = await getMainActorValueAsyncTransferring() let _ = await newValue2 } + +/////////////////////////////////// +// MARK: Indirect Sending Result // +/////////////////////////////////// + +func indirectSending(_ t: T) -> sending T { + return t // expected-warning {{returning task-isolated 't' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning task-isolated 't' risks causing data races since the caller assumes that 't' can be safely sent to other isolation domains}} +} + +func indirectSendingStructField(_ t: GenericNonSendableStruct) -> sending T { + return t.t // expected-warning {{returning task-isolated 't.t' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning task-isolated 't.t' risks causing data races since the caller assumes that 't.t' can be safely sent to other isolation domains}} +} + +func indirectSendingStructOptionalField(_ t: GenericNonSendableStruct) -> sending T { + return t.t2! // expected-warning {{returning task-isolated 't.t2.some' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning task-isolated 't.t2.some' risks causing data races since the caller assumes that 't.t2.some' can be safely sent to other isolation domains}} +} + +func indirectSendingClassField(_ t: GenericNonSendableKlass) -> sending T { + return t.t // expected-warning {{returning task-isolated 't.t' as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning task-isolated 't.t' risks causing data races since the caller assumes that 't.t' can be safely sent to other isolation domains}} +} + +func indirectSendingOptionalClassField(_ t: GenericNonSendableKlass) -> sending T { + return t.t2! // expected-warning {{returning a task-isolated 'Optional' value as a 'sending' result risks causing data races}} + // expected-note @-1 {{returning a task-isolated 'Optional' value risks causing races since the caller assumes the value can be safely sent to other isolation domains}} + // expected-note @-2 {{'Optional' is a non-Sendable type}} +} From 10862642ca91dee8e767885f024d5d9d9c6fcd04 Mon Sep 17 00:00:00 2001 From: Michael Gottesman Date: Thu, 18 Jul 2024 22:33:57 -0700 Subject: [PATCH 6/6] [region-isolation] Stub out PartitionOpEvaluator::doesFunctionHaveSendingResult() so that the unittests can override it. The unittests for PartitionUtils pass in mocked operands and instructions that cannot be dereferenced. Adding this static CRTP helper allows for the unittest PartitionOpEvaluator subclass to just return false for it instead of dereferencing operands or instructions. The rest of the evaluators just get to use the default "normal" implementation that actually accesses program state. --- .../swift/SILOptimizer/Utils/PartitionUtils.h | 24 ++++++++++++------- unittests/SILOptimizer/PartitionUtilsTest.cpp | 4 ++++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/include/swift/SILOptimizer/Utils/PartitionUtils.h b/include/swift/SILOptimizer/Utils/PartitionUtils.h index 600ab60e74def..9b859ef34c372 100644 --- a/include/swift/SILOptimizer/Utils/PartitionUtils.h +++ b/include/swift/SILOptimizer/Utils/PartitionUtils.h @@ -1087,6 +1087,14 @@ struct PartitionOpEvaluator { return Impl::getIsolationInfo(partitionOp); } + /// Some evaluators do not support accessing fields on their SILInstruction + /// since they just pass in "mocked" SILInstruction. We allow for them to just + /// return false for this case to prevent dereference issues. + static bool + doesParentFunctionHaveSendingResult(const PartitionOp &partitionOp) { + return Impl::doesFunctionHaveSendingResult(partitionOp); + } + std::optional getElement(SILValue value) const { return asImpl().getElement(value); } @@ -1130,10 +1138,7 @@ struct PartitionOpEvaluator { // See if we are assigning an a non-disconnected value into a 'out // sending' parameter. In such a case, we emit a diagnostic. - if (op.getSourceInst() - ->getFunction() - ->getLoweredFunctionType() - ->hasSendingResult()) { + if (doesParentFunctionHaveSendingResult(op)) { if (auto instance = getRepresentativeValue(op.getOpArgs()[0])) { if (auto value = instance.maybeGetValue()) { if (auto *fArg = dyn_cast(value)) { @@ -1246,10 +1251,7 @@ struct PartitionOpEvaluator { // See if we are assigning an a non-disconnected value into a 'out // sending' parameter. In such a case, we emit a diagnostic. - if (op.getSourceInst() - ->getFunction() - ->getLoweredFunctionType() - ->hasSendingResult()) { + if (doesParentFunctionHaveSendingResult(op)) { if (auto instance = getRepresentativeValue(op.getOpArgs()[0])) { if (auto value = instance.maybeGetValue()) { if (auto *fArg = dyn_cast(value)) { @@ -1571,6 +1573,12 @@ struct PartitionOpEvaluatorBaseImpl : PartitionOpEvaluator { static SILIsolationInfo getIsolationInfo(const PartitionOp &partitionOp) { return SILIsolationInfo::get(partitionOp.getSourceInst()); } + static bool doesFunctionHaveSendingResult(const PartitionOp &partitionOp) { + return partitionOp.getSourceInst() + ->getFunction() + ->getLoweredFunctionType() + ->hasSendingResult(); + } }; /// A subclass of PartitionOpEvaluatorBaseImpl that doesn't have any special diff --git a/unittests/SILOptimizer/PartitionUtilsTest.cpp b/unittests/SILOptimizer/PartitionUtilsTest.cpp index a228b98578737..8e712b3328163 100644 --- a/unittests/SILOptimizer/PartitionUtilsTest.cpp +++ b/unittests/SILOptimizer/PartitionUtilsTest.cpp @@ -61,6 +61,10 @@ struct MockedPartitionOpEvaluator final static SILIsolationInfo getIsolationInfo(const PartitionOp &partitionOp) { return {}; } + + static bool doesFunctionHaveSendingResult(const PartitionOp &partitionOp) { + return false; + } }; } // namespace