Skip to content

[Runtime] Add function_cast, switch from std::bit_cast. #80516

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
merged 1 commit into from
Apr 11, 2025

Conversation

mikeash
Copy link
Contributor

@mikeash mikeash commented Apr 4, 2025

Function types aren't always trivially copyable, e.g. with address-discriminated signed pointers on ARM64e. Introduce a function_cast helper and use that instead.

@bnbarham
Copy link
Contributor

bnbarham commented Apr 4, 2025

@swift-ci please test

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

This adds explicit UB rather than implicit UB. The std::bit_cast is absolutely required even if it doesn’t work on arm64e. We should instead be stripping pointer signing and resigning after the copy for ptrauth enabled environments. That would allow this to work on all Von Neumann architectures.

The implicit UB that we form in the std::bit_cast approach is for Harvard architectures or more complex memory architectures such as GPU, which may have multiple address spaces. In both those scenarios, the pointer itself is inconvertible and we would form something which is invalid.

@mikeash
Copy link
Contributor Author

mikeash commented Apr 4, 2025

Manually stripping and re-signing would be pretty annoying and error-prone. What if we implement function_cast in terms of bit_cast when there's no ptrauth, and implement it with reinterpret_cast only when __has_feature(ptrauth_calls)? Or to make the condition directly address the problem, only when the types are not is_trivially_copyable.

@rjmccall
Copy link
Contributor

rjmccall commented Apr 4, 2025

Stripping and resigning is not allowed; pointer authentication is, in fact, actually supposed to do something.

@mikeash
Copy link
Contributor Author

mikeash commented Apr 4, 2025

Right, we'd really want to auth and re-sign. But doing that manually is still not an attractive prospect.

@mikeash
Copy link
Contributor Author

mikeash commented Apr 4, 2025

Seems that it's even simpler. The ptrauth annotation drops off the source once we're inside function_cast, so we can implement it as just an unconditional wrapper around std::bit_cast. I've updated the PR with this approach.

@mikeash
Copy link
Contributor Author

mikeash commented Apr 7, 2025

@compnerd Are you good with this new approach?

/// doesn't work on ARM64e with address-discriminated signed function types.
template <typename Destination, typename Source>
Destination function_cast(Source source) {
// Ptrauth attributes decay away here, so we can now bit_cast in peace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Ptrauth attributes decay away here, so we can now bit_cast in peace.
// ptrauth attributes decay away here, so we can now bit_cast in peace.

#ifndef SWIFT_BASIC_CASTING_H
#define SWIFT_BASIC_CASTING_H

#include "STLCompatibility.h"
Copy link
Member

Choose a reason for hiding this comment

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

Can we inline the std::bit_cast implementation for compatibility with older C++ standards?

@mikeash
Copy link
Contributor Author

mikeash commented Apr 10, 2025

@swift-ci please test

@mikeash mikeash enabled auto-merge April 10, 2025 13:42
@mikeash
Copy link
Contributor Author

mikeash commented Apr 11, 2025

@swift-ci please smoke test macos platform

Function types aren't always trivially copyable, e.g. with address-discriminated signed pointers on ARM64e. Introduce a function_cast helper and use that instead.
@mikeash
Copy link
Contributor Author

mikeash commented Apr 11, 2025

@swift-ci please test

@mikeash mikeash merged commit 433ca8e into swiftlang:main Apr 11, 2025
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants