Skip to content
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

Replace mach dependency with mach2 #8049

Merged
merged 7 commits into from
Mar 8, 2024

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Mar 5, 2024

Rebase of #6164 with added audit.

Fixes #6000
Fixes #6164
Fixes #8044

@fitzgen fitzgen requested review from a team as code owners March 5, 2024 16:20
@fitzgen fitzgen requested review from alexcrichton and cfallin and removed request for a team and alexcrichton March 5, 2024 16:20
@@ -1735,6 +1735,15 @@ dependencies = [
"libc",
]
Copy link
Contributor

Choose a reason for hiding this comment

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

The region crate used by cranelift-jit still depends on mach.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine, its not a dependency of wasmtime, and we can snip that dependency in a follow up PR if desired.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the region PR for moving to mach2 has been open for several months: darfink/region-rs#27 And worse the last release has been from over 2 years ago and once the current version gets released, it will lock the libc crate to version 0.2.107 from 2 years ago (for OpenBSD 6.9 support, which has been EOL since May 2022. libc presumably dropped support for it because it is EOL)

Copy link
Member

@cfallin cfallin left a comment

Choose a reason for hiding this comment

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

Thanks!

@cfallin cfallin added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@fitzgen fitzgen enabled auto-merge March 5, 2024 17:16
@fitzgen fitzgen added this pull request to the merge queue Mar 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 5, 2024
@fitzgen
Copy link
Member Author

fitzgen commented Mar 5, 2024

@cfallin would you be up for fixing the aarch64 macOS build here? The last commit was too aggressive, and seems to have removed things that are still needed in mach2. The problem is I can't even do cargo check --target ... because I can't run the build scripts on my machine.

@cfallin
Copy link
Member

cfallin commented Mar 5, 2024

I can take a look, sure.

@cfallin
Copy link
Member

cfallin commented Mar 5, 2024

Pushed a fix; with this it cargo checks on macOS/aarch64. There's some weird inconsistency between the exception_behavior_t (a c_int) and the constants for the parameter (defined as c_uints); got a try_into/unwrap for now.

@cfallin
Copy link
Member

cfallin commented Mar 5, 2024

Ah, but with that there is a panic at that unwrap during a cargo test. Looking further.

@cfallin
Copy link
Member

cfallin commented Mar 5, 2024

OK, so the issue (one issue anyway) is that MACH_EXCEPTION_CODES is defined as 0x8000_0000, which works great for its u32 type, but not the i32 type behind exception_behavior_t. I had hoped this was just a C-ism and tried an as cast to YOLO the signedness; that now runs into a SIGABRT on cargo test. This is officially Above My macOS Knowledge and needs someone else to look at it, I think!

@alexcrichton
Copy link
Member

Testing locally I'm seeing something that looks like it's running into this comment, namely the bindings for __Request__exception_raise_t are not aligned. I don't know if the bindings are right or what's going on here, but I vaguely remember seeing in the past another project define the type locally and use 64-bit values as well.

@alexcrichton
Copy link
Member

Notably the mach2 crate uses integer_t there, which is i32, not i64.

@alexcrichton
Copy link
Member

I've pushed a commit which passes tests locally by copying over the copy of the struct that we have today and additionally queues up all tests to run in CI.

@alexcrichton alexcrichton added this pull request to the merge queue Mar 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 8, 2024
@alexcrichton alexcrichton added this pull request to the merge queue Mar 8, 2024
Merged via the queue into bytecodealliance:main with commit 556fe42 Mar 8, 2024
43 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.

dependency on unmaintained mach crate RUSTSEC-2020-0168: mach is unmaintained
5 participants