Skip to content

Inject hop_to_executor after self is fully initialized in unrestricted actor inits #38215

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jul 17, 2021
Merged

Inject hop_to_executor after self is fully initialized in unrestricted actor inits #38215

merged 6 commits into from
Jul 17, 2021

Conversation

kavon
Copy link
Member

@kavon kavon commented Jul 2, 2021

Since async actor initializers (designated ones) are allowed to make unrestricted use of self after the point where it is fully-initialized, we need to hop to the self executor after those points in order to prevent races in the init.

The design of initializer declarations in Swift means that there are multiple such fully-initialized points. To find them, you need to use a flow-sensitive analysis of stores to properties, etc.

We already do this analysis in DefiniteInitialization for classes, etc. So, this PR takes advantage of the information DI already computes to find these points and insert the hop instructions, thus plugging the data-race.

Resolves rdar://78790683

@kavon
Copy link
Member Author

kavon commented Jul 2, 2021

@swift-ci please smoke test

@kavon kavon added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features labels Jul 2, 2021
@kavon
Copy link
Member Author

kavon commented Jul 3, 2021

@swift-ci please smoke test

gottesmm
gottesmm reviewed Jul 3, 2021
View reviewed changes
gottesmm
gottesmm reviewed Jul 3, 2021
View reviewed changes
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quick structural comments.

gottesmm
gottesmm reviewed Jul 3, 2021
View reviewed changes
gottesmm
gottesmm reviewed Jul 3, 2021
View reviewed changes
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more things.

eeckstein
eeckstein reviewed Jul 5, 2021
View reviewed changes
eeckstein
eeckstein reviewed Jul 5, 2021
View reviewed changes
@kavon kavon force-pushed the flowsensitive-init-hop branch 2 times, most recently from 969a70a to 2368cb6 Compare July 7, 2021 01:09
eeckstein
eeckstein reviewed Jul 7, 2021
View reviewed changes
gottesmm
gottesmm reviewed Jul 7, 2021
View reviewed changes
Copy link
Contributor

@gottesmm gottesmm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments. Need to go through again for the algorithm.

@kavon kavon force-pushed the flowsensitive-init-hop branch 2 times, most recently from 580ff03 to 0678583 Compare July 8, 2021 02:44
@kavon
Copy link
Member Author

kavon commented Jul 8, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the flowsensitive-init-hop branch 3 times, most recently from caba62f to cfac663 Compare July 9, 2021 00:56
@kavon
Copy link
Member Author

kavon commented Jul 10, 2021

There's a complication right now: a hop_to_executor cannot appear within a begin_access/end_access pair. AFAIK its not trivial to determine where these accesses are. Might need to mark the definite init locations and then break out the injection into a follow-up pass.

For now, trying something hacky to see what breaks: blindly inject at the beginning of every async init.

@kavon
Copy link
Member Author

kavon commented Jul 10, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the flowsensitive-init-hop branch 2 times, most recently from 21f6ce0 to 2645e9d Compare July 12, 2021 22:34
atrick
atrick reviewed Jul 12, 2021
View reviewed changes
@kavon kavon force-pushed the flowsensitive-init-hop branch 5 times, most recently from 5d3d04a to 4ec1d69 Compare July 14, 2021 01:00
@kavon kavon force-pushed the flowsensitive-init-hop branch from 4ec1d69 to ba2b0ee Compare July 14, 2021 01:12
@kavon kavon marked this pull request as ready for review July 14, 2021 01:13
@kavon
Copy link
Member Author

kavon commented Jul 14, 2021

@swift-ci please test

atrick
atrick reviewed Jul 14, 2021
View reviewed changes
atrick
atrick reviewed Jul 14, 2021
View reviewed changes
atrick
atrick reviewed Jul 14, 2021
View reviewed changes
Copy link
Contributor

@atrick atrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me (as a way to hide the problem for now). Is there value in adding the minimal reproducer as a runtime test?

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - ba2b0ee32b9afbee3b34a59ddfa7a9a4c8999c33

@kavon kavon force-pushed the flowsensitive-init-hop branch from ba2b0ee to 4f64054 Compare July 14, 2021 19:26
@kavon
Copy link
Member Author

kavon commented Jul 14, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the flowsensitive-init-hop branch 3 times, most recently from aecd101 to 6b56d60 Compare July 15, 2021 02:23
@kavon
Copy link
Member Author

kavon commented Jul 15, 2021

@swift-ci please smoke test and merge

@kavon kavon mentioned this pull request Jul 16, 2021
Merged
kavon added 6 commits July 16, 2021 15:01
@kavon
add assertion to catch AccessSet usage errors
70b7b34 
I ran into this while implementing
async actor inits; where I was emitting
an actor hop in the middle of
an access region, which caused the
access set to be unexpectedly empty.
@kavon
inject hop_to_executor in async actor inits
d23858f 
Because `self` can be used unrestricted after it is
fully-initialized in an async actor init, we perform
a hop_to_executor(self) immediately after `self`
is fully-initialized on all paths within the
initializer.

If we did not, then code could race with the
initializer if it, e.g., spawns a task from
the initializer and mutates actor state that
the initializer then reads. By hopping to the
executor, we prevent that task from running
until _after_ the initializer completes.
@kavon
insert hops after end_access when possible
1e54503
@kavon
SIL test for hop_to_executor in async actor inits
336ff2d
@kavon
runtime regression test for async actor init
c481749
@kavon
more coverage for SILGen async_initializer test
ddf041f
@kavon kavon force-pushed the flowsensitive-init-hop branch from 6b56d60 to ddf041f Compare July 16, 2021 22:01
@kavon
Copy link
Member Author

kavon commented Jul 16, 2021

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 4537499 into swiftlang:main Jul 17, 2021
@kavon kavon mentioned this pull request Jul 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gottesmm gottesmm gottesmm left review comments

@atrick atrick atrick left review comments

@eeckstein eeckstein eeckstein left review comments

Assignees
No one assigned
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kavon @swift-ci @gottesmm @atrick @eeckstein