Skip to content

Move all tests out of cranelift-wasm - #8147

Merged
abrown merged 4 commits into
bytecodealliance:mainfrom
alexcrichton:remove-all-cranelift-wasm-tests
Mar 16, 2024
Merged

abrown merged 4 commits into
bytecodealliance:mainfrom
alexcrichton:remove-all-cranelift-wasm-tests

Conversation

@alexcrichton

Copy link
Copy Markdown
Member

This commit moves all remaining tests out of the cranelift-wasm crate into other various test suites. My end goal is to eventually remove the Dummy* trait for wasm translation so we don't have to keep updating that over time, but for now these tests I think reside in a more appropriate location nowadays.

Move these up to Wasmtime's misc testsuite to get translated and
instantiated by Wasmtime.

Note that the max-function-index-in-name-section test was removed here
as that's tested by the support added in bytecodealliance#3509.
This is pretty thoroughly tested elsewhere in Wasmtime that we respect
the name section, for example many of the trap tests assert that the
name of the function comes from the text format.
Instead add them to the disassembly test suite to ensure we don't
generate dead code. Additionally this has a lot of coverage via fuzzing
too.
Move them into `tests/disas` so we can easily see the CLIF.
@alexcrichton
alexcrichton requested review from a team as code owners March 15, 2024 16:30
@alexcrichton
alexcrichton requested review from abrown and fitzgen and removed request for a team March 15, 2024 16:30
@github-actions github-actions Bot added cranelift Issues related to the Cranelift code generator cranelift:wasm labels Mar 15, 2024
@abrown
abrown added this pull request to the merge queue Mar 15, 2024
Merged via the queue into bytecodealliance:main with commit 27153d0 Mar 16, 2024
@alexcrichton
alexcrichton deleted the remove-all-cranelift-wasm-tests branch March 16, 2024 00:11
@jameysharp

Copy link
Copy Markdown
Contributor

I was too slow to make these changes and you got to them first 😆

I had done a bit more historical research about the issue-1306 test case which I'll throw here in case anyone wonders later:

This test case was extracted from a fuzz bug in 2019, before Cranelift was merged into the Wasmtime repo:
bytecodealliance/cranelift#1306

That test is interesting, but running it here only tests code in DummyEnvironment, not anything reachable from Wasmtime. There was a report of exactly the same bug in Wasmtime two years later (#3509), and the fix included tests in tests/misc_testsuite/empty.wast to cover this case.

The empty.wast test is better than this one because it has minimal and well-commented binary modules which I've verified still trigger the same bug if the fix is reverted. By contrast, since this older test was produced by a fuzzer, it is larger than necessary and the end of its name section is malformed and unparseable.

Also, I had concluded that the reachability tests were pretty well covered by tests/disas/*reach*.wat, but your addition of tests/disas/dead-code.wat certainly can't hurt.

jameysharp added a commit to jameysharp/wasmtime that referenced this pull request Mar 16, 2024
github-merge-queue Bot pushed a commit that referenced this pull request Mar 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cranelift:wasm cranelift Issues related to the Cranelift code generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants