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

Predefine a few math C functions like ldexpf, even without WASI? #407

Closed
hrydgard opened this issue Apr 30, 2019 · 10 comments
Closed

Predefine a few math C functions like ldexpf, even without WASI? #407

hrydgard opened this issue Apr 30, 2019 · 10 comments
Labels
🎉 enhancement New feature!

Comments

@hrydgard
Copy link

hrydgard commented Apr 30, 2019

When using f32.powf(f32) from a wasm module written in Rust, an unresolved dependency is created on ldexpf.

Maybe should predefine a few functions like this? Here's a working, although not very well tested, implementation:

fn wasm_ldexpf(
    _ctx: &mut wasmer_runtime::Ctx,
    arg: f32,
    exp: i32,
) -> f32 {
    // Construct a float directly with the desired exponent and multiply with it.
    let multiplier : f32 = unsafe { std::mem::transmute((exp + 127) << 23) };
    arg * multiplier
}

fn libc_imports() -> wasmer_runtime::ImportObject {
    wasmer_runtime::imports! {
        "env" => {
            "ldexpf" => wasmer_runtime::func!(wasm_ldexpf),
        },
    }
}

Or alternatively, maybe the LLVM wasm compiler should be fixed to add intrinsics for these...

@hrydgard hrydgard added the 🎉 enhancement New feature! label Apr 30, 2019
@Hywan
Copy link
Contributor

Hywan commented Apr 30, 2019

Hello, thanks for the issue!

What backend are you using? Cranelift, LLVM, or singlepass? What ABI are you using: WASI ou emscripten?

Thanks :-)!

@hrydgard
Copy link
Author

hrydgard commented Apr 30, 2019

Cranelift. No ABI, just straight Wasmer.

I suspect the real issue is why the Rust wasm compiler backend is generating it in the first place, and not really a wasmer issue... I was doing 2.0f32.powf(something) which I could imagine getting optimized to ldexpf, but then that should be implemented, somewhere...

@syrusakbary
Copy link
Member

syrusakbary commented Apr 30, 2019

To summarize what @hrydgard commented (please correct me if I got the wasi target assumption wrong).

He had Rust project that have a 2.0f32.powf(something) in the code. This project, when compiled using the wasi target was requiring the normal wasi imports (wasi_unstable namespace), plus an extra env namespace, with a function named ldexpf on it.

Right now, our wasi implementation only have the wasi_unstable namespace, but no env namespace for the libc imports.

I think it should be easy to resolve in our side.
I would like to know though, if that's going to be a norm when compiling certain certain code from Rust, so we can cover the rest of expected functions in the env namespace. And if so, what are the other env functions that we should cover?

@wasmerio wasmerio deleted a comment from xmclark Apr 30, 2019
@syrusakbary
Copy link
Member

To follow up on this. We will need to know if this is intentional when compiling Rust to the WebAssembly-WASI target or if it's a bug on the Rust WASI target (and fix it in one side or the other).

@alexcrichton

@xmclark
Copy link
Contributor

xmclark commented May 1, 2019

@hrydgard to clarify, you're compiling to wasm32-unknown-unknown?

I am not sure if that target will correctly polyfill for f32.powf. I just tried building a basic program for the wasm32-unknown-wasi target and it does fill that function correctly. If you are building for the -unknown target, you won't be able to execute it on wasmer cli in the same way we can execute wasi or emscritpen wasm binaries.

You can execute functions in wasm built for the -unknown target if you embed the wasmer runtime, and you can add that additional import.

I think having some predefined imports for projects built with C or Rust is a fine idea, but we haven't come up with a good way of doing that yet. We are currently only supporting the imports defined by the ABI.

@hrydgard
Copy link
Author

hrydgard commented May 2, 2019

Clarifying: Yeah, compiling to wasm32-unknown-unknown.

However, I've been trying to hack https://github.com/wasmerio/wasmer-rust-example into a clean little repro, but have not yet succeeded - it works there. Will post again here if I do find a solid repro.

@alexcrichton
Copy link

alexcrichton commented May 2, 2019

It's a bug in the Rust wasm32-unknown-unknown target if it references any symbols that the user didn't explicitly reference themselves. This case, for example, is a bug because it's doubtful your program ever explicitly referenced the ldexpf symbol. LLVM, however, can inject references to these symbols:

#[no_mangle]                     
pub extern fn foo(a: u8) -> f32 {
    2.0f32.powf(a as f32)        
}                                

when compiled as:

$ rustc +nightly foo.rs -O --target wasm32-unknown-unknown --crate-type cdylib
$ wasm2wat foo.wasm

yields:

(module
  (type (;0;) (func (param f32 i32) (result f32)))
  (type (;1;) (func))
  (type (;2;) (func (param i32) (result f32)))
  (import "env" "ldexpf" (func $ldexpf (type 0)))
  (func $__wasm_call_ctors (type 1))
  (func $foo (type 2) (param i32) (result f32)
    f32.const 0x1p+0 (;=1;)
    local.get 0
    call $ldexpf)
  (table (;0;) 1 1 funcref)
  (memory (;0;) 16)
  (global (;0;) (mut i32) (i32.const 1048576))
  (global (;1;) i32 (i32.const 1048576))
  (global (;2;) i32 (i32.const 1048576))
  (export "memory" (memory 0))
  (export "__heap_base" (global 1))
  (export "__data_end" (global 2))
  (export "foo" (func $foo)))

so all in all this is a bug in the Rust target, we should be providing a definition of ldexpf for LLVM to use since it may inject references to it.

EDIT: this will be fixed once https://github.com/rust-lang/rust/pull/60491/files merges and is published in nightly for Rust

@hrydgard
Copy link
Author

hrydgard commented May 2, 2019

Right, good example! I'd argue though that LLVM is in the wrong to emit a reference to such a trivial function though - it could just as well emit the few CPU instructions necessary to implement it, although that might be an optimization level thing?

alexcrichton added a commit to alexcrichton/libm that referenced this issue May 2, 2019
@Hywan
Copy link
Contributor

Hywan commented May 7, 2019

We will close this issue as soon as rust-lang/rust#60491 is merged.

Thanks @alexcrichton and @hrydgard!

@syrusakbary
Copy link
Member

Closing the issue as the side PR was just merged into Rust ( rust-lang/rust#60491 ).
Thanks everyone for your help on debugging and fixing the issue!

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

No branches or pull requests

5 participants