Stream: git-wasmtime

Topic: wasmtime / PR #13330 Fix usage of GC barriers when throw...

Wasmtime GitHub notifications bot (May 08 2026 at 20:37):

alexcrichton opened PR #13330 from alexcrichton:fix-exnref-ownership to bytecodealliance:main:

• set_pending_exception didn't have a write barrier to handle any
  previously configured exception.

• When an exnref was caught it didn't go through expose_gc_ref_to_wasm.

• During throw_impl the exnref wasn't cloned to give a strong
  reference to the store.

All of these should now be fixed with some minor refactorings and such
to ensure we've got the right barriers in the right places.

Closes https://github.com/bytecodealliance/wasmtime/issues/13316

Wasmtime GitHub notifications bot (May 08 2026 at 20:37):

alexcrichton requested fitzgen for a review on PR #13330.

Wasmtime GitHub notifications bot (May 08 2026 at 20:37):

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

Wasmtime GitHub notifications bot (May 08 2026 at 21:50):

alexcrichton updated PR #13330.

Wasmtime GitHub notifications bot (May 09 2026 at 01:16):

github-actions[bot] added the label wasmtime:ref-types on PR #13330.

Wasmtime GitHub notifications bot (May 09 2026 at 01:16):

github-actions[bot] added the label wasmtime:api on PR #13330.

Wasmtime GitHub notifications bot (May 09 2026 at 01:17):

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

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 (May 12 2026 at 18:37):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 18:37):

:speech_balloon: fitzgen created PR review comment:

Usually we return an Err, not panic, in this scenario in our public API. Is this a pre-existing? If so, fine to fix here or in a follow-up PR or file a follow-up issue. If not, I don't think we should diverge from the typical behavior here, unless we have a really good reason.

Wasmtime GitHub notifications bot (May 12 2026 at 18:37):

:speech_balloon: fitzgen created PR review comment:

An ExnRef should always be non-null (assuming we are still in its rooting scope) because it should represent (ref exn) (and Option<ExnRef> should represent (ref null exn)). At least, that's how it is with {Array,Struct,Any,Extern,...}Ref, but it is possible I missed something when reviewing ExnRef and if so we should file a follow-up issue to fix this.

I'm curious if you are actually seeing this case...

Wasmtime GitHub notifications bot (May 12 2026 at 18:37):

:speech_balloon: fitzgen created PR review comment:

I wonder if this should be a debug-assert and then we just always throw the most-recently-pending exception in release mode?

Wasmtime GitHub notifications bot (May 12 2026 at 18:37):

:speech_balloon: fitzgen created PR review comment:

Why this change? Seems like a step backwards, and we should always be able to cast a VMExnRef to a VMGcRef via helper methods if necessary.

Wasmtime GitHub notifications bot (May 12 2026 at 18:37):

:speech_balloon: fitzgen created PR review comment:

Probably pre-existing, but this isn't just for ExternRefs anymore.

Wasmtime GitHub notifications bot (May 12 2026 at 20:07):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 20:07):

:speech_balloon: alexcrichton created PR review comment:

I'm just copy/pasting existing docs. I'll update though

Wasmtime GitHub notifications bot (May 12 2026 at 20:07):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 20:07):

:speech_balloon: alexcrichton created PR review comment:

Nah just a copy/paste of existing docs without reading, I'll go through and edit

Wasmtime GitHub notifications bot (May 12 2026 at 20:08):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 20:08):

:speech_balloon: alexcrichton created PR review comment:

I think that might actually match the now-implemented behavior, I'll rewrite this.

Wasmtime GitHub notifications bot (May 12 2026 at 20:09):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 20:09):

:speech_balloon: alexcrichton created PR review comment:

The main reason was gc_store.write_gc_ref(&mut self.pending_exception, Some(exnref)) where it was easier to change this than to add a full matrix of helpers for VMExnRef for this one call. How strongly do you feel this should remain VMExnRef?

Wasmtime GitHub notifications bot (May 12 2026 at 20:48):

alexcrichton updated PR #13330.

Wasmtime GitHub notifications bot (May 12 2026 at 20:49):

alexcrichton updated PR #13330.

Wasmtime GitHub notifications bot (May 12 2026 at 22:13):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 22:13):

