Skip to content

[5.5] Fix SILGen of access to isolated static properties #38190

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

Merged

Conversation

kavon
Copy link
Member

@kavon kavon commented Jul 1, 2021

Explanation: Accesses to actor-isolated static properties, such as those isolated to the MainActor, were being miscompiled, which led to missing synchronization and "hopping" to the main thread.
Risk: Low.
Original PRs: #38114
Radar/SR Issue: rdar://78292384
Reviewed By: Konrad Malawski
Testing: Regression tests are included

kavon added 2 commits June 30, 2021 17:13
When accessing a static property isolated to a
global-actor, such as MainActor, a `hop_to_executor`
was not being emitted. This was caught by an assertion
I had left to catch such cases, but of course in release
builds this will lead to incorrect SIL, etc.

This issue revealed some other problems with how
the implementation was done for other kinds of
accesses starting from a static property, e.g.,
emitting a redundant hop for the same access.
This was fixed by modelling the actor-isolation
placed into a component as only being accessible
by consuming it from the component. This prevents
the emission of the hops more than once.

Thus, the regression test was updated to catch
unexpected hop_to_executor instructions.

Resolves rdar://78292384
Added an implicit CHECK-NOT to catch any unexpected
hop_to_executor instructions in the output of SILGen.
@kavon kavon added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. r5.5 concurrency Feature: umbrella label for concurrency language features labels Jul 1, 2021
@kavon kavon requested a review from a team as a code owner July 1, 2021 01:36
@kavon
Copy link
Member Author

kavon commented Jul 1, 2021

@swift-ci please test

Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Reviewed this before on main, looks good and is important to fix, LGTM 👍

@swift-ci
Copy link
Contributor

swift-ci commented Jul 1, 2021

Build failed
Swift Test Linux Platform
Git Sha - b0ec79c

@kavon
Copy link
Member Author

kavon commented Jul 1, 2021

@swift-ci please test Linux platform

@kavon
Copy link
Member Author

kavon commented Jul 1, 2021

@swift-ci please nominate

@swift-ci
Copy link
Contributor

swift-ci commented Jul 1, 2021

Build failed
Swift Test Linux Platform
Git Sha - b0ec79c

@kavon
Copy link
Member Author

kavon commented Jul 2, 2021

@swift-ci please clean test and merge

@kavon
Copy link
Member Author

kavon commented Jul 2, 2021

@swift-ci please clean test linux platform

@kavon kavon merged commit c2567ca into swiftlang:release/5.5 Jul 2, 2021
@kavon kavon deleted the 5.5-isolated-static-property-silgen branch July 2, 2021 16:50
@AnthonyLatsis AnthonyLatsis added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.5 labels Jan 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features 🍒 release cherry pick Flag: Release branch cherry picks swift 5.5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants