Consolidate platform-specific definitions in Wasmtime #7626
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit extends the gating behavior of the preexisting `debug-builtins` Cargo feature to cover more GDB-related functionality associated with debugging. This can additionally slim down the set of exposed symbols from Wasmtime over the default with them included.
This commit adds a `timing` feature to the `cranelift-codegen` crate which is enabled by default. This feature gates the timing functionality in Cranelift to enable turning it off if desired. The goal of this commit is to remove a system dependency on `Instant` for possibly esoteric environments.
Prior to this commit Wasmtime did not not have a style or system for containing platform-specific logic in files. The goal of this commit is to consolidate all platform-specific functionality into one location to make it easier to port Wasmtime to new systems. This commit creates a `sys` module within the `wasmtime-runtime` crate which conditionally defines all of the platform support that Wasmtime requires, namely things related to virtual memory management and trap handling. Many of the previous `unix.rs` files interspersed throughout the tree are now all located in a single `unix` directory. This additionally helps split out `miri`-specific functionality by pretending `miri` is its own platform. This change additionally goes through `#[cfg]` directives throughout `wasmtime-runtime`, `wasmtime-jit`, and `wasmtime` itself to place all of this target-specific functionality within this `sys` directory. The end state is that there are two new top-level modules in the `wasmtime-runtime` crate: * `arch` - this conditionally defines architecture-specific logic such as the state used by backtraces, libcalls, etc. * `sys` - this conditionally defines OS or platform-specific logic such as virtual memory management. One secondary goal of this commit is to enable the ability to easily add new platforms to Wasmtime, even if it's in a fork of Wasmtime. Previously patches might have to touch a good number of locations where now they'd ideally only have to touch `sys/mod.rs` to declare a new platform and `sys/$platform/*.rs` to define all the functionality.
alexcrichton
requested review from
abrown and
pchickey
and removed request for
a team
github-actions
bot
added
the
cranelift
abrown
approved these changes
Dec 4, 2023
There was a problem hiding this comment.
I think this is good to go. The idea of centralizing all the platform-specific bits into one place is a good one. @alexcrichton, my take is that none of this refactoring should substantially change any logic, at least observably for users; is that right?
| @@ -47,6 +88,7 @@ macro_rules! wasm_to_libcall_trampoline { | |||
| ); | |||
| }; | |||
| } | |||
| pub(crate) use wasm_to_libcall_trampoline; | |||
|
|
|||
| #[cfg(test)] | |||
| mod wasm_to_libcall_trampoline_offsets_tests { | |||
There was a problem hiding this comment.
I guess a future refactor could move the MPK stuff here as well?
There was a problem hiding this comment.
True! I forgot to go back and handle that one as well.
The disabled.rs file would be used on Windows/MIRI/non-x86_64. Some bits would probably go into arch like pkru.rs and others would go into unix/mpk.rs perhaps where non-Linux systems would use disabled.rs and Linux would use the "real version"
Correct! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this commit Wasmtime did not not have a style or system for
containing platform-specific logic in files. The goal of this commit is
to consolidate all platform-specific functionality into one location to
make it easier to port Wasmtime to new systems. This commit creates a
sysmodule within thewasmtime-runtimecrate which conditionallydefines all of the platform support that Wasmtime requires, namely
things related to virtual memory management and trap handling. Many of
the previous
unix.rsfiles interspersed throughout the tree are nowall located in a single
unixdirectory. This additionally helps splitout
miri-specific functionality by pretendingmiriis its ownplatform.
This change additionally goes through
#[cfg]directives throughoutwasmtime-runtime,wasmtime-jit, andwasmtimeitself to place allof this target-specific functionality within this
sysdirectory. Theend state is that there are two new top-level modules in the
wasmtime-runtimecrate:arch- this conditionally defines architecture-specific logic suchas the state used by backtraces, libcalls, etc.
sys- this conditionally defines OS or platform-specific logic suchas virtual memory management.
One secondary goal of this commit is to enable the ability to easily
add new platforms to Wasmtime, even if it's in a fork of Wasmtime.
Previously patches might have to touch a good number of locations where
now they'd ideally only have to touch
sys/mod.rsto declare a newplatform and
sys/$platform/*.rsto define all the functionality.