Stream: git-wasmtime

Topic: wasmtime / PR #1269 [wasi-common]: add armv7 support to w...

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 12:31):

kubkon opened PR #1269 from wasi-common-armv7 to master:

This commit enables target_pointer_width = 32 compatibility for wasi-common (and by transitivity, any crate found inside, e.g., yanix). I've also added a simplistic (bare minimum) check to our CI to ensure that wasi-common cross-compiles to armv7-unknown-gnueabihf fine. While here, I've done the same for wasm32-unknown-emscripten.

To avoid code duplication, I've added a custom ext trait FileTimeExt which implements seconds and nanoseconds accessors for with different impls for different pointer widths. I'm not sure whether that's the cleanest approach but definitely seemed like it at the time. Lemme know what you reckon!

Fixes #1219

cc @stefson hopefully this fixes it for you :-)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 12:31):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1269.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 12:31):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1269.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:05):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:05):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:05):

sunfishcode created PR Review Comment:

Actually, seeing this written out reminds me that some 32-bit targets are transitioning to a 64-bit time_t and making their tv_sec a time_t to fix the 2038 bug. So what we could do instead here would be to change the Result<i32> here to a Result<libc::time_t>, and that should work for both 32-bit and 64-bit targets. It can still use try_into, but it should get optimized away in the case where time_t is 64-bit.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:05):

sunfishcode created PR Review Comment:

We should use libc::c_long here. Or even better, can we use libc::UTIME_NOW and libc::UTIME_OMIT instead of defining our own copies?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:05):

sunfishcode created PR Review Comment:

For nanoseconds, the filetime library documents that the nanoseconds value is always less than 1 billion, so any value it returns should be convertible to i32, so we can just .try_into().unwrap().

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:59):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:59):

kubkon created PR Review Comment:

That's a good point actually. As far as I can remember, the main reason why we'd define them ourselves was to support older libc crates where those constants were incorrectly defined for some targets (netbsd and macos are the awkward ones among others). Given that they are now fixed in libc, I'm happy to reuse their definitions instead :+1:

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:59):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:59):

kubkon created PR Review Comment:

Good catch, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:59):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 13:59):

kubkon created PR Review Comment:

Excellent!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 14:00):

kubkon updated PR #1269 from wasi-common-armv7 to master:

This commit enables target_pointer_width = 32 compatibility for wasi-common (and by transitivity, any crate found inside, e.g., yanix). I've also added a simplistic (bare minimum) check to our CI to ensure that wasi-common cross-compiles to armv7-unknown-gnueabihf fine. While here, I've done the same for wasm32-unknown-emscripten.

To avoid code duplication, I've added a custom ext trait FileTimeExt which implements seconds and nanoseconds accessors for with different impls for different pointer widths. I'm not sure whether that's the cleanest approach but definitely seemed like it at the time. Lemme know what you reckon!

Fixes #1219

cc @stefson hopefully this fixes it for you :-)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 14:01):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1269.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:42):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:42):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:42):

alexcrichton created PR Review Comment:

Could we avoid having this #[cfg]? It seems like it'd be best to just always have the try_into().unwrap(), and that I think means we could also remove the _checked from the name, right?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:42):

alexcrichton created PR Review Comment:

Is the #[cfg] here necessary?

I'd love to have as few #[cfg] as necessary and it seems like we'd want this logic on all platforms?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:42):

alexcrichton created PR Review Comment:

Since this likely executes pretty quickly I don't think we necessarily need to have two whole builders for this, could these be collapsed perhaps into a single "check all the targets" builder?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:43):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:43):

kubkon created PR Review Comment:

Agreed! You beat me to it actually! I was just reworking this bit as it seems silly to have unwrap wrapped in Result :-D

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:45):

kubkon submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:45):

kubkon created PR Review Comment:

Agreed about #[cfg]s.

Hmm, do you reckon we could collapse both from_raw versions into one with try_into?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:45):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:45):

sunfishcode submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:45):

sunfishcode created PR Review Comment:

We can take this even further: tv_nsec is always a libc::c_long, so we can return that for both 32-bit and 64-bit platforms and avoid the target_pointer_width checks, and since the check always succeeds, we can just return the value instead of a Result.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:45):

sunfishcode created PR Review Comment:

And now that I think about it, we can do a similar thing here -- just have one version for both 32-bit and 64-bit platforms, which returns a Result<libc::c_long>, and we can rely on the compiler to optimize away the map_err in the case where c_long is 64-bit.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:48):

alexcrichton created PR Review Comment:

Ah yeah @sunfishcode put it well here

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 15:48):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 16:05):

kubkon updated PR #1269 from wasi-common-armv7 to master:

This commit enables target_pointer_width = 32 compatibility for wasi-common (and by transitivity, any crate found inside, e.g., yanix). I've also added a simplistic (bare minimum) check to our CI to ensure that wasi-common cross-compiles to armv7-unknown-gnueabihf fine. While here, I've done the same for wasm32-unknown-emscripten.

To avoid code duplication, I've added a custom ext trait FileTimeExt which implements seconds and nanoseconds accessors for with different impls for different pointer widths. I'm not sure whether that's the cleanest approach but definitely seemed like it at the time. Lemme know what you reckon!

Fixes #1219

cc @stefson hopefully this fixes it for you :-)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 16:09):

kubkon updated PR #1269 from wasi-common-armv7 to master:

This commit enables target_pointer_width = 32 compatibility for wasi-common (and by transitivity, any crate found inside, e.g., yanix). I've also added a simplistic (bare minimum) check to our CI to ensure that wasi-common cross-compiles to armv7-unknown-gnueabihf fine. While here, I've done the same for wasm32-unknown-emscripten.

To avoid code duplication, I've added a custom ext trait FileTimeExt which implements seconds and nanoseconds accessors for with different impls for different pointer widths. I'm not sure whether that's the cleanest approach but definitely seemed like it at the time. Lemme know what you reckon!

Fixes #1219

cc @stefson hopefully this fixes it for you :-)

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 16:09):

kubkon requested alexcrichton for a review on PR #1269.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 16:09):

kubkon requested alexcrichton, and sunfishcode for a review on PR #1269.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 16:15):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 10 2020 at 18:19):

kubkon merged PR #1269.

Last updated: Dec 23 2024 at 12:05 UTC