Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210414184604.23473-1-ojeda@kernel.org> <20210414184604.23473-5-ojeda@kernel.org>
 <CAKwvOdkjttdX83tL4pw+J5EnHM1MgEYDPp=YTpEagV4RrhdxwA@mail.gmail.com>
In-Reply-To: <CAKwvOdkjttdX83tL4pw+J5EnHM1MgEYDPp=YTpEagV4RrhdxwA@mail.gmail.com>
From:   Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Date:   Thu, 15 Apr 2021 02:43:18 +0200
Message-ID: <CANiq72ksLeuL_uqoqbf3fhLP7M0j-7TdEvRDDmxThdmrEqD2Lw@mail.gmail.com>
Subject: Re: [PATCH 04/13] Kbuild: Rust support
To:     Nick Desaulniers <ndesaulniers@google.com>,
        Masahiro Yamada <masahiroy@kernel.org>
Cc:     Miguel Ojeda <ojeda@kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        rust-for-linux@vger.kernel.org,
        Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
        Linux Doc Mailing List <linux-doc@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Alex Gaynor <alex.gaynor@gmail.com>,
        Geoffrey Thomas <geofft@ldpreload.com>,
        Finn Behrens <me@kloenk.de>,
        Adam Bratschi-Kaye <ark.email@gmail.com>,
        Wedson Almeida Filho <wedsonaf@google.com>,
        Michael Ellerman <mpe@ellerman.id.au>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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