Panic when used on .wasm built with -C link-arg=--shared

[hpohl]

When trying to use wasm-bindgen on a .wasm file built with -C link-arg=--shared, it panics when using closures.

Steps to Reproduce

$ rustup default beta (or nightly)
$ cargo init --lib
$ cat src/lib.rs
use wasm_bindgen::prelude::*;

#[no_mangle]
pub extern "C" fn foo() {
    Closure::wrap(Box::new(move || { }) as Box<dyn FnMut()>);
}
$ cat .cargo/config
[target.wasm32-unknown-unknown]
rustflags = [
    "-C", "link-arg=--shared",
]
$ cargo build --target=wasm32-unknown-unknown
$ wasm-bindgen target/wasm32-unknown-unknown/debug/file.wasm --out-dir out
thread 'main' panicked at 'out of bounds read of function table', src/libcore/option.rs:1038:5

Additional Context

Using the latest wasm-bindgen from github also panics, but at a different location.

[alexcrichton]

Thanks for the report!

I suspect there's definitely bugs in wasm-bindgen supporting the --shared mode, but I also think that the --shared support in LLD right now is probably buggy. AFAIK the position-independent-code support in LLVM for wasm is seeing a lot of development in LLVM master right now and the version that Rust is using isn't ready to go.

Could you elaborate a bit on what you're thinking on using --shared for?

[hpohl]

True, it's in active development and I wanted to try out the work done recently in LLVM/LLD so I'm working with a locally built rust compiler based on the latest LLVM master (9.0.0). So far it's looking promising :). It just broke with rust beta already, so I was showcasing that one.

We are looking into loading our library dynamically instead of statically to not have to recompile the main app every time and to ship updates faster. For that use case the JS output also is not ideal, but can be fixed manually afterwards (I think).

[alexcrichton]

Nice! Can you gist a wasm binary that you're generating with your custom toolchain for the above program? (aka what comes out of rustc)

The one I generated locally with beta/nightly looked like it was incorrectly passing along relative indices to the imported function rather than adding the local index to the global offset. We'll need to handle that internally in wasm-bindgen, but that's where I think the beta/nightly toolchains are just fundamentally broken, but it's likely fixed on LLVM master!

[hpohl]

Will do tomorrow! Don't have it here right now.

[hpohl]

https://gist.github.com/hpohl/b508d4b43d1168f13e0cebf89b5ffa79

Looks like it's adding the __table_base offset correctly.

When using the LLVM 9.0 toolchain, you don't even need to pass --shared to make it panic.

Do you want the full binary with the dylink section as well?

Also I've noticed that walrus moves the dylink section at the end of the file which makes it harder to parse. After running wasm-opt on it, it's at the top again.

[alexcrichton]

Ok that indeed looks correct! What I see there is:

 (func $_ZN12wasm_bindgen7closure16Closure$LT$T$GT$4wrap17breaks_if_inlined17hf603bccd9da952ceE.llvm.16383384261386403029 (; 12 ;) (type $5) (param $0 i32) (param $1 i32) (result i32)
  (call $__wbindgen_describe_closure
   (local.get $0)
   (local.get $1)
   (i32.add
    (global.get $gimport$4)
    (i32.const 3)
   )
  )
 )

(notice the i32.add (global.get ...))

When looking at the current toolchain with --shared I see:

 (func $_ZN12wasm_bindgen7closure16Closure$LT$T$GT$4wrap17breaks_if_inlined17hf603bccd9da952ceE.llvm.16383384261386403029 (; 12 ;) (type $5) (param $0 i32) (param $1 i32) (result i32)
  (call $__wbindgen_describe_closure
   (local.get $0)
   (local.get $1)
   (i32.const 3)
  )
 )

which doesn't have the global.add!

That wasm-bindgen breaks with LLVM 9 though is quite worrisome! The dylink section I don't know much about and I think is very new. If it needs to be at the start we need to likely add support in walrus for that. I think to fix this I'll need to get an LLVM 9-based Rust toolchain locally to start debugging and see what's going on.

[hpohl]

Here you go: https://github.com/hpohl/rust

[alexcrichton]

Ok it looks like https://bugs.llvm.org/show_bug.cgi?id=41383 is at least one upstream LLVM bug causing issues, and it looks like when that's fixed we should work by default, but still be incompatible with position independent code

[alexcrichton]

Hm no that's a bad bug as I had the wrong LLVM build used, let me try again locally...

[alexcrichton]

Ok so the bug for the default output of the target is that wasm32-unknown-unknown is still configured to use "pic" as its relocation model for LLVM, meaning it's accidentally switching to the experimental PIC output for LLVM 9, and I've fixed that in rust-lang/rust#59712.

I don't really know much about the PIC story or how it's all supposed to end up, so for now I think there's not much we can do in wasm-bindgen, but when PIC has settled down we can look to implement support!

[hpohl]

Alright! Can you guess how much work needs to be done and if it's limited to wasm-bindgen to get something working right now? I'm considering looking into it myself even though it's rather unstable right now.

[alexcrichton]

I'm honestly not entirely sure how much work needs to be done to get this 'truly working'. Small bits and pieces could get working here and there, but it seems like the purpose of PIC code is to touch on the subject of splitting wasm modules and having them dynamically load one another. That whole scenario we've given very little thought to in Rust and WebAssembly, and so almost no questions have answers and you'd need to take on a lot of the design work or fill in answers for things that we don't ourselves have answers for.

On a more micro level the closure rewriting needs to detect when a relative argument is passed out. This means that we'll need some degree of symbolic execution in the small interpreter that we have, and that's unlikely to be too much fun to do so. I suspect it'll be pretty difficult to get this working even on wasm-bindgen.

There's also questions about how this even works with JS bindings, for example how does the wasm module get the table and memory offset? The questions sort of just go on from there...

All in all you're more than welcome to try to forge on here, but we unfortunately may not be of much help other than being able to answer questions about the current state of wasm-bindgen. Without intimate technical knowledge of how PIC is supposed to work and a vision of what the end state looks like, though, it's likely to be difficult to make progress.

[hpohl]

I understand. My case should be very basic still so maybe I will give it a shot. From what I have seen the JS bindings don't need many changes to get them to work and the dynamic loader I can somewhat base on the emscripten dynamic loading code.

[alexcrichton]

This hasn't seen activity in some time now and at this time --shared changes so many things that wasm-bindgen is pretty unlikely to support it. I'm going to close this and we can follow up later if necessary.