[PATCH v2] rust: make compiler-builtin stubs non-global

[PATCH v2] rust: make compiler-builtin stubs non-global

[Gary Guo]

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: https://github.com/Rust-for-Linux/linux/pull/779

Signed-off-by: Gary Guo <gary@garyguo.net>

diff --git a/rust/Makefile b/rust/Makefile
index ff70c4c916f8..aed6f7eca36f 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -359,8 +359,22 @@ rust-analyzer:
 	$(Q)$(srctree)/scripts/generate_rust_analyzer.py $(srctree) $(objtree) \
 		$(RUST_LIB_SRC) > $(objtree)/rust-project.json
 
+redirect-intrinsics = \
+	__eqsf2 __gesf2 __lesf2 __nesf2 __unordsf2 \
+	__unorddf2 \
+	__muloti4 __multi3 \
+	__udivmodti4 __udivti3 __umodti3
+
+ifneq ($(or $(CONFIG_ARM64),$(and $(CONFIG_RISCV),$(CONFIG_64BIT))),)
+	# These intrinsics are defined for ARM64 and RISCV64
+	redirect-intrinsics += \
+		__ashrti3 \
+		__ashlti3 __lshrti3
+endif
+
 $(obj)/core.o: private skip_clippy = 1
 $(obj)/core.o: private skip_flags = -Dunreachable_pub
+$(obj)/core.o: private rustc_objcopy = $(foreach sym,$(redirect-intrinsics),--redefine-sym $(sym)=__rust$(sym))
 $(obj)/core.o: private rustc_target_flags = $(core-cfgs)
 $(obj)/core.o: $(RUST_LIB_SRC)/core/src/lib.rs $(obj)/target.json FORCE
 	$(call if_changed_dep,rustc_library)
