wasmtime / PR #13695 Inline `{enter,exit}_sync_call` tram... · git-wasmtime

I realize this is preexisting, but is there an issue on file and/or could you file an issue for deduplicating all this? This seems pretty verbose, duplicative, and error-prone if we have refactorings in the future. I'd love to move towards a world where VMContext and/or VMOffsets is largely defined by a macro for example which could perhaps help a lot here. I don't have any idea how we'd do that, though, and I don't want to block this, but I think it'd be good to track this in an issue.

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

Could this include some small notes/references to how this is implemented in a dual of CLIF-generated code + the libcall itself?

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

Mind going through these tests and trimming down these comments? I suspect most of these can be shortened to just decribing what the test is doing

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

Perhaps Option<VmPtr<VMDeferredThread>> to juggle provenance?

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

Like above, could this have a comment as well?

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

(not blocking) this is another file where a macro might be able to help reduce all the boilerplate needed here...

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

Could this have a comment explaining what it's doing?

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

before I forget about this, can you add notes/docs to enter/exit as-implemented in the concurrent.rs file indicating that the implementation is additionally in JIT code and the two should be kept in-sync?

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

Orthogonally (sort of) it's kind of a bummer that a whole new FuncKey is added here when it's basically not used anywhere. Could the "known imports" field transition to being a enum { FuncKey, Trampoline } and would that help avoid adding this as a new FuncKey and having to handle it places?

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:09):

:speech_balloon: alexcrichton created PR review comment:

This is a good point, and something that I'm a bit worried about. Calling set_trapped above is not something that memory safety relies on, just correctness. Here however this is load-bearing for memory safety and if we ever forget to do this it's quite bad.

I'm not confident that this block is hit for every single possible trap in wasm code. Basically I'm worried we have preexisting niche cases that set_trapped doesn't get called today, and those cases are now extending to this. I'm not certain of this, though, because I can't articulate the exact sequence that would lead to this...

WDYT though about moving this reset to JIT code itself? That should be possible, I believe, from our entry trampolines when concurrency is enabled. Does that sound ok? I'd be a bit more comfortable in accepting that that would catch everything personally. This is a pretty mild preference, though, so feel free to push back.

Wasmtime GitHub notifications bot (Jun 22 2026 at 04:15):

:thumbs_up: alexcrichton submitted PR review:

Actually I'll go ahead and flag this for merge. One thing I'd recommend, if you can, is to get an LLM review locally as well to double-check this signs off with no known bugs. If nothing materially changes from my suggestions/comments though seems fine to land.

Wasmtime GitHub notifications bot (Jun 22 2026 at 15:46):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 22 2026 at 15:46):

:speech_balloon: fitzgen created PR review comment:

Sure. I've wanted to use macros to generate all the offsets stuff and the type definition itself for a while now, and this is just another place where we would reuse that same infrastructure.

Wasmtime GitHub notifications bot (Jun 22 2026 at 15:52):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 22 2026 at 15:52):

:speech_balloon: fitzgen created PR review comment:

Filed https://github.com/bytecodealliance/wasmtime/issues/13707

Wasmtime GitHub notifications bot (Jun 22 2026 at 16:17):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 22 2026 at 16:17):

:speech_balloon: fitzgen created PR review comment:

I was confusing sync type vs sync ABI (that is, self.types[adapter.lift.ty].async_ versus adapter.lift.options.async_). The inlining only happens for sync-to-sync ABIs but the callee's type can still be either async or sync, and replaying the deferred thread creation requires remembering this in VMDeferredThread::callee_async, IIUC.

Wasmtime GitHub notifications bot (Jun 22 2026 at 18:03):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 22 2026 at 18:17):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 22 2026 at 18:19):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 22 2026 at 18:19):

:speech_balloon: fitzgen created PR review comment:

Done in https://github.com/bytecodealliance/wasmtime/pull/13695/commits/029c1e7ad3792ca82f2de675923e78e0ec2f40f9

Wasmtime GitHub notifications bot (Jun 22 2026 at 18:29):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 22 2026 at 21:04):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 24 2026 at 17:52):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 25 2026 at 17:20):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 25 2026 at 18:29):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 25 2026 at 18:32):