:speech_balloon: fitzgen created PR review comment:

Do we not already have as_gc_ref[_mut] helpers for VMExnRef like we do for VM{Struct,Array,...}Ref? Then we probably should, and they should each just be one liners

Wasmtime GitHub notifications bot (May 12 2026 at 22:35):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 22:35):

:speech_balloon: alexcrichton created PR review comment:

We've got conversions from &mut VMExnRef -> &mut VMGcRef, but this is a different conversion from &mut Option<VMExnRef> -> &mut Option<VMGcRef> which I don't believe we have any precedent for

Wasmtime GitHub notifications bot (May 12 2026 at 22:51):

alexcrichton updated PR #13330.

Wasmtime GitHub notifications bot (May 12 2026 at 23:45):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 12 2026 at 23:45):

:speech_balloon: fitzgen created PR review comment:

Unless you feel strongly, I'd prefer to keep this as VMExnRef and add the Option-based helpers as necessary

Wasmtime GitHub notifications bot (May 13 2026 at 04:02):

github-actions[bot] added the label wasmtime:c-api on PR #13330.

Wasmtime GitHub notifications bot (May 13 2026 at 14:53):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 13 2026 at 14:53):

:speech_balloon: alexcrichton created PR review comment:

Personally I'm pretty wary to add more helpers over what we already have. GC already suffers from quite a lot of code duplication which makes it difficult to change things and keep everything in-sync. Adding more conversions with more unsafe implementations (I don't think there's any way to do this safely or built on existing primitives) for a small amount of what is effectively documentation I feel isn't worth it.

For example I think that this would require this on every VM*Ref type:

     pub fn option_slot_as_gc_ref(slot: &mut Option<VMExnRef>) -> &mut Option<VMGcRef> {
         // SAFETY: `Option<VMExnRef>` and `Option<VMGcRef>` have the same layout, since
         // `VMExnRef` is `repr(transparent)` over `VMGcRef`.
         unsafe { &mut *(slot as *mut Option<VMExnRef> as *mut Option<VMGcRef>) }
     }

On top of that we'd theoretically need the inverse as well, going from &mut Option<VMGcRef> back to the typed representation eventually. On top of that most of these wouldn't be used so they'd be #[expect(dead_code)] or similar for now.

Overall it feels like quite a lot of infrastructure for not really much benefit. This only affects one field in one location which is already named "pending exception" and I can touch up the documentation to clearly indicate that it needs to be a VMExnRef.

Wasmtime GitHub notifications bot (May 13 2026 at 15:27):

:memo: fitzgen submitted PR review.

Wasmtime GitHub notifications bot (May 13 2026 at 15:27):

:speech_balloon: fitzgen created PR review comment:

Okay, that sounds fine for now. I think we can address these issues with a macro to reduce code duplication in a follow up though.

This only affects one field in one location which is already named "pending exception" and I can touch up the documentation to clearly indicate that it needs to be a VMExnRef.

Can you add debug-asserts if possible too? Fine to skip if ultimately too difficult.

Wasmtime GitHub notifications bot (May 13 2026 at 17:50):

alexcrichton updated PR #13330.

Wasmtime GitHub notifications bot (May 13 2026 at 17:50):

alexcrichton has enabled auto merge for PR #13330.

Wasmtime GitHub notifications bot (May 13 2026 at 17:50):

alexcrichton has disabled auto merge for PR #13330.

Wasmtime GitHub notifications bot (May 13 2026 at 17:51):

:memo: alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 13 2026 at 17:51):

:speech_balloon: alexcrichton created PR review comment:

Sure yeah, most of the debug asserts were already there and I went ahead and added another that seemed appropriate too

Wasmtime GitHub notifications bot (May 14 2026 at 16:22):

:thumbs_up: fitzgen submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (May 14 2026 at 16:22):

fitzgen added PR #13330 Fix usage of GC barriers when throwing an exnref to the merge queue.

Wasmtime GitHub notifications bot (May 14 2026 at 16:46):

:check: fitzgen merged PR #13330.

Wasmtime GitHub notifications bot (May 14 2026 at 16:46):

fitzgen removed PR #13330 Fix usage of GC barriers when throwing an exnref from the merge queue.

Last updated: Jun 01 2026 at 09:49 UTC