Wasmtime GitHub notifications bot (Apr 05 2021 at 19:03): Wasmtime GitHub notifications bot (Apr 05 2021 at 19:03):sunfishcode commented on Issue #2806:
If the wasm C ABI adopts multiple return values as a way to return small-ish structs, and if a hypothetical future Rust supports this when returning
repr(C)structs toextern "C"functions, then we could optimize this feature without user-visible API changes, right?Ah, and as a minor bikeshed, rather than "WasmtimeSystemV" and "WasmtimeFastcall", what would you think of a more explicit name, like "SingleRetSystemV" and "SingleRetFastcall" or so, to help keep it clear what its purpose is?
Wasmtime GitHub notifications bot (Apr 05 2021 at 19:21):alexcrichton commented on Issue #2806:
Can you clarify what you mean about the wasm C ABI? The current proposed wasm-c-api
wasm.hheader file provides no way to get a natively-callable function pointer which exposes ABI details, that's all internal to the engine. Currently we have no support to do so in the wasmtime extensions either afaik.For the naming bits, I would personally think that we should probably keep "wasmtime" in the name to give ourselves the liberty to easily change it in the future. For example baldrdash has a whole bunch of custom conventions and such throughout cranelift, and I could imagine us wanting to add something like that for wasmtime in the future to optimize various bits and pieces here and there.
Wasmtime GitHub notifications bot (Apr 05 2021 at 19:30):sunfishcode commented on Issue #2806:
Ah, I meant the toolchain C ABI used by clang, where we've talked about adding multi-value for small structs once it's sufficiently supported in engines.
If WasmtimeSystemV etc. are meant to adapt to other Wasmtime needs in the future, should we document that they aren't ABI-stable, similar to Fast and Cold?
Wasmtime GitHub notifications bot (Apr 05 2021 at 19:40):alexcrichton commented on Issue #2806:
Ah I see! I think, yes, we're able to optimize this in the future without any user-visible changes. We don't expose anything about the ABI details from the
wasmtimecrate today (intentionally). We're limited in that we have to be able to define all wasm signatures as mapping to a particular Rust signature (which we previously weren't able to do so), but beyond that we can do whatever we want to the internals of the ABI.And sure, I can document that these are unstable ABIs.
Wasmtime GitHub notifications bot (Apr 05 2021 at 20:02):github-actions[bot] commented on Issue #2806:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:wasm", "lightbeam", "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 06 2021 at 20:25):alexcrichton commented on Issue #2806:
Ok I think I got everything working now! Since it was a very recent change, @cfallin can you confirm you saw https://github.com/bytecodealliance/wasmtime/pull/2806/commits/95f86be431557253e41baaef0da68ccf0b3bc0fd? That was necessary to fix some segfaults that were cropping up on windows since the 4-byte return values were getting stored as 8-byte results. The comment there makes me suspicious though...
I made a stab at getting this implemented in the old backend but I ended up giving up. The old backend implements multi-value returns very differently than the new backend. The old backend spills everything to the stack if it doesn't all fit in registers, which isn't the behavior that we want here (one in registers everything else on the stack). I couldn't figure out how to update the old backend to do this but I figured it wasn't really that necessary. I just removed some trait impls for the old backend in the
wasmtimecrate sowasmtimeis still usable but won't have some abilities. (technically not a valid use of Cargo features but who's enabling the feature anyway?)I'll tag @peterhuene for the review of the wasmtime bits, thanks for looking at the Cranelift bits @cfallin!
Wasmtime GitHub notifications bot (Apr 06 2021 at 21:01):cfallin commented on Issue #2806:
Ok I think I got everything working now! Since it was a very recent change, @cfallin can you confirm you saw 95f86be? That was necessary to fix some segfaults that were cropping up on windows since the 4-byte return values were getting stored as 8-byte results. The comment there makes me suspicious though...
That was indeed a suspicious comment. I am not sure exactly what the original intent was but I double-checked just now that this
storehelper is not being used to codegen stores; those directly produce correctly-sized instructions. I suspect this originally came from spill/reload helpers in which case "always spill/reload the full reg" is a reasonable conservative approach but even then we have the invariant that upper bits (beyond a type's width) in a register are undefined and extended when needed by an op so this should be fine I think. Thanks for calling it out!
Wasmtime GitHub notifications bot (Apr 07 2021 at 15:48):alexcrichton commented on Issue #2806:
I was a little worried about the unwind info myself (and kept switching it back and forth from
Fastto the default to see if it ever fixed failing tests, it never did). In the traps tests though we do have a number of tests already with "internal" wasm function frames on the stack (such as this one) so I think we're at least covering the basics of representing unwind information for varying abis. I'm not sure if this is guaranteed to work in the limit, though.@cfallin also were you thinking of more than the tests I added in https://github.com/bytecodealliance/wasmtime/pull/2806/commits/5962d4c9118a8816afdfaa0a8a70f854d2ba60ab? I tried to break things in various ways but I think it all seems to be working pretty well so far at least.
Wasmtime GitHub notifications bot (Apr 07 2021 at 15:53):cfallin commented on Issue #2806:
@cfallin also were you thinking of more than the tests I added in 5962d4c? I tried to break things in various ways but I think it all seems to be working pretty well so far at least.
@alexcrichton That's very thorough, thanks!
I may be missing something but I don't see any
wasmtime_system_vorwasmtime_fastcallsignatures -- my main concern was the interleaving of those with externally-usable ABIs. I think it may be enough to just do awasmtime_system_v+system_vset of tests instead of the SysV/Fastcall combos you have in that file now. Thanks!
Wasmtime GitHub notifications bot (Apr 07 2021 at 17:34):alexcrichton commented on Issue #2806:
Ok I've managed to get all tests green and I think I've got all the various tests here in place. It's of course always easy though to use the same abi everywhere if any issues arise, but I'm gonna go ahead and merge this. If there are more tests desired though I'm happy to add some!
Wasmtime GitHub notifications bot (Apr 15 2021 at 18:30):bnjbvr commented on Issue #2806:
Just a note for future reference that this commit has also caused another regression on our side, causing stack overflows on Windows in async tests. I unfortunately don't have more to share, because debugging doesn't work reliably: the called wasm function panics (which is the subject of the test, so it's supposed to do so). When running in the VSCode debugger I hit SIGILL and then the program aborts, while running outside of a debugger there's a stack overflow somewhere. I haven't managed to make an isolated test case either, since I don't have any clues about what's causing the failure.
Last updated: Dec 23 2024 at 12:05 UTC