Skip to content

🍒[5.9][DiscardingTG] Undo pointer auth workaround; fix memory leaks #66103

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 24, 2023

Description:
There are two leaks in the group.

"first error task leaks"
(a) the "the first error task that we RETAIN" and later on use to automatically re-throw the "first thrown by a child task error" task was not released;

"all values leak"
(b) due to the "hot fix" for pointer auth #65220 done for rdar://107574868 where we changed the signature of addTask from ()->() (what it must be), to ()->T in order to work around pointer-auth issues we introduced a leak and ALL "discarded" values would be leaking since we would not release that T value. The correct way to solve this is to undo this workaround and use correct pointer auth, which this PR does.

We must fix the method signature and undo the hack which #65223 was.

Risk: Medium; in order to fix this we must revert a signature change of the addTask parameter from ()->() to ()->T introduced by a rushed fix for a pointer auth issue done for rdar://107574868 and bring it back to the correct ()->T; Not only is the ->T causing leaks, it also is semantically wrong and we should not have done such workaround but fixed pointer auth to begin with. Pointer auth is confirmed to be correct in this PR. The ()->() API did not ship in any shipping release yet, so this is the one/only moment to make it correct.

Review by: @DougGregor @rjmccall
Testing: CI testing, verified pointer auth by building and testing on arm64e manually
Original PR: #65613
Radar: rdar://108829466

@ktoso ktoso changed the title [DiscardingTG] Undo pointer auth workaround; fix memory leaks 🍒[5.9][DiscardingTG] Undo pointer auth workaround; fix memory leaks May 24, 2023
@ktoso ktoso added distributed Feature → concurrency: distributed actor 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9 labels May 26, 2023
@ktoso
Copy link
Contributor Author

ktoso commented May 26, 2023

Included some lit+windows fix that didn't seem to be required on this branch but on main it was, so let's keep them aligned.
And a test stability improvement from #66166 since the test ended up too strictly asserting order of deinits on CI actually

@ktoso
Copy link
Contributor Author

ktoso commented May 26, 2023

@swift-ci please test and merge

@swift-ci swift-ci merged commit 74fd8ce into swiftlang:release/5.9 May 26, 2023
@ktoso ktoso deleted the pick-wip-discarding-error-task-leak branch May 26, 2023 15:32
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.9
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants