-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Concurrency] Correct memory effect attributes of task_create #82179
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
[Concurrency] Correct memory effect attributes of task_create #82179
Conversation
|
@swift-ci please smoke test |
Without this, llvm would sometimes wrongly assume there's no indirect accesses and the optimizations can lead to a runtime crash, by optimizing away initializing options properly. Resolves rdar://152548190
73b6746 to
3aa28b4
Compare
|
@swift-ci please smoke test |
| name: String? = nil, | ||
| priority: TaskPriority? = nil, | ||
| % # NOTE: This closure cannot be 'sending' because we'll trigger ' pattern that the region based isolation checker does not understand how to check' | ||
| % # In this case: `func syncOnMyGlobalActor() { Task.immediate { @MyGlobalActor in } }` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we actually did adopt sending here already, so we can remove the comment about needing to do so when a bug was fixed
| // 69: - async work on queue | ||
| // 71: Inner thread id = 0x00007000054f5000 | ||
| // 72: Current thread id = 0x000070000567e000 | ||
| // 73: inside immediate, done [thread:0x000070000567e000] @ :414 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to keep this in tree, was debug information
| Task.immediate(name: "hello") { @MainActor in print("Task.immediate { @MainActor } (\(Task.name!))") } | ||
| Task.immediate() { @MainActor in print("Task.immediate { @MainActor }") } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
testing both with names and not, to be "extra" sure these don't regress somehow
fhahn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me, IIUC it accesses memory via pointers loaded from the arguments, hence argmemonly is not correct.
rjmccall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. It'd be nice to have a weaker version of argmemonly that only allows accesses to memory reachable from arguments, but argmemonly is quite clear that it is not that.
|
@swift-ci please smoke test |
|
Thanks for review folks |
Without this, llvm would sometimes wrongly assume there's no indirect
accesses and the optimizations can lead to a runtime crash, by
optimizing away initializing options properly.
Resolves rdar://152548190