Stream: git-wasmtime

Topic: wasmtime / PR #9710 Migrate libcalls to checking for traps

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:48):

alexcrichton requested abrown for a review on PR #9710.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:48):

alexcrichton opened PR #9710 from alexcrichton:libcall-manual-trap to bytecodealliance:main:

This PR is the equivalent of https://github.com/bytecodealliance/wasmtime/pull/9675 for libcalls used in core wasm and components.
All libcalls now communicate whether or not they trapped through their
return value instead of implicitly calling longjmp to exit from the
libcall. This is to make integration with Pulley easier to avoid the
need to longjmp over Pulley execution.

Libcall definitions have changed where appropriate and the
catch_unwind_and_record_trap function introduced in https://github.com/bytecodealliance/wasmtime/pull/9675 was
refactored to better support multiple types of values being returned
from libcalls (instead of just Result<()>).

Note that changes have been made to both the Cranelift translation layer
and the Winch translation layer for this as the ABI of various libcalls
are all changing.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:48):

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

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:48):

alexcrichton requested pchickey for a review on PR #9710.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:48):

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

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:49):

alexcrichton created PR review comment:

@saulecabrera I'm particularly curious to get your eyes on this file and the changes I made here. I had to make this change to keep things working because fuel checks now involve calling 2 libcalls instead of 1 and the second one tried to reload the VMContext before it was saved, so I pushed this below the spill_register_arguments below.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:50):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:50):

alexcrichton created PR review comment:

Similarly @saulecabrera I had to move this up since fuel_var was assigned rax but rax was needed as the return value of the hostcall, so this was needed to fix some panics during compilation.

Wasmtime GitHub notifications bot (Dec 02 2024 at 22:53):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 02 2024 at 23:08):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 03 2024 at 01:05):

github-actions[bot] commented on PR #9710:

Subscribe to Label Action

cc @fitzgen

<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"

Thus the following users have been cc'd because of the following labels:

• fitzgen: wasmtime:ref-types

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 (Dec 04 2024 at 01:23):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 04 2024 at 01:25):

alexcrichton requested fitzgen for a review on PR #9710.

Wasmtime GitHub notifications bot (Dec 04 2024 at 01:33):

sunfishcode commented on PR #9710:

Does this significantly change overall code size of Cranelift-generated code, due to needing inline trap check code at each libcall callsite?

Wasmtime GitHub notifications bot (Dec 04 2024 at 15:44):

alexcrichton commented on PR #9710:

Yes as-is all libcalls that can trap have a check after them, so it's inflating code by at least that much.

In thinking though this can be pretty easily optimized. We already generate a trampoline-per-libcall so I'll move the check-for-trap logic to the trampoline instead of the callsite of the trampoline.

Wasmtime GitHub notifications bot (Dec 04 2024 at 16:52):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 04 2024 at 16:53):

alexcrichton commented on PR #9710:

Ok I've updated this where the trampoline itself now checks for a trap, so the code size impact on wasm itself should be minimal.

Wasmtime GitHub notifications bot (Dec 04 2024 at 16:54):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 04 2024 at 17:49):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 05 2024 at 16:56):

fitzgen submitted PR review:

LGTM modulo one comment below

Wasmtime GitHub notifications bot (Dec 05 2024 at 16:56):

fitzgen created PR review comment:

Wait, we should only ever have i32 values in stack maps, but this will make it include an i64 that represents a Result<u32> IIUC, so this change looks pretty suspect to me. Is this intentional?

Wasmtime GitHub notifications bot (Dec 05 2024 at 16:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 16:58):

alexcrichton created PR review comment:

Oh I thought that the ireduce preceding this would take care of that, but is that not the case?

Wasmtime GitHub notifications bot (Dec 05 2024 at 18:19):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 05 2024 at 18:19):

fitzgen created PR review comment:

Ah whoops I misread this as moving the ireduce after the needs-stack-map declaration. Carry on!

Wasmtime GitHub notifications bot (Dec 05 2024 at 18:25):

alexcrichton updated PR #9710.

Wasmtime GitHub notifications bot (Dec 05 2024 at 18:25):

alexcrichton has enabled auto merge for PR #9710.

Wasmtime GitHub notifications bot (Dec 05 2024 at 18:51):

alexcrichton merged PR #9710.

Last updated: Dec 23 2024 at 12:05 UTC