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

Something broken in JS test case generation #95

Closed
backes opened this issue Oct 28, 2024 · 2 comments · Fixed by #96
Closed

Something broken in JS test case generation #95

backes opened this issue Oct 28, 2024 · 2 comments · Fixed by #96

Comments

@backes
Copy link
Member

backes commented Oct 28, 2024

I am trying to get the newest spec tests to pass in V8. I am stuck at a problem which seems to be on the spec side.

Take this example (reduced from table_fill.wast):

(module
  (table $t 10 externref)
  (table $t64 i64 10 externref)

  (func (export "get") (param $i i32) (result externref)
    (table.get $t (local.get $i))
  )

  (func (export "get64") (param $i i64) (result externref)
    (table.get $t64 (local.get $i))
  )
)

(assert_return (invoke "get" (i32.const 1)) (ref.null extern))
(assert_return (invoke "get64" (i64.const 1)) (ref.null extern))

I run this through test/core/run.py --out /tmp test.wast.
The resulting test.js contains:

// test.wast:1
let $$1 = module("\x00\x61\x73\x6d\x01\x00\x00\x00\x01\x8b\x80\x80\x80\x00\x02\x60\x01\x7f\x01\x6f\x60\x01\x7e\x01\x6f\x03\x83\x80\x80\x80\x00\x02\x00\x01\x04\x87\x80\x80\x80\x00\x02\x6f\x00\x0a\x6f\x04\x0a\x07\x8f\x80\x80\x80\x00\x02\x03\x67\x65\x74\x00\x00\x05\x67\x65\x74\x36\x34\x00\x01\x0a\x97\x80\x80\x80\x00\x02\x86\x80\x80\x80\x00\x00\x20\x00\x25\x00\x0b\x86\x80\x80\x80\x00\x00\x20\x00\x25\x01\x0b");

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

// test.wast:14
assert_return(() => call($1, "get", [1]), null);

This looks like a reasonable straight-forward translation.

The second assertion translates to:

// test.wast:15
run(() => call(instance(module("\x00\x61\x73\x6d\x01\x00\x00\x00\x01\x94\x80\x80\x80\x00\x04\x60\x00\x00\x60\x01\x7f\x01\x6e\x60\x02\x6d\x6d\x01\x7f\x60\x01\x7e\x01\x6f\x02\xb5\x80\x80\x80\x00\x03\x06\x6d\x6f\x64\x75\x6c\x65\x05\x67\x65\x74\x36\x34\x00\x03\x08\x73\x70\x65\x63\x74\x65\x73\x74\x07\x68\x6f\x73\x74\x72\x65\x66\x00\x01\x08\x73\x70\x65\x63\x74\x65\x73\x74\x06\x65\x71\x5f\x72\x65\x66\x00\x02\x03\x82\x80\x80\x80\x00\x01\x00\x07\x87\x80\x80\x80\x00\x01\x03\x72\x75\x6e\x00\x03\x0a\x93\x80\x80\x80\x00\x01\x8d\x80\x80\x80\x00\x00\x02\x40\x42\x01\x10\x00\x0c\x00\x0f\x0b\x00\x0b"), exports($1)),  "run", []));  // assert_return(() => call($1, "get64", [int64("1")]), null)

The run function exported by the module is this:

  (func $run (;3;) (export "run")
    block $label0
      i64.const 1
      call $module.get64
      br $label0
      return
    end $label0
    unreachable
  )

This obviously results in unreachable being executed.

Does anyone spot what is broken here?
@bvisness @sbc100 @rossberg

Note: If the br would be a br_on_non_null, the test would test what it's supposed to test. No idea though if generating this was the intention.

@bvisness
Copy link
Collaborator

The wast itself seems fine; it's clear that there is no difference except the i64 index, and indeed it should return ref.null extern. So I guess this problem must lie in the testing machinery itself. I am not familiar with how that works, only with our own implementation in SpiderMonkey. (We consume the wast directly, and don't use the Python script in this repo.)

@rossberg
Copy link
Member

This looks like a bug in the JS conversion backend. WebAssembly/spec#1836 should fix it, PTAL.

@backes backes mentioned this issue Oct 29, 2024
hubot pushed a commit to v8/v8 that referenced this issue Oct 29, 2024
This in particular includes
WebAssembly/memory64#96 which fixes
WebAssembly/memory64#95 via
WebAssembly/spec#1836.

[email protected]

Bug: 364917766, 364913810
Change-Id: I62ad43742628216f4b0385b4f837212a57b76108
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5973202
Reviewed-by: Eva Herencsárová <[email protected]>
Commit-Queue: Clemens Backes <[email protected]>
Cr-Commit-Position: refs/heads/main@{#96887}
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 a pull request may close this issue.

3 participants