Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent GC behavior calling a host function returning an externref #8433

Open
alexcrichton opened this issue Apr 22, 2024 · 1 comment

Comments

@alexcrichton
Copy link
Member

For this program:

use wasmtime::*;

fn main() {
    let engine = Engine::default();

    let module = Module::new(
        &engine,
        r#"
            (module
                (import "" "a" (func $a (result externref)))

                (func (export "a")
                    (loop $l
                        call $a
                        drop
                        br $l
                    )
                )
            )
        "#,
    )
    .unwrap();

    let mut linker = Linker::new(&engine);
    linker
        .func_wrap("", "a", |mut caller: Caller<'_, ()>| {
            Ok(Some(ExternRef::new(&mut caller, 100)?))
        })
        .unwrap();

    let ty = FuncType::new(&engine, [], [ValType::EXTERNREF]);
    linker
        .func_new("", "b", ty, |mut caller, _, results| {
            results[0] = Some(ExternRef::new(&mut caller, 100)?).into();
            Ok(())
        })
        .unwrap();

    let mut store = Store::new(&engine, ());
    let i = linker.instantiate(&mut store, &module).unwrap();
    let f = i.get_typed_func::<(), ()>(&mut store, "a").unwrap();
    f.call(&mut store, ()).unwrap();
}

it will, as-is, run with:

$ cargo run -q
thread 'main' panicked at src/main.rs:42:28:
called `Result::unwrap()` on an `Err` value: error while executing at wasm backtrace:
    0:   0x2c - <unknown>!<wasm function 1>

Caused by:
    0: failed to allocate `externref`
    1: GC heap out of memory
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

However if the "a" is switched to "b" to use the func_new definition instead of func_wrap, it will run infinitely and have steady memory usage.

The reason is that this block is only present in the func_new path, not the func_wrap path.

cc @fitzgen

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Apr 22, 2024
This commit fixes a panic when a host function defined with `Func::new`
returned GC references and was called in async mode. The logic to
auto-gc before the return values go to wasm asserted that a synchronous
GC was possible but the context this function is called in could be
either async or sync. The fix applied in this commit is to remove the
auto-gc. This means that hosts will need to explicitly GC in these
situations until auto-gc is re-added back to Wasmtime.

cc bytecodealliance#8433 as this will make the behavior consistent, but we'll want to
re-add the gc behavior.
@alexcrichton
Copy link
Member Author

In #8434 I'm making the behavior "consistent" but in the wrong direction. That PR is removing the auto-gc block I outlined above, which was the source of the panic. This is the "wrong direction", however, as we should still retain the gc-before-returning so hosts aren't required necessarily to GC themselves. (or we should perhaps decide that this is indeed the desired semantics).

In that sense I'm going to leave this issue open after that PR lands.

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Apr 22, 2024
…lliance#8434)

This commit fixes a panic when a host function defined with `Func::new`
returned GC references and was called in async mode. The logic to
auto-gc before the return values go to wasm asserted that a synchronous
GC was possible but the context this function is called in could be
either async or sync. The fix applied in this commit is to remove the
auto-gc. This means that hosts will need to explicitly GC in these
situations until auto-gc is re-added back to Wasmtime.

cc bytecodealliance#8433 as this will make the behavior consistent, but we'll want to
re-add the gc behavior.
github-merge-queue bot pushed a commit that referenced this issue Apr 22, 2024
This commit fixes a panic when a host function defined with `Func::new`
returned GC references and was called in async mode. The logic to
auto-gc before the return values go to wasm asserted that a synchronous
GC was possible but the context this function is called in could be
either async or sync. The fix applied in this commit is to remove the
auto-gc. This means that hosts will need to explicitly GC in these
situations until auto-gc is re-added back to Wasmtime.

cc #8433 as this will make the behavior consistent, but we'll want to
re-add the gc behavior.
fitzgen pushed a commit that referenced this issue Apr 22, 2024
…8439)

This commit fixes a panic when a host function defined with `Func::new`
returned GC references and was called in async mode. The logic to
auto-gc before the return values go to wasm asserted that a synchronous
GC was possible but the context this function is called in could be
either async or sync. The fix applied in this commit is to remove the
auto-gc. This means that hosts will need to explicitly GC in these
situations until auto-gc is re-added back to Wasmtime.

cc #8433 as this will make the behavior consistent, but we'll want to
re-add the gc behavior.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant