[test] Add tentative JS API tests for Exported GC Object #355

takikawa · 2023-02-17T22:33:20Z

Adds tests for the "Exported GC Object" described in the proposed JS API spec in #352.

This requires updating the wasm module builder helper library, which is now fairly de-synced with the V8 library it's based on. I've only synced a bare minimum in this PR to be able to construct some GC modules (syncing the whole thing causes other tests to fail).

The V8 version also uses a slightly different opcode numbering than the current spec document in this repo. In this PR I went with the spec numbering (in the end they will all need to be synchronized).

This doesn't have tests for all the ToWasmValue/ToJSValue cases for conversions, which would also be useful to add.

takikawa · 2023-02-17T22:34:14Z

test/js-api/wasm-module-builder.js

+}
+
+// GC opcodes
+let kExprStructNew = 0x01;


Note: the opcode numbers for struct.new and array.new differ from V8/SM currently.

That's because the implementations all along the stack (engines, Binaryen, *-to-Wasm compilers) have been maintaining (incremental) backwards compatibility when adding and removing instructions as the proposal evolved, whereas the official spec is not concerned with such mundane details.

This will resolve itself when we do the large final breaking opcode reshuffling. In the meantime, see https://bit.ly/3cWcm6Q for the authoritative documentation on what toolchains and engines implement.

A note in the spec about this would be useful, IMO. In the meantime, it might be best to use the de facto opcode numbers in the tests, so that they can be run against implementations - not much point in them otherwise.

test/js-api/gc/exported-object.tentative.any.js

takikawa · 2023-02-17T22:36:00Z

test/js-api/gc/exported-object.tentative.any.js

