-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[6.3][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. #86601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/6.3
Are you sure you want to change the base?
Conversation
…e and produce a new BoxType with mutated mutability of its fields. This is properly prepared for multiple field boxes since we take in an initializer_list of fields/mutability changes. Given that I made this change to SILBoxType, I aligned the same API on SILLayout to also take an initializer_list of fields/mutability changes instead of just assuming a single field layout. (cherry picked from commit 3078924)
Before this patch it was easy to try to work with the types in a SILBoxType by accessing the layout of the SILBoxType... especially if one was working with mutability since there wasn't any comment/APIs on SILBoxType that pointed one at the appropriate API on SILType. To make this easier to use and self document, I added some new helper APIs onto SILBoxType that: 1. Let one grab the fields without touching the layout. The layout is an internal detail of SILBoxType that shouldn't be touched unless it is necessary. 2. Get a specific properly specialized SILType of a SILType for a given SILFunction. 3. Access the number of SILFIelds and also whether or not a specific SILField is mutable. 4. Yields a transform range that transform the SILFields in the SILBoxType into a range of properly specialized SILTypes. This should prevent these sorts of mistakes from happening in the future. (cherry picked from commit f10ca1c)
|
@swift-ci test |
|
I think 1-2 tests may fail due to differences on this branch related to diagnostics. I am building locally now in case this happens. But no reason not to start the testing in case I am wrong. |
|
Original PR: #86312 |
43a9e54 to
eaaec3a
Compare
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
… boxes Introduce a new optional inferred-immutable flag on SILFunctionArgument to mark closure-captured box parameters that are never written to despite being mutable. This flag will enable in future commits: - Marking captured mutable boxes as immutable when interprocedural analysis proves they are never modified - Treating these captures as Sendable when they contain Sendable types - Improving region-based isolation analysis for concurrent code This complements the inferred-immutable flag on alloc_box by allowing immutability information to flow through closure boundaries. (cherry picked from commit 581cfcd) Conflicts: test/SIL/Parser/basic2.sil
… for mutable capture boxes. The reason I am doing this is that I want to be careful and make sure that we can distinguish in between weak var and weak let var decls and real captures. In the caller, we do not actually represent the capture's var decl information in a meaningful way since the actual var decl usage is only in the closure. After inlining, we get that var decl information from the debug_value of the argument. So there isn't any reason not to do it and it will simplify the other work I am doing. (cherry picked from commit 6fc9789)
Introduce a new optional flag on the alloc_box SIL instruction to mark boxes as inferred immutable, indicating that static analysis has proven they are never written to despite having a mutable type. The flag is preserved through serialization/deserialization and properly printed/parsed in textual SIL format. I am doing this to prepare for treating these boxes as being Sendable when they contain a sendable weak reference. (cherry picked from commit 5d0f2e3) Conflicts: lib/Serialization/ModuleFormat.h
…checks throughout This commit systematically replaces all calls to `SILIsolationInfo::isNonSendableType(type, fn)` and `SILIsolationInfo::isSendableType(type, fn)` with their value-based equivalents `SILIsolationInfo::isNonSendable(value)` and `SILIsolationInfo::isSendable(value)`. This refactoring enables more precise Sendability analysis for captured values in closures, which is a prerequisite for treating inferred-immutable weak captures as Sendable, a modification I will be making a subsequent commit. I made the type-based `isSendableType(type, fn)` methods private to prevent future misuse. The only place where isSendableType was needed to be used outside of SILIsolationInfo itself was when checking the fields of a box. Rather than exposing the API for that one purpose, I added two APIs specifically for that use case. (cherry picked from commit e74e17d) Conflicts: lib/SILOptimizer/Mandatory/SendNonSendable.cpp
…e around closures before I change it. (cherry picked from commit dfa7d4c)
…mutable 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 swiftlang#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 (cherry picked from commit 0ce3729)
…ck to their state so it passes in ToT. I have a patch out of tree that fixes these tests so that the error is not emitted. But to make the overall change cherry-pickable, I decided to leave out that change. Once the main change lands, I will commit the other change and remove these diagnostics from the test file. (cherry picked from commit 7d0aec4)
a934441 to
ff587b4
Compare
|
Test failed due to a cherry-picking error. I put in a parser test from main that wasn't on 6.3... :sigh: |
|
@swift-ci test |
|
@swift-ci test windows platform |
1 similar comment
|
@swift-ci test windows platform |
Explanation: This fix resolves a regression introduced in #82427 where weak
self-captures in closures began triggering concurrency errors.
The problem occurs because:
var) even when they're never modified after initializationThis breaks a pattern widely used in Swift code:
Note how even though
selfis Sendable since it is MainActor-isolated, the boxcontaining the weak reference is not Sendable because it is mutable.
To fix this, I added a new pass that recognizes such boxes as actually being
immutable and updated the SILIsolationInfo infrastructure to use that
information.
In more detail: The pass works by:
Walking the module looking for alloc_box that are used for mutable weak var
closure captures.
The uses of such boxes are analyzed recursively and interprocedurally to prove
that the box is never written to beyond an initial initialization store.
If we prove that there are no such writes, we mark the box and all function
arguments associated with any closures we visited as immutable.
Scope: Narrowly targeted to weak variable captures only. The pass uses
conservative pattern matching specific to this use case, minimizing impact on
other compiler functionality.
Issues: rdar://166081666
Original PRs: #86312
Risk:
Compiler Crash: Medium. I think this is reasonable since any introduction of a
new compiler pass contains some risk. But this is mitigated by the
conservativeness and simplicity of the pass. As an example: we do not
introduce any new dataflow, any new data structures, and instead just use a
standard DenseSet and a recursive dataflow walk.
Semantic Risks: Low. We could introduce new holes in the concurrency model by
considering safe mutable weak vars that are written to. This is mitigated by
the pass using a pattern match allow list. All patterns that do not match this
allow list are assumed unsafe, so we should fail to pattern match anything
else and just get the current behavior.
Testing: Swift -verify tests and SIL FileCheck tests
Reviewers: @rjmccall