Re: [PATCH v2 2/3] kbuild: rust: provide an option to inline C helpers into Rust

[Gary Guo]

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