Skip to content

🍒[5.7-04182022][Distributed] Force the order of properties in AST #58748

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

ktoso
Copy link
Contributor

@ktoso ktoso commented May 8, 2022

Issue Summary:

  • Using the LocalTestingDistributedActorSystem crashes without this fix.
  • In general, using distributed actor systems declared in library evolution mode compiled modules would fail

Original PR: #58745
Radar:

  • rdar://92712849 - the root cause of all those issues
  • rdar://92910719 - which disabled the still failing test since the workaround was not good enough

Risk: Low, no impact on adopters

Details:

This is the actual solution for all our mystical offset issues -- the order of the synthesized AST fields MUST match the order IRGen enforces (and the DA needs in any case); where the id and system are the FIRST fields, and must be specifically: id, system because that's what IRGen emits and many places seem to expect the order matches between the original AST and whatever gets derived form it and IR.

This probably also resolves a few other crashers we had with similar crash reasons of weird mismatching offsets, I'll be verifying those.

@ktoso ktoso requested a review from a team as a code owner May 8, 2022 10:56
@ktoso ktoso requested review from xedin and DougGregor May 8, 2022 10:56
@ktoso ktoso changed the title [Distributed] Force the order of properties in AST 🍒[5.7-04182022][Distributed] Force the order of properties in AST May 8, 2022
@ktoso
Copy link
Contributor Author

ktoso commented May 8, 2022

@swift-ci please test

@ktoso ktoso added the distributed Feature → concurrency: distributed actor label May 8, 2022
@ktoso ktoso added this to the Swift 5.7 milestone May 8, 2022
@ktoso ktoso added the r5.7 label May 8, 2022
@ktoso ktoso force-pushed the pick-pick-wip-init-crash-fix branch from 49b5776 to fee52c6 Compare May 9, 2022 07:29
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

Sigh... this patch fixed it properly on main but seems something else needs to be cherry picked to 5.7 for the fix to work on watchsimulator-i386 - this is the only failure right now. Detective time... 🕵️

@ktoso ktoso force-pushed the pick-pick-wip-init-crash-fix branch from 2062aa9 to de8486a Compare May 9, 2022 12:12
@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please test

@ktoso
Copy link
Contributor Author

ktoso commented May 9, 2022

@swift-ci please nominate

…t will always be synthesized or present already
@ktoso
Copy link
Contributor Author

ktoso commented May 10, 2022

@swift-ci please test and merge

@swift-ci swift-ci merged commit 97d0487 into swiftlang:release/5.7-04182022 May 10, 2022
@ktoso ktoso deleted the pick-pick-wip-init-crash-fix branch May 10, 2022 13:12
@AnthonyLatsis AnthonyLatsis added 🍒 release cherry pick Flag: Release branch cherry picks swift 5.7 labels Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distributed Feature → concurrency: distributed actor 🍒 release cherry pick Flag: Release branch cherry picks swift 5.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants