Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conformity to binaryen's modifications to DWARF #151

Merged
merged 5 commits into from Aug 13, 2023
Merged

Conformity to binaryen's modifications to DWARF #151

merged 5 commits into from Aug 13, 2023

Conversation

dzfrias
Copy link
Contributor

@dzfrias dzfrias commented Aug 11, 2023

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
Copy link
Owner

brson commented Aug 11, 2023

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
Copy link
Contributor Author

dzfrias commented Aug 12, 2023

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
Copy link
Contributor Author

dzfrias commented Aug 12, 2023

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
Copy link
Contributor Author

dzfrias commented Aug 12, 2023

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

@brson
Copy link
Owner

brson commented Aug 13, 2023

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

@brson brson merged commit 7c54553 into brson:master Aug 13, 2023
16 of 18 checks passed
@brson
Copy link
Owner

brson commented Aug 14, 2023

@dzfrias I published 0.114.1 with your fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants