Skip to content

Commit 0ce3729

Browse files
committed
[rbi] Mark mutable weak capture boxes containing Sendable types as immutable if they are never interprocedurally written to and teach SILIsolationInfo::isSendable that they are meant to be treated as Sendable.
The pass works by walking functions in the modules looking for mutable alloc_box that contains a weak variable and is knowably a capture. In such a case, the pass checks all uses of the alloc_box interprocedurally including through closures and if provably immutable marks the box and all closure parameters as being inferred immutable. This change also then subsequently changes SILIsolationInfo to make it so that such boxes are considered Sendable in a conservative manner that pattern matches the weak reference code emission pretty closely. The reason why I am doing this is that issue #82427 correctly tightened region isolation checking to catch unsafe concurrent access to mutable shared state. However, this introduced a regression for a common Swift pattern: capturing `self` weakly in escaping closures. The problem occurs because: 1. Weak captures are stored in heap-allocated boxes. 2. By default, these boxes are **mutable** (`var`) even if never written to after initialization 3. Mutable boxes are non-Sendable (they could be unsafely mutated from multiple threads) 4. Region isolation now correctly errors when sending non-Sendable values across isolation boundaries This breaks code like: ```swift @mainactor class C { func test() { timer { [weak self] in // Captures self in a mutable box Task { @mainactor in self?.update() // ERROR: sending mutable box risks data races } } } } ``` Note how even though `self` is Sendable since it is MainActor-isolated, the *box containing* the weak reference is not Sendable because it is mutable. With the change in this commit, we now recognize that the box can safely be treated as Sendable since we would never write to it. rdar://166081666
1 parent e74e17d commit 0ce3729

File tree

8 files changed

+2211
-22
lines changed

8 files changed

+2211
-22
lines changed

include/swift/SIL/OperandDatastructures.h

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,19 @@ class OperandWorklist {
102102
return false;
103103
}
104104

