Skip to content

Conversation

@ktoso
Copy link
Contributor

@ktoso ktoso commented Apr 17, 2023

Description: Pointer authentication used in swift_create_task works with closures that return a generic type and using a closure like () -> () in the way discarding task group does was causing pointer auth failures on arm64e preventing adoption of DiscardingTaskGroup on that platform. The solution is to have fix the closure signature, but just discard the returned value.
Risk: Medium, this changes the signature of the addTask APIs from () -> () to () -> T, however is source compatible. Adopters depending on ABI using a discarding group would have to be rebuilt. Code change wise, this is low risk.
Review by: @mikeash
Testing: PR testing and have manually verified the pointer auth failure goes away on arm64e
Original PR: #65220
Radar: rdar://107574868

@ktoso ktoso requested a review from a team as a code owner April 17, 2023 07:10
@ktoso ktoso added swift 5.9 🍒 release cherry pick Flag: Release branch cherry picks labels Apr 17, 2023
@ktoso
Copy link
Contributor Author

ktoso commented Apr 17, 2023

@swift-ci please test

@ktoso ktoso added the concurrency Feature: umbrella label for concurrency language features label Apr 17, 2023
@ktoso ktoso closed this Apr 17, 2023
@ktoso ktoso reopened this Apr 17, 2023
@ktoso
Copy link
Contributor Author

ktoso commented Apr 17, 2023

This actually was green already but I wrongly closed the PR...

We have now confirmed this DOES fix the issue.

@ktoso
Copy link
Contributor Author

ktoso commented Apr 17, 2023

@swift-ci please test

@ktoso ktoso requested review from hborla and rjmccall April 18, 2023 00:16
@FranzBusch
Copy link
Member

Very interesting bug @ktoso! I sadly have no idea how to fix the issue of getting signed pointers for the () -> () case but I just wanted to chime in since I don't know if we should go forward with the current proposed fix.

This changes the API of the addTask method where one can now write the following code and not get any warnings

await withDiscardingTaskGroup() { group in
    group.addTask {
        return 1 // This currently produces a warning since the return type is Void
    }
}

Would love if we could find another way to fix the arm64e issue without changing the API

@airspeedswift airspeedswift merged commit c19a4db into swiftlang:release/5.9 Apr 18, 2023
@ktoso ktoso deleted the pick-wip-discarding-ptr-auth branch April 18, 2023 21:24
@ktoso
Copy link
Contributor Author

ktoso commented Apr 19, 2023

Yeah that's unfortunate, thanks for mentioning this @FranzBusch -- I'll still see if we can do something about it perhaps; It's not a big deal to be honest though I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

concurrency Feature: umbrella label for concurrency language features 🍒 release cherry pick Flag: Release branch cherry picks swift 5.9

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants