Stream: git-wasmtime

Topic: wasmtime / PR #7626 Consolidate platform-specific definit...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 05:53):

alexcrichton opened PR #7626 from alexcrichton:consolidate-platform-definitions to bytecodealliance:main:

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:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 05:53):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 05:53):

alexcrichton requested abrown for a review on PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 05:53):

alexcrichton requested pchickey for a review on PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 05:53):

alexcrichton requested wasmtime-core-reviewers for a review on PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 05:53):

alexcrichton requested wasmtime-default-reviewers for a review on PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 06:37):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 06:43):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 17:38):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 17:40):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 18:34):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 03 2023 at 18:39):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown submitted PR review:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown submitted PR review:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown created PR review comment:

pub unsafe fn expose_existing_mapping(ptr: *mut u8, len: usize) -> io::Result<()> {

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown created PR review comment:

    source: MemoryImageSource,

Maybe? Since it seems you're doing clarity refactors in this PR...

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown created PR review comment:

// For MIRI, set up just enough of a setjmp/longjmp with catching panics

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown created PR review comment:

//! or Windows, for example. The goal of this module is to be the "single

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown created PR review comment:

I guess a future refactor could move the MPK stuff here as well?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 17:39):

abrown created PR review comment:

Why not make this a compile-time check instead, then?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:34):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:42):

alexcrichton updated PR #7626.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:44):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:44):

alexcrichton created PR review comment:

Could you elaborate? Do you mean asserting that this file isn't used on macos? (it's used on macos intentionally so wanted to clarify)

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:48):

alexcrichton commented on PR #7626:

my take is that none of this refactoring should substantially change any logic, at least observably for users; is that right?

Correct!

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:49):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 18:49):

alexcrichton created PR review 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"

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 22:53):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 22:53):

abrown created PR review comment:

Oh, sorry... I was just looking at the cfg! part but clearly machos_use_mach_ports is runtime-defined. Never mind!

view this post on Zulip Wasmtime GitHub notifications bot (Dec 04 2023 at 23:23):

abrown merged PR #7626.

Last updated: Dec 23 2024 at 12:05 UTC