On Thu, 6 Mar 2025 18:00:10 -0500 Tamir Duberstein <tamird@xxxxxxxxx> wrote: > > 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`. > > Do we know why the target strings aren't the same? Are there citations > that could be included here? I've added an explaination in new patch series. > > > okay since this is one of the hardening features and we shouldn't have > > null pointer dereferences in these helpers. > > Is the implication that kernel C is compiled with this flag, but Rust > code isn't? Do we know why? ABI is compatible with and without this. I've added a short explaination in the new version. > > The checks can also be bypassed with force inlining (`__always_inline`) > > but the behaviour is the same with extra options. > > If the behavior is the same, wouldn't it be better to use > `__always_inline`? Otherwise LLVM's behavior might change such that > inlining is lost and we won't notice. If everything works as expected, then the behaviour is the same, but not focing inline can be used to detect mistakes, e.g. when an inline function gets too large. Most C side don't use `__always_inline` but rather just `inline` so I want to keep helpers the same. > > > > +config RUST_INLINE_HELPERS > > + bool "Inline C helpers into Rust crates" > > + depends on RUST && RUSTC_CLANG_LLVM_COMPATIBLE > > + help > > + Links C helpers into with Rust crates through LLVM IR. > > + > > + If this option is enabled, instead of generating object files directly, > > + rustc is asked to produce LLVM IR, which is then linked together with > > + the LLVM IR of C helpers, before object file is generated. > > s/IR/bitcode/g > > Right? I'd rather keep "IR" here as it's a more general concept. Bitcode generation is an implementation detail really and user shouldn't care. If we remove bitcode steps then the whole idea still works as expected. > > # 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/helpers.o > > CFLAGS_REMOVE_helpers/helpers.o = -Wmissing-prototypes -Wmissing-declarations > > Should this also move up into the else branch above? > > > + $(LLVM_AS) $(patsubst %.bc,%.ll,$@) -o $@ > > + > > +$(obj)/helpers/helpers.bc: $(obj)/helpers/helpers.c FORCE > > + +$(call if_changed_dep,rust_helper) > > Should all these rules be defined iff CONFIG_RUST_INLINE_HELPERS? > Always defining them seems like it could lead to subtle bugs, but > perhaps there's Makefile precedent I'm not aware of. I don't think that's needed the way Kbuild works. For all C source files, we have targets for all .o files regardless if a config is enabled (enabling a config merely adds the corresponding .o to obj-y). So I don't think this is needed for helpers either. > > +ifdef CONFIG_RUST_INLINE_HELPERS > > +$(obj)/kernel.o: private link_helper = 1 > > +$(obj)/kernel.o: $(obj)/helpers/helpers.bc > > +endif > > Can this be combined with the other `ifdef CONFIG_RUST_INLINE_HELPERS`? I want all kernel.o related lines to be closer together. > > +#ifndef RUST_HELPERS_H > > +#define RUST_HELPERS_H > > + > > +#include <linux/compiler_types.h> > > + > > +#ifdef __BINDGEN__ > > +#define __rust_helper > > +#else > > +#define __rust_helper inline > > +#endif > > Could you mention this in the commit message? It's not obvious to me > what this does and why it depends on __BINDGEN__ rather than > CONFIG_RUST_INLINE_HELPERS. I explained about the bindgen part in the new patch. For CONFIG_RUST_INLINE_HELPERS, I don't think I have a good place to fit it into the commit message, so I'll explain here: `inline` in kernel is not the C `inline`. It's actually the GNU89 legacy inline, which both compiles as a standalone function (strong external linkage) and provide inlining definition, so this works if CONFIG_RUST_INLINE_HELPERS is not enabled. Best, Gary
