Stream: git-wasmtime

Topic: wasmtime / PR #1620 Implement basic support for the Linux...

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2020 at 13:35):

whitequark opened PR #1620 from x86_32-mvp to master:

This PR, together with #1612, #1615 and #1616 makes it possible to run simple WASI binaries on x86_32 Linux. There are some caveats:

* Because of the approach taken in #1612, it is necessary to always use --enable-simd.
* There are still some issues in the 32-bit legalizer that prevent complex applications from running.
* Debug information is not generated.

Other than that it should be fully functional, or at least I don't see anything obviously broken.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2020 at 14:08):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2020 at 14:08):

bjorn3 created PR Review Comment:

ArgumentPurpose::Normal is not correct.

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

bjorn3 submitted PR Review.

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

bjorn3 created PR Review Comment:

You should use pos.func.stack_slots.make_incoming_arg(types::I32, offset) and then load from the resulting stack slot.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 28 2020 at 14:11):

bjorn3 edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 03:42):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 03:42):

whitequark created PR Review Comment:

Does that actually work in the prologue itself?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 04:09):

iximeow submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 04:09):

iximeow created PR Review Comment:

Ish. Stack slots are handled correctly in x86 after rsp's final adjustment, even though there may be a little more prologue afterward (for example, x86_64 XMM preservation relies on this). It looks like this is too early for an explicit stack_load here.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 04:15):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 04:15):

whitequark created PR Review Comment:

That was my understanding too, which is why I spent several hours figuring out a solution that does not involve a stack_load.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 04:15):

whitequark created PR Review Comment:

I agree. What would be appropriate here, assuming stack_load indeed isn't, as discussed below?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 04:15):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 06:54):

bjorn3 submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 29 2020 at 06:54):

bjorn3 created PR Review Comment:

Maybe use fill? That one takes the SSA var stored on stack and outputs it ib a register.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2020 at 04:53):

whitequark created PR Review Comment:

@bjorn3 fill has the exact same problem as stack_load, no? It also uses stack slots, which is what I am avoiding in this code.

view this post on Zulip Wasmtime GitHub notifications bot (May 02 2020 at 04:53):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 05 2020 at 21:21):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 05 2020 at 21:21):

whitequark created PR Review Comment:

@iximeow Could you advise on any workable way to implement this operation? I'll be happy to implement whatever the upstream likes here. I think I could use a stack_load with an ad-hoc fixup if that's considered OK, though that looks pretty gross to me personally.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 01:42):

iximeow submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 01:42):

iximeow created PR Review Comment:

I think this is the workable approach, since stack slot machinery isn't usable this early in the function. I agree that stack_load with some later fixup would not be better.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:02):

iximeow submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:02):

iximeow submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:02):

iximeow created PR Review Comment:

This is to read the item before the return address, right? That happens to be the leftmost argument and VMContext typically, but I think this ought to be

let offset = offset + i32::from(pos.isa.pointer_bytes() * (1 + vmctx_index))
``` to be correct if VMContext happens to be some later argument.
~~~

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:02):

iximeow created PR Review Comment:

I'd give ArgumentPurpose::CalleeSaved a shot here, this is well after register allocation so it's probably free of risk in side effects there. The primary benefit of being descriptive here is to be clear this isn't an argument from the original function somehow ending up in esp, I don't think the ArgumentPurpose at this point would have any actual effect.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:02):

iximeow created PR Review Comment:

                    // The following access can be marked `trusted` because it is a load of an argument. We
                    // know it is safe because it was safe to write it in preparing this function call.
                    let ret =

Just because while we can be quite confident it's a trusted access, it might not be immediately obvious to the reader.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:14):

whitequark updated PR #1620 from x86_32-mvp to master:

This PR, together with #1612, #1615 and #1616 makes it possible to run simple WASI binaries on x86_32 Linux. There are some caveats:

* Because of the approach taken in #1612, it is necessary to always use --enable-simd.
* There are still some issues in the 32-bit legalizer that prevent complex applications from running.
* Debug information is not generated.

Other than that it should be fully functional, or at least I don't see anything obviously broken.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:15):

whitequark created PR Review Comment:

You're being kind heh, I'm pretty sure I, uh, just copy-and-pasted that trusted bit without ever stopping to give it any thought.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:15):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:18):

whitequark updated PR #1620 from x86_32-mvp to master:

This PR, together with #1612, #1615 and #1616 makes it possible to run simple WASI binaries on x86_32 Linux. There are some caveats:

* Because of the approach taken in #1612, it is necessary to always use --enable-simd.
* There are still some issues in the 32-bit legalizer that prevent complex applications from running.
* Debug information is not generated.

Other than that it should be fully functional, or at least I don't see anything obviously broken.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:18):

whitequark submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:18):

whitequark created PR Review Comment:

Good catch, fixed

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:19):

whitequark updated PR #1620 from x86_32-mvp to master:

This PR, together with #1612, #1615 and #1616 makes it possible to run simple WASI binaries on x86_32 Linux. There are some caveats:

* Because of the approach taken in #1612, it is necessary to always use --enable-simd.
* There are still some issues in the 32-bit legalizer that prevent complex applications from running.
* Debug information is not generated.

Other than that it should be fully functional, or at least I don't see anything obviously broken.

view this post on Zulip Wasmtime GitHub notifications bot (May 06 2020 at 02:22):

iximeow edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 08 2020 at 05:42):

whitequark updated PR #1620 from x86_32-mvp to master:

This PR, together with #1612, #1615 and #1616 makes it possible to run simple WASI binaries on x86_32 Linux. There are some caveats:

* Because of the approach taken in #1612, it is necessary to always use --enable-simd.
* There are still some issues in the 32-bit legalizer that prevent complex applications from running.
* Debug information is not generated.

Other than that it should be fully functional, or at least I don't see anything obviously broken.

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

iximeow submitted PR Review.

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

iximeow merged PR #1620.

Last updated: Nov 22 2024 at 16:03 UTC