Message-ID: <CAGSQo02_siEiS07H+7MCi_T2REGoyF6FFuGHVmiUFi4EocLizQ@mail.gmail.com>
Date: Fri, 7 Jun 2024 07:40:45 -0700
From: Matthew Maurer <mmaurer@...gle.com>
To: Gary Guo <gary@...yguo.net>
Cc: Masahiro Yamada <masahiroy@...nel.org>, Nathan Chancellor <nathan@...nel.org>, 
	Nicolas Schier <nicolas@...sle.eu>, Andrew Morton <akpm@...ux-foundation.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Alex Gaynor <alex.gaynor@...il.com>, 
	Wedson Almeida Filho <wedsonaf@...il.com>, Boqun Feng <boqun.feng@...il.com>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...sung.com>, 
	Alice Ryhl <aliceryhl@...gle.com>, Nick Desaulniers <ndesaulniers@...gle.com>, 
	Bill Wendling <morbo@...gle.com>, Justin Stitt <justinstitt@...gle.com>, linux-kbuild@...r.kernel.org, 
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org, 
	llvm@...ts.linux.dev
Subject: Re: [RFC PATCH 2/2] kbuild: rust: provide an option to inline C
 helpers into Rust

The LLVM `.ll` textual bitcode representation may change from version
to version, which makes using `sed` to edit it potentially fragile.
Would it be possible to attach similar attributes without this? (Or
other attributes that can accomplish something similar, see the second
part.)

Would it be possible to use a Rust-only-LTO (actual LTO) flag rather
than a `llvm-link`? It seems likely that someone that wants to use a
synced `clang` and `rustc` to allow helpers to be inlined would be
unlikely to object to thin LTO of Rust. This would avoid introducing a
new tool.

As one possible route around editing the `.ll` file, we could consider
*not* hiding these symbols, just marking them as "good idea to inline
if possible", and then masking any symbol that ends up in the final
`.o` via `objcopy --localize-symbol=blah`. If we're doing full-kernel
LTO rather than rust-only LTO, we'd need to use `llvm-objcopy`
instead, but the same process would follow - a postprocessing step on
the final crate object that localizes the symbol, with LTO giving the
compiler the option (but not requirement) to inline these functions if
it seems like a good idea.


