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.

Sign up for GitHub

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

Jump to bottom

Mark {f32,f64}::{next_up,next_down,midpoint} inline #128749

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Mark {f32,f64}::{next_up,next_down,midpoint} inline #128749

merged 1 commit into from
Aug 8, 2024

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Aug 6, 2024

Most float functions are marked #[inline] so any float symbols used by these functions only need to be provided if the function itself is used. RFL recently noticed that next_up, next_down, and midpoint for f32 and f64 are not inline, which causes linker errors when building with certain configurations https://lore.kernel.org/all/20240806150619.192882-1-ojeda@kernel.org/.

Add the missing attributes so the symbols should no longer be required.

@rustbot
Copy link
Collaborator

rustbot commented Aug 6, 2024

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 6, 2024
@ojeda ojeda mentioned this pull request Aug 6, 2024
Open
13 tasks
@ojeda
Copy link
Contributor

ojeda commented Aug 6, 2024 •
edited
Loading

@rustbot label I-rust-for-linux

Thanks Trevor!

@rustbot rustbot added the A-rust-for-linux Relevant for the Rust-for-Linux project label Aug 6, 2024
@tgross35 tgross35 changed the title Mark {f16,f32}::{next_up,next_down,midpoint} inline Mark {f32,f64}::{next_up,next_down,midpoint} inline Aug 7, 2024
@tgross35
Mark {f32,f64}::{next_up,next_down,midpoint} inline
6b3feb4 
Most float functions are marked `#[inline]` so any float symbols used by
these functions only need to be provided if the function itself is used.
RFL recently noticed that `next_up`, `next_down`, and `midpoint` for
`f32` and `f64` are not inline, which causes linker errors when building
with certain configurations [1].

Add the missing attributes so the symbols should no longer be required.

Cc: Gary Guo <gary@garyguo.net>
Reported-by: Alice Ryhl <aliceryhl@google.com>
Link: https://lore.kernel.org/all/20240806150619.192882-1-ojeda@kernel.org/ [1]
@scottmcm
Copy link
Member

scottmcm commented Aug 8, 2024

I'm kinda surprised it matters, but marking non-generic methods inline is basically always fine (other than things that are trying to polymorphize), so sure.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 8, 2024

📌 Commit 6b3feb4 has been approved by scottmcm

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Aug 8, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
@bors
Auto merge of rust-lang#128835 - matthiaskrgr:rollup-apzlbxy, r=matth…
3e9bd8b 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#128306 (Update NonNull::align_offset quarantees)
 - rust-lang#128612 (Make `validate_mir` ensure the final MIR for all bodies)
 - rust-lang#128648 (Add regression test)
 - rust-lang#128749 (Mark `{f32,f64}::{next_up,next_down,midpoint}` inline)
 - rust-lang#128795 (Update E0517 message to reflect RFC 2195.)
 - rust-lang#128825 (rm `declared_features` field in resolver)
 - rust-lang#128826 (Only suggest `#[allow]` for `--warn` and `--deny` lint level flags)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 127fbc8 into rust-lang:master Aug 8, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
@rust-timer
Unrolled build for rust-lang#128749
445c9d8 
Rollup merge of rust-lang#128749 - tgross35:float-inline, r=scottmcm

Mark `{f32,f64}::{next_up,next_down,midpoint}` inline

Most float functions are marked `#[inline]` so any float symbols used by these functions only need to be provided if the function itself is used. RFL recently noticed that `next_up`, `next_down`, and `midpoint` for `f32` and `f64` are not inline, which causes linker errors when building with certain configurations <https://lore.kernel.org/all/20240806150619.192882-1-ojeda@kernel.org/>.

Add the missing attributes so the symbols should no longer be required.
@tgross35 tgross35 deleted the float-inline branch August 8, 2024 22:09
ojeda added a commit to Rust-for-Linux/linux that referenced this pull request Aug 9, 2024
@ojeda
rust: add intrinsics to fix -Os builds
a74e56a 
Alice reported [1] that an arm64 build failed with:

    ld.lld: error: undefined symbol: __extendsfdf2
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a

    ld.lld: error: undefined symbol: __truncdfsf2
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a

Rust 1.80.0 or later together with `CONFIG_CC_OPTIMIZE_FOR_SIZE=y`
is what triggers it.

In addition, x86_64 builds also fail the same way.

Similarly, compiling with Rust 1.82.0 (currently in nightly) makes
another one appear, possibly due to the LLVM 19 upgrade there:

    ld.lld: error: undefined symbol: __eqdf2
    >>> referenced by core.20495ea57a9f069d-cgu.0
    >>>               rust/core.o:(<f64>::next_up) in archive vmlinux.a
    >>> referenced by core.20495ea57a9f069d-cgu.0
    >>>               rust/core.o:(<f64>::next_down) in archive vmlinux.a

Gary adds [1]:

