Support binaryen version_121
Closes #166.
Besides updating the project to be compatible with binaryen version_121, I made a few other changes:
- Use derive feature for
struminstead of separate dependency onstrum_macros. - Use serialize-all for enums instead of manually adding a serialize attribute to every variant
- Remove
Pass::nameand instead useIntoStaticStrto generate static strings for enum variants. - Update third party dependencies of each component crate.
- Add fields and functions related to
PassOptionsgenerateStackIRandoptimizeStackIRfields (these are present in binaryen version_121 but did not exist yet in version_116). - Add binding for
containsPass(present in binaryen version_121 but did not exist yet in version_116). - Apply changes suggested by running
cargo clippy.
I'm opening this as a draft since I'm not really familiar with binaryen or the WASM spec more generally. I made this PR by referring to previous commits in this repository adding support for newer binaryen versions.
@brson @Aimeedeer pinging you two to make sure you see this since this repo hasn't received any new commits on the master branch for close to a year and you are the primary contributors.
Thanks a lot for this work @SleeplessOne1917 - here's hoping we can get this merged.
I see that a test failed during the CI build, but I ran it locally and it was all green.
Thanks for doing all this legwork @SleeplessOne1917.
Pass arguments
The most difficult failure here looks to be related to binaryen "pass arguments". This causes the failures in pass_arg works and wasm_to_wasm_pass_arg. The latter is in the conformance tests, which can be run with
cargo test --manifest-path=components/conformance-tests/Cargo.toml
Some of the problems here are in the integration module, which is used to model the wasm-opt CLI on top of the wasm-opt-rs API. The integration module is perhaps not very important and could be left broken, except that it is used by conformance-tests, which needs to keep working.
The pass args are kind of an obscure and underdocumented feature of binaryen. We could just break them, but I think this can be fixed.
In the broken tests, this problem arises when using the extract-function pass, which has a required argument naming the function to extract.
On the command line, this used to look like
wasm-opt --extract-function --pass-arg extract-function@rust_begin_unwind
Now it appears to only work as
wasm-opt --extract-function=rust_begin_unwind
Internally to binaryen the Pass object has an optional string argument, which the bindings never set.
It looks like to me like the fix is:
- adjust the
PassRunnershims to add aaddWithArgumentmethod that accepts the argument as the second string, calls theinner.addmethod with it. - add the appropirate binding
- adjust the
Passes::more_passesvec to be (I guess)Vec<(Pass, Option<String>)>. Kinda ugly. - adjust
create_and_run_pass_runnerto handle the new pass-with-argument case - modify the
integration::parse_command_argsparser to handle the--pass-name=valuesyntax, then apply it to the new api. there's an existing todo for this syntax and I guess it previously could be worked around. the existing parser for theextract-function@rust_begin_unwindsyntax is probably irrelevant now and may need to be updated for current usage of the--pass-argflag, which looks like it may no longer accept thek@vsyntax. - fix the two tests
I will hack on this myself shortly unless someone else really wants to tackle it, but I don't expect anything more after waiting so long here.
I haven't looked at the failure in optimization_read_module_error_works but this test seems like it should still be valid.
Almost all the conformance tests are failing. This indicates that the API is not configuring binaryen the same way as wasm-opt is. I have not looked into it yet.
These are run with
cargo test --manifest-path=components/conformance-tests/Cargo.toml
The initial run takes a long time because it is rebuilding the c++ wasm-opt to compare against.
@brson I made your suggested changes except for fixing the tests. The conformance tests are now failing with a different issue and I'm not sure how to advance. Most of the conformance tests are failing when the API outfile is compared with the Rust out file. All the test output says is that one massive byte vector was produced when it should have been another massive byte vector and I can't see anything that indicates what the actual issue is.
Is this ready to be merged? binaryen 116 does not provide a binary for linux arm64 which was added with 117 :( So, would be awesome to have this merged
Besdies the CI build failing, we'll need to address the points I made in the comments I haven't marked as resolved already before merging. I'm hoping this can be merged sooner rather than later since cargo-leptos uses this library and I use Leptos.