On Wed, May 29, 2024 at 1:30 PM Gary Guo <gary@...yguo.net> wrote:
>
> A new Kconfig option, `RUST_LTO_HELPERS` is added to allow C helpers
> (which was created to allow Rust to call into inline/macro C functions
> without having to re-implement the logic in Rust) to be inlined into
> Rust crates without performing a global LTO.
>
> If the option is enabled, the following is performed:
> * For helpers, instead of compiling them to object file to be linked
>   into vmlinux, we compile them to LLVM IR.
> * The LLVM IR is patched to add `linkonce_odr` linkage. This linkage
>   means that the function is inlineable (effect of `_odr`), and the
>   symbols generated will have weak linkage if emitted into object file
>   (important since as later described, we might have multiple copies of
>   the same symbol) and it will may be discarded if it is not invoked or
>   all invocations are inlined.
> * The LLVM IR is compiled to bitcode (This is step is not necessary, but
>   is a performance optimisation to prevent LLVM from always have to
>   reparse the same IR).
> * When a Rust crate is compiled, instead of generating object file, we
>   ask LLVM bitcode to be generated.
> * llvm-link is invoked to combine the helper bitcode with the crate
>   bitcode. This step is similar to what's done in a full LTO, hence the
>   option name, but this is much faster since it only needs to inline the
>   helpers.
> * clang is invoked to turn the combined bitcode into object file.
>
> Some caveats with the option:
> * clang and Rust doesn't have the exact target string. Manual inspection
>   shows that they should be compatible, but since they are not exactly
>   the same LLVM seems to prefer not inlining them. This is bypassed with
>   `--ignore-tti-inline-compatible`.
> * LLVM doesn't want to inline functions combined with
>   `-fno-delete-null-pointer-checks` with code compiled without. So we
>   remove this command when compiling helpers. I think this should be
>   okay since this is one of the hardening features and we shouldn't have
>   null pointer dereferences in these helpers.
>
> The checks can also be bypassed with force inlining (`__always_inline`)
> but the behaviour is the same with extra options.
>
> This option requires clang to be used for CC and the LLVM version of
> clang and rustc to be matching (at least to the same major version), and
> therefore is gated as EXPERT option.
>
> Co-developed-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>
> Signed-off-by: Gary Guo <gary@...yguo.net>
> ---
>  Makefile               |  4 +++-
>  lib/Kconfig.debug      | 10 ++++++++++
>  rust/Makefile          | 34 ++++++++++++++++++++++++++++++----
>  rust/exports.c         |  3 +++
>  rust/helpers.c         | 41 +++++++++++++++++++++--------------------
>  scripts/Makefile.build |  5 ++++-
>  6 files changed, 71 insertions(+), 26 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index f975b6396328..c6f12093fb1c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -492,6 +492,8 @@ OBJCOPY             = $(LLVM_PREFIX)llvm-objcopy$(LLVM_SUFFIX)
>  OBJDUMP                = $(LLVM_PREFIX)llvm-objdump$(LLVM_SUFFIX)
>  READELF                = $(LLVM_PREFIX)llvm-readelf$(LLVM_SUFFIX)
>  STRIP          = $(LLVM_PREFIX)llvm-strip$(LLVM_SUFFIX)
> +LLVM_LINK      = $(LLVM_PREFIX)llvm-link$(LLVM_SUFFIX)
> +LLVM_AS        = $(LLVM_PREFIX)llvm-as$(LLVM_SUFFIX)
>  else
>  CC             = $(CROSS_COMPILE)gcc
>  LD             = $(CROSS_COMPILE)ld
> @@ -601,7 +603,7 @@ endif
>  export RUSTC_BOOTSTRAP := 1
>
>  export ARCH SRCARCH CONFIG_SHELL BASH HOSTCC KBUILD_HOSTCFLAGS CROSS_COMPILE LD CC HOSTPKG_CONFIG
> -export RUSTC RUSTDOC RUSTFMT RUSTC_OR_CLIPPY_QUIET RUSTC_OR_CLIPPY BINDGEN CARGO
> +export RUSTC RUSTDOC RUSTFMT RUSTC_OR_CLIPPY_QUIET RUSTC_OR_CLIPPY BINDGEN CARGO LLVM_LINK LLVM_AS
>  export HOSTRUSTC KBUILD_HOSTRUSTFLAGS
>  export CPP AR NM STRIP OBJCOPY OBJDUMP READELF PAHOLE RESOLVE_BTFIDS LEX YACC AWK INSTALLKERNEL
>  export PERL PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 59b6765d86b8..8b9a1bd8ce71 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -3049,6 +3049,16 @@ config RUST_KERNEL_DOCTESTS
>
>           If unsure, say N.
>
> +config RUST_LTO_HELPERS
> +    bool "LTO C helpers with Rust crates"
> +    depends on RUST && CC_IS_CLANG && EXPERT
> +    help
> +      Links C helpers together with Rust crates using LTO.
> +
> +      This is achieved by compiling both helpers and Rust crates into
> +      LLVM bitcode and use llvm-link to combine them.
> +
> +      This requires a matching LLVM version for Clang and rustc.
> +
> +      If unsure, say N.
> +
>  endmenu # "Rust"
>
>  endmenu # Kernel hacking
> diff --git a/rust/Makefile b/rust/Makefile
> index b4d63ea9209f..ad0797467102 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -6,9 +6,14 @@ rustdoc_output := $(objtree)/Documentation/output/rust/rustdoc
>  obj-$(CONFIG_RUST) += core.o compiler_builtins.o
>  always-$(CONFIG_RUST) += exports_core_generated.h
>
> +ifdef CONFIG_RUST_LTO_HELPERS
> +always-$(CONFIG_RUST) += helpers.bc
> +else
> +obj-$(CONFIG_RUST) += helpers.o
> +always-$(CONFIG_RUST) += exports_helpers_generated.h
> +endif
>  # Missing prototypes are expected in the helpers since these are exported
>  # for Rust only, thus there is no header nor prototypes.
> -obj-$(CONFIG_RUST) += helpers.o
>  CFLAGS_REMOVE_helpers.o = -Wmissing-prototypes -Wmissing-declarations
>
>  always-$(CONFIG_RUST) += libmacros.so
> @@ -17,7 +22,7 @@ no-clean-files += libmacros.so
>  always-$(CONFIG_RUST) += bindings/bindings_generated.rs bindings/bindings_helpers_generated.rs
>  obj-$(CONFIG_RUST) += alloc.o bindings.o kernel.o
>  always-$(CONFIG_RUST) += exports_alloc_generated.h exports_bindings_generated.h \
> -    exports_kernel_generated.h exports_helpers_generated.h
> +    exports_kernel_generated.h
>
>  always-$(CONFIG_RUST) += uapi/uapi_generated.rs
>  obj-$(CONFIG_RUST) += uapi.o
> @@ -350,12 +355,24 @@ $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_flags = \
>      --blocklist-type '.*' --allowlist-var '' \
>      --allowlist-function 'rust_helper_.*'
>  $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_cflags = \
> -    -I$(objtree)/$(obj) -Wno-missing-prototypes -Wno-missing-declarations
> +    -I$(objtree)/$(obj) $(CFLAGS_helpers.o) -Wno-missing-prototypes -Wno-missing-declarations
>  $(obj)/bindings/bindings_helpers_generated.rs: private bindgen_target_extra = ; \
>      sed -Ei 's/pub fn rust_helper_([a-zA-Z0-9_]*)/#[link_name="rust_helper_\1"]\n    pub fn \1/g' $@
>  $(obj)/bindings/bindings_helpers_generated.rs: $(src)/helpers.c FORCE
>         $(call if_changed_dep,bindgen)
>
> +# When compiling helpers, filter `-fno-delete-null-pointer-checks` since LLVM
> +# prevents inlining such functions to be inlined into functions compiled
> +# without the option (e.g. Rust functions).
> +quiet_cmd_rust_helper = HELPER  $@
> +      cmd_rust_helper = \
> +       $(CC) $(filter-out -fno-delete-null-pointer-checks $(CFLAGS_REMOVE_helpers.o), $(c_flags) $(CFLAGS_helpers.o)) -S $< -emit-llvm -o $(patsubst %.bc,%.ll,$@); \
> +       sed -i 's/^define dso_local/define linkonce_odr dso_local/g' $(patsubst %.bc,%.ll,$@); \
> +       $(LLVM_AS) $(patsubst %.bc,%.ll,$@) -o $@
> +
> +$(obj)/helpers.bc: $(obj)/helpers.c FORCE
> +       +$(call if_changed_dep,rust_helper)
> +
>  quiet_cmd_exports = EXPORTS $@
>        cmd_exports = \
>         $(NM) -p --defined-only $< \
> @@ -396,11 +413,13 @@ quiet_cmd_rustc_library = $(if $(skip_clippy),RUSTC,$(RUSTC_OR_CLIPPY_QUIET)) L
>         OBJTREE=$(abspath $(objtree)) \
>         $(if $(skip_clippy),$(RUSTC),$(RUSTC_OR_CLIPPY)) \
>                 $(filter-out $(skip_flags),$(rust_flags) $(rustc_target_flags)) \
> -               --emit=dep-info=$(depfile) --emit=obj=$@ \
> +               --emit=dep-info=$(depfile) --emit=$(if $(link_helper),llvm-bc=$(patsubst %.o,%.bc,$@),obj=$@) \
>                 --emit=metadata=$(dir $@)$(patsubst %.o,lib%.rmeta,$(notdir $@)) \
>                 --crate-type rlib -L$(objtree)/$(obj) \
>                 --crate-name $(patsubst %.o,%,$(notdir $@)) $< \
>                 --sysroot=/dev/null \
> +       $(if $(link_helper),;$(LLVM_LINK) $(patsubst %.o,%.bc,$@) $(obj)/helpers.bc -o $(patsubst %.o,%.m.bc,$@); \
> +               $(CC) $(KBUILD_CFLAGS) -mllvm=--ignore-tti-inline-compatible -c $(patsubst %.o,%.m.bc,$@) -o $@) \
>         $(if $(rustc_objcopy),;$(OBJCOPY) $(rustc_objcopy) $@)
>
>  rust-analyzer:
> @@ -463,4 +482,11 @@ $(obj)/kernel.o: $(src)/kernel/lib.rs $(obj)/alloc.o $(obj)/build_error.o \
>      $(obj)/libmacros.so $(obj)/bindings.o $(obj)/uapi.o FORCE
>         +$(call if_changed_dep,rustc_library)
>
> +ifdef CONFIG_RUST_LTO_HELPERS
> +
> +$(obj)/kernel.o: private link_helper = 1
> +$(obj)/kernel.o: $(obj)/helpers.bc
> +
> +endif
> +
>  endif # CONFIG_RUST
> diff --git a/rust/exports.c b/rust/exports.c
> index aa1218b325e5..212c1ec1d38b 100644
> --- a/rust/exports.c
> +++ b/rust/exports.c
> @@ -19,7 +19,10 @@
>  #include "exports_alloc_generated.h"
>  #include "exports_bindings_generated.h"
>  #include "exports_kernel_generated.h"
> +
> +#ifndef CONFIG_RUST_LTO_HELPERS
>  #include "exports_helpers_generated.h"
> +#endif
>
>  // For modules using `rust/build_error.rs`.
>  #ifdef CONFIG_RUST_BUILD_ASSERT_ALLOW
> diff --git a/rust/helpers.c b/rust/helpers.c
> index 895f4b696962..3abf96f14148 100644
> --- a/rust/helpers.c
> +++ b/rust/helpers.c
> @@ -32,18 +32,19 @@
>  #include <linux/spinlock.h>
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
> +#include "helpers.h"
>
> -__noreturn void rust_helper_BUG(void)
> +__rust_helper __noreturn void rust_helper_BUG(void)
>  {
>         BUG();
>  }
>
> -void rust_helper_mutex_lock(struct mutex *lock)
> +__rust_helper void rust_helper_mutex_lock(struct mutex *lock)
>  {
>         mutex_lock(lock);
>  }
>
> -void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
> +__rust_helper void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
>                                   struct lock_class_key *key)
>  {
>  #ifdef CONFIG_DEBUG_SPINLOCK
> @@ -53,82 +54,82 @@ void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
>  #endif
>  }
>
> -void rust_helper_spin_lock(spinlock_t *lock)
> +__rust_helper void rust_helper_spin_lock(spinlock_t *lock)
>  {
>         spin_lock(lock);
>  }
>
> -void rust_helper_spin_unlock(spinlock_t *lock)
> +__rust_helper void rust_helper_spin_unlock(spinlock_t *lock)
>  {
>         spin_unlock(lock);
>  }
>
> -void rust_helper_init_wait(struct wait_queue_entry *wq_entry)
> +__rust_helper void rust_helper_init_wait(struct wait_queue_entry *wq_entry)
>  {
>         init_wait(wq_entry);
>  }
>
> -int rust_helper_signal_pending(struct task_struct *t)
> +__rust_helper int rust_helper_signal_pending(struct task_struct *t)
>  {
>         return signal_pending(t);
>  }
>
> -refcount_t rust_helper_REFCOUNT_INIT(int n)
> +__rust_helper refcount_t rust_helper_REFCOUNT_INIT(int n)
>  {
>         return (refcount_t)REFCOUNT_INIT(n);
>  }
>
> -void rust_helper_refcount_inc(refcount_t *r)
> +__rust_helper void rust_helper_refcount_inc(refcount_t *r)
>  {
>         refcount_inc(r);
>  }
>
> -bool rust_helper_refcount_dec_and_test(refcount_t *r)
> +__rust_helper bool rust_helper_refcount_dec_and_test(refcount_t *r)
>  {
>         return refcount_dec_and_test(r);
>  }
>
> -__force void *rust_helper_ERR_PTR(long err)
> +__rust_helper __force void *rust_helper_ERR_PTR(long err)
>  {
>         return ERR_PTR(err);
>  }
>
> -bool rust_helper_IS_ERR(__force const void *ptr)
> +__rust_helper bool rust_helper_IS_ERR(__force const void *ptr)
>  {
>         return IS_ERR(ptr);
>  }
>
> -long rust_helper_PTR_ERR(__force const void *ptr)
> +__rust_helper long rust_helper_PTR_ERR(__force const void *ptr)
>  {
>         return PTR_ERR(ptr);
>  }
>
> -const char *rust_helper_errname(int err)
> +__rust_helper const char *rust_helper_errname(int err)
>  {
>         return errname(err);
>  }
>
> -struct task_struct *rust_helper_get_current(void)
> +__rust_helper struct task_struct *rust_helper_get_current(void)
>  {
>         return current;
>  }
>
> -void rust_helper_get_task_struct(struct task_struct *t)
> +__rust_helper void rust_helper_get_task_struct(struct task_struct *t)
>  {
>         get_task_struct(t);
>  }
>
> -void rust_helper_put_task_struct(struct task_struct *t)
> +__rust_helper void rust_helper_put_task_struct(struct task_struct *t)
>  {
>         put_task_struct(t);
>  }
>
> -struct kunit *rust_helper_kunit_get_current_test(void)
> +__rust_helper struct kunit *rust_helper_kunit_get_current_test(void)
>  {
>         return kunit_get_current_test();
>  }
>
> -void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
> +__rust_helper void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
>                                     bool onstack, const char *name,
>                                     struct lock_class_key *key)
>  {
> @@ -139,7 +140,7 @@ void rust_helper_init_work_with_key(struct work_struct *work, work_func_t func,
>         work->func = func;
>  }
>
> -void * __must_check __realloc_size(2)
> +__rust_helper void * __must_check __realloc_size(2)
>  rust_helper_krealloc(const void *objp, size_t new_size, gfp_t flags)
>  {
>         return krealloc(objp, new_size, flags);
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index efacca63c897..201e7dc5ae5d 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -288,7 +288,10 @@ rust_common_cmd = \
>  # would not match each other.
>
>  quiet_cmd_rustc_o_rs = $(RUSTC_OR_CLIPPY_QUIET) $(quiet_modtag) $@
> -      cmd_rustc_o_rs = $(rust_common_cmd) --emit=obj=$@ $<
> +      cmd_rustc_o_rs = $(rust_common_cmd) --emit=$(if $(CONFIG_RUST_LTO_HELPERS),llvm-bc=$(patsubst %.o,%.bc,$@),obj=$@) $< \
> +       $(if $(CONFIG_RUST_LTO_HELPERS),;$(LLVM_LINK) $(patsubst %.o,%.bc,$@) $(objtree)/rust/helpers.bc -o $(patsubst %.o,%.m.bc,$@); \
> +               $(CC) $(KBUILD_CFLAGS) -mllvm=--ignore-tti-inline-compatible -c $(patsubst %.o,%.m.bc,$@) -o $@)
> +
>
>  $(obj)/%.o: $(obj)/%.rs FORCE
>         +$(call if_changed_dep,rustc_o_rs)
> --
> 2.42.0
>
>

Powered by blists - more mailing lists