> Usually the fix on rustc side is to mark those functions as `#[inline]`
>
> All of {midpoint,next_up,next_down} are indeed unstable functions not
> marked as inline...

Fix all those by adding those intrinsics to our usual workaround.

[ Trevor quickly submitted a fix to upstream Rust [2] that has already
  been merged, to be released in Rust 1.82.0 (2024-10-17). - Miguel ]

Cc: Gary Guo <gary@garyguo.net>
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/x/topic/x/near/455637364 [1]
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: rust-lang/rust#128749 [2]
Link: https://lore.kernel.org/r/20240806150619.192882-1-ojeda@kernel.org
[ Shortened Zulip link. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to Rust-for-Linux/linux that referenced this pull request Aug 9, 2024
@ojeda
rust: add intrinsics to fix -Os builds
02dfd63 
Alice reported [1] that an arm64 build failed with:

    ld.lld: error: undefined symbol: __extendsfdf2
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a

    ld.lld: error: undefined symbol: __truncdfsf2
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a

Rust 1.80.0 or later together with `CONFIG_CC_OPTIMIZE_FOR_SIZE=y`
is what triggers it.

In addition, x86_64 builds also fail the same way.

Similarly, compiling with Rust 1.82.0 (currently in nightly) makes
another one appear, possibly due to the LLVM 19 upgrade there:

    ld.lld: error: undefined symbol: __eqdf2
    >>> referenced by core.20495ea57a9f069d-cgu.0
    >>>               rust/core.o:(<f64>::next_up) in archive vmlinux.a
    >>> referenced by core.20495ea57a9f069d-cgu.0
    >>>               rust/core.o:(<f64>::next_down) in archive vmlinux.a

Gary adds [1]:

> Usually the fix on rustc side is to mark those functions as `#[inline]`
>
> All of {midpoint,next_up,next_down} are indeed unstable functions not
> marked as inline...

Fix all those by adding those intrinsics to our usual workaround.

[ Trevor quickly submitted a fix to upstream Rust [2] that has already
  been merged, to be released in Rust 1.82.0 (2024-10-17). - Miguel ]

Cc: Gary Guo <gary@garyguo.net>
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/x/topic/x/near/455637364 [1]
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: rust-lang/rust#128749 [2]
Link: https://lore.kernel.org/r/20240806150619.192882-1-ojeda@kernel.org
[ Shortened Zulip link. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Mr-Bossman pushed a commit to Mr-Bossman/linux that referenced this pull request Sep 16, 2024
@ojeda @Mr-Bossman
rust: add intrinsics to fix -Os builds
626f43e 
Alice reported [1] that an arm64 build failed with:

    ld.lld: error: undefined symbol: __extendsfdf2
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a

    ld.lld: error: undefined symbol: __truncdfsf2
    >>> referenced by core.a6f5fc5794e7b7b3-cgu.0
    >>>               rust/core.o:(<f32>::midpoint) in archive vmlinux.a

Rust 1.80.0 or later together with `CONFIG_CC_OPTIMIZE_FOR_SIZE=y`
is what triggers it.

In addition, x86_64 builds also fail the same way.

Similarly, compiling with Rust 1.82.0 (currently in nightly) makes
another one appear, possibly due to the LLVM 19 upgrade there:

    ld.lld: error: undefined symbol: __eqdf2
    >>> referenced by core.20495ea57a9f069d-cgu.0
    >>>               rust/core.o:(<f64>::next_up) in archive vmlinux.a
    >>> referenced by core.20495ea57a9f069d-cgu.0
    >>>               rust/core.o:(<f64>::next_down) in archive vmlinux.a

Gary adds [1]:

> Usually the fix on rustc side is to mark those functions as `#[inline]`
>
> All of {midpoint,next_up,next_down} are indeed unstable functions not
> marked as inline...

Fix all those by adding those intrinsics to our usual workaround.

[ Trevor quickly submitted a fix to upstream Rust [2] that has already
  been merged, to be released in Rust 1.82.0 (2024-10-17). - Miguel ]

Cc: Gary Guo <gary@garyguo.net>
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://rust-for-linux.zulipchat.com/#narrow/stream/x/topic/x/near/455637364 [1]
Reviewed-by: Trevor Gross <tmgross@umich.edu>
Tested-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Boqun Feng <boqun.feng@gmail.com>
Reviewed-by: Gary Guo <gary@garyguo.net>
Link: rust-lang/rust#128749 [2]
Link: https://lore.kernel.org/r/20240806150619.192882-1-ojeda@kernel.org
[ Shortened Zulip link. - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbdd0121 nbdd0121 nbdd0121 approved these changes

Assignees

@scottmcm scottmcm

Labels
A-rust-for-linux Relevant for the Rust-for-Linux project S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.82.0
Development

Successfully merging this pull request may close these issues.
6 participants
@tgross35 @rustbot @ojeda @scottmcm @bors @nbdd0121