Message-ID: <CAABy=s12gOZadhYC+=6=TbbyN9j5L0B19ZLSGR_VFEdt1jhwEA@mail.gmail.com>
Date: Mon, 9 Dec 2024 16:50:45 -0800
From: "Hong, Yifan" <elsk@...gle.com>
To: Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Cc: Miguel Ojeda <ojeda@...nel.org>, Masahiro Yamada <masahiroy@...nel.org>, 
	Alex Gaynor <alex.gaynor@...il.com>, Jonathan Corbet <corbet@....net>, 
	Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas@...sle.eu>, linux-kbuild@...r.kernel.org, 
	Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>, 
	Björn Roy Baron <bjorn3_gh@...tonmail.com>, 
	Benno Lossin <benno.lossin@...ton.me>, Andreas Hindborg <a.hindborg@...nel.org>, 
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	patches@...ts.linux.dev
Subject: Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

On Mon, Dec 9, 2024 at 10:30 AM Miguel Ojeda
<miguel.ojeda.sandonis@...il.com> wrote:
>
> On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@...nel.org> wrote:
> >
> >   - Removed "additional" from the documentation and commit message,
> >     since this actually replaces the other flags, unlike other cases.
>
> Some news regarding this: we asked upstream Rust about supporting
> overriding all flags (including e.g. `--edition`, `--target` and
> `--sysroot`) and apparently this was already accepted via an MCP
> (thanks Oli Scherer for the pointer!):
>
>     https://github.com/rust-lang/compiler-team/issues/731
>
> So, in the future, `rustc` will likely get support for this. Thus it
> may be best to go with an "additional" approach (rather than
> "replace"), so that this environment variable works the same way as
> the rest.
>
> We can do that by simply waiting until `rustc` implements it and we
> upgrade the minimum, or by implementing a workaround on our side
> meanwhile. For instance, something simple like:
>
>     $(filter-out --target=%,$(s)) $(lastword $(filter --target=%,$(s)))
>
> would be probably enough to cover Android's use case since we use the
> syntax with `=` elsewhere rather than with a space -- the equal sign
> plays well with Make's string functions. We can also add other flags
> if needed.

My original intention was to have PROCMACROLDFLAGS to be a completely
separate thing. That was partially why I used an $(or) there. This was
because "the list of flags to link hostprogs is not necessarily the
same as the list of flags used to link libmacros.so" (see commit
message). The fallback to HOSTLDFLAGS was just to be backwards
compatible so existing users don't get surprises.

In details, here's what Android does with V3 of the patch, roughly:

HOSTLDFLAGS=
    -L<paths>... -fuse-ld=lld --rtlib=compiler-rt
--sysroot=<musl_sysroot> -Wl,-rpath,<paths>
PROCMACROLDFLAGS=
    -fuse-ld=lld --rtlib=compiler-rt --sysroot=<glibc_sysroot>

With https://github.com/rust-lang/compiler-team/issues/731 fixed and
this idea of appending flags, our --sysroot flag should be able to be
properly overridden. But the -L and -Wl,-rpath's remains, and could
potentially be disturbing.

The reason for this difference is that we build hostprogs like
sign-file, fixdep, etc. with prebuilt libraries (e.g. sign-file needs
libcrypto, etc.) that were built against a prebuilt musl libc. On the
other hand, the Rust toolchain we are using was built against glibc,
and won't need these -L and -Wl,-rpath flags for the libraries.

So if I understand what you mean correctly, with this:
  KBUILD_PROCMACROLDFLAGS := $(HOSTLDFLAGS) $(PROCMACROLDFLAGS)
Android might need a separate mechanism (another variable?) to filter
out our -L/-Wl,-rpath from HOSTLDFLAGS. (Dumb question: We can't take
-L/-Wl,-rpath away by prepending/appending more flags, right?)

>
> I will send a v4 unless someone thinks it is a bad idea.
>
> Cheers,
> Miguel

Powered by blists - more mailing lists