+test(() => {
+  const struct = functions.makeStruct()
+  const array = functions.makeArray()
+  assert_throws_js(TypeError, () => Object.getPrototypeOf(struct));


When I tried this in V8, this test failed because the getPrototypeOf didn't throw.

Yeah, when we made [[Get]] not throw we also had to make [[GetPrototypeOf]] not throw (but return null instead). Looks like I forgot to update the draft doc accordingly, sorry about that. Fixed it there just now.

Ok thanks, I updated this test case to check for null instead as well. I think this actually makes it easier for JSC to implement [[Get]]'s behavior too, though I'm wondering if it'll be sufficient to future-proof for shared structs.

Yeah, I'm a little worried about future-proofing for improved prototypes later (related to alignment with shared structs). Is this is the only way we can make it so that async functions can return Wasm GC objects without blowing up? If so, it seems like this is the best possible compromise.

takikawa · 2023-02-17T22:37:38Z

Other than the points I mentioned in the tests above, I was able to run this in V8 by manually importing it into v8/test/wasm-js/tests and editing a few things like opcodes. I also ran it against a WIP implementation in JSC.

takikawa · 2023-02-17T22:41:00Z

test/js-api/gc/exported-object.tentative.any.js

+test(() => {
+  const struct = functions.makeStruct()
+  const array = functions.makeArray()
+  assert_throws_js(TypeError, () => { struct.foo = 5; });


Right now [[Set]] is specified to always throw, which matches the test cases here.

It is arguably a bit inconsistent with the rest of JS though, because usually [[Set]] returns false on failure but doesn't throw. In the usual case, that means that in non-strict mode, struct.foo = 5 will not add/mutate the field and just silently fail without throwing. With Wasm structs/arrays, it will throw even in non-strict mode. Not sure anyone will care about this difference, but I thought it might be worth mentioning.

Yeah, returning false probably makes more sense. In any case, we should probably make sure to test these cases both in strict and loose mode

Yeah, that makes sense. I've changed the test now to have strict and non-strict versions, with no error raised in the non-strict version (and the property shouldn't be added/deleted). We'll have to change the spec to match this though.

takikawa · 2023-03-07T18:20:35Z

Who would be the best person to review this and provide review approval? @Ms2ger would you be interested in taking a look?

ajklein · 2023-03-07T19:00:04Z

I can't seem to add him as a reviewer, but it would make sense for @syg to do at least an unofficial review of this given the intersection with JS Structs.

Ms2ger · 2023-04-18T14:04:49Z

test/js-api/wasm-module-builder.js

+// Use these for multi-byte instructions (opcode > 0x7F needing two LEB bytes):
+function GCInstr(opcode) {
+  if (opcode <= 0x7F) return [kGCPrefix, opcode];
+  return [kGCPrefix, 0x80 | (opcode & 0x7F), opcode >> 7];


Can this use wasmSignedLeb?

It might be able to.

In general for these wasm-module-builder.js changes I took the upstream V8 code as-is when I could, so merging new changes would be easier without a complicated diff. Do you prefer that I make changes here anyway and let it diverge with upstream? (also applies to other comments)

Perhaps someone from the V8 side could comment. @jakobkummerow seems to have some blame around here.

Would be a nice project for someone with time to consolidate all the versions going around.

Yeah, this probably could use wasmSignedLeb, but it also doesn't matter as it seems exceedingly unlikely that we'll have that many opcodes any time soon (if ever). From a V8 perspective, we see our wasm-module-builder.js as very frequently changing code anyway: we update it whenever we need to (typically to support some new feature), and that happens a lot. Basically the reason we have our own thing there rather than importing someone else's implementation is because we need to change it so much, and want that to have low friction and low latency.

If you want to submit a patch to improve it, I'd volunteer to review that, but I'm currently not inclined to spend my own time to make this code more polished-looking.

Ms2ger · 2023-04-18T14:08:22Z

test/js-api/wasm-module-builder.js

+}
+
+// GC opcodes
+let kExprStructNew = 0x01;


A note in the spec about this would be useful, IMO. In the meantime, it might be best to use the de facto opcode numbers in the tests, so that they can be run against implementations - not much point in them otherwise.

Ms2ger · 2023-04-18T14:18:13Z

test/js-api/wasm-module-builder.js

+  }
+
+  emit_type(type) {
+    if ((typeof type) == 'number') {


Suggested change

if ((typeof type) == 'number') {

if (typeof type === 'number') {

Ms2ger · 2023-04-18T14:18:42Z

test/js-api/wasm-module-builder.js

@@ -674,6 +742,35 @@ class WasmTableBuilder {
  }
 }

+function makeField(type, mutability) {
+  if ((typeof mutability) != 'boolean') {


Suggested change

if ((typeof mutability) != 'boolean') {

if (typeof mutability !== 'boolean') {

Ms2ger · 2023-04-18T14:21:03Z

test/js-api/wasm-module-builder.js

+  // We use {is_final = true} so that the MVP syntax is generated for
+  // signatures.


What's this about, and should it really be controlled on a type-by-type basis?

In the GC proposal type declarations for structs & arrays can be declared final or not. Since the finality is declared for particular type defs I think it has to be type-by-type.

I don't understand the MVP comment, then.

Ms2ger · 2023-04-18T15:07:05Z

test/js-api/gc/exported-object.tentative.any.js

+test(() => {
+  const struct = functions.makeStruct()
+  const array = functions.makeArray()
+  assert_throws_js(TypeError, () => { struct.foo = 5; });


Yeah, returning false probably makes more sense. In any case, we should probably make sure to test these cases both in strict and loose mode

test/js-api/gc/exported-object.tentative.any.js

takikawa · 2023-04-19T06:25:34Z

Thanks for the comments, I resolved them aside from the ones about the module builder (see comment). The main change is to allow not throwing an exception in non-strict mode (and silently failing) for adding properties and deleting.

This modified version should succeed on V8 except for the non-strict tests. On JSC it also passes (except non-strict) with some minor changes in opcodes. In JSC I implemented a quick fix for the non-strict case and got all tests to pass too.

jakobkummerow · 2023-04-20T10:20:50Z

I have a concern about the change to not throw in non-strict mode.
I think this new behavior (not throwing) would make perfect sense if we aimed to specify some final state of affairs. But the idea behind the throwing was specifically to keep the door wide open to future changes in behavior here (when, post-MVP, we discuss options for richer interop stories).

That's why I'd prefer to keep throwing in both strict and non-strict mode for now.
If in the future we reach the conclusion that the "no-frills" interop is actually enough, and we want to declare it final, then we can relax the non-strict mode behavior at that point.
Whereas if we develop any approach that actually allows foo.bar notation in the future, it could very well be very important for not breaking the web with that rollout that we can be sure that nobody, not even non-strict mode users, is currently shipping any code that would break with this rollout.

Also, in general, when a PR named "add tentative tests" makes such a change to behavior, I think this needs to be explicitly called out to a wider audience (one might even argue that it should happen as a separate PR).

Ms2ger · 2023-04-20T13:22:01Z

Also, in general, when a PR named "add tentative tests" makes such a change to behavior, I think this needs to be explicitly called out to a wider audience (one might even argue that it should happen as a separate PR).

Note that this PR doesn't change the spec.

rossberg · 2023-07-25T08:46:00Z

What is the status of this PR?

Adds a test for the draft JS API spec from this PR: WebAssembly#352 This primarily tests the internal object methods described in the spec. It doesn't yet test the ToWebAssemblyValue / ToJSValue conversions. Also syncs a portion of the module builder code with V8 to allow constructing a subset of GC-using modules.

* Update opcodes to final versions in MVP document * Undid strict/non-strict behavior difference

rossberg · 2023-09-09T17:27:07Z

@takikawa, ping, is this PR still relevant?

takikawa · 2023-09-09T18:30:26Z

@takikawa, ping, is this PR still relevant?

Hi @rossberg, yes I think it's relevant but it needs a rebase to adjust for the final opcodes and to address Jakob's concerns. I have some local changes that I was planning to push after testing.

takikawa · 2023-09-09T19:38:12Z

Had to force push due to the rebase. This should now be updated to throw in non-strict mode as well, like @jakobkummerow requested (i.e., reverted to the original test behavior for this).

It should also be updated to match the final opcodes in MVP.md. I've tested it on JSC/WebKit but haven't had a chance to run it on V8 or SpiderMonkey.

jakobkummerow · 2023-09-09T19:38:42Z

@takikawa For testing this on V8 with the final binary encoding, you can patch in crrev.com/c/4756846. We're planning to land that after the Phase 4 vote.

* The exported-object test should run on V8 now * Non-GC JS API tests should run correctly too

takikawa · 2023-09-15T21:37:52Z

I've now tested this on V8 (on commit 014e0bb5ac4691c85b1da2dac579af77d02d17b7, already updated to new opcodes) in the wasm-js test suite and on Firefox (commit f4b0e304ecebf431616ce014dd198c1d361b37c1, with patches from bug 1845374 applied) with WPT tests.

I also pushed some fixes that I found while testing this, all in the wasm-module-builder.js helper.

On Firefox, some exception handling proposal tests do fail. This is because the exception tests (that are in WPT as tentative tests) use some type code names that have changed in the upstream wasm-module-builder.js and in this PR. In this case, I think the exception tests (specifically this one) should just be changed to match as that proposal is in phase 3.

takikawa · 2023-09-15T21:42:05Z

In any case, I think this is ready to merge if other people are happy with it. Separately, I'm planning to work on on the rest of the JS API tests (testing ToWebAssemblyValue and ToJSValue) soon.

takikawa commented Feb 17, 2023

View reviewed changes

test/js-api/gc/exported-object.tentative.any.js Show resolved Hide resolved

takikawa commented Feb 17, 2023

View reviewed changes

takikawa mentioned this pull request Feb 20, 2023

[spec] Initial JS API formal spec for the GC proposal #352

Merged

Ms2ger reviewed Apr 18, 2023

View reviewed changes

takikawa force-pushed the gc-object-wpt-test branch from 02409ff to 508aeb7 Compare April 19, 2023 06:11

takikawa added 4 commits September 8, 2023 14:02

Update getPrototypeOf test

1779efb

Address PR feedback

15156e0

Rebase exported object test

920e636

* Update opcodes to final versions in MVP document * Undid strict/non-strict behavior difference

takikawa force-pushed the gc-object-wpt-test branch from 508aeb7 to 920e636 Compare September 9, 2023 19:35

rossberg mentioned this pull request Sep 12, 2023

Phase 4 requirements #371

Closed

14 tasks

Fix wasm-module-builder for some missed updates

25fb7d4

* The exported-object test should run on V8 now * Non-GC JS API tests should run correctly too

takikawa mentioned this pull request Sep 19, 2023

[test] Add JS API tests for I31 refs #437

Merged

rossberg merged commit bdad982 into WebAssembly:main Oct 5, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Add tentative JS API tests for Exported GC Object #355

[test] Add tentative JS API tests for Exported GC Object #355

takikawa commented Feb 17, 2023

takikawa Feb 17, 2023

jakobkummerow Feb 20, 2023

Ms2ger Apr 18, 2023

takikawa Feb 17, 2023

jakobkummerow Feb 20, 2023

takikawa Feb 20, 2023

littledan Jun 6, 2023

takikawa commented Feb 17, 2023

takikawa Feb 17, 2023 •

edited

Loading

Ms2ger Apr 18, 2023

takikawa Apr 19, 2023

takikawa commented Mar 7, 2023

ajklein commented Mar 7, 2023

Ms2ger Apr 18, 2023

takikawa Apr 19, 2023

Ms2ger Apr 19, 2023

jakobkummerow Apr 19, 2023

Ms2ger Apr 18, 2023

Ms2ger Apr 18, 2023

Ms2ger Apr 18, 2023

Ms2ger Apr 18, 2023

takikawa Apr 19, 2023

Ms2ger Apr 19, 2023

Ms2ger Apr 18, 2023

takikawa commented Apr 19, 2023

jakobkummerow commented Apr 20, 2023

Ms2ger commented Apr 20, 2023

rossberg commented Jul 25, 2023

rossberg commented Sep 9, 2023

takikawa commented Sep 9, 2023

takikawa commented Sep 9, 2023

jakobkummerow commented Sep 9, 2023

takikawa commented Sep 15, 2023 •

edited

Loading

takikawa commented Sep 15, 2023

	if ((typeof type) == 'number') {
	if (typeof type === 'number') {

	if ((typeof mutability) != 'boolean') {
	if (typeof mutability !== 'boolean') {

		// We use {is_final = true} so that the MVP syntax is generated for
		// signatures.

[test] Add tentative JS API tests for Exported GC Object #355

[test] Add tentative JS API tests for Exported GC Object #355

Conversation

takikawa commented Feb 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takikawa commented Feb 17, 2023

takikawa Feb 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takikawa commented Mar 7, 2023

ajklein commented Mar 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takikawa commented Apr 19, 2023

jakobkummerow commented Apr 20, 2023

Ms2ger commented Apr 20, 2023

rossberg commented Jul 25, 2023

rossberg commented Sep 9, 2023

takikawa commented Sep 9, 2023

takikawa commented Sep 9, 2023

jakobkummerow commented Sep 9, 2023

takikawa commented Sep 15, 2023 • edited Loading

takikawa commented Sep 15, 2023

takikawa Feb 17, 2023 •

edited

Loading

takikawa commented Sep 15, 2023 •

edited

Loading