brson / wasm-opt-rs

Rust bindings for Binaryen's wasm-opt
Apache License 2.0
64 stars 10 forks source link

Conformity to binaryen's modifications to DWARF #151

Closed dzfrias closed 1 year ago

dzfrias commented 1 year ago

This PR includes the third party LLVM source used in binaryen. This is necessary because the related DWARF stuff depends on this LLVM code. Along with BUILD_LLVM_DWARF, I set NDEBUG, as I ran into a linker error without it. This is done by CMake automatically in binaryen anyway, so this is still conforming to the upstream wasm-opt.

Lastly, I had to silence warnings because LLVM uses some deprecated APIs. binaryen silences these cleanly with CMake, but I'm not sure how to do the same with cxx_build. So, for the time being at least, -w is passed to the compiler.

Related issue: #150.

brson commented 1 year ago

Hi @dzfrias. Thank you for your investigations on the linked issue and this patch.

I am guessing that we haven't seen these discrepancies in dwarf handling because we have only tested with a small number of wasms, output by the rust compiler, presumably without emitting dwarf.

We have a project called components/conformance-tests that is supposed to verify that the crate behaves the same as the upstream binaryen wasm-opt. It is run with cargo test --manifest-path=components/conformance-tests/Cargo.toml. It uses pre-built wasms located in components/wasm-opt/tests.

I would first like to add a test case there that uses a dwarf-containing wasm input, and verifies the incorrect dwarf behavior. If you would like to write that test it would be welcome, otherwise I'll spend some time on it this weekend. I need to either figure out how to get rustc to emit dwarf info, or use tinygo as in your example.

There are two things I want to change about this patch:

First it has a minor a build error:

https://github.com/brson/wasm-opt-rs/actions/runs/5836602579/job/15833360228?pr=151

error[E0277]: `[PathBuf; 63]` is not an iterator
  --> components/wasm-opt-sys/build.rs:81:16
   |
81 |         .files(llvm_files)
   |                ^^^^^^^^^^
   |                |
   |                expected an implementor of trait `IntoIterator`
   |                help: consider borrowing here: `&llvm_files`
   |
   = note: the trait bound `[PathBuf; 63]: IntoIterator` is not satisfied
   = note: required because of the requirements on the impl of `IntoIterator` for `[PathBuf; 63]`

Second, you've written some code to disambiguate the name of the "DWARF.cpp" object file. This is needed because of limitations in the cc / cxx_build crates - they dump all the object files into a single cargo target directory, so any duplicate input file names cause collisions in the object file names.

We have already run into this and there is a helper function, disambiguate_file, in bulid.rs to handle this case. Can you please update the patch to use that function?

dzfrias commented 1 year ago

I just wrote a fix for the compiler error and the code now uses disambiguate_file!

I'll work on a test, too. I took a look at the sections of the two WebAssembly binaries, and hello_world.wasm has DWARF debug info already in it. However, the corresponding .wat file does not, as notation for custom sections hasn't been finalized yet (I think; this is the case of wasm2wat). I haven't taken a look at the contents of the tests, but it's possible that none of them test conformity using hello_world.wasm? After doing a manual conformity test between the old wasm-opt-rs and wasm-opt, they don't match up.

dzfrias commented 1 year ago

I'm noticing a linking error on macos. I'm on a mac, and this issue strangely only occurs when I run cargo test at the root... I'll work on fixing it.

edit: Even stranger, the tests all pass individually. Just running at root causes the error. Do you have any idea why this happens?

edit2: I was mistaken. The issue is related to running cargo test in wasm-opt-sys. Not sure what the differences could be... unless cxx does some #[cfg(test)] stuff.

dzfrias commented 1 year ago

I pushed a test for DWARF line-info conformity, after verifying that it worked without my previous commits.

brson commented 1 year ago

This looks perfect. Thank you for taking the time to make this fixes @dzfrias. I'll start working on a new release.

brson commented 1 year ago

@dzfrias I published 0.114.1 with your fixes.