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 tests broken after #1796 #1828

Open
backes opened this issue Oct 7, 2024 · 9 comments
Open

js-api tests broken after #1796 #1828

backes opened this issue Oct 7, 2024 · 9 comments
Assignees

Comments

@backes
Copy link
Member

backes commented Oct 7, 2024

I don't have time to investigate this further currently (and my OCaml knowledge is limited), but after #1796 we now fail most tests in the memory64 and js-promise-integration repositories, which seem to be rebased on a branch which contains that change.

The resulting JS code looks like

// address.wast:3                                                           
let $$1 = "\x00\x61\x73\x6d [...] \x7a";

// address.wast:3
let $1 = instance($$1);

Where instance simply does:

function instance(mod, imports = registry) {
  return new WebAssembly.Instance(mod, imports);
}

This can't work.

To reproduce, run something like:
make -C interpreter wasm && (cd test/core && ./run.py --wasm ../../interpreter/wasm --js $HOME/v8/out/Optdebug/d8 address.wast)

This produces an error like:

test/core/_output/address.js:62: TypeError: WebAssembly.Instance(): Argument 0 must be a WebAssembly.Module
  return new WebAssembly.Instance(mod, imports);
         ^
TypeError: WebAssembly.Instance(): Argument 0 must be a WebAssembly.Module
    at instance (test/core/_output/address.js:62:10)
    at test/core/_output/address.js:203:10
@sbc100
Copy link
Member

sbc100 commented Oct 7, 2024

Do we not run any of these tests are part of CI? If not, I think we can/should add support for running at least some tests using jsvu version of d8/spidermonkey/jsc?

@sbc100
Copy link
Member

sbc100 commented Oct 7, 2024

(BTW since #1728 landed you don't need to add --wasm ../../interpreter/wasm to the run.py command line since the default should always work).

@rossberg
Copy link
Member

rossberg commented Oct 7, 2024

Oops, sorry, my bad. Please see #1829.

@sbc100, normally this runs on CI via node, but that is deactivated on this branch because current node.js cannot handle some of the Wasm 3.0 features yet (or couldn't last time I checked).

@sbc100
Copy link
Member

sbc100 commented Oct 7, 2024

Oops, sorry, my bad. Please see #1829.

@sbc100, normally this runs on CI via node, but that is deactivated on this branch because current node.js cannot handle some of the Wasm 3.0 features yet (or couldn't last time I checked).

Ah I see. Perhaps we could use jsvu to install a nightly version of v8/spidermonkey instead? In emscripten we also use the canary version of node (https://nodejs.org/download/v8-canary/) which gets build with (I believe) tip-of-tree v8.

@rossberg
Copy link
Member

rossberg commented Oct 7, 2024

I'm fine with either, but don't feel knowledgeable enough to set that up. ;)

@sbc100
Copy link
Member

sbc100 commented Oct 7, 2024

I'm fine with either, but don't feel knowledgeable enough to set that up. ;)

I'll take a stab at it. I'd also love it if we could run some of the tests under test/js-api/ as part of CI... I'm currently struggling to run them at all but maybe I'm missing something.

@sbc100
Copy link
Member

sbc100 commented Oct 11, 2024

I tried running the spec tests under JS and things still seems broken in various ways.

I downloaded https://nodejs.org/download/v8-canary/v${VERSION}/node-v23.0.0-v8-canary20240819f52f1c2f1c-linux-x64.tar.xz and then did:

$ NODE=$HOME/bin/node-v23.0.0-v8-canary20240819f52f1c2f1c-linux-x64/bin/node
$ (cd interpreter && opam exec make JS=$NODE ci)

It ran into a bunch of issues that look like incorrect code being generated by interpreter/script/js.ml.

For example in the generated br_table.js I see:

// br_table.wast:3                                                               
let $1 = instance($$1);  

But $$1 is a string so think this should be instance(module($$1)). After fixing that I ran into a bunch of other issues.

@rossberg
Copy link
Member

@sbc100, is this after #1832?

@sbc100
Copy link
Member

sbc100 commented Oct 11, 2024

@sbc100, is this after #1832?

Doh! Sorry my local wasm-3.0 branch was behind.

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

3 participants