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

[js-api] add tests for global imports with GC types #498

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

takikawa
Copy link
Contributor

@takikawa takikawa commented Dec 18, 2023

This PR adds tests for the update to the JS API made in #467 to enable global imports of various reftype values.

These tests pass in V8 and there's a pending patch for JSC. I'm looking into the status for Spidermonkey.

There is one open question with these tests, which is that all the error cases are assumed to throw a WebAssembly.LinkError. However, I believe the current spec actually mandates throwing TypeError instead. Which behavior would we prefer here?

(note: V8 & Spidermonkey currently disagree on the error type to throw)

Adds JS API tests for the "read the imports" part of the spec for globals.
@takikawa
Copy link
Contributor Author

Update: I ran the tests in Firefox/SM as well and they all pass if I change WebAssembly.LinkError to TypeError.

@eqrion
Copy link
Contributor

eqrion commented Dec 19, 2023

There is one open question with these tests, which is that all the error cases are assumed to throw a WebAssembly.LinkError. However, I believe the current spec actually mandates throwing TypeError instead. Which behavior would we prefer here?

(note: V8 & Spidermonkey currently disagree on the error type to throw)

The issue here (IIUC) is that 'read the imports' generally tries to throw LinkError, but it can delegate to ToWebAssemblyValue which can throw TypeError for GC types if match_valtype fails?

Digging a little deeper, it seems like we have a pattern in 'read the imports' of catching every case that could cause ToWebAssemblyValue to fail and eagerly throwing a LinkError. So e.g. we check for BigInt for i64 to ensure that ToWebAssemblyValue won't fail to convert. I think we could amend the spec to hoist the match_valtype check out into
read the imports' and match that eager throwing of LinkError behavior.

webkit-commit-queue pushed a commit to takikawa/WebKit that referenced this pull request Dec 20, 2023
https://bugs.webkit.org/show_bug.cgi?id=264655

Reviewed by Justin Michaud.

The GC proposal JS API was recently updated
(WebAssembly/gc#467) to allow direct import of reftype
globals in more cases.

This patch also includes a version of the pending WPT tests for this case
(tracked upstream here: WebAssembly/gc#498) which
should be updated later if there are any changes in the accepted upstream
version.

* JSTests/wasm/gc/js-api.js:
(testImport.):
(testImport):
* LayoutTests/imported/w3c/web-platform-tests/wasm/jsapi/gc/global-import.tentative.any.js: Added.
(setup.doLink):
(setup):
(test):
* Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:
(JSC::fromJSValue):
* Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:
(JSC::WebAssemblyModuleRecord::initializeImports):

Canonical link: https://commits.webkit.org/272367@main
@rossberg
Copy link
Member

Sorry, I had somehow missed this PR before the holidays.

I agree that it looks like the original intention was that all failures in linking throw LinkError. Perhaps the most modular fix to the spec is to catch a TypeError from ToWebAssemblyValue and rethrow it as LinkError?

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

Successfully merging this pull request may close these issues.

3 participants