wasm-opt-rs icon indicating copy to clipboard operation
wasm-opt-rs copied to clipboard

Support binaryen version_121

Open SleeplessOne1917 opened this issue 11 months ago • 6 comments

Closes #166.

Besides updating the project to be compatible with binaryen version_121, I made a few other changes:

  • Use derive feature for strum instead of separate dependency on strum_macros.
  • Use serialize-all for enums instead of manually adding a serialize attribute to every variant
  • Remove Pass::name and instead use IntoStaticStr to generate static strings for enum variants.
  • Update third party dependencies of each component crate.
  • Add fields and functions related to PassOptions generateStackIR and optimizeStackIR fields (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.

SleeplessOne1917 avatar Feb 04 '25 21:02 SleeplessOne1917

Thanks a lot for this work @SleeplessOne1917 - here's hoping we can get this merged.

tysen avatar Apr 02 '25 21:04 tysen

I see that a test failed during the CI build, but I ran it locally and it was all green.

tysen avatar Apr 02 '25 21:04 tysen

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 PassRunner shims to add a addWithArgument method that accepts the argument as the second string, calls the inner.add method with it.
  • add the appropirate binding
  • adjust the Passes::more_passes vec to be (I guess) Vec<(Pass, Option<String>)>. Kinda ugly.
  • adjust create_and_run_pass_runner to handle the new pass-with-argument case
  • modify the integration::parse_command_args parser to handle the --pass-name=value syntax, 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 the extract-function@rust_begin_unwind syntax is probably irrelevant now and may need to be updated for current usage of the --pass-arg flag, which looks like it may no longer accept the k@v syntax.
  • 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 avatar Apr 03 '25 20:04 brson

@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.

SleeplessOne1917 avatar Apr 04 '25 22:04 SleeplessOne1917

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

HerrMuellerluedenscheid avatar Apr 17 '25 09:04 HerrMuellerluedenscheid

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.

SleeplessOne1917 avatar Apr 17 '25 19:04 SleeplessOne1917