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

[interpreter] Fix subtype condition for result patterns #1836

Merged
merged 4 commits into from
Oct 29, 2024

Conversation

rossberg
Copy link
Member

This should fix WebAssembly/memory64#95.

@backes
Copy link
Member

backes commented Oct 29, 2024

Hm, if I cherry-pick this fix in the memory64 repository, I get failures on run.py on table_fill.wast:

F
======================================================================
FAIL: test table_fill.wast (__main__.RunTests.test table_fill.wast)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/local/google/home/clemensb/src/wasm-spec-memory64/test/core/run.py", line 133, in <lambda>
    setattr(RunTests, testName, lambda self, file=fileName: self._runTestFile(file))
                                                            ^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/google/home/clemensb/src/wasm-spec-memory64/test/core/run.py", line 87, in _runTestFile
    self._runCommand(('%s -d "%s" -o "%s"') % (wasmCommand, inputPath, jsPath), logPath)
  File "/usr/local/google/home/clemensb/src/wasm-spec-memory64/test/core/run.py", line 65, in _runCommand
    self.assertEqual(expectedExitCode, exitCode, msg % (exitCode, expectedExitCode, command, log))
AssertionError: 0 != 2 : failed with exit code 2 (expected 0)
Command:
  /usr/local/google/home/clemensb/src/wasm-spec-memory64/interpreter/wasm  -d "test/core/table_fill.wast" -o "/tmp/table_fill.js"
Log:
/usr/local/google/home/clemensb/src/wasm-spec-memory64/interpreter/wasm: uncaught exception File "script/js.ml", line 449, characters 6-12: Assertion failed

It's this assertion which fails:

448     | RefResult (RefPat _) ->
449       assert false

@rossberg
Copy link
Member Author

@backes, that is odd, I don't see that error on the wasm-3.0 branch. I amended the PR, that should address the assertion failure you're seeing. Let me know if that works.

@backes
Copy link
Member

backes commented Oct 29, 2024

Thanks, Andreas, we are getting further through the table_fill test now.

New failing test (reduced from table_fill.wast in the memory64 repo):

(module
  (table $t64 i64 10 externref)

  (func (export "fill-t64") (param $i i64) (param $r externref) (param $n i64)
    (table.fill $t64 (local.get $i) (local.get $r) (local.get $n))
  )
)

(assert_return (invoke "fill-t64" (i64.const 2) (ref.extern 1) (i64.const 3)))

This generates this Wasm module:

(module
  (func $module.fill-t64 (;0;) (import "module" "fill-t64") (param i64 externref i64))
  (func $spectest.hostref (;1;) (import "spectest" "hostref") (param i32) (result anyref))
  (func $spectest.eq_ref (;2;) (import "spectest" "eq_ref") (param eqref eqref) (result i32))
  (func $run (;3;) (export "run")
    block
      i64.const 2
      i32.const 1
      call $spectest.hostref
      i64.const 3
      call $module.fill-t64
      return
    end
    unreachable
  )
)

This module is ill-typed, passing an anyref (returned from $spectest.hostref) in the place of an externref (parameter to $module.fill-t64).

@rossberg
Copy link
Member Author

Okay, there were a couple of issues with the generation of JS wrapper modules for ref.extern (and also either patterns). Those didn't pop up on this branch, because no wrapper modules were needed in any of the cases where ref.extern was used.

I also added a sanity check that all generated modules are valid, and activated CI for the wasm-3.0 branch, so that these problems should get caught earlier in the future.

Copy link
Member

@backes backes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this seems to fix all issues!

@backes
Copy link
Member

backes commented Oct 29, 2024

I am merging this, and then creating a PR on memory64 to merge the updated wasm-3.0 branch.

@backes backes merged commit 1e10102 into wasm-3.0 Oct 29, 2024
9 checks passed
@backes backes deleted the wasm-3.0-js-refpat-bug branch October 29, 2024 15:49
hubot pushed a commit to v8/v8 that referenced this pull request 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 this pull request may close these issues.

2 participants