Bug 220914 - [JSC] WebAssembly.Global should support Funcref and Externref
Summary: [JSC] WebAssembly.Global should support Funcref and Externref
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Keywords: InRadar
Depends on:
Reported: 2021-01-25 03:48 PST by Yusuke Suzuki
Modified: 2021-01-30 11:51 PST (History)
10 users (show)

See Also:

Patch (6.25 KB, patch)
2021-01-26 09:48 PST, Dmitry
no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2021-01-28 23:44 PST, Dmitry
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Yusuke Suzuki 2021-01-25 03:50:03 PST

1. Wasm::Global itself supports externref / funcref already.
2. But JS binding around WebAssembly.Global does not support it (e.g. new WebAssembly.Global({ type: "externref", mutable: true });
Comment 2 Dmitry 2021-01-26 09:48:25 PST
Created attachment 418433 [details]
Comment 3 Yusuke Suzuki 2021-01-26 11:59:10 PST
Comment on attachment 418433 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=418433&action=review

r=me with comments.

> Source/JavaScriptCore/wasm/js/WebAssemblyGlobalConstructor.cpp:146
> +    RETURN_IF_EXCEPTION(throwScope, encodedJSValue());

Use `RETURN_IF_EXCEPTION(throwScope, {});` in newer code.

> Source/JavaScriptCore/wasm/js/WebAssemblyGlobalConstructor.cpp:151
> +        if (type == Wasm::Type::Funcref && argument.isUndefined())
> +            argument = jsNull();

Can we share this "default value" part with WasmTable etc.?

> Source/JavaScriptCore/wasm/js/WebAssemblyGlobalConstructor.cpp:152
> +        jsWebAssemblyGlobal->global()->set(globalObject, argument);

Insert `RETURN_IF_EXCEPTION(throwScope, { });` after calling a function which takes `globalObject` as a first parameter (this is a rough indicator of "it can throw").

> Source/JavaScriptCore/wasm/js/WebAssemblyGlobalConstructor.cpp:154
> +    RELEASE_AND_RETURN(throwScope, JSValue::encode(jsWebAssemblyGlobal));

RELEASE_AND_RETURN is not necessary if the evaluated part will not throw. Let's use `return`.

> JSTests/wasm/references/globals.js:42
> +  assert.throws(() => new WebAssembly.Global({ value: "anyfunc", mutable: true }, new Pelmen(calories)), WebAssembly.RuntimeError, "Funcref must be an exported wasm function (evaluating 'new WebAssembly.Global({ value: \"anyfunc\", mutable: true }, new Pelmen(calories))')");

Let's add a test, setting a wrong value via `.value = xxx`.
Comment 4 Dmitry 2021-01-28 23:44:20 PST
Created attachment 418707 [details]
Comment 5 Yusuke Suzuki 2021-01-29 11:09:14 PST
Comment on attachment 418707 [details]

Comment 6 EWS 2021-01-29 11:26:42 PST
Committed r272071: <https://trac.webkit.org/changeset/272071>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 418707 [details].
Comment 7 Radar WebKit Bug Importer 2021-01-29 11:27:14 PST
Comment 8 Yusuke Suzuki 2021-01-30 10:50:01 PST
Oops! ::set should not be used since mutability can be an immutable. In that case, assertion hits. https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fwasm%2Fjsapi%2Fglobal%2Ftype.tentative.any.html

I'll fix.
Comment 9 Yusuke Suzuki 2021-01-30 11:51:23 PST
Committed r272119: <https://trac.webkit.org/changeset/272119>