Stream: git-wasmtime

Topic: wasmtime / PR #10919 Cranelift: implement an "unwinder" c...

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:50):

cfallin opened PR #10919 from cfallin:lets-kick-back-and-unwind-a-bit to bytecodealliance:main:

This commit introduces the next major piece of machinery (after the previously-landed try_call support) that we will eventually use to implement Wasm exceptions in Wasmtime. In particular, it implements a generic unwinder as a new crate that supports (i) walking a stack produced by Cranelift code, (ii) serializing Cranelift exception metadata to compact tables (in a way very similar to address maps in Wasmtime, so they will be mappable directly from disk), (iii) using these serialized tables to find handlers during a stack-walk, and (iv) jumping to handlers (i.e., actually unwinding). This crate is currently used in the filetests runner, and will next be used in Wasmtime.

The commit first performs code-motion: it moves stack-walking code from Wasmtime to cranelift-unwinder. This itself has no functional effect, but isolates the code that understands contiguous sequences of Cranelift frames ("activations") from that which is specific to Wasmtime's activation delimiters and metadata.

It then implements a compact exception-table format. This format uses the object crate's mechanisms for directly referencing in-memory arrays of little-endian u32s in a way that will allow us to find handlers when mapping exception metadata directly from an ELF section in a .cwasm (for example). The format consists of four sorted u32 arrays in a way that allows us to look up a callsite first, then search its sorted array of handler offsets by tags.

It next implements the actual unwind control flow: it contains an assembly stub for each supported architecture that transfers control to a PC, SP, and FP value "up the stack", with payload values placed in the payload registers we have defined per our exception ABI in Cranelift.

Finally, it puts these pieces together in the filetest runner. Note that the runtest does a lot "by hand": we don't have entry and exit trampolines as we do in Wasmtime, so the filetest contains three functions, with the middle one invoking the "throw hostcall" and entry and exit trampolines around it grabbing the appropriate entry/exit FPs and exit PC. The dance to call back to host code is also somewhat delicate, as we haven't done this before. The JITModule's linking + relocation support does not seem sufficient to properly define a symbol, so instead we scan for func_addr instructions referencing a well-known name (__cranelift_throw) and replace them with iconsts with the function address at runtime, baking it in. This is somewhat ugly, but it works. All of these filetest-specific details will be handled much more nicely in the Wasmtime version of this functionality, as we have proper abstractions for entry/exit trampolines and hostcalls.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:50):

cfallin requested abrown for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:50):

cfallin requested wasmtime-compiler-reviewers for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:50):

cfallin requested dicej for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:50):

cfallin requested wasmtime-core-reviewers for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:50):

cfallin requested wasmtime-default-reviewers for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:51):

cfallin requested fitzgen for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 05:54):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:26):

bjorn3 created PR review comment:

Making this a return_call_indirect would avoid the need to get any frame pointer, right? You only need to change the return address before returning from __cranelift_throw, not unwind the stack back to the caller of %throw.

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:26):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:26):

bjorn3 created PR review comment:

nit: missing trailing newline

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:29):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:29):

bjorn3 created PR review comment:

extern "C-unwind" otherwise unwinding out of this is UB (you are skipping past the aborting landingpad due to using a different unwinding mechanism).

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:33):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Jun 04 2025 at 09:55):

bjorn3 commented on PR #10919:

By the way why is the unwind info format defined in the cranelift-unwinder crate? It is Wasmtime specific and I would assume that Winch and Pulley will reuse the same format.

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

Not necessarily required for this PR, but could this be moved back to Wasmtime? It feels a bit odd to define it all the way over here when Pulley is defined somewhere else.

Placement-wise the only use of UnwindPulley is here so it might make sense to add an unwinder method on struct Interpreter which would enable moving all of this to this file (and a stub in this file)

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

Ideally I'd prefer to not duplicate this with the main Wasmtime crate as I suspect the two will diverge over time. For example on iOS we should probably disable cranelift by default despite having a backend for aarch64. Now that doesn't actually matter for this crate though since it'll still all compile just fine on iOS (in theory), it just wouldn't be useful to have at runtime.

Would it be possible to actually drop this build script entirely? For example the arch module would become naturally empty on unsupported architectures and structures like UnwindHost would only appear for supported architectures. That'd mean that this build script would basically fall out of the cfg-if if/else branches for the arch support

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

