brson / wasm-opt-rs

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

Namespace conflicts with LLVM #154

Open xermicus opened 1 year ago

xermicus commented 1 year ago

In Solang, a Solidity compiler, we use LLVM (via inkwell) for emitting code and wasm-opt for optimizing our Wasm artifacts.

We noticed that, starting from version 0.114.0 on, wasm-opt introduces many namespace conflicts during linking, if it is used together with llvm-sys or as a transitive dependency.

Examples (full output too long to fit in the issue) = note: /usr/bin/ld: /home/glow/code/foo/target/debug/deps/libllvm_sys-a8e7e255aa417530.rlib(Debug.cpp.o):(.bss._ZN4llvm9DebugFlagE+0x0): multiple definition of `llvm::DebugFlag'; /home/gl ow/code/foo/target/debug/deps/libwasm_opt_sys-39024ee919d3d0a2.rlib(4036ad2f21ce3a71-LLVMDebug.o):/home/glow/code/foo/target/debug/build/wasm-opt-sys-6495f62a81ee79e6/out/LLVMDebug.cpp:43: first defined here 1 /usr/bin/ld: /home/glow/code/foo/target/debug/deps/libllvm_sys-a8e7e255aa417530.rlib(Debug.cpp.o): in function `llvm::dbgs() [clone .localalias]': 2 Debug.cpp:(.text._ZN4llvm4dbgsEv+0x0): multiple definition of `llvm::dbgs()'; /home/glow/code/foo/target/debug/deps/libwasm_opt_sys-39024ee919d3d0a2.rlib(4036ad2f21ce3a71-LLVMDeb ug.o):/home/glow/code/foo/target/debug/build/wasm-opt-sys-6495f62a81ee79e6/out/LLVMDebug.cpp:148: first defined here ... Path.cpp:(.text._ZN4llvm3sys4path6nativeERKNS_5TwineERNS_15SmallVectorImplIcEENS1_5StyleE+0x0): multiple definition of `llvm::sys::path::native(llvm::Twine const&, llvm::SmallVectorImpl&, llvm::sys::path::Style)'; /home/glow/code/foo/target/debug/deps/libwasm_opt_sys-39024ee919d3d0a2.rlib(dae784741d7c58d5-Path.o):/home/glow/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wasm-opt-sys-0.114.1/binaryen/third_party/llvm-project/Path.cpp:477: first defined here collect2: error: ld returned 1 exit status

On a quick glance this seems to be caused by wasm-opt including a bunch of LLVM source dependencies.

This leaves anyone depending on LLVM and wasm-opt at the same time in an unfortunate situation.

To make matters worse, while Linux ld detects that and aborts the compilation, we noticed on our CI that on Mac (arm) the problem is worse: The linker on MacOS doesn't seem to care and compiles it anyways, instead resulting in a broken build artifact segfaulting at runtime :sweat_smile:

Minimal reproducer

Just cargo build on a new rust project with the following:

Cargo.toml

[package]
name = "foo"
version = "0.1.0"
edition = "2021"

[dependencies]
llvm-sys = "150.0.0"
wasm-opt = "0.114"

src/main.rs

fn main() {
    unsafe { llvm_sys::core::LLVMModuleCreateWithName(std::ptr::null()) };
    wasm_opt::OptimizationOptions::new_opt_level_0();
}
brson commented 1 year ago

Hi @xermicus.

Thanks for this report. We've reproduced the problem.

It was introduced in 0.114.1, in this pr https://github.com/brson/wasm-opt-rs/pull/151. I thought this would not be a breaking change, but hadn't considered the possibility you have run into.

As a temporary workaround, if you pin wasm-opt to 0.114.0, the problem should go away. This can be done by using "=0.114.0" as the version specifier.

Binaryen includes LLVM source code to handle DWARF, and previously we (accidentally) did not compile these files, so prior to 0.114.1 the wasm-opt crate did not correctly execute any of binaryen's DWARF passes. This likely hadn't been noticed because most users are telling rustc not to emit DWARF for release.

At the moment I can think of two possible resolutions:

In the short-term we can add a cargo feature to control whether the LLVM/DWARF support is compiled, probably "dwarf". For a 0.114.2 release this would probably be a non-default feature for compatibility with 0.114.0, but for 0.115.0 it would be a default feature that would need to be manually disabled. I would probably then yank the 0.114.1 release.

Would that work for you @xermicus?

A more desirable solution would be to somehow disambiguate the LLVM names used by binaryen. It's not obvious to me how to do this at the moment. Just rewriting the LLVM namespaces to something unambiguous seems possible, though from a glance it looks like some of the LLVM code is not namespaced.

cc @dzfrias some discussion here about potential future changes to how the LLVM source files are compiled. Could affect your usage.

dzfrias commented 12 months ago

Hmm, that's a tough problem. I think the "dwarf" feature is a good idea, and making it enabled by default in 0.115.0 seems reasonable. Thank you for letting me know for when 0.114.2 comes out!

xermicus commented 12 months ago

Hi @brson

I see, thanks for breaking down the problem. Yeah we we're concerned about this being not trivial to resolve. Solving the problem properly by disambiguating the namespaces is probably what we would need in the long term. But I think a feature flag might work for us short term (wasm-opt is also a transitive dependency in our case). Thanks for catching it up!

tareknaser commented 11 months ago

Hey @brson

Any updates on this issue? Are you planning to release a new version with a fix soon?

brson commented 11 months ago

@tareknaser I think we'll put out a new build of 0.114 this week, and a also 0.116.

brson commented 11 months ago

We've done as discussed previously:

xermicus commented 10 months ago

@brson thank you!