105+
/// Pushes the operands of all uses of \p value onto the worklist if the
106+
/// operands have never been pushed before. Returns \p true if we inserted
107+
/// /any/ values.
108+
///
109+
/// This is a bulk convenience API.
110+
bool pushResultOperandsIfNotVisited(SILValue value) {
111+
bool insertedOperand = false;
112+
for (auto *use : value->getUses()) {
113+
insertedOperand |= pushIfNotVisited(use);
114+
}
115+
return insertedOperand;
116+
}
117+
105118
/// Pushes the operands of all uses of \p instruction onto the worklist if the
106119
/// operands have never been pushed before. Returns \p true if we inserted
107120
/// /any/ values.
@@ -110,9 +123,7 @@ class OperandWorklist {
110123
bool pushResultOperandsIfNotVisited(SILInstruction *inst) {
111124
bool insertedOperand = false;
112125
for (auto result : inst->getResults()) {
113-
for (auto *use : result->getUses()) {
114-
insertedOperand |= pushIfNotVisited(use);
115-
}
126+
insertedOperand |= pushResultOperandsIfNotVisited(result);
116127
}
117128
return insertedOperand;
118129
}

include/swift/SILOptimizer/PassManager/Passes.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,6 +485,8 @@ LEGACY_PASS(DiagnoseUnnecessaryPreconcurrencyImports, "sil-diagnose-unnecessary-
485485
"Diagnose any preconcurrency imports that Sema and TransferNonSendable did not use")
486486
LEGACY_PASS(ThunkLowering, "sil-thunk-lowering",
487487
"Lower thunk instructions to actual thunks")
488+
LEGACY_PASS(MarkNeverWrittenMutableClosureBoxesAsImmutable, "mark-never-written-mutable-closure-boxes-as-immutable",
489+
"Mark never written mutable closure boxes as immutable")
488490
LEGACY_PASS(PruneVTables, "prune-vtables",
489491
"Mark class methods that do not require vtable dispatch")
490492

lib/SILOptimizer/Mandatory/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ target_sources(swiftSILOptimizer PRIVATE
5050
YieldOnceCheck.cpp
5151
OSLogOptimization.cpp
5252
MoveOnlyWrappedTypeEliminator.cpp
53+
MarkNeverWrittenMutableClosureBoxesAsImmutable.cpp
5354
RegionAnalysisInvalidationTransform.cpp
5455
DiagnosticDeadFunctionElimination.cpp
5556
OwnershipModelEliminator.cpp)
Lines changed: 273 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,273 @@
1+
//===--- MarkNeverWrittenMutableClosureBoxesAsImmutable.cpp ---------------===//
2+
//
3+
// This source file is part of the Swift.org open source project
4+
//
5+
// Copyright (c) 2014 - 2026 Apple Inc. and the Swift project authors
6+
// Licensed under Apache License v2.0 with Runtime Library Exception
7+
//
8+
// See https://swift.org/LICENSE.txt for license information
9+
// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors
10+
//
11+
//===----------------------------------------------------------------------===//
12+
13+
#define DEBUG_TYPE "sil-mark-never-written-mutable-closure-boxes-as-immutable"
14+
15+
#include "swift/SIL/ApplySite.h"
16+
#include "swift/SIL/OperandDatastructures.h"
17+
#include "swift/SILOptimizer/PassManager/Passes.h"
18+
#include "swift/SILOptimizer/PassManager/Transforms.h"
19+
#include "swift/SILOptimizer/Utils/SILIsolationInfo.h"
20+
21+
using namespace swift;
22+
23+
//===----------------------------------------------------------------------===//
24+
// MARK: Implementation
25+
//===----------------------------------------------------------------------===//
26+
27+
static bool isImmutable(SILValue start, StoreWeakInst *allowableWeakStore,
28+
llvm::DenseSet<SILFunctionArgument *> &visitedArgs) {
29+
LLVM_DEBUG(llvm::dbgs() << "Checking in function "
30+
<< start->getFunction()->getName() << ": " << *start);
31+
// We store the partial apply that we are going to visit serially after we
32+
// finish processing the partial_apply so that we do not create too many
33+
// OperandWorklist. We can only create a finite amount of them.
34+
SmallVector<SILFunctionArgument *, 8> funcArgsToVisit;
35+
36+
{
37+
// Walk the uses to search for the partial_apply. If we have a debug_value,
38+
// a move_value [lexical], or a begin_borrow [lexical].
39+
OperandWorklist worklist(start->getFunction());
40+
worklist.pushResultOperandsIfNotVisited(start);
41+
42+
while (auto *use = worklist.pop()) {
43+
auto *user = use->getUser();
44+
LLVM_DEBUG(llvm::dbgs() << " Visiting User: " << *user);
45+
46+
// Uses to skip.
47+
if (isa<EndBorrowInst>(user) || isa<DestroyValueInst>(user) ||
48+
isa<LoadWeakInst>(user) || isa<EndAccessInst>(user) ||
49+
isa<DebugValueInst>(user) || isa<MarkFunctionEscapeInst>(user)) {
50+
LLVM_DEBUG(llvm::dbgs() << " Ignoring!\n");
51+
continue;
52+
}
53+
54+
// Uses to look through.
55+
if (isa<CopyValueInst>(user) || isa<ProjectBoxInst>(user) ||
56+
isa<MoveValueInst>(user) || isa<BeginBorrowInst>(user) ||
57+
isa<BeginAccessInst>(user)) {
58+
LLVM_DEBUG(llvm::dbgs() << " Looking through!\n");
59+
worklist.pushResultOperandsIfNotVisited(user);
60+
continue;
61+
}
62+
63+
// If we have a store_weak, continue if it is the store_weak that we are
64+
// ok with.
65+
if (auto *swi = dyn_cast<StoreWeakInst>(user);
66+
swi && swi == allowableWeakStore) {
67+
LLVM_DEBUG(llvm::dbgs() << " Ignoring allowable store_weak!\n");
68+
continue;
69+
}
70+
71+
// Visit partial_apply uses and see if:
72+
//
73+
// 1. We can look up the function.
74+
//
75+
// 2. If we already know that the function argument is inferred
76+
// immutable. In that case, we can just continue.
77+
//
78+
// 3. Then we check if we already visited the function argument. That
79+
// means that we know that it is not immutable if it has not been marked
80+
// yet... so just return false.
81+
//
82+
// 4. Otherwise, we add it to a worklist to process after we finish
83+
// walking uses in this function. We do this to ensure we do not create
84+
// too many OperandWorklists at the same time since we can only create a
85+
// finite amount of them at the same time.
86+
if (auto *pai = dyn_cast<PartialApplyInst>(user)) {
87+
if (auto *calleeFunc = pai->getReferencedFunctionOrNull()) {
88+
auto calleeArgIndex = ApplySite(pai).getCalleeArgIndex(*use);
89+
auto *fArg = cast<SILFunctionArgument>(
90+
calleeFunc->getArgument(calleeArgIndex));
91+
if (fArg->isInferredImmutable()) {
92+
LLVM_DEBUG(llvm::dbgs()
93+
<< " Found partial_apply with inferred immutable "
94+
"function arg. Can ignore it!\n");
95+
continue;
96+
}
97+
if (visitedArgs.count(fArg)) {
98+
LLVM_DEBUG(llvm::dbgs()
99+
<< " Found mutable function arg user!\n");
100+
101+
return false;
102+
}
103+
LLVM_DEBUG(llvm::dbgs()
104+
<< " Found partial apply to check later!\n");
105+
funcArgsToVisit.push_back(fArg);
106+
continue;
107+
}
108+
}
109+
110+
// Unrecognized user. Bail.
111+
LLVM_DEBUG(llvm::dbgs()
112+
<< " Not transforming due to unhandled user!\n");
113+
return false;
114+
}
115+
}
116+
117+
// Now check recursively if our function argument users are immutable. We do
118+
// this after we walk to avoid creating too many OperandWorklist.
119+
bool allFArgUsersImmutable = true;
120+
for (auto *fArg : funcArgsToVisit) {
121+
assert(!fArg->isInferredImmutable() && "Should have been checked earlier");
122+
visitedArgs.insert(fArg);
123+
if (isImmutable(fArg, nullptr, visitedArgs)) {
124+
fArg->setInferredImmutable(true);
125+
continue;
126+
}
127+
allFArgUsersImmutable = false;
128+
}
129+
130+
return allFArgUsersImmutable;
131+
}
132+
133+
/// Make sure that the given box fits out pattern matching conditions and return
134+
/// its single initializing begin_borrow scope and store_weak so we can do a
135+
/// later more intensive recursive check.
136+
///
137+
/// The conditions are:
138+
///
139+
/// 1. The box must be mutable.
140+
///
141+
/// 2. The box must contain a weak reference to a Sendable type.
142+
///
143+
/// 3. The box must have a single begin_borrow user that all uses are
144+
/// initialized from.
145+
///
146+
/// 4. There must be a single store_weak that initializes the box from a
147+
/// project_box from the single begin_borrow.
148+
///
149+
/// 5. The box should not have a debug_value use.
150+
///
151+
/// This is safe since later we are going to recursively look at uses of the
152+
/// begin_borrow and if we find any memory uses that are a load_weak or a
153+
/// different store_weak besides the one we found, we fail the box.
154+
static StoreWeakInst *isPatternMatchableBox(AllocBoxInst *abi) {
155+
LLVM_DEBUG(llvm::dbgs() << "Checking if box can be matched: " << *abi);
156+
157+
CanSILBoxType boxType = abi->getType().castTo<SILBoxType>();
158+
if (boxType->getNumFields() != 1 ||
159+
!SILIsolationInfo::boxContainsOnlySendableFields(abi)) {
160+
LLVM_DEBUG(llvm::dbgs() << " Cannot match since either has multiple "
161+
"fields or a non-Sendable field\n");
162+
return nullptr;
163+
}
164+
165+
// For now to be conservative, only do this if we have a weak
166+
// parameter.
167+
if (auto ownership = boxType->getFieldType(*abi->getFunction(), 0)
168+
.getReferenceStorageOwnership();
169+
!ownership || *ownership != ReferenceOwnership::Weak) {
170+
LLVM_DEBUG(llvm::dbgs()
171+
<< " Cannot match since field is not a weak reference\n");
172+
return nullptr;
173+
}
174+
175+
BeginBorrowInst *singleBBI = nullptr;
176+
for (auto *use : abi->getUses()) {
177+
if (isa<DestroyValueInst>(use->getUser()) ||
178+
isa<DeallocBoxInst>(use->getUser()) ||
179+
isa<CopyValueInst>(use->getUser()))
180+
continue;
181+
auto *bbi = dyn_cast<BeginBorrowInst>(use->getUser());
182+
if (!bbi) {
183+
LLVM_DEBUG(llvm::dbgs()
184+
<< " Cannot match since has a non-begin_borrow, "
185+
"destroy_value, dealloc_box immediate user: "
186+
<< *use->getUser());
187+
return nullptr;
188+
}
189+
190+
if (bbi->isFromVarDecl()) {
191+
LLVM_DEBUG(llvm::dbgs()
192+
<< " Cannot match since begin_borrow from var_decl\n");
193+
return nullptr;
194+
}
195+
196+
if (singleBBI) {
197+
LLVM_DEBUG(llvm::dbgs() << " Cannot match since found multiple "
198+
"begin_borrow initializations\n");
199+
return nullptr;
200+
}
201+
singleBBI = bbi;
202+
}
203+
204+
if (!singleBBI) {
205+
LLVM_DEBUG(llvm::dbgs() << " Cannot match since did not find "
206+
"begin_borrow for initialization\n");
207+
return nullptr;
208+
}
209+
210+
// Now look for a single store_weak from a project_box from our singleBBI.
211+
//
212+
// DISCUSSION: We could be lazier here and leave the checking of multiple
213+
// store_weak to the later recursive check... but why not just check now and
214+
// end earlier.
215+
StoreWeakInst *singleStoreWeak = nullptr;
216+
for (auto *use : singleBBI->getUsersOfType<ProjectBoxInst>()) {
217+
if (auto *swi = use->getSingleUserOfType<StoreWeakInst>()) {
218+
if (singleStoreWeak) {
219+
LLVM_DEBUG(llvm::dbgs()
220+
<< " Cannot match since found multiple store_weak\n");
221+
return nullptr;
222+
}
223+
singleStoreWeak = swi;
224+
}
225+
}
226+
if (!singleStoreWeak) {
227+
LLVM_DEBUG(llvm::dbgs() << " Cannot match since did not find a single "
228+
"store_weak initialization\n");
229+
return {};
230+
}
231+
232+
return singleStoreWeak;
233+
}
234+
235+
namespace {
236+
237+
class MarkNeverWrittenMutableClosureBoxesAsImmutable
238+
: public SILModuleTransform {
239+
void run() override {
240+
bool madeChange = false;
241+
llvm::DenseSet<SILFunctionArgument *> visitedArgs;
242+
for (auto &fn : *getModule()) {
243+
for (auto &block : fn) {
244+
for (auto &inst : block) {
245+
auto *abi = dyn_cast<AllocBoxInst>(&inst);
246+
if (!abi)
247+
continue;
248+
auto *singleInitialization = isPatternMatchableBox(abi);
249+
if (!singleInitialization ||
250+
!isImmutable(abi, singleInitialization, visitedArgs))
251+
continue;
252+
253+
abi->setInferredImmutable(true);
254+
LLVM_DEBUG(llvm::dbgs() << "Marking Box as Inferred Immutable!\n");
255+
madeChange = true;
256+
}
257+
}
258+
}
259+
260+
if (madeChange)
261+
invalidateAll();
262+
};
263+
};
264+
265+
} // namespace
266+
267+
//===----------------------------------------------------------------------===//
268+
// MARK: Top Level Entrypoint
269+
//===----------------------------------------------------------------------===//
270+
271+
SILTransform *swift::createMarkNeverWrittenMutableClosureBoxesAsImmutable() {
272+
return new MarkNeverWrittenMutableClosureBoxesAsImmutable();
273+
}

lib/SILOptimizer/PassManager/PassPipeline.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@ static void addDefiniteInitialization(SILPassPipelinePlan &P) {
115115
// should be in the -Onone pass pipeline and the prepare optimizations pipeline.
116116
static void addMandatoryDiagnosticOptPipeline(SILPassPipelinePlan &P) {
117117
P.startPipeline("Mandatory Diagnostic Passes + Enabling Optimization Passes");
118+
119+
P.addMarkNeverWrittenMutableClosureBoxesAsImmutable();
118120
P.addDiagnoseInvalidEscapingCaptures();
119121
P.addReferenceBindingTransform();
120122
P.addNestedSemanticFunctionCheck();

lib/SILOptimizer/Utils/SILIsolationInfo.cpp

Lines changed: 40 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1581,8 +1581,46 @@ void SILIsolationInfo::printForOneLineLogging(SILFunction *fn,
15811581
}
15821582

15831583
bool SILIsolationInfo::isSendable(SILValue value) {
1584-
// For now just rely on the type based analysis.
1585-
return isSendableType(value->getType(), value->getFunction());
1584+
// If the type system says we are sendable, then we are always sendable.
1585+
if (isSendableType(value->getType(), value->getFunction()))
1586+
return true;
1587+
1588+
if (auto *fArg = dyn_cast<SILFunctionArgument>(value);
1589+
fArg && fArg->isClosureCapture() && fArg->isInferredImmutable()) {
1590+
CanSILBoxType boxType = fArg->getType().getAs<SILBoxType>();
1591+
if (!boxType || boxType->getNumFields() != 1)
1592+
return false;
1593+
auto innerType = boxType->getFieldType(*fArg->getFunction(), 0);
1594+
// We can only do this if the underlying type is Sendable.
1595+
if (isNonSendableType(innerType, fArg->getFunction()))
1596+
return false;
1597+
// For now to be conservative, only do this if we have a weak parameter.
1598+
if (auto ownership = innerType.getReferenceStorageOwnership();
1599+
!ownership || *ownership != ReferenceOwnership::Weak)
1600+
return false;
1601+
// Ok, we can treat this as Sendable.
1602+
return true;
1603+
}
1604+
1605+
if (auto *abi = dyn_cast<AllocBoxInst>(lookThroughOwnershipInsts(value));
1606+
abi && abi->isInferredImmutable()) {
1607+
CanSILBoxType boxType = abi->getType().castTo<SILBoxType>();
1608+
if (boxType->getNumFields() != 1)
1609+
return false;
1610+
1611+
auto innerType = boxType->getFieldType(*abi->getFunction(), 0);
1612+
if (isNonSendableType(innerType, abi->getFunction()))
1613+
return false;
1614+
1615+
// For now to be conservative, only do this if we have a weak parameter.
1616+
if (auto ownership = innerType.getReferenceStorageOwnership();
1617+
!ownership || *ownership != ReferenceOwnership::Weak)
1618+
return false;
1619+
1620+
return true;
1621+
}
1622+
1623+
return false;
15861624
}
15871625

15881626
// Check if the passed in type is NonSendable.

0 commit comments

Comments
 (0)