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

rustc: wasm_import_module confuses module names of identically-named functions #50021

Closed
lostman opened this issue Apr 17, 2018 · 1 comment · Fixed by #67363
Closed

rustc: wasm_import_module confuses module names of identically-named functions #50021

lostman opened this issue Apr 17, 2018 · 1 comment · Fixed by #67363
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@lostman
Copy link

lostman commented Apr 17, 2018

The following code illustrates the issue:

#![allow(dead_code, unused_imports)]
#![feature(lang_items, core_intrinsics)]
#![feature(custom_attribute)] // <- required for wasm_import_module
#![feature(wasm_import_module)]
#![no_std]
use core::intrinsics;

#[lang = "panic_fmt"]
#[no_mangle]
pub extern fn rust_begin_panic(
    _msg: core::fmt::Arguments,
    _file: &'static str,
    _line: u32,
    _column: u32) -> ! {
        unsafe { intrinsics::abort() }
}

mod m1 {
    #[wasm_import_module = "m1"]
    extern "C" {
        pub fn f();
    }
    #[wasm_import_module = "m1"]
    extern "C" {
        pub fn g();
    }
}

mod m2 {
    #[wasm_import_module = "m2"]
    extern "C" {
        pub fn f(_: i32);
    }
}

#[no_mangle]
pub unsafe fn run() {
    m1::g();

    // In generated code, expected:
    // (import "m2" "f" (func $f (param i32)))
    // but got:
    // (import "m1" "f" (func $f (param i32)))
    m2::f(0);
}

To compile:

❯ rustc \
--crate-type=cdylib \
--target=wasm32-unknown-unknown \
-o result.wasm \
src/lib.rs

Then, using binaryen's wasm-dis:

❯ wasm-dis foo.wasm | grep import
(import "m1" "g" (func $g))
(import "m1" "f" (func $f (param i32)))

The type of f is correct since m1::f() takes no arguments, but the module name is not. Looks like rustc has the right function but with wrong module name.

@XAMPPRocky XAMPPRocky added A-linkage Area: linking into static, shared libraries and binaries T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ C-bug Category: This is a bug. labels Aug 27, 2018
@lostman
Copy link
Author

lostman commented Oct 4, 2018

Just checked with the latest nightly:

$ rustc --version
rustc 1.31.0-nightly (5597ee8a6 2018-10-03)

This code still generates an incorrect Wasm:

#![allow(dead_code, unused_imports)]
#![feature(lang_items, core_intrinsics)]
#![feature(custom_attribute)] // <- required for wasm_import_module
#![no_std]
use core::intrinsics;
use core::panic::PanicInfo;

#[panic_handler]
fn panic_handler(_: &PanicInfo) -> ! {
    unsafe {
        ::core::intrinsics::abort()
    }
}

mod m1 {
    #[link(wasm_import_module = "m1")]
    extern "C" {
        pub fn f();
    }
    #[link(wasm_import_module = "m1")]
    extern "C" {
        pub fn g();
    }
}

mod m2 {
    #[link(wasm_import_module = "m2")]
    extern "C" {
        pub fn f(_: i32);
    }
}

#[no_mangle]
pub unsafe fn run() {
    m1::f();
    m1::g();
    m2::f(0);
}

The current issue is different though:

(module
 (type $0 (func))
 (type $1 (func (param i32)))
 (import "m2" "f" (func $f))
 (import "m1" "g" (func $g))
 (global $global$0 (mut i32) (i32.const 1048576))
 (global $global$1 i32 (i32.const 1048576))
 (global $global$2 i32 (i32.const 1048576))
 (table 1 1 anyfunc)
 (memory $0 16)
 (export "memory" (memory $0))
 (export "__indirect_function_table" (table $0))
 (export "__heap_base" (global $global$1))
 (export "__data_end" (global $global$2))
 (export "run" (func $run))
 (func $__wasm_call_ctors (; 2 ;) (type $0)
 )
 (func $run (; 3 ;) (type $0)
  (call $f)
  (call $g)
  (call $.Lf_bitcast
   (i32.const 0)
  )
 )
 (func $.Lf_bitcast (; 4 ;) (type $1) (param $var$0 i32)
  (call $f)
 )
)

We are calling the same function f twice. Once with and once without the integer argument!

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Dec 19, 2019
…=eddyb

Fix handling of wasm import modules and names

