Could this be skipped if the exception tables section is empty?

Mind adding a tests/disas test for this showing the exception tables?

Could this be made safe and return an action to perform rather than performing the action? I get jittery skipping Rust frames nowadays and I feel it works best when this propagates upwards as far as it can the action to take before actually taking the action. One example is that with Pulley this can't return ! because it'll need to propagate to the interpreter loop to update interpreter state to perform the pseudo-longjmp

In the long-term I think we'll want to handle this differently because store here aliases nogc without the compiler knowing. This is probably fine for now but if this sticks around can you file an issue to improve this in the future?

One possible idea would be to move the InstanceId into the VMContext and to read that out and then lookup through nogc the InstanceId. Another option would be to add a helper method to Instance which takes the store and the VMContext and returns Pin<&mut Instance> connected to the lifetime of the store, avoiding creating a second store pointer and requiring the store has nothing else borrowed at the same time.

One possible idea would be to move the InstanceId into the VMContext and to read that out and then lookup through nogc the InstanceId. Another option would be to add a helper method to Instance which takes the store and the VMContext and returns Pin<&mut Instance> connected to the lifetime of the store, avoiding creating a second store pointer and requiring the store has nothing else borrowed at the same time.

Another variant on this idea: we could also have a method on the store that takes a vmctx and gives an instance, something like

impl StoreOpaque {
    // Safety: vmctx must be a valid pointer
    pub unsafe fn instance_from_vmctx(&mut self, vmctx: *mut VMContext) -> Option<Pin<&mut Instance>> {
        // Validate that the vmctx is from this store and all that...
    }
}

I'm not sure whether it's viable, but personally what I'd ideally like to see is the request-to-throw propagated even through the libcall here through the Result. That'd likely require some finesse and refactoring but I suspect such refactoring is going to be required no matter what for Pulley support eventually.

In lieu of that though I'd ideally like to see the "do the throw" action no higher than here, so my comment below about threading the action-to-do would be handled around here. (although I could also be missing other places where it's acted upon)

Could you expand the documentation here that this should be used judiciously and specifically mention that the closure f is executed under a read lock? That'll help communicate that this can block for awhile waiting for a write lock to finish and additionally this is blocking all writers at the same time.

Where is this used? I don't actually see it used anywhere and C-f isn't finding anything.

Also, it is probably better, in terms of horizontal scalability, to clone the layout out of the registry than to hold the lock for the duration of whatever code needs to work with it. If that is something we need to do often, we can look into making that cheaper, eg by storing layouts in an Arc or something.

Sorry, this is left over from an earlier implementation that fetched layouts dynamically before I had put the tag info at a constant offset in exception objects. (Or rather, it was always at a constant offset per allocator, but I hadn't done the plumbing that array-lengths have to avoid getting it from the layout.)

I'll remove this; but to clarify, we have no need to access layouts or the type registry at all at runtime with the exceptions implementation. The unwinder is completely generic over exception layouts and only compares tag identities.

To confirm, parse here is a pretty cheap operation?

Yes -- it reads two u32s for lengths of arrays, then builds an ExceptionTable with slices over arrays.

Could this be done with a #[cfg(debug_assertions)] perhaps? That'll help avoid paging this in if necessary and by construction this should always be true as a result of compilation such that it shouldn't be required to check here too.

This might be best to switch to GC_TYPES instead of GC since that's the true reliance here, there's no need to enable structs/arrays to enable exceptions too. (just that under the hood exceptions is implemented with GC types)

Mind double-checking there are tests for these new offset methods (and the ones below) in vmcontext.rs?

Might as well smallvec![] these temporaries.

I don't think this is correct? I think we mark our own (callee) vmctx as "the" vmctx for Cranelift and the caller's vmctx is the second argument that follows afterwards. For example:

This is in the context of a Call (codegen for a callsite), so the "caller" is the current function; and this change is pulling out an existing expression that was bound as let caller_vmctx = ... (see the diff in unchecked_call_impl for example). Happy to rename it to something else, but callee_vmctx would be inaccurate/misleading in this context, IMHO. our_vmctx maybe?

Rather than dynamically assert that these are unreachable at runtime, we can statically assert that they are unreachable at compile time via

impl From<VMStructRef> for VMGcRef {
    fn from(s: VMStructRef) -> VMGcRef {
        match s {}
    }
}

etc...

This is nice because any mistakes are caught at compile time.

Ah, good call; the first two (methods on VMGcRef) can't follow that pattern because VMGcRef is not actually an uninhabitable enum in the GC-disabled build, so I followed that in the From impls as well, but those can follow the empty-match pattern...

One possible idea would be to move the InstanceId into the VMContext and to read that out and then lookup through nogc the InstanceId. Another option would be to add a helper method to Instance which takes the store and the VMContext and returns Pin<&mut Instance> connected to the lifetime of the store, avoiding creating a second store pointer and requiring the store has nothing else borrowed at the same time.

Another variant on this idea: we could also have a method on the store that takes a vmctx and gives an instance, something like

impl StoreOpaque {
    // Safety: vmctx must be a valid pointer
    pub unsafe fn instance_from_vmctx(&mut self, vmctx: *mut VMContext) -> Option<Pin<&mut Instance>> {
        // Validate that the vmctx is from this store and all that...
    }
}

and there are no frames on the stack between the Wasm and here that must run to maintain some safety invariant, and cannot be unwound over.

This is ~impossible to determine in arbitrary code, which is why pushing the command-return all the way out to the libcall is nice, since we know there is ~nothing on the stack before the Wasm frames at that point.

Where is this used? I don't actually see it used anywhere and C-f isn't finding anything.

Also, it is probably better, in terms of horizontal scalability, to clone the layout out of the registry than to hold the lock for the duration of whatever code needs to work with it. If that is something we need to do often, we can look into making that cheaper, eg by storing layouts in an Arc or something.