Messages in this thread | | | From | Miguel Ojeda <> | Date | Thu, 15 Apr 2021 02:43:18 +0200 | Subject | Re: [PATCH 04/13] Kbuild: Rust support |
| |
On Thu, Apr 15, 2021 at 1:19 AM Nick Desaulniers <ndesaulniers@google.com> 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
| |