diff --git a/rust/compiler_builtins.rs b/rust/compiler_builtins.rs
index f8f39a3e6855..5f833dcca131 100644
--- a/rust/compiler_builtins.rs
+++ b/rust/compiler_builtins.rs
@@ -28,7 +28,7 @@ macro_rules! define_panicking_intrinsics(
     ($reason: tt, { $($ident: ident, )* }) => {
         $(
             #[doc(hidden)]
-            #[no_mangle]
+            #[export_name = concat!("__rust", stringify!($ident))]
             pub extern "C" fn $ident() {
                 panic!($reason);
             }
@@ -61,3 +61,6 @@ define_panicking_intrinsics!("`u128` should not be used", {
     __udivti3,
     __umodti3,
 });
+
+// NOTE: if you are adding a new intrinsic is added here, you should also add it to
+// `redirect-intrinsics` in `rust/Makefile`.
-- 
2.34.1

Re: [PATCH v2] rust: make compiler-builtin stubs non-global

[Miguel Ojeda]

On Mon, Dec 5, 2022 at 12:17 AM Gary Guo <gary@garyguo.net> wrote:
>
> This patch was previous discussed on GitHub.

previous -> previously

> Link: https://github.com/Rust-for-Linux/linux/pull/779
>
> Signed-off-by: Gary Guo <gary@garyguo.net>

Hmm... Something happened to the patch (compared to v1): the triple
dashes, the diffstat, the base-commit etc. are not there. It can still
be applied, so no worries, but it feels a bit strange.

The newline between the tags should not be there. Instead, please put
it on top of `Link:` to keep tags separated (if you wanted to keep it
close to the sentence to refer to it, you can use "[1]" instead and
then "Link: https... [1]" in the tag).

> +// NOTE: if you are adding a new intrinsic is added here, you should also add it to
> +// `redirect-intrinsics` in `rust/Makefile`.

"is added" -> "". Also, what about putting the message at the top
instead (below the docs or perhaps after the macro but before the
calls to it)?

For the title of the patch, in the end I went with "rust: xxx: ..."
(where xxx is typically the name of the crate or the `kernel` crate
module), so perhaps "rust: compiler_builtins: make stubs non-global"
or similar?

Thanks a lot!

Cheers,
Miguel

Re: [PATCH v2] rust: make compiler-builtin stubs non-global

[Nick Desaulniers]

On Sun, Dec 4, 2022 at 3:17 PM Gary Guo <gary@garyguo.net> wrote:
>
> 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: https://github.com/Rust-for-Linux/linux/pull/779
>
> Signed-off-by: Gary Guo <gary@garyguo.net>

Thank you for addressing my feedback!

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>

>
> diff --git a/rust/Makefile b/rust/Makefile
> index ff70c4c916f8..aed6f7eca36f 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -359,8 +359,22 @@ rust-analyzer:
>         $(Q)$(srctree)/scripts/generate_rust_analyzer.py $(srctree) $(objtree) \
>                 $(RUST_LIB_SRC) > $(objtree)/rust-project.json
>
> +redirect-intrinsics = \
> +       __eqsf2 __gesf2 __lesf2 __nesf2 __unordsf2 \
> +       __unorddf2 \
> +       __muloti4 __multi3 \
> +       __udivmodti4 __udivti3 __umodti3
> +
> +ifneq ($(or $(CONFIG_ARM64),$(and $(CONFIG_RISCV),$(CONFIG_64BIT))),)
> +       # These intrinsics are defined for ARM64 and RISCV64
> +       redirect-intrinsics += \
> +               __ashrti3 \
> +               __ashlti3 __lshrti3
> +endif
> +
>  $(obj)/core.o: private skip_clippy = 1
>  $(obj)/core.o: private skip_flags = -Dunreachable_pub
> +$(obj)/core.o: private rustc_objcopy = $(foreach sym,$(redirect-intrinsics),--redefine-sym $(sym)=__rust$(sym))
>  $(obj)/core.o: private rustc_target_flags = $(core-cfgs)
>  $(obj)/core.o: $(RUST_LIB_SRC)/core/src/lib.rs $(obj)/target.json FORCE
>         $(call if_changed_dep,rustc_library)
> diff --git a/rust/compiler_builtins.rs b/rust/compiler_builtins.rs
> index f8f39a3e6855..5f833dcca131 100644
> --- a/rust/compiler_builtins.rs
> +++ b/rust/compiler_builtins.rs
> @@ -28,7 +28,7 @@ macro_rules! define_panicking_intrinsics(
>      ($reason: tt, { $($ident: ident, )* }) => {
>          $(
>              #[doc(hidden)]
> -            #[no_mangle]
> +            #[export_name = concat!("__rust", stringify!($ident))]
>              pub extern "C" fn $ident() {
>                  panic!($reason);
>              }
> @@ -61,3 +61,6 @@ define_panicking_intrinsics!("`u128` should not be used", {
>      __udivti3,
>      __umodti3,
>  });
> +
> +// NOTE: if you are adding a new intrinsic is added here, you should also add it to
> +// `redirect-intrinsics` in `rust/Makefile`.
> --
> 2.34.1
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH v2] rust: make compiler-builtin stubs non-global

[Gary Guo]

On Mon, 5 Dec 2022 21:26:27 +0100
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:

> On Mon, Dec 5, 2022 at 12:17 AM Gary Guo <gary@garyguo.net> wrote:
> >
> > This patch was previous discussed on GitHub.  
> 
> previous -> previously
> 
> > Link: https://github.com/Rust-for-Linux/linux/pull/779
> >
> > Signed-off-by: Gary Guo <gary@garyguo.net>  
> 
> Hmm... Something happened to the patch (compared to v1): the triple
> dashes, the diffstat, the base-commit etc. are not there. It can still
> be applied, so no worries, but it feels a bit strange.

Checked my bash history, and realised that there is a mistakenly added
`p` in my argument to `git format-patch` :)

> 
> The newline between the tags should not be there. Instead, please put
> it on top of `Link:` to keep tags separated (if you wanted to keep it
> close to the sentence to refer to it, you can use "[1]" instead and
> then "Link: https... [1]" in the tag).
> 
> > +// NOTE: if you are adding a new intrinsic is added here, you should also add it to
> > +// `redirect-intrinsics` in `rust/Makefile`.  
> 
> "is added" -> "". Also, what about putting the message at the top
> instead (below the docs or perhaps after the macro but before the
> calls to it)?

People usually add new things to the bottom, and that's my rationale
for adding the note there.

> 
> For the title of the patch, in the end I went with "rust: xxx: ..."
> (where xxx is typically the name of the crate or the `kernel` crate
> module), so perhaps "rust: compiler_builtins: make stubs non-global"
> or similar?

Will address, along with other suggestions, in v3.

Best,
Gary