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

Remove __aeabi_uldivmod stub #779

Open
wants to merge 2 commits into
base: rust
Choose a base branch
from
Open

Remove __aeabi_uldivmod stub #779

wants to merge 2 commits into from

Conversation

nbdd0121
Copy link
Member

Rust's libcore genuinely needs 64-bit division for formatting, but we
can't change these to use kernel's division routine; so __aeabi_uldivmod
calls will be generated.

Instead of defining a stub that can affect all code, use objcopy to redirect
these calls and forward them to div64_u64_rem.

bjorn3
bjorn3 reviewed May 26, 2022
View reviewed changes
rust/compiler_builtins.rs Outdated
#[doc(hidden)]
#[naked]
#[no_mangle]
pub extern "C" fn rust_aeabi_uldivmod() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you still have to use the correct function signature even for #[naked] functions. In addition the calling convention needs to match what is actually used. Finally you don't need pub here. #[no_mangle] is enough to export the symbol.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature for #[naked] function doesn't matter for the assembly. It's only a description that how Rust code should call it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like compiler-builtins doesn't specify the parameters either due to them having a custom calling convention. It does mark them as unsafe though.

rust/Makefile Outdated
@@ -370,6 +370,7 @@ rust-analyzer:

$(obj)/core.o: private skip_clippy = 1
$(obj)/core.o: private skip_flags = -Dunreachable_pub
$(obj)/core.o: private rustc_objcopy = --redefine-sym __aeabi_uldivmod=rust_aeabi_uldivmod
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure those calls would stay limited to libcore and can't be inlined into other rust code, even with LTO enabled?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I am actually not sure if this works with LTO object files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked, LTO object files aren't even ELF files, so objcopy doesn't work on it. However currently we don't have LTO support for in-kernel Rust code and kernel LTO is not supported for 32-bit ARM either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, when LTO is enabled the division will remain as division and will not generate compiler-rt calls -- so this hack definitely won't work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if any libcore method that uses it is marked as #[inline] and would thus get inlined even without LTO.

@bjorn3
Copy link
Member

bjorn3 commented May 26, 2022

Instead of defining a stub that can affect all code

__aeabi_uldivmod is specified by arm and not an implementation detail of llvm or gcc, so that shouldn't matter. It may even make sense to define it on the non-rust side unconditionally for arm.

@nbdd0121
Copy link
Member Author

__aeabi_uldivmod is specified by arm and not an implementation detail of llvm or gcc, so that shouldn't matter. It may even make sense to define it on the non-rust side unconditionally for arm.

The kernel explicitly does not define these functions to catch usage of 64-bit division. Kernel code is discouraged from doing that because it's expensive (and need to use specific division routines if necessary).

@ojeda
Copy link
Member

ojeda commented May 26, 2022

A couple things we could add is a small test and a comment on the situations the hack may not work. And maybe a mention that we want to add a custom formatting library in the future which should avoid the problem altogether.

@nbdd0121
rust: make compiler-builtin stubs non-global
2d98d37 
Currently we define a number of stubs for compiler-builtin intrinsics
that compiled libcore generates. The defined stubs are weak so they will
not conflict with genuine implementation of these intrinsics, but their
effect is global and will cause non-libcore code that accidently
generate these intrinsics calls compile and bug on runtime.

Instead of defining a stub that can affect all code, this patch uses
objcopy's `--redefine-sym` flag to redirect these calls (from libcore
only) to a prefixed version (e.g. redirect `__multi3` to `__rust_multi3`),
so we can define panciking stubs that are only visible to libcore.

Signed-off-by: Gary Guo <gary@garyguo.net>
@nbdd0121
rust: replace __aeabi_uldivmod stub with actual implementation
17ea967 
Rust's libcore genuinely needs 64-bit division for formatting, but we
can't change these to use kernel's division routine; so `__aeabi_uldivmod`
calls will be generated.

Instead of redirecting these calls to a panicking stub, forward them to
`div64_u64_rem`.

Signed-off-by: Gary Guo <gary@garyguo.net>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Dec 4, 2022
@nbdd0121 @intel-lab-lkp
rust: make compiler-builtin stubs non-global
97538a1 
Currently we define a number of stubs for compiler-builtin intrinsics
that compiled libcore generates. The defined stubs are weak so they will
not conflict with genuine implementation of these intrinsics, but their
effect is global and will cause non-libcore code that accidently
generate these intrinsics calls compile and bug on runtime.

Instead of defining a stub that can affect all code, this patch uses
objcopy's `--redefine-sym` flag to redirect these calls (from libcore
only) to a prefixed version (e.g. redirect `__multi3` to `__rust_multi3`),
so we can define panciking stubs that are only visible to libcore.

This patch was previous discussed on GitHub.
Link: Rust-for-Linux#779

Signed-off-by: Gary Guo <gary@garyguo.net>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Dec 5, 2022
@nbdd0121 @intel-lab-lkp
rust: compiler_builtins: make stubs non-global
4322468 
Currently we define a number of stubs for compiler-builtin intrinsics
that compiled libcore generates. The defined stubs are weak so they will
not conflict with genuine implementation of these intrinsics, but their
effect is global and will cause non-libcore code that accidently
generate these intrinsics calls compile and bug on runtime.

Instead of defining a stub that can affect all code, this patch uses
objcopy's `--redefine-sym` flag to redirect these calls (from libcore
only) to a prefixed version (e.g. redirect `__multi3` to `__rust_multi3`),
so we can define panciking stubs that are only visible to libcore.

This patch was previously discussed on GitHub [1].

Link: Rust-for-Linux#779 [1]
Signed-off-by: Gary Guo <gary@garyguo.net>
intel-lab-lkp pushed a commit to intel-lab-lkp/linux that referenced this pull request Dec 5, 2022
@nbdd0121 @intel-lab-lkp
rust: compiler_builtins: make stubs non-global
d8e5ece 
Currently we define a number of stubs for compiler-builtin intrinsics
that compiled libcore generates. The defined stubs are weak so they will
not conflict with genuine implementation of these intrinsics, but their
effect is global and will cause non-libcore code that accidently
generate these intrinsics calls compile and bug on runtime.

Instead of defining a stub that can affect all code, this patch uses
objcopy's `--redefine-sym` flag to redirect these calls (from libcore
only) to a prefixed version (e.g. redirect `__multi3` to `__rust_multi3`),
so we can define panciking stubs that are only visible to libcore.

This patch was previously discussed on GitHub [1]. This approach was also
independently proposed by Nick Desaulniers in [2].

Link: Rust-for-Linux#779 [1]
Link: https://lore.kernel.org/lkml/CAKwvOdkc0Qhwu=gfe1+H23TnAa6jnO6A3ZCO687dH6mSrATmDA@mail.gmail.com/
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
ojeda pushed a commit to ojeda/linux that referenced this pull request Jan 16, 2023
@nbdd0121 @ojeda
rust: compiler_builtins: make stubs non-global
cb7d9de 
Currently we define a number of stubs for compiler-builtin intrinsics
that compiled libcore generates. The defined stubs are weak so they will
not conflict with genuine implementation of these intrinsics, but their
effect is global and will cause non-libcore code that accidently
generate these intrinsics calls compile and bug on runtime.

Instead of defining a stub that can affect all code, this patch uses
objcopy's `--redefine-sym` flag to redirect these calls (from libcore
only) to a prefixed version (e.g. redirect `__multi3` to `__rust_multi3`),
so we can define panciking stubs that are only visible to libcore.

This patch was previously discussed on GitHub [1]. This approach was also
independently proposed by Nick Desaulniers in [2].

Link: Rust-for-Linux#779 [1]
Link: https://lore.kernel.org/lkml/CAKwvOdkc0Qhwu=gfe1+H23TnAa6jnO6A3ZCO687dH6mSrATmDA@mail.gmail.com/
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Wei Liu <wei.liu@kernel.org>
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
jannau pushed a commit to jannau/linux that referenced this pull request Mar 9, 2023
@nbdd0121 @marcan
rust: compiler_builtins: make stubs non-global
3a6aca8 
Currently we define a number of stubs for compiler-builtin intrinsics
that compiled libcore generates. The defined stubs are weak so they will
not conflict with genuine implementation of these intrinsics, but their
effect is global and will cause non-libcore code that accidently
generate these intrinsics calls compile and bug on runtime.

Instead of defining a stub that can affect all code, this patch uses
objcopy's `--redefine-sym` flag to redirect these calls (from libcore
only) to a prefixed version (e.g. redirect `__multi3` to `__rust_multi3`),
so we can define panciking stubs that are only visible to libcore.

This patch was previously discussed on GitHub [1]. This approach was also
independently proposed by Nick Desaulniers in [2].

Link: Rust-for-Linux#779 [1]
Link: https://lore.kernel.org/lkml/CAKwvOdkc0Qhwu=gfe1+H23TnAa6jnO6A3ZCO687dH6mSrATmDA@mail.gmail.com/
Suggested-by: Nick Desaulniers <ndesaulniers@google.com>
Acked-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Wei Liu <wei.liu@kernel.org>
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

@bjorn3 bjorn3 bjorn3 left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nbdd0121 @bjorn3 @ojeda