FWIW you can use &* to translate *const T to &T, no transmute necessary

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

It should be ok to skip this comment since the type system basically all guarantees that for us (or perhaps move this beforehand as a lead-in explanation for what the function's doing?)

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

FWIW if you're feeling enterprising IIRC the only reason we have this is get_stack_pointer on s390x where inline assembly wasn't supported originally, but it is supported now, so a small inline assembly snippet would probably suffice for removing this crate dependency.

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

Could you elaborate on this? I was under the impression that C-unwind had to do with the system unwinder, or whatever the default is for the platform. For this unwinder landing pads aren't invoked no matter what, so by the same logic C-unwind is also UB because landing pads still aren't executed.

To me it seems that the soundness of this function relies on there not actually being any landing pads, which is something we'd have to uphold?

Wasmtime GitHub notifications bot (Jun 04 2025 at 15:32):

alexcrichton created PR review comment:

I'm hesitating with this in that this feels like a new significant dependency edge that didn't exist prior, but I can't really articulate what exactly it is. Currently in all other locations though cranelift-codegen is isolated to wasmtime-cranelift/Winch and nowhere else. My assumption is that the wasmtime crate will depend on cranelift-unwinder (and maybe also the wasmtime-cranelift crate to pass in the cranelift data structures?). I realize that wasmtime itself won't enable this dependency, but WDYT about making this built-in to cranelift-codegen directly? Or maybe a separate crate?

I don't feel too too strong about this, but I think it sort of depends on how exactly this'll get integrated in to Wasmtime as well.

Wasmtime GitHub notifications bot (Jun 04 2025 at 17:34):

alexcrichton created PR review comment:

I've written up more thoughts on this in https://github.com/bytecodealliance/wasmtime/issues/10923

Wasmtime GitHub notifications bot (Jun 04 2025 at 17:34):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 18:20):

fitzgen created PR review comment:

Maybe add an olive branch of documentation about the safety requirements here? We've been trying to improve this stuff in the core VM.

Wasmtime GitHub notifications bot (Jun 04 2025 at 18:20):

fitzgen created PR review comment:

I don't care too much what we name this crate, in some sense it could be either wasmtime-unwinder or cranelift-unwinder because the dependency graph is a little convoluted here, but I think this description is a bit misleading. People who are writing a random Cranelift project could see this and think it is a solution to their unwinding issues -- especially since using Cranelift means pulling in a bunch of cranelift-foo deps -- and mayyyybe it could be, but it is really tailored for Wasmtime's usage and isn't going to be very flexible to other use cases.

We should probably make it clear in this description that this is an internal implementation detail of Wasmtime and Cranelift's unit testing.

Wasmtime GitHub notifications bot (Jun 04 2025 at 18:20):

fitzgen created PR review comment:

If this function is already monomorphized over R, we might as well use an &impl Unwind instead of a &dyn Unwind

Wasmtime GitHub notifications bot (Jun 04 2025 at 18:20):

fitzgen created PR review comment:

I had to read this pretty closely to understand how the below "expanded" representation corresponds to the above encoding example. I think what would have allowed me to understand at a relatively light skim would have been inline comments in the ranges array expanding what actual tags/handlers ranges they represent, as well as having links the other way for the tags and handlers, something like this:

callsites: [0x10, 0x50, 0xf0],
ranges: [
  // tag/handlers range for callsite 0x10: 0..2
  2,
  // tag/handlers range for callsite 0x50: 2..4
  4,
  // tag/handlers range for callsite 0xf0: 4..5
  5,
]
tags: [
  // Tags for callsite 0x10
  1,
  5,
  // Tags for callsite 0x50
  1,
  -1,
  // Tags for callsite 0xf0
  -1
],
handlers: [
  // Handlers for callsite 0x10
  0x40,
  0x42,
  // Handlers for callsite 0x50
  0x6f,
  0x71,
  // Handlers for callsite 0xf0
  0xf5,
]

Basically, it wasn't immediately clear to me at a glance which arrays were part of the "same struct" (in a struct-of-arrays vs array-of-structs isomorphism) and for the ones which had separate indexing, how that indexing worked, and I ended up having to read this comment closely a couple times.

