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/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/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..9b859ef34c372 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. /// @@ -463,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, @@ -488,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, @@ -940,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 { @@ -1021,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); } @@ -1029,6 +1103,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()) { @@ -1052,13 +1130,38 @@ 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 (doesParentFunctionHaveSendingResult(op)) { + 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"); @@ -1139,15 +1242,39 @@ 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 (doesParentFunctionHaveSendingResult(op)) { + 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"); @@ -1371,10 +1498,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. /// @@ -1416,6 +1552,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. /// @@ -1433,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 @@ -1448,4 +1594,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/include/swift/SILOptimizer/Utils/SILIsolationInfo.h b/include/swift/SILOptimizer/Utils/SILIsolationInfo.h index 06122d4db1ac2..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; } } @@ -493,4 +513,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 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/Analysis/RegionAnalysis.cpp b/lib/SILOptimizer/Analysis/RegionAnalysis.cpp index 5e936e4cd88ce..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; } @@ -3267,6 +3297,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 +3539,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..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, @@ -2030,6 +2291,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) @@ -2061,6 +2326,7 @@ void TransferNonSendableImpl::runDiagnosticEvaluator() { transferOpToRequireInstMultiMap, transferredNonTransferrableInfoList, inoutSendingNotDisconnectedInfoList, + assignIsolatedIntoOutSendingParameterInfoList, regionInfo->getTransferringOpToStateMap()); // And then evaluate all of our partition ops on the entry partition. @@ -2094,6 +2360,7 @@ void TransferNonSendableImpl::emitDiagnostics() { emitTransferredNonTransferrableDiagnostics(); emitUseAfterTransferDiagnostics(); emitInOutSendingNotDisconnectedInfoList(); + emitAssignIsolatedIntoSendingResultDiagnostics(); } namespace { 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/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/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() } + } +} 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}} +} 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