Always have math functions but with weak linking attribute if we can
#577
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Amjad50
force-pushed
the
weak_link_libm
branch
from
537e8cd
to
7b161ab
Compare
|
For some reason, windows building fails, and the error is confusing, it can't find some symbols that are unrelated to what have changed. |
|
Can you check this @Amanieu? Thanks |
|
We're currently waiting for fixes on the rustc side before work on compiler-builtins can continue: rust-lang/rust#121552 |
|
Ah, thanks for the info. |
|
CI should be fixed now, can you rebase? |
|
Thanks, Builds now |
Amanieu
reviewed
Mar 28, 2024
Amanieu
reviewed
Mar 29, 2024
src/math.rs
Outdated
| all(target_arch = "xtensa", target_os = "none"), | ||
| all(target_vendor = "fortanix", target_env = "sgx") | ||
| ))] | ||
| #[cfg(not(target_os = "windows"))] |
There was a problem hiding this comment.
Suggested change
| #[cfg(not(target_os = "windows"))] | |
| #[cfg_attr(all(not(windows), not(target_vendor = "apple")), weak)] |
There was a problem hiding this comment.
We do have this already inside no_mangle, so probably just remove the cfg completely is a better choice
There was a problem hiding this comment.
No, we shouldn't provide the math intrinsics at all on windows/apple targets since weak linkage doesn't work properly. On those target we should always be getting these symbols from libc. The only exception is lgamma_r and lgammaf_r which are missing on MSVC targets.
There was a problem hiding this comment.
then you probably mean to use cfg and not cfg_attr? The suggested change is to use cfg_attr(weak) for non windows/apple
This is a replacement for rust-lang/libm#290 This fixes crashes during compilations for targets that don't have math symbols by default. So, we will provide them libm symbols, but mark it as `weak` (if its supported), so that the linker will choose the system builtin functions, since those are sometimes more optimized. If the linker couldn't find those, it will go with `libm` implementation.
taiki-e
added a commit
to taiki-e/portable-atomic
that referenced
this pull request
Apr 17, 2024
```
error[E0658]: use of unstable library feature 'proc_macro_byte_character'
--> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/proc-macro2-1.0.80/src/wrapper.rs:871:21
|
871 | proc_macro::Literal::byte_character(byte)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #115268 <rust-lang/rust#115268> for more information
= help: add `#![feature(proc_macro_byte_character)]` to the crate attributes to enable
= note: this compiler was built on 2024-03-21; consider upgrading it if it is out of date
error[E0658]: use of unstable library feature 'proc_macro_c_str_literals'
--> /home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/proc-macro2-1.0.80/src/wrapper.rs:898:21
|
898 | proc_macro::Literal::c_string(string)
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: see issue #119750 <rust-lang/rust#119750> for more information
= help: add `#![feature(proc_macro_c_str_literals)]` to the crate attributes to enable
= note: this compiler was built on 2024-03-21; consider upgrading it if it is out of date
```
Also, the latest nightly cannot be used due to rust-lang/compiler-builtins#577.
```
= note: /usr/lib/gcc/avr/5.4.0/../../../avr/lib/avr6/libm.a(addsf3.o): In function `__addsf3':
(.text.avr-libc.fplib+0x2): multiple definition of `__addsf3'
/home/runner/work/portable-atomic/portable-atomic/target/no-std-test/avr-unknown-gnu-atmega2560/debug/deps/libcompiler_builtins-81c93cbf49042e5b.rlib(compiler_builtins-81c93cbf49042e5b.compiler_builtins.3808c58458fddf11-cgu.07.rcgu.o):/home/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/compiler_builtins-0.1.109/src/macros.rs:500: first defined here
```
taiki-e
added a commit
to taiki-e/portable-atomic
that referenced
this pull request
Apr 17, 2024
rust-lang/compiler-builtins#577 caused regression on no-std hexagon: ``` error: symbol 'fma' is already defined error: symbol 'fmax' is already defined ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a replacement for rust-lang/libm#290
This fixes crashes during compilations for targets that don't have math symbols by default.
So, we will provide them libm symbols, but mark it as
weak(if its supported), so that the linker will choose the system builtin functions, since those are sometimes more optimized.If the linker couldn't find those, it will go with
libmimplementation.