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

expose math symbols on wasm32-unknown-unknown #248

Merged
merged 3 commits into from
Jul 18, 2018

Conversation

japaric
Copy link
Member

@japaric japaric commented Jul 13, 2018

r? @alexcrichton

I think this should do the trick. I wasn't sure how to write a test that checks that the symbols make it to a wasm file -- it seems that the wasm linker is lenient and doesn't throw errors when undefined symbols make it to the final artifact.

@japaric
Copy link
Member Author

japaric commented Jul 15, 2018

latest commit adds more math functions. Only four are missing from the list in rustwasm/team#84 (comment), but people are already working on them.

@japaric
Copy link
Member Author

japaric commented Jul 15, 2018

Once those four are in we'll also have all the functions required to put math support in core so we should think about the possible ways to do that.

(a) One way is to merge src/libstd/{f32,f64}.rs into src/libcore/num/{f32,f64}.rs (e.g. we move the current f32.sin implementation into core) and add all math symbols to compiler-bulitins. In that case the src/math.rs file in this repo should be available on all targets, and not just on wasm.

(b) The other way is to source import libm into core and implement the math methods by directly calling the libm functions, instead of using llvm intrinsics (e.g. intrinsics::sinf32). I don't know if there's any (optimization) advantage to using the llvm intrinsics, though.

The downside to (a) is that downstream users could run into linker errors ("undefined reference") if we forget to include some math symbol. (a) may also cause linker errors ("duplicate symbol") when linking statically, which is how the thumb targets do linking, if the user also links to e.g. glibc's libm.

The advantages to (b) are (i) llvm would be able to inline code (*) and remove unreachable branches in some cases producing smaller binaries, and (ii) it would be possible to turn the math methods into const functions in the future (we can't mark functions that call llvm intrinsics as const fn -- unless we use compiler magic, maybe).

(*) it might be possible to achieve inlining using approach (a) if we get link-time / cross-language LTO working.

I think doing either (a) or (b) would have the effect that all users would use our libm implementation of math functions (instead of e.g. glibc's libm implementation) and would close the possibility of providing your own implementation of math functions -- unless we do (a) and make the symbols weak.

So I would vote for approach (b) but we should implement it right after the next beta cutoff to maximize testing time.

@alexcrichton
Copy link
Member

Hm I agree that the first strategy probably isn't workable, even for normal Linux distributions the linker command would look like:

cc .... crate.o ... -lm ... libcompiler_builtins.rlib

If crate.o references the sin symbol that'll instruct the linker to pull in libm.so to resolve the symbol. The symbol sin, though, is defined in some object in libcompiler_bulitins.rlib. That object may have some other symbols as well. If crate.o also references one of those other symbols in the compiler-builtins rlib then we'll get a duplicate symbol error for sin I believe.

Note that this strategy I think is workable if we compile each function in compiler-builtins into its own object file, which is indeed always a possibility!

For strategy (b) I'm a little hesitant to do that as well. I agree that it should solve the symbol visibility issue, but it has the downside of forcing users to use our own implementation which may not be as fast as the native implementations. Additionally I'm not sure if we want to commit long-term to making these const fns :)

Now all of this is somewhat moot if it's just for wasm specifically which has no other competing library here (other than shelling out to Math.sin and such). I'm curious on your thoughts on this though?

@japaric
Copy link
Member Author

japaric commented Jul 17, 2018

My main motivation for this is making (pure Rust) math support available to targets that don't have std support, like the thumb targets. I think that the Redox project might also be interested in pure Rust math support (cc @jackpot51); in their case they probably want the same as wasm (symbols in compiler-builtins) because I think they compile rust-lang/rust's libstd as it is.

In the case of the thumb targets they could mostly use the libm crate as it is (it contains extension traits that add methods like sin to f32 and f64) to get pure Rust math support. Except in the case of the modulo operation on floats; I believe in that case LLVM generates calls to the fmod / fmodf symbols and those would need to be provided by compiler-builtins or by some C implementation of libm (because of how linkers work). But that operation doesn't seem too common so it may not be an issue in practice.

In conclusion, I think that right now it may be best to provide only these math symbols for wasm and see how the use of the libm crate works out for the thumb targets. We can revisit putting math support in core after the edition release.

@alexcrichton
Copy link
Member

Sounds reasonable to me! I think it'd also be fine to include this on thumb targets as well, but if it doesn't affect libcore other than fmod it may have limited utility :(

@japaric
Copy link
Member Author

japaric commented Jul 18, 2018

All the required functions by wasm are in now.

I think it'd also be fine to include this on thumb targets as well, but if it doesn't affect libcore other than fmod it may have limited utility

I think that might cause linker errors if the program is already being statically linked to libm but I can't check that right now / this week so I would hold off from doing that in this PR.

@alexcrichton
Copy link
Member

Sounds reasonable to me! I think the wasm CI is still failing though?

@japaric
Copy link
Member Author

japaric commented Jul 18, 2018

Make sure they respect the `mangled-names` feature as well as have the `"C"`
ABI.
@alexcrichton
Copy link
Member

I pushed up another commit to use intrinsics! to ensure that these respect the mangled-names feature as well as have the C abi, but otherwise looks great!

Thanks so much for pushing on this @japaric, this is some amazing work!

@alexcrichton alexcrichton merged commit b4a3645 into rust-lang:master Jul 18, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jul 18, 2018
This commit updates the compiler-builtins submodule, primarily pulling in
rust-lang/compiler-builtins#248 which should resolve a number of
undefined symbols on the wasm target by default (instead of leaving them in the
final binary).
@japaric japaric deleted the libm branch July 21, 2018 17:31
@parasyte
Copy link

parasyte commented Aug 12, 2018

@japaric I would like to use this for my own custom target, too. Any thoughts on best approach? Right now I am using libm directly, and including a modified copy of math.rs for the intrinsics.

edit: Reached out to you on Discord, too.

@japaric
Copy link
Member Author

japaric commented Aug 13, 2018

@parasyte You mean you are compiling std for your custom target and are writing std applications? If that's your use case then we could provide the symbols behind a "math" Cargo feature.

If you are writing no_std applications then libm and its extension traits is the (stable) way to go.

@parasyte
Copy link

@japaric It's no_std for now, but I'm looking into what it will take to port the most important parts of std or your very own steed.

Thanks though, that answers my immediate question. 👍

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

3 participants