-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
iOS target support #7506
base: main
Are you sure you want to change the base?
iOS target support #7506
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! If you're ok with it though because this is a pretty small patch I'd personally recommend not merging this just yet and moving on to the next steps of trying to get code running (if you're interested in doing those next steps). I'm a bit worried about how there's a number of open questions and I find it easier to remember the questions if they're highlighted in a diff here rather than hunting around a preexisting codebase.
If you'd prefer though I also think it'd be reasonable to land ahead of time.
One suggestion I'd have is that you can probably void updating any tests at this time. I think we're probably still aways out from running tests on iOS so I think it'd be ok to cross that bridge when we get there.
@@ -115,7 +115,7 @@ pub fn infer_native_flags(isa_builder: &mut dyn Configurable) -> Result<(), &'st | |||
isa_builder.enable("has_pauth").unwrap(); | |||
} | |||
|
|||
if cfg!(target_os = "macos") { | |||
if cfg!(any(target_os = "macos", target_os = "ios")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something that I think may need to be confirmed. The comment below for example is incorrect since not all iOS devices will run on Apple Silicon, but I'm sure all recent ones do. That being said I don't know the story for return-address-signing on iOS and whether or not it's different than macOS.
if cfg!(all( | ||
any(target_os = "macos", target_os = "ios"), | ||
target_arch = "aarch64" | ||
)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also something to watch out for once you're at the point you can run some code. This block is true for macOS but iOS may very well be different in its calling convention. I'm not sure either way (or where to look it up), so it's mostly something to watch out for to see if apps crash when you load them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can see from https://developer.apple.com/documentation/xcode/writing-arm64-code-for-apple-platforms, it doesn't look like macOS and iOS have different calling conventions on arm64.
@@ -22,7 +22,7 @@ use super::wasmtime_fiber_start; | |||
use wasmtime_asm_macros::asm_func; | |||
|
|||
cfg_if::cfg_if! { | |||
if #[cfg(target_os = "macos")] { | |||
if #[cfg(any(target_os = "macos", target_os = "ios"))] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This relates to my comment about pointer authentication above, because if iOS uses the A key instead of the B key then this block would need to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at https://support.apple.com/en-ng/guide/security/sec8b776536b/1/web/1#sec0167b469d, there does not seem to be any indication there are any differences across iOS and macOS regarding which key is used, it always seems to be IB for return addresses.
@@ -178,7 +178,7 @@ unsafe extern "C" fn trap_handler( | |||
// more accurate definition. When that PR and/or issue is resolved then this | |||
// should no longer be necessary. | |||
#[repr(C)] | |||
#[cfg(all(target_arch = "aarch64", target_os = "macos"))] | |||
#[cfg(all(target_arch = "aarch64", any(target_os = "macos", target_os = "ios")))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible you'll want to use libc::ucontext_t
instead of this. This structure is one that has a high risk of being different between iOS and macOS. I posted #7507 to remove this structure definition entirely as it's not longer necessary (the PR referenced in the above comment has landed and has been published). I think you should be able to use libc::ucontext_t
for iOS, but that depends on the libc
crate which I can't say with certainty.
#[cfg(any( | ||
target_os = "linux", | ||
all(target_os = "macos", feature = "posix-signals-on-macos") | ||
all( | ||
any(target_os = "macos", target_os = "ios"), | ||
feature = "posix-signals-on-macos" | ||
) | ||
))] | ||
#[cfg(not(miri))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a heads up I've posted #7509 which will conflict with this, although what you're doing here of "where macos is needed also allow ios" would also apply to that PR.
@@ -12,7 +12,7 @@ fn lldb_with_script(args: &[&str], script: &str) -> Result<String> { | |||
let mut cmd = Command::new(&lldb_path); | |||
|
|||
cmd.arg("--batch"); | |||
if cfg!(target_os = "macos") { | |||
if cfg!(any(target_os = "macos", target_os = "ios")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's ok to skip iOS in this file, it's unlikely these tests will ever complete successfully on iOS (and they're disabled by default too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ditto for all other tests/all/debug/*.rs
files)
@alexcrichton @jmp-0x7C0 I just wanted to report that making similar changes on Even if some details, like trap and signal handling, might still need to be fully tested, it would be great to allow building wasmtime for iOS upstream. I'm happy to open a PR on top of |
@turbolent oh that's awesome, thanks for testing! If you'd like I think it would be reasonable to open an PR against |
This commit is somewhat of a rebase of bytecodealliance#7506 to port most of it to `main`. I've left out any test-related changes since we're not testing anything just yet. I've also found that rustc now has `target_vendor = "apple"` to cover both macOS and iOS targets (and presumably other targets like tvOS as well if they get added) Closes bytecodealliance#7506
As discussed with @alexcrichton on zulip, there appears to be some interest in supporting iOS as a platform. This PR makes the necessary changes for
wasmtime
to build when targetingaarch64-apple-ios
.A few notes about this PR:
wasmtime
for the iOS target, being able to run*.cwasm
modules is not yet possible. From what @alexcrichton explained to me it appears there would be some additional work would be neededto postprocess a
*.cwasm
file into a linkable object files before this can be used in an iOS application. I would be interested in taking on this future work however my understanding of what this would involve is still very limited and I would need help sketching this out in detail.macos
module in the runtime crate toapple
or similar.As this is the first time contributing, please let me know if this PR makes sense or would need any changes to align with the contributing guidelines for this project.