Wasmtime GitHub notifications bot (Jun 04 2025 at 18:20):

fitzgen submitted PR review:

r=me with things we discussed in the cranelift meeting

Wasmtime GitHub notifications bot (Jun 04 2025 at 19:25):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:15):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:16):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:17):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:17):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:17):

cfallin created PR review comment:

Fixed, thanks!

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:18):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:18):

cfallin created PR review comment:

Updated to use C-unwind -- thanks for catching this.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:20):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:22):

cfallin created PR review comment:

Done as suggested in Cranelift meeting today -- there are now pub uses to re-export with doc-comments attached. cfgs minimized and put right next to each other -- only the one cfg_if chain for the implementations and one with cfg(any(...)) to wrap all the pub uses and the UnwindHost impl.

I've verified that wasmtime-unwinder checks on i686-unknown-linux-gnu.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:22):

cfallin created PR review comment:

Yep, renamed to wasmtime-unwinder -- thanks all for the feedback on this!

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:26):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:26):

cfallin created PR review comment:

Interestingly, this ends up propagating all the way up to e.g. store.unwinder() -- the dyn is infectious and we'd have a monomorphize a bunch more to make this work (or build a proxy-to-dyn impl to monomorphize on). Since this seems like just an opportunistic thing, I'll leave it as dyn Unwind for now...

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:26):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:26):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:33):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:33):

bjorn3 created PR review comment:

Please make this an optional dependency. I don't want to pull this in for cg_clif.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:34):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:34):

bjorn3 created PR review comment:

    pub fn lookup_wasmtime_exception_data<'a>(

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:37):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:37):

cfallin created PR review comment:

Perhaps, but one of my goals here was to closely mirror how the trampolines work in Wasmtime, and also I don't know if we really need to optimize the testing infrastructure like this.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:44):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:44):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:44):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:55):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:55):

alexcrichton created PR review comment:

Yeah changing this away from dyn Unwind, while possible, I think is best to defer to later.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:58):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin created PR review comment:

Sorry about that, didn't think through all the use-cases -- done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 20:59):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:00):

cfallin created PR review comment:

Done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:00):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:00):

cfallin created PR review comment:

Ah, right, much simpler -- done!

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:02):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:02):

cfallin created PR review comment:

Now that the crate is called wasmtime-unwinder I hesitate to put it as a dependency (even optional) of cranelift-codegen; I suppose in the fullness of time we could have a wasmtime-unwinder-cranelift to split off the translation-from-Cranelift-metadata functionality but I also don't feel too strongly about it. For now at least it's under a feature and optional. Happy to see this refactored later if we want to go that way or have other ideas.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:08):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:08):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:08):

cfallin created PR review comment:

Updated, thanks -- I like this way of writing it out a lot better.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:10):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:23):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:23):

cfallin requested alexcrichton for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 21:23):

cfallin requested wasmtime-fuzz-reviewers for a review on PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 22:03):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 22:03):

cfallin has enabled auto merge for PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 22:49):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 22:49):

cfallin has enabled auto merge for PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 23:12):

cfallin commented on PR #10919:

This appears to have bounced off the merge queue because it raced with #10388 and lost; and that PR heavily modified backtracing as well...

Wasmtime GitHub notifications bot (Jun 04 2025 at 23:23):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 04 2025 at 23:30):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:00):

alexcrichton commented on PR #10919:

github's not great about rendering all the time, but the failure was here (32-bit build)

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:34):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:34):

cfallin has enabled auto merge for PR #10919.

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:46):

cfallin has disabled auto merge for PR #10919.

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:46):

cfallin commented on PR #10919:

Grr, now there's another merge conflict because the version bump went in ahead of this one.

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:48):

cfallin updated PR #10919.

Wasmtime GitHub notifications bot (Jun 05 2025 at 00:48):

cfallin has enabled auto merge for PR #10919.

Wasmtime GitHub notifications bot (Jun 05 2025 at 01:39):

cfallin commented on PR #10919:

Wow, CI really does not like this PR -- network error here downloading CMake.

Wasmtime GitHub notifications bot (Jun 05 2025 at 02:00):

cfallin merged PR #10919.

Last updated: Dec 06 2025 at 06:05 UTC