fitzgen has enabled auto merge for PR #13695.

Wasmtime GitHub notifications bot (Jun 25 2026 at 18:40):

fitzgen added PR #13695 Inline {enter,exit}_sync_call trampolines into sync-to-sync fused adapters to the merge queue.

Wasmtime GitHub notifications bot (Jun 25 2026 at 19:05):

github-merge-queue[bot] removed PR #13695 Inline {enter,exit}_sync_call trampolines into sync-to-sync fused adapters from the merge queue.

Wasmtime GitHub notifications bot (Jun 26 2026 at 15:52):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 26 2026 at 15:52):

fitzgen has enabled auto merge for PR #13695.

Wasmtime GitHub notifications bot (Jun 26 2026 at 16:38):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 26 2026 at 17:22):

fitzgen updated PR #13695.

Wasmtime GitHub notifications bot (Jun 26 2026 at 18:33):

fitzgen added PR #13695 Inline {enter,exit}_sync_call trampolines into sync-to-sync fused adapters to the merge queue.

Wasmtime GitHub notifications bot (Jun 26 2026 at 18:57):

:check: fitzgen merged PR #13695.

Wasmtime GitHub notifications bot (Jun 26 2026 at 18:57):

fitzgen removed PR #13695 Inline {enter,exit}_sync_call trampolines into sync-to-sync fused adapters from the merge queue.

Wasmtime GitHub notifications bot (Jun 29 2026 at 13:08):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2026 at 13:08):

:speech_balloon: alexcrichton created PR review comment:

TODO FITZGEN

Wasmtime GitHub notifications bot (Jun 29 2026 at 13:08):

:speech_balloon: alexcrichton created PR review comment:

Could this "1" constant be moved to the wasmtime_environ crate or similar?

Wasmtime GitHub notifications bot (Jun 29 2026 at 13:08):

:speech_balloon: alexcrichton created PR review comment:

Either that or could there be a compile-time assertion that "1" is the pattern used here, with a comment saying that if the constant changes it needs to update the compiler too?

Wasmtime GitHub notifications bot (Jun 29 2026 at 13:08):

:speech_balloon: alexcrichton created PR review comment:

How come this is necessary? (same for the one below too)

Wasmtime GitHub notifications bot (Jun 29 2026 at 15:29):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2026 at 15:29):

:speech_balloon: fitzgen created PR review comment:

Whoops, thanks for catching this

Wasmtime GitHub notifications bot (Jun 29 2026 at 15:32):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2026 at 15:32):

:speech_balloon: fitzgen created PR review comment:

It's more defensive than strictly required as the code exists today. The problem is that we hand out ComponentTaskState which contains the concurrent state, which contains the unforced current thread, but by doing so the code that gets that state can't have access to the store at teh same time due to borrow rules so it can't get the actual/forced current thread, so we defensively update the unforced current thread eagerly before passing out the ComponentTaskState.

I think ideally we would completely remove the current thread from the concurrent state and only have it on store and refactor the rest of the code accordingly, but that seemed like a pretty big task to do in this PR.

Wasmtime GitHub notifications bot (Jun 29 2026 at 16:01):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2026 at 16:01):

:speech_balloon: alexcrichton created PR review comment:

Code isn't expected to access unforced_current_thread though, right? I'd expect that if the current thread was needed it'd be done with StoreOpaque before getting ComponentTaskState, and by getting ComponentTaskState code would otherwise follow the convention of not even looking at unforced_current_state due to the scary name.

Wasmtime GitHub notifications bot (Jun 29 2026 at 16:04):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2026 at 16:04):

:speech_balloon: fitzgen created PR review comment:

Ideally, yes, but there is a lot of existing code that doesn't have access to a store and only has the ComponentTaskState. I felt it was better to be safe than sorry.

Wasmtime GitHub notifications bot (Jun 29 2026 at 16:55):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 29 2026 at 16:55):

:speech_balloon: alexcrichton created PR review comment:

Makes sense yeah, I've addressed these in https://github.com/bytecodealliance/wasmtime/pull/13758 and https://github.com/bytecodealliance/wasmtime/pull/13759

Last updated: Jul 29 2026 at 05:03 UTC