Re: [PATCH 04/13] Kbuild: Rust support

[Miguel Ojeda]

On Thu, Apr 15, 2021 at 1:19 AM Nick Desaulniers
<ndesaulniers@xxxxxxxxxx> wrote:
>
> Rather than check the origin (yikes, are we intentionally avoiding env
> vars?), can this simply be
> ifneq ($(CLIPPY),)
>   KBUILD_CLIPPY := $(CLIPPY)
> endif
>
> Then you can specify whatever value you want, support command line or
> env vars, etc.?

I was following the other existing cases like `V`. Masahiro can
probably answer why they are done like this.

> -Oz in clang typically generates larger kernel code than -Os; LLVM
> seems to aggressively emit libcalls even when the setup for a call
> would be larger than the inlined call itself.  Is z smaller than s for
> the existing rust examples?

I will check if the `s`/`z` flags have the exact same semantics as
they do in Clang, but as a quick test (quite late here, sorry!), yes,
it seems `z` is smaller:

      text data bss    dec   hex filename

    126568    8 104 126680 1eed8 drivers/android/rust_binder.o [s]
    122923    8 104 123035 1e09b drivers/android/rust_binder.o [z]

    212351    0   0 212351 33d7f rust/core.o [s]
    207653    0   0 207653 32b25 rust/core.o [z]

> This is a mess; who thought it would be a good idea to support
> compiling the rust code at a different optimization level than the
> rest of the C code in the kernel?  Do we really need that flexibility
> for Rust kernel code, or can we drop this feature?

I did :P

The idea is that, since it seemed to work out of the box when I tried,
it could be nice to keep for debugging and for having another degree
of freedom when testing the compiler/nightlies etc.

Also, it is not intended for users, which is why I put it in the
"hacking" menu -- users should still only modify the usual global
option.

However, it is indeed strange for the kernel and I don't mind dropping
it if people want to see it out (one could still do it manually if
needed...).

(Related: from what I have been told, the kernel does not support
lower levels in C just due to old problems with compilers; but those
may be gone now).

> Don't the target.json files all set `"eliminate-frame-pointer":
> false,`?  Is this necessary then?  Also, which targets retain frame
> pointers at which optimization levels is quite messy (in C compilers),
> as well as your choice of unwinder, which is configurable for certain
> architectures.

For this (and other questions regarding the target definition files
you have below): the situation is quite messy, indeed. Some of these
options can be configured via flags too. Also, the target definition
files are actually unstable in `rustc` because they are too tied to
LLVM. So AFAIK if a command-line flag exists, we should use that. But
I am not sure if the target definition file is supposed to support
removing keys etc.

Anyway, the plan here short-term is to generate the target definition
file on the fly taking into account the options, and keep it working
w.r.t. `rustc` nightlies (this is also why we don't have have big
endian for ppc or 32-bit x86). Longer-term, hopefully `rustc` adds
enough command-line flags to tweak as needed, or stabilizes the target
files somehow, or stabilizes all the target combinations we need (i.e.
as built-in targets in the compiler).

In fact, I will add this to the "unstable features" list.

> Seems like a good way for drive by commits where someone reformatted
> the whole kernel.

We enforce the formatting for all the code at the moment in the CI and
AFAIK `rustfmt` tries to keep formatting stable (as long as one does
not use unstable features), so code should always be formatted.

Having said that, I'm not sure 100% how stable it actually is in
practice over long periods of time -- I guess we will find out soon
enough.

> Might be nice to invoke this somehow from checkpatch.pl somehow for
> changes to rust source files. Not necessary in the RFC, but perhaps
> one day.

We do it in the CI (see above).

> Yuck. This should be default on and not configurable.

See above for the opt-levels.

Cheers,
Miguel