The WebAssembly targets of rustc have weird issues around name mangling
and import the same name from different modules. This all largely stems
from the fact that we're using literal symbol names in LLVM IR to
represent what a function is called when it's imported, and we're not
using the wasm-specific `wasm-import-name` attribute. This in turn leads
to two issues:

* If, in the same codegen unit, the same FFI symbol is referenced twice
  then rustc, when translating to LLVM IR, will only reference one
  symbol from the first wasm module referenced.

* There's also a bug in LLD [1] where even if two codegen units
  reference different modules, having the same symbol names means that
  LLD coalesces the symbols and only refers to one wasm module.

Put another way, all our imported wasm symbols from the environment are
keyed off their LLVM IR symbol name, which has lots of collisions today.
This commit fixes the issue by implementing two changes:

1. All wasm symbols with `#[link(wasm_import_module = "...")]` are
   mangled by default in LLVM IR. This means they're all given unique names.

2. Symbols then use the `wasm-import-name` attribute to ensure that the
   WebAssembly file uses the correct import name.

When put together this should ensure we don't trip over the LLD bug [1]
and we also codegen IR correctly always referencing the right symbols
with the right import module/name pairs.

Closes rust-lang#50021
Closes rust-lang#56309
Closes rust-lang#63562

[1]: https://bugs.llvm.org/show_bug.cgi?id=44316
Centril added a commit to Centril/rust that referenced this issue Dec 20, 2019
…=eddyb

Fix handling of wasm import modules and names

The WebAssembly targets of rustc have weird issues around name mangling
and import the same name from different modules. This all largely stems
from the fact that we're using literal symbol names in LLVM IR to
represent what a function is called when it's imported, and we're not
using the wasm-specific `wasm-import-name` attribute. This in turn leads
to two issues:

* If, in the same codegen unit, the same FFI symbol is referenced twice
  then rustc, when translating to LLVM IR, will only reference one
  symbol from the first wasm module referenced.

* There's also a bug in LLD [1] where even if two codegen units
  reference different modules, having the same symbol names means that
  LLD coalesces the symbols and only refers to one wasm module.

Put another way, all our imported wasm symbols from the environment are
keyed off their LLVM IR symbol name, which has lots of collisions today.
This commit fixes the issue by implementing two changes:

1. All wasm symbols with `#[link(wasm_import_module = "...")]` are
   mangled by default in LLVM IR. This means they're all given unique names.

2. Symbols then use the `wasm-import-name` attribute to ensure that the
   WebAssembly file uses the correct import name.

When put together this should ensure we don't trip over the LLD bug [1]
and we also codegen IR correctly always referencing the right symbols
with the right import module/name pairs.

Closes rust-lang#50021
Closes rust-lang#56309
Closes rust-lang#63562

[1]: https://bugs.llvm.org/show_bug.cgi?id=44316
Centril added a commit to Centril/rust that referenced this issue Dec 20, 2019
…=eddyb

Fix handling of wasm import modules and names

The WebAssembly targets of rustc have weird issues around name mangling
and import the same name from different modules. This all largely stems
from the fact that we're using literal symbol names in LLVM IR to
represent what a function is called when it's imported, and we're not
using the wasm-specific `wasm-import-name` attribute. This in turn leads
to two issues:

* If, in the same codegen unit, the same FFI symbol is referenced twice
  then rustc, when translating to LLVM IR, will only reference one
  symbol from the first wasm module referenced.

* There's also a bug in LLD [1] where even if two codegen units
  reference different modules, having the same symbol names means that
  LLD coalesces the symbols and only refers to one wasm module.

Put another way, all our imported wasm symbols from the environment are
keyed off their LLVM IR symbol name, which has lots of collisions today.
This commit fixes the issue by implementing two changes:

1. All wasm symbols with `#[link(wasm_import_module = "...")]` are
   mangled by default in LLVM IR. This means they're all given unique names.

2. Symbols then use the `wasm-import-name` attribute to ensure that the
   WebAssembly file uses the correct import name.

When put together this should ensure we don't trip over the LLD bug [1]
and we also codegen IR correctly always referencing the right symbols
with the right import module/name pairs.

Closes rust-lang#50021
Closes rust-lang#56309
Closes rust-lang#63562

[1]: https://bugs.llvm.org/show_bug.cgi?id=44316
@bors bors closed this as completed in aa0ef5a Dec 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries C-bug Category: This is a bug. O-wasm Target: WASM (WebAssembly), http://webassembly.org/ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants