Add a disas test suite for Wasmtime#8129
Conversation
This commit adds a suite of tests at `tests/asm/*.wat` which is intended to replace the tests in `cranelift/filetests/filetests/wasm`. Tests are configured differently than before using Wasmtime CLI flags rather than a custom TOML-based configuration scheme. Otherwise though the same shape of tests is supported. This commit migrates a small handful of tests as a showcase and bulk migration is left for a follow-up.
alexcrichton
requested review from
fitzgen and
jameysharp
and removed request for
a team
|
This was inspired by #8125 and the discussion there |
github-actions
Bot
added
the
cranelift
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Instead of tests/asm maybe we should call this tests/dis or tests/disas since there are CLIF "disassemblies" in there too?
| for line in contents.lines() { | ||
| let directive = match line.strip_prefix(";;!") { | ||
| Some("") | None => continue, | ||
| Some(s) => s.trim(), | ||
| }; | ||
| if directive == "compile" { | ||
| compile = true; | ||
| continue; | ||
| } | ||
| if directive == "optimize" { | ||
| optimize = true; | ||
| continue; | ||
| } | ||
| if let Some(s) = directive.strip_prefix("flags: ") { | ||
| flags.extend( | ||
| s.split_whitespace() | ||
| .filter(|s| !s.is_empty()) | ||
| .map(|s| s.to_string()), | ||
| ); | ||
| continue; | ||
| } | ||
| if let Some(s) = directive.strip_prefix("target: ") { | ||
| if target.is_some() { | ||
| bail!("two targets have been specified"); | ||
| } | ||
| target = Some(s); | ||
| continue; | ||
| } | ||
|
|
||
| bail!("unknown directive: {directive}"); | ||
| } |
There was a problem hiding this comment.
Can we avoid manual parsing and do the TOML thing like the other tests? Can still use exact CLI flags by having the toml just contain a string of CLI flags for that particular setting.
But manual parsing is just annoying to extend down the line.
There was a problem hiding this comment.
I was actually hoping we could skip TOML entirely since that felt heavyweight enough to justify with custom heaps/tables but with just a few booleans and such it feels sort of overkill. For example it feels overly verbose to do:
;;! flags = ["-Oopt-level=s", "-Ccranelift-has-avx"]as opposed to:
;;! flags: -O opt-level=s -C cranelift-has-avxI don't necessarily have a great feeling for how extensible we want this to be down the line though. I was sort of also taking inspiration from compiletest-based tests in rust-lang/rust, but I know the parsing there is sort of gnarly nowadays given all the directives it supports.
There was a problem hiding this comment.
I was thinking we would have a single string for all CLI flags rather than an array of strings of individual flags, which should make it a little less verbose:
;;! flags: "-O opt-level=s -C cranelift-has-avx"
There was a problem hiding this comment.
I was sort of also taking inspiration from compiletest-based tests in rust-lang/rust, but I know the parsing there is sort of gnarly nowadays given all the directives it supports.
Yeah this is the kind of thing I am afraid of
There was a problem hiding this comment.
Ok, I've switched back to TOML
alexcrichton
changed the title
asm test suite for Wasmtimedisas test suite for Wasmtime
* Add an `asm` test suite for Wasmtime This commit adds a suite of tests at `tests/asm/*.wat` which is intended to replace the tests in `cranelift/filetests/filetests/wasm`. Tests are configured differently than before using Wasmtime CLI flags rather than a custom TOML-based configuration scheme. Otherwise though the same shape of tests is supported. This commit migrates a small handful of tests as a showcase and bulk migration is left for a follow-up. * Organize the asm.rs test with methods/functions * Switch back to TOML for config parsing
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Takes a bit too long in cranelift
2 participants
This commit adds a suite of tests at
tests/disas/*.watwhich is intended to replace the tests incranelift/filetests/filetests/wasm. Tests are configured differently than before using Wasmtime CLI flags rather than a custom TOML-based configuration scheme. Otherwise though the same shape of tests is supported.This commit migrates a small handful of tests as a showcase and bulk migration is left for a follow-up.