diff --git a/include/swift/AST/DiagnosticsSIL.def b/include/swift/AST/DiagnosticsSIL.def index b2c227882b9a7..3961bf7b8eb07 100644 --- a/include/swift/AST/DiagnosticsSIL.def +++ b/include/swift/AST/DiagnosticsSIL.def @@ -984,6 +984,9 @@ NOTE(regionbasedisolation_isolated_since_in_same_region_string, 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", @@ -992,6 +995,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 d1997056d8a73..046b2fdc5d8a6 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 d136603c289f4..db21ac4c637f5 100644 --- a/include/swift/SIL/SILInstruction.h +++ b/include/swift/SIL/SILInstruction.h @@ -7018,6 +7018,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) { @@ -7030,6 +7036,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; } @@ -7039,6 +7057,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 797c92b74ca9e..1ae400441b3d0 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,21 +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"); - // If we are using a region that was transferred as our assignment source - // value... emit an error. - if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[1])) { - for (auto transferredOperand : transferredOperandSet->data()) { - handleLocalUseAfterTransferHelper(op, op.getOpArgs()[1], - transferredOperand); + + // 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"); @@ -1147,29 +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"); - // if attempting to merge a transferred region, handle the failure - if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[0])) { - for (auto transferredOperand : transferredOperandSet->data()) { - handleLocalUseAfterTransferHelper(op, op.getOpArgs()[0], - transferredOperand); - } - } - if (auto *transferredOperandSet = p.getTransferred(op.getOpArgs()[1])) { - for (auto transferredOperand : transferredOperandSet->data()) { - handleLocalUseAfterTransferHelper(op, op.getOpArgs()[1], - transferredOperand); + // 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"); @@ -1270,13 +1375,6 @@ struct PartitionOpEvaluator { void handleLocalUseAfterTransferHelper(const PartitionOp &op, Element elt, Operand *transferringOp) const { if (shouldTryToSquelchErrors()) { - if (auto isolationInfo = getIsolationInfo(op)) { - if (isolationInfo.isActorIsolated() && - isolationInfo.hasSameIsolation( - SILIsolationInfo::get(transferringOp->getUser()))) - return; - } - if (SILValue equivalenceClassRep = getRepresentative(transferringOp->get())) { @@ -1393,10 +1491,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. /// @@ -1438,6 +1545,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. /// @@ -1455,6 +1566,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 @@ -1470,4 +1587,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..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 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 3e3c18b99cde4..16220c17a3511 100644 --- a/lib/SILOptimizer/Analysis/RegionAnalysis.cpp +++ b/lib/SILOptimizer/Analysis/RegionAnalysis.cpp @@ -65,6 +65,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()) @@ -417,9 +432,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()); @@ -460,19 +475,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()); } }; @@ -1144,20 +1162,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) { @@ -1177,30 +1196,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) { @@ -1606,8 +1626,9 @@ class PartitionOpTranslator { void translateSILMultiAssign(const TargetRange &resultValues, const SourceRange &sourceValues, - SILIsolationInfo resultIsolationInfoOverride = {}) { - SmallVector assignOperands; + SILIsolationInfo resultIsolationInfoOverride = {}, + bool requireSrcValues = true) { + SmallVector, 8> assignOperands; SmallVector assignResults; // A helper we use to emit an unknown patten error if our merge is @@ -1621,9 +1642,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) @@ -1672,13 +1695,21 @@ class PartitionOpTranslator { } } - // Require all srcs. - for (auto src : assignOperands) - builder.addRequire(src); + // Require all srcs if we are supposed to. (By default we do). + // + // DISCUSSION: The reason that this may be useful is for special + // instructions like store_borrow. On the one hand, we want store_borrow to + // act like a store in the sense that we want to combine the regions of its + // src and dest... but at the same time, we do not want to treat the store + // itself as a use of its parent value. We want that to be any subsequent + // uses of the store_borrow. + if (requireSrcValues) + for (auto src : assignOperands) + 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. @@ -1693,7 +1724,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); } @@ -1708,7 +1740,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. @@ -1716,7 +1748,7 @@ class PartitionOpTranslator { SILValue next = assignResultsRef.front(); assignResultsRef = assignResultsRef.drop_front(); - builder.addAssign(next, front); + builder.addAssign(next, assignOperands.front().first); } } @@ -1888,7 +1920,8 @@ class PartitionOpTranslator { SmallVector applyResults; getApplyResults(pai, applyResults); - translateSILMultiAssign(applyResults, pai->getOperandValues()); + translateSILMultiAssign(applyResults, + makeOperandRefRange(pai->getAllOperands())); } void translateCreateAsyncTask(BuiltinInst *bi) { @@ -1912,13 +1945,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. @@ -1935,7 +1969,7 @@ class PartitionOpTranslator { builder.addTransfer(value->getRepresentative().getValue(), &op); } } else { - nonTransferringParameters.push_back(op.get()); + nonTransferringParameters.push_back(&op); } } } @@ -1953,7 +1987,7 @@ class PartitionOpTranslator { &selfOperand); } } else { - nonTransferringParameters.push_back(selfOperand.get()); + nonTransferringParameters.push_back(&selfOperand); } } @@ -2105,13 +2139,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, @@ -2121,7 +2155,7 @@ class PartitionOpTranslator { /// isolationInfo is set. void translateSILAssignFresh(SILValue val) { return translateSILMultiAssign(TinyPtrVector(val), - TinyPtrVector()); + TinyPtrVector()); } void translateSILAssignFresh(SILValue val, SILIsolationInfo info) { @@ -2136,21 +2170,27 @@ 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)) { - builder.addMerge(trackableDest->getRepresentative().getValue(), - trackableSrc->getRepresentative().getValue()); + 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(), op); } } } template <> - void translateSILMerge(SILValue dest, SILValue src) { - return translateSILMerge(dest, TinyPtrVector(src)); + void translateSILMerge(SILValue dest, Operand *src, + bool requireOperands) { + return translateSILMerge(dest, TinyPtrVector(src), + requireOperands); } void translateSILAssignmentToTransferringParameter(TrackableValue destRoot, @@ -2187,7 +2227,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: @@ -2203,10 +2242,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. @@ -2227,10 +2266,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. @@ -2243,11 +2284,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); } @@ -2261,7 +2302,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); } } @@ -2361,8 +2402,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()) @@ -2435,7 +2476,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 { @@ -2641,7 +2685,6 @@ CONSTANT_TRANSLATION(InitExistentialValueInst, LookThrough) CONSTANT_TRANSLATION(CopyAddrInst, Store) CONSTANT_TRANSLATION(ExplicitCopyAddrInst, Store) CONSTANT_TRANSLATION(StoreInst, Store) -CONSTANT_TRANSLATION(StoreBorrowInst, Store) CONSTANT_TRANSLATION(StoreWeakInst, Store) CONSTANT_TRANSLATION(MarkUnresolvedMoveAddrInst, Store) CONSTANT_TRANSLATION(UncheckedRefCastAddrInst, Store) @@ -2895,6 +2938,45 @@ LOOKTHROUGH_IF_NONSENDABLE_RESULT_AND_OPERAND(UncheckedTakeEnumDataAddrInst) // Custom Handling // +TranslationSemantics +PartitionOpTranslator::visitStoreBorrowInst(StoreBorrowInst *sbi) { + // A store_borrow is an interesting instruction since we are essentially + // temporarily binding an object value to an address... so really any uses of + // the address, we want to consider to be uses of the parent object. So we + // basically put source/dest into the same region, but do not consider the + // store_borrow itself to be a require use. This prevents the store_borrow + // from causing incorrect diagnostics. + SILValue destValue = sbi->getDest(); + + auto nonSendableDest = tryToTrackValue(destValue); + if (!nonSendableDest) + return TranslationSemantics::Ignored; + + // In the following situations, we can perform an assign: + // + // 1. A store to unaliased storage. + // 2. A store that is to an entire value. + // + // DISCUSSION: If we have case 2, we need to merge the regions since we + // are not overwriting the entire region of the value. This does mean that + // we artificially include the previous region that was stored + // specifically in this projection... but that is better than + // miscompiling. For memory like this, we probably need to track it on a + // per field basis to allow for us to assign. + if (nonSendableDest.value().isNoAlias() && + !isProjectedFromAggregate(destValue)) { + translateSILMultiAssign(sbi->getResults(), + makeOperandRefRange(sbi->getAllOperands()), + SILIsolationInfo(), false /*require src*/); + } else { + // Stores to possibly aliased storage must be treated as merges. + translateSILMerge(destValue, &sbi->getAllOperands()[StoreBorrowInst::Src], + false /*require src*/); + } + + return TranslationSemantics::Special; +} + TranslationSemantics PartitionOpTranslator::visitAllocStackInst(AllocStackInst *asi) { // Before we do anything, see if asi is Sendable or if it is non-Sendable, @@ -2977,7 +3059,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; } @@ -2986,7 +3068,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; } @@ -2996,7 +3078,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; } @@ -3160,7 +3242,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; } @@ -3211,6 +3294,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) @@ -3449,6 +3536,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 06bbcf7dce80c..6bca8beb3e022 100644 --- a/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp +++ b/lib/SILOptimizer/Mandatory/TransferNonSendable.cpp @@ -474,6 +474,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. @@ -513,6 +540,8 @@ class TransferNonSendableImpl { transferredNonTransferrableInfoList; SmallVector inoutSendingNotDisconnectedInfoList; + SmallVector + assignIsolatedIntoOutSendingParameterInfoList; public: TransferNonSendableImpl(RegionAnalysisFunctionInfo *regionInfo) @@ -526,6 +555,7 @@ class TransferNonSendableImpl { void emitUseAfterTransferDiagnostics(); void emitTransferredNonTransferrableDiagnostics(); void emitInOutSendingNotDisconnectedInfoList(); + void emitAssignIsolatedIntoSendingResultDiagnostics(); }; } // namespace @@ -1818,6 +1848,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 //===----------------------------------------------------------------------===// @@ -1841,6 +2073,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 @@ -1849,6 +2087,8 @@ struct DiagnosticEvaluator final &transferredNonTransferrable, SmallVectorImpl &inoutSendingNotDisconnectedInfoList, + SmallVectorImpl + &assignIsolatedIntoOutSendingParameterInfo, TransferringOperandToStateMap &operandToStateMap) : PartitionOpEvaluatorBaseImpl( workingPartition, info->getOperandSetFactory(), operandToStateMap), @@ -1856,7 +2096,9 @@ struct DiagnosticEvaluator final transferOpToRequireInstMultiMap(transferOpToRequireInstMultiMap), transferredNonTransferrable(transferredNonTransferrable), inoutSendingNotDisconnectedInfoList( - inoutSendingNotDisconnectedInfoList) {} + inoutSendingNotDisconnectedInfoList), + assignIsolatedIntoOutSendingParameterInfoList( + assignIsolatedIntoOutSendingParameterInfo) {} void handleLocalUseAfterTransfer(const PartitionOp &partitionOp, Element transferredVal, @@ -1974,6 +2216,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, @@ -2029,6 +2290,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) @@ -2060,6 +2325,7 @@ void TransferNonSendableImpl::runDiagnosticEvaluator() { transferOpToRequireInstMultiMap, transferredNonTransferrableInfoList, inoutSendingNotDisconnectedInfoList, + assignIsolatedIntoOutSendingParameterInfoList, regionInfo->getTransferringOpToStateMap()); // And then evaluate all of our partition ops on the entry partition. @@ -2093,6 +2359,7 @@ void TransferNonSendableImpl::emitDiagnostics() { emitTransferredNonTransferrableDiagnostics(); emitUseAfterTransferDiagnostics(); emitInOutSendingNotDisconnectedInfoList(); + emitAssignIsolatedIntoSendingResultDiagnostics(); } namespace { diff --git a/lib/SILOptimizer/Utils/VariableNameUtils.cpp b/lib/SILOptimizer/Utils/VariableNameUtils.cpp index 822457d962d12..60c4c8f710c07 100644 --- a/lib/SILOptimizer/Utils/VariableNameUtils.cpp +++ b/lib/SILOptimizer/Utils/VariableNameUtils.cpp @@ -638,7 +638,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/concurrent_value_checking.swift b/test/Concurrency/concurrent_value_checking.swift index 7d0f4c2ae22a6..b64908d5973c1 100644 --- a/test/Concurrency/concurrent_value_checking.swift +++ b/test/Concurrency/concurrent_value_checking.swift @@ -6,8 +6,7 @@ // REQUIRES: asserts class NotConcurrent { } // expected-note 13{{class 'NotConcurrent' does not conform to the 'Sendable' protocol}} -// expected-complete-note @-1 13{{class 'NotConcurrent' does not conform to the 'Sendable' protocol}} -// expected-tns-allow-typechecker-note @-2 {{class 'NotConcurrent' does not conform to the 'Sendable' protocol}} +// expected-tns-allow-typechecker-note @-1 {{class 'NotConcurrent' does not conform to the 'Sendable' protocol}} // ---------------------------------------------------------------------- // Sendable restriction on actor operations @@ -67,23 +66,23 @@ actor A2 { } func testActorCreation(value: NotConcurrent) async { - _ = A2(value: value) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent' into actor-isolated context may introduce data races}} + _ = A2(value: value) // expected-tns-warning @-1 {{sending 'value' risks causing data races}} // expected-tns-note @-2 {{sending task-isolated 'value' to actor-isolated initializer 'init(value:)' risks causing data races between actor-isolated and task-isolated uses}} - _ = await A2(valueAsync: value) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent' into actor-isolated context may introduce data races}} + _ = await A2(valueAsync: value) // expected-tns-warning @-1 {{sending 'value' risks causing data races}} // expected-tns-note @-2 {{sending task-isolated 'value' to actor-isolated initializer 'init(valueAsync:)' risks causing data races between actor-isolated and task-isolated uses}} - _ = A2(delegatingSync: value) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent' into actor-isolated context may introduce data races}} + _ = A2(delegatingSync: value) // expected-tns-warning @-1 {{sending 'value' risks causing data races}} // expected-tns-note @-2 {{sending task-isolated 'value' to actor-isolated initializer 'init(delegatingSync:)' risks causing data races between actor-isolated and task-isolated uses}} - _ = await A2(delegatingAsync: value, 9) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent' into actor-isolated context may introduce data races}} + _ = await A2(delegatingAsync: value, 9) // expected-tns-warning @-1 {{sending 'value' risks causing data races}} // expected-tns-note @-2 {{sending task-isolated 'value' to actor-isolated initializer 'init(delegatingAsync:_:)' risks causing data races between actor-isolated and task-isolated uses}} - _ = await A2(nonisoAsync: value, 3) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent' into actor-isolated context may introduce data races}} + _ = await A2(nonisoAsync: value, 3) // expected-tns-warning @-1 {{sending 'value' risks causing data races}} // expected-tns-note @-2 {{sending task-isolated 'value' to actor-isolated initializer 'init(nonisoAsync:_:)' risks causing data races between actor-isolated and task-isolated uses}} } @@ -104,7 +103,7 @@ extension A1 { // expected-warning@-1 {{expression is 'async' but is not marked with 'await'}} // expected-note@-2 {{property access is 'async'}} _ = await other.synchronous() // expected-warning{{non-sendable type 'NotConcurrent?' returned by call to actor-isolated function cannot cross actor boundary}} - _ = await other.asynchronous(nil) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent?' into actor-isolated context may introduce data races}} + _ = await other.asynchronous(nil) } } @@ -142,8 +141,9 @@ func globalTest() async { // expected-warning@+2 {{expression is 'async' but is not marked with 'await'}} // expected-note@+1 {{property access is 'async'}} let a = globalValue // expected-warning{{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated let 'globalValue' cannot cross actor boundary}} - await globalAsync(a) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}} - await globalSync(a) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}} + await globalAsync(a) // expected-tns-warning {{sending 'a' risks causing data races}} + // expected-tns-note @-1 {{sending global actor 'SomeGlobalActor'-isolated 'a' to global actor 'SomeGlobalActor'-isolated global function 'globalAsync' risks causing data races between global actor 'SomeGlobalActor'-isolated and local nonisolated uses}} + await globalSync(a) // expected-tns-note {{access can happen concurrently}} // expected-warning@+2 {{expression is 'async' but is not marked with 'await'}} // expected-note@+1 {{property access is 'async'}} @@ -154,7 +154,7 @@ func globalTest() async { // expected-typechecker-note@+2 {{call is 'async'}} // expected-typechecker-note@+1 {{property access is 'async'}} globalAsync(E.notSafe) - // expected-complete-warning@-1 {{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}} + // expected-typechecker-warning@-2 {{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated static property 'notSafe' cannot cross actor boundary}} #endif } @@ -178,9 +178,10 @@ func globalTestMain(nc: NotConcurrent) async { // expected-warning@+2 {{expression is 'async' but is not marked with 'await'}} // expected-note@+1 {{property access is 'async'}} let a = globalValue // expected-warning {{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated let 'globalValue' cannot cross actor boundary}} - await globalAsync(a) // expected-complete-warning {{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}} - await globalSync(a) // expected-complete-warning {{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}} - _ = await ClassWithGlobalActorInits(nc) // expected-complete-warning {{passing argument of non-sendable type 'NotConcurrent' into global actor 'SomeGlobalActor'-isolated context may introduce data races}} + await globalAsync(a) // expected-tns-warning {{sending 'a' risks causing data races}} + // expected-tns-note @-1 {{sending global actor 'SomeGlobalActor'-isolated 'a' to global actor 'SomeGlobalActor'-isolated global function 'globalAsync' risks causing data races between global actor 'SomeGlobalActor'-isolated and local main actor-isolated uses}} + await globalSync(a) // expected-tns-note {{access can happen concurrently}} + _ = await ClassWithGlobalActorInits(nc) // expected-warning @-1 {{non-sendable type 'ClassWithGlobalActorInits' returned by call to global actor 'SomeGlobalActor'-isolated function cannot cross actor boundary}} // expected-tns-warning @-2 {{sending 'nc' risks causing data races}} // expected-tns-note @-3 {{sending main actor-isolated 'nc' to global actor 'SomeGlobalActor'-isolated initializer 'init(_:)' risks causing data races between global actor 'SomeGlobalActor'-isolated and main actor-isolated uses}} diff --git a/test/Concurrency/sendable_checking.swift b/test/Concurrency/sendable_checking.swift index 9a51673d76679..56364418b6135 100644 --- a/test/Concurrency/sendable_checking.swift +++ b/test/Concurrency/sendable_checking.swift @@ -295,9 +295,12 @@ func testNonSendableBaseArg() async { let t = NonSendable() await t.update() // expected-targeted-and-complete-warning @-1 {{passing argument of non-sendable type 'NonSendable' into main actor-isolated context may introduce data races}} + // expected-tns-warning @-2 {{sending 't' risks causing data races}} + // expected-tns-note @-3 {{sending 't' to main actor-isolated instance method 'update()' risks causing data races between main actor-isolated and local nonisolated uses}} _ = await t.x // expected-warning @-1 {{non-sendable type 'NonSendable' passed in implicitly asynchronous call to main actor-isolated property 'x' cannot cross actor boundary}} + // expected-tns-note @-2 {{access can happen concurrently}} } // We get the region isolation error here since t.y is custom actor isolated. diff --git a/test/Concurrency/transfernonsendable.swift b/test/Concurrency/transfernonsendable.swift index 2059bc8dcc55b..d283f34bf8dc7 100644 --- a/test/Concurrency/transfernonsendable.swift +++ b/test/Concurrency/transfernonsendable.swift @@ -14,7 +14,7 @@ //////////////////////// /// Classes are always non-sendable, so this is non-sendable -class NonSendableKlass { // expected-complete-note 51{{}} +class NonSendableKlass { // expected-complete-note 53{{}} // expected-typechecker-only-note @-1 3{{}} // expected-tns-note @-2 {{}} var field: NonSendableKlass? = nil @@ -145,10 +145,7 @@ func closureInOut(_ a: MyActor) async { // expected-tns-note @-3 {{sending 'ns0' to actor-isolated instance method 'useKlass' risks causing data races between actor-isolated and local nonisolated uses}} if await booleanFlag { - // This is not an actual use since we are passing values to the same - // isolation domain. - await a.useKlass(ns1) - // expected-complete-warning @-1 {{passing argument of non-sendable type 'NonSendableKlass'}} + await a.useKlass(ns1) // expected-tns-note {{access can happen concurrently}} } else { closure() // expected-tns-note {{access can happen concurrently}} } @@ -1224,11 +1221,11 @@ func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive4() async { // good... that is QoI though. await transferToMain(test) // expected-tns-warning {{sending 'test' risks causing data races}} // expected-tns-note @-1 {{sending 'test' to main actor-isolated global function 'transferToMain' risks causing data races between main actor-isolated and local nonisolated uses}} - // expected-complete-warning @-2 {{passing argument of non-sendable type 'StructFieldTests' into main actor-isolated context may introduce data races}} + // expected-tns-note @-2 {{access can happen concurrently}} // This is treated as a use since test is in box form and is mutable. So we // treat assignment as a merge. - test = StructFieldTests() // expected-tns-note {{access can happen concurrently}} + test = StructFieldTests() cls = { useInOut(&test.varSendableNonTrivial) } @@ -1259,7 +1256,7 @@ func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive5() async { } test.varSendableNonTrivial = SendableKlass() - useValue(test) // expected-tns-note {{access can happen concurrently}} + useValue(test) } func varNonSendableNonTrivialLetStructFieldClosureFlowSensitive6() async { @@ -1428,7 +1425,7 @@ func controlFlowTest2() async { x = NonSendableKlass() } - useValue(x) // expected-tns-note {{access can happen concurrently}} + useValue(x) } //////////////////////// @@ -1738,6 +1735,17 @@ func sendableGlobalActorIsolated() { print(x) // expected-tns-note {{access can happen concurrently}} } +// We do not get an error here since we are transferring x both times to a main +// actor isolated thing function. We used to emit an error when using region +// isolation since we would trip on the store_borrow we used to materialize the +// value. +func testIndirectParameterSameIsolationNoError() async { + let x = NonSendableKlass() + await transferToMain(x) // expected-tns-warning {{sending 'x' risks causing data races}} + // expected-tns-note @-1 {{sending 'x' to main actor-isolated global function 'transferToMain' risks causing data races between main actor-isolated and local nonisolated uses}} + await transferToMain(x) // expected-tns-note {{access can happen concurrently}} +} + extension MyActor { func testNonSendableCaptures(sc: NonSendableKlass) { Task { diff --git a/test/Concurrency/transfernonsendable_asynclet.swift b/test/Concurrency/transfernonsendable_asynclet.swift index e030954cfebc7..c4768dfbe29d8 100644 --- a/test/Concurrency/transfernonsendable_asynclet.swift +++ b/test/Concurrency/transfernonsendable_asynclet.swift @@ -37,6 +37,13 @@ final actor FinalMyActor { func useKlass(_ x: NonSendableKlass) {} } +actor CustomActorInstance {} + +@globalActor +struct CustomActor { + static let shared = CustomActorInstance() +} + func useInOut(_ x: inout T) {} @discardableResult func useValue(_ x: T) -> T { x } @@ -52,6 +59,7 @@ func useMainActorValueNoReturn(_ x: T) -> () { fatalError() } @MainActor func returnValueFromMain() async -> T { fatalError() } @MainActor func transferToMain(_ t: T) async {} @MainActor func transferToMainInt(_ t: T) async -> Int { 5 } +@CustomActor func transferToCustomInt(_ t: T) async -> Int { 5 } @MainActor func transferToMainIntOpt(_ t: T) async -> Int? { 5 } func transferToNonIsolated(_ t: T) async {} @@ -69,15 +77,6 @@ struct TwoFieldKlassBox { var k2 = NonSendableKlass() } -actor CustomActorInstance {} - -@globalActor -struct CustomActor { - static let shared = CustomActorInstance() -} - -@CustomActor func transferToCustomInt(_ t: T) async -> Int { 5 } - ///////////////////////////////////// // MARK: Async Let Let Actor Tests // ///////////////////////////////////// @@ -294,14 +293,13 @@ func asyncLet_Let_ActorIsolated_CallBuriedInOtherExpr3() async { let _ = await y } -// Make sure that we emit an error for transferToMainInt in the async val -// function itself. func asyncLet_Let_ActorIsolated_CallBuriedInOtherExpr4() async { let x = NonSendableKlass() - async let y = useValue(transferToMainInt(x) + transferToMainInt(x)) // expected-warning {{sending 'x' risks causing data races}} - // expected-note @-1 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}} - // expected-note @-2:67 {{access can happen concurrently}} + async let y = useValue(transferToMainInt(x) + transferToMainInt(x)) + // expected-warning @-1:26 {{sending 'x' risks causing data races}} + // expected-note @-2:26 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}} + // expected-note @-3:49 {{access can happen concurrently}} let _ = await y } @@ -314,7 +312,7 @@ func asyncLet_Let_ActorIsolated_CallBuriedInOtherExpr5() async { async let y = useValue(transferToMainInt(x) + transferToCustomInt(x)) // expected-warning @-1 {{sending 'x' risks causing data races}} // expected-note @-2 {{sending 'x' to main actor-isolated global function 'transferToMainInt' risks causing data races between main actor-isolated and local nonisolated uses}} - // expected-note @-3:69 {{access can happen concurrently}} + // expected-note @-3:49 {{access can happen concurrently}} let _ = await y } 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_instruction_matching.sil b/test/Concurrency/transfernonsendable_instruction_matching.sil index 6350e1d219d5a..47e0e0952e12c 100644 --- a/test/Concurrency/transfernonsendable_instruction_matching.sil +++ b/test/Concurrency/transfernonsendable_instruction_matching.sil @@ -645,14 +645,12 @@ bb0: fix_lifetime %value : $SendableKlass - // This is not considered a bad use of the raw pointer since this is - // sending to the same isolation domain. apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferRawPointer(%rawPointer) : $@convention(thin) @async (Builtin.RawPointer) -> () // expected-warning @-1 {{}} + // expected-note @-2 {{access can happen concurrently}} fix_lifetime %rawPointer : $Builtin.RawPointer // expected-note @-1 {{access can happen concurrently}} - // expected-note @-2 {{access can happen concurrently}} destroy_value %value : $SendableKlass %9999 = tuple () @@ -725,10 +723,10 @@ bb0: apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferRawPointer(%rawPointer) : $@convention(thin) @async (Builtin.RawPointer) -> () // expected-warning @-1 {{}} + // expected-note @-2 {{access can happen concurrently}} fix_lifetime %rawPointer : $Builtin.RawPointer // expected-note @-1 {{access can happen concurrently}} - // expected-note @-2 {{access can happen concurrently}} destroy_value %value : $SendableKlass %9999 = tuple () @@ -804,10 +802,10 @@ bb0: apply [caller_isolation=nonisolated] [callee_isolation=global_actor] %transferRawPointer(%rawPointer) : $@convention(thin) @async (Builtin.RawPointer) -> () // expected-warning @-1 {{}} + // expected-note @-2 {{access can happen concurrently}} fix_lifetime %rawPointer : $Builtin.RawPointer // expected-note @-1 {{access can happen concurrently}} - // expected-note @-2 {{access can happen concurrently}} end_borrow %valueB : $SendableKlass destroy_value %value : $SendableKlass diff --git a/test/Concurrency/transfernonsendable_sending_results.swift b/test/Concurrency/transfernonsendable_sending_results.swift index 586be6b34abf6..a87b813f6cae7 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