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
