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

Fix some typos in *.wast tests #217

Open
wants to merge 1 commit into
base: upstream-rebuild
Choose a base branch
from

Conversation

alexcrichton
Copy link
Contributor

@alexcrichton alexcrichton commented Oct 18, 2023

Most of the tests' $Check modules imported from "mem" which was failing when I integrated this into Wasmtime. This may very well be a bug in my understanding of the *.wast format, however, but currently via (module $Mem ...) that defines the name "Mem" for modules to use but not "mem". If this is supposed to work as-is I can update Wasmtime instead.

Additionally some tests' comments were checking L_1 but were actually checking against local.get 0 so I've updated them to local.get 1 which fixed some failing tests on Wasmtime.

Most of the test's `$Check` modules imported from `"mem"` which was
failing when I integrated this into Wasmtime. This may very well be a
bug in my understanding of the `*.wast` format, however, but currently
via `(module $Mem ...)` that defines the name `"Mem"` for modules to use
but not `"mem"`. If this is supposed to work as-is I can update Wasmtime
instead.

Additionally some tests' comments were checking `L_1` but were actually
checking against `local.get 0` so I've updated them to `local.get 1`
which fixed some failing tests on Wasmtime.
@rossberg
Copy link
Member

I agree that the import from "mem" should fail – no module is registered under that name on the main thread. But likewise "Mem" shouldn't work either – FWIW, the symbolic module name $Mem is unrelated to import names. The correct fix is to add a (register "mem" $Mem) to the main thread.

It's a bug that this works in the interpreter as is (I failed to make the registry per-thread correctly). I'll prepare a fix.

It also seems odd that some test succeeds while checking the wrong local. @conrad-watt, any idea?

@conrad-watt
Copy link
Collaborator

It's a bug that this works in the interpreter as is (I failed to make the registry per-thread correctly). I'll prepare a fix.

Thanks!

It also seems odd that some test succeeds while checking the wrong local. @conrad-watt, any idea?

If I'm reading correctly, there are executions where those tests would succeed. I think I simply didn't run those three tests enough times :)

@alexcrichton
Copy link
Contributor Author

Yes of the tests I fixed they were nondeterministically failing locally as well, so can confirm that some executions succeed and others don't

@rossberg
Copy link
Member

See #219 for the import issue (and others).

@conrad-watt
Copy link
Collaborator

Hi @alexcrichton, I just wanted to drop in to apologise for not working through this PR and the related #219 more quickly. I should have bandwidth to tackle these next week.

@alexcrichton
Copy link
Contributor Author

No worries! No rush on my end

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