[region-isolation] Cherry-pick to swift 6 #72584
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Just running tests |
|
@swift-ci smoke test |
…tors.swift. We already have transfernonsendable_global_actors.swift which handles all of the test cases in the removed test + more. (cherry picked from commit d93da30)
(cherry picked from commit 99e2f2b)
…erTransfer. Now that we have other forms of error callbacks, having such a general name for any specific failure is misleading and hinds intent. (cherry picked from commit 465bb23)
ActorIsolation::getActor() asserts if an actor cannot be found. This new helper method just returns nullptr instead. (cherry picked from commit 18aa7d8)
…h -> PartitionUtils.h. I am going to need this so that I can use it when evaluating partition ops. Specifically, so I can when I find two values that do not merge correctly, emit an error. (cherry picked from commit b23b791)
…itionOpEvaluator. This will let me look up the dynamic isolation region associated with \p elt while performing dataflow. (cherry picked from commit f7d1f2a)
…e the dynamic merged IsolationRegionInfo found during dataflow. I also eliminated the very basic "why is this task isolated" part of the warning in favor of the larger, better, region history impl that I am going to land soon. The diagnostic wasn't that good in the first place and also was fiddly. So I removed it for now. rdar://124960994 (cherry picked from commit afbcf85)
…Info. (cherry picked from commit f796f11)
(cherry picked from commit 6b1258c)
(cherry picked from commit a2d1adf)
…transfer error. This is just a pseudo-why are these two things part of the same region error. I am going to remove this for now and the proper form of this diagnostic will come back when I land the region history functionality. (cherry picked from commit 7368de2)
… store_borrow initialized variables. This eliminates a bunch of cases where we couldn't infer the name of a variable and used the type based diagnostic. It also eliminates an 'unknown' case for move checking. (cherry picked from commit 8b899b8)
… patterns. Just slicing off from a larger patch. (cherry picked from commit 29e3069)
…ests since libswiftBasic can include contents from libswiftAST. I hit this in swiftlang#72476. I put the declaration that hit this in a header, but as I thought about it... there was no real harm in just fixing the issue and preventing future breakage. (cherry picked from commit aac3cd7)
… ptr type from a pointer type. I need to start tracking the dynamic IsolationRegionInfo for the transferring operand so I can ignore uses that are part of the same IsolationRegionInfo. IsolationRegionInfo doesn't fit into a pointer, so just to keep things the same, I am going to just allocate it. This is an initial staging commit that tests out the bump ptr allocating without expanding the type yet. (cherry picked from commit e0fdce1)
This enables one to use ActorIsolation in a FoldingSetNode. I also fixed the hash combine of the type to contain state.silParsed (which I think was an oversight). (cherry picked from commit e9cd2ae)
Specifically, I made it so that when one calls dump(), we put in a newline afterwards so we get a nice easy to read msg in the debugger. I also defined dumpForDiagnostics() to make it easy to dump out in the debugger how the ActorIsolation will show up in the diagnostics. (cherry picked from commit e6a1d83)
…ls.h This is in preparation for beginning to track an IsolationRegionInfo in TransferringOperand. Just splitting this into a separate commit to make it easier to read. (cherry picked from commit 0d96a16)
Importantly, if we have an actor isolation, we always defer to it. (cherry picked from commit 12aad7c)
Just slicing down a larger diff to make it easier to review. (cherry picked from commit d72f078)
…a helper factory method on IsolationRegionInfo. Long term I would like to get region analysis and transfer non sendable out of the business of directly interpreting the AST... but if we have to do it now, I would rather us do it through a helper struct. At least the helper struct can be modified later to work with additional SIL concurrency support when it is added. (cherry picked from commit e00fbfa)
… entire region into a helper. I also did a little bit of renaming of variable names to make the code a little clearer. (cherry picked from commit f991517)
That is really what IsolationRegionInfo is. (cherry picked from commit 5b81712)
… from an instruction. I am making this specific API since I am going to make it so that SILIsolationInfo::get(SILInstruction *) can infer isolation info from self even from functions that are not apply isolation crossing points. For example, in the following, we need to understand that test is main actor isolated and we shouldn't emit an error. ```swift @mainactor func test(_ x: NonSendable) {} @OtherActor func doSomething() { let x = NonSendable() Task.init { @mainactor in print(x) } test(x) } ``` (cherry picked from commit c09b9f8)
Just doing a little cleaning. (cherry picked from commit c2dcdb9)
…tionArgument onto SILIsolationInfo::get(...). (cherry picked from commit 608493c)
…of non-isolation crossing apply. (cherry picked from commit 6a32284)
(cherry picked from commit 226c4ac)
…ation::dumpForDiagnostics out of line. (cherry picked from commit 56a3270)
…ic isolation information of the transfered operand value in its diagnostic message.
As an example of the change:
- // expected-note @-1 {{'x' is transferred from nonisolated caller to main actor-isolated callee. Later uses in caller could race with potential uses in callee}}
+ // expected-note @-1 {{transferring disconnected 'x' to main actor-isolated callee could cause races in between callee main actor-isolated and local nonisolated uses}}
Part of the reason I am doing this is that I am going to be ensuring that we
handle a bunch more cases and I wanted to fix this diagnostic before I added
more incaranations of it to the tests.
(cherry picked from commit 357a53a)
I am doing this since it isn't always going to be an access. We may not have memory. We are talking about uses here! I was able to just use sed so it was an easy fix. (cherry picked from commit 2f9b519)
…and parameters. rdar://125211624 (cherry picked from commit 363c563)
…ngly transfering parameter.
Specifically, I added a named version of the diagnostic:
func testSimpleTransferLet() {
let k = Klass()
- transferArg(k) // expected-warning {{binding of non-Sendable type 'Klass' accessed after being transferred; later accesses could race}}
+ transferArg(k) // expected-warning {{transferring 'k' may cause a race}}
+ // expected-note @-1 {{'k' used after being passed as a transferring parameter}}
useValue(k) // expected-note {{use here could race}}
}
and I also cleaned up the typed version of the diagnostic that is used
if we fail to find a name:
func testSimpleTransferLet() {
let k = Klass()
- transferArg(k) // expected-warning {{binding of non-Sendable type 'Klass' accessed after being transferred; later accesses could race}}
- transferArg(k) // expected-warning {{value of non-Sendable type 'Klass' accessed after being transferred; later accesses could race}}
useValue(k) // expected-note {{use here could race}}
}
This is the 2nd to the last part of a larger effort to rework all of
the region based diagnostics to first try and use names and only go back
to the old typed diagnostics when we fail to look up a name (which should
be pretty rare, but is always possible).
At some point if I really feel confident enough with the name lookup code, I am
most likely just going to get rid of the typed diagnostic code and just emit a
compiler doesnt understand error. The user will still not be able to ship the
code but would also be told to file a bug so that we can fix the name
inference.
(cherry picked from commit 5580bb4)
…pture diagnostic. Just converting another diagnostic to its final named form. (cherry picked from commit 7d07fb5)
…r to use a named variant. It was pretty easy to do since I already have the name from the AST. (cherry picked from commit 7cda418)
(cherry picked from commit 4f957fc)
…to cpp file. Just making PartitionUtils.h a little easier to walk through by moving more of the impl into the .cpp file. This reduces the header from ~1500 lines to ~950 lines which is more managable. This is especially important since I am going to be adding IsolationHistory to the header file which will expand it even further. (cherry picked from commit 901c7c2)
…the full value state of the value instead of just the representative. Just makes it easier to debug. (cherry picked from commit b19081d)
…d of "GlobalActor" for its custom global actor. Just makes it easier to read. NFC. (cherry picked from commit 3450cf6)
… of a never transferrable value, use the dynamic isolation. Do not assume it is task isolated. rdar://125372336 (cherry picked from commit 5e58aa6)
… capture by an isolated closure diagnostic to be a named diagnostic. rdar://125372790 (cherry picked from commit d0d2f2d)
…onsider it transferred if the transfer statically was to that same actor isolation. This issue can come up when a value is initially statically disconnected, but after we performed dataflow, we discovered that it was actually actor isolated at the transfer point, implying that we are not actually transferring. Example: ```swift @mainactor func testGlobalAndGlobalIsolatedPartialApplyMatch2() { var ns = (NonSendableKlass(), NonSendableKlass()) // Regions: (ns.0, ns.1), {(mainActorIsolatedGlobal), @mainactor} ns.0 = mainActorIsolatedGlobal // Regions: {(ns.0, ns.1, mainActorIsolatedGlobal), @mainactor} // This is not a transfer since ns is already main actor isolated. let _ = { @mainactor in print(ns) } useValue(ns) } ``` To do this, I also added to SILFunction an actor isolation that SILGen puts on the SILFunction during pre function visitation. We don't print it or serialize it for now. rdar://123474616 (cherry picked from commit 8243b5c)
…ely the same isolation as the transfer inst. To be more specific this means that either: 1. The use is actually isolated to the same actor. This could mean that the use is global actor isolated to the same function. 2. The use is nonisolated but is executing within a function that is globally isolated to the same isolation domain. rdar://123474616 (cherry picked from commit e36aab6)
To squelch errors, we need access to functionality not available in the unittests. The unittests do not require this functionality anyways, so just disable squelching during the unittests. (cherry picked from commit 6f66849)
gottesmm
force-pushed
the
release/6.0-region-iso
branch
from
429c24b to
dacc4b0
Compare
|
Rebased with some more stuff. Linux failure was unrelated. |
|
@swift-ci test |
|
The stat (ignoring test updates): |
hborla
approved these changes
Mar 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation: A bunch of work in region isolation that occurred after the branch point.
Issue: This is fixing a bunch of different issues. Radars:
rdar://123474616
rdar://123474616
rdar://125372790
rdar://125372336
rdar://125211624
rdar://124960994
Original PRs:
#72583
#72572
#72546
#72511
#72544
#72510
#72476
#72481
#72435
#72403
#72385
#72378
Risk: Low. Only affects region isolation.
Testing: Added many functional and unit tests.
Reviewer: N/A