[PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Miguel Ojeda]

From: HONG Yifan <elsk@google.com>

These are flags to be passed when linking proc macros for the Rust
toolchain. If unset, it defaults to $(KBUILD_HOSTLDFLAGS).

This is needed because the list of flags to link hostprogs is not
necessarily the same as the list of flags used to link libmacros.so.
When we build proc macros, we need the latter, not the former (e.g. when
using a Rust compiler binary linked to a different C library than host
programs).

To distinguish between the two, introduce this new variable to stand
out from KBUILD_HOSTLDFLAGS used to link other host progs.

Signed-off-by: HONG Yifan <elsk@google.com>
Link: https://lore.kernel.org/r/20241017210430.2401398-2-elsk@google.com
[ v3:

  - `export`ed the variable. Otherwise it would not be visible in
    `rust/Makefile`.

  - Removed "additional" from the documentation and commit message,
    since this actually replaces the other flags, unlike other cases.

  - Added example of use case to documentation and commit message.
    Thanks Alice for the details on what Google needs!

  - Instead of `HOSTLDFLAGS`, used `KBUILD_HOSTLDFLAGS` as the fallback
    to preserve the previous behavior as much as possible, as discussed
    with Alice/Yifan. Thus moved the variable down too (currently we
    do not modify `KBUILD_HOSTLDFLAGS` elsewhere) and avoided
    mentioning `HOSTLDFLAGS` directly in the documentation.

  - Fixed documentation header formatting.

  - Reworded slightly.

         - Miguel ]
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
---
Masahiro: if Kbuild wants to pick this up, that is great. Otherwise, I am happy
picking this up early next cycle, if you give an `Acked-by` since this is
changing the interface for Kbuild users given we are introducing a new
environment variable. Thanks!

Note that the `or` means if the string is empty, we will use the default rather
than nothing. I didn't change that from Yifan's version, but maybe we want to do
otherwise. Users can still provide e.g. an empty space to avoid any flag.

Yifan/Alice: please double-check the changes. Thanks!

v3: see changes above.
v2: https://lore.kernel.org/rust-for-linux/20241017210430.2401398-2-elsk@google.com/
v1: https://lore.kernel.org/rust-for-linux/20241017200138.2390077-2-elsk@google.com/

 Documentation/kbuild/kbuild.rst | 11 +++++++++++
 Makefile                        |  3 ++-
 rust/Makefile                   |  2 +-
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
index 1796b3eba37b..9cb876ccc363 100644
--- a/Documentation/kbuild/kbuild.rst
+++ b/Documentation/kbuild/kbuild.rst
@@ -91,6 +91,17 @@ HOSTRUSTFLAGS
 -------------
 Additional flags to be passed to $(HOSTRUSTC) when building host programs.

+PROCMACROLDFLAGS
+----------------
+Flags to be passed when linking Rust proc macros. Since proc macros are loaded
+by rustc at build time, they must be linked in a way that is compatible with
+the rustc toolchain being used.
+
+For instance, it can be useful when rustc uses a different C library than
+the one the user wants to use for host programs.
+
+If unset, it defaults to the flags passed when linking host programs.
+
 HOSTLDFLAGS
 -----------
 Additional flags to be passed when linking host programs.
diff --git a/Makefile b/Makefile
index a9e723cb0596..3efb001bada5 100644
--- a/Makefile
+++ b/Makefile
@@ -471,6 +471,7 @@ KBUILD_HOSTRUSTFLAGS := $(rust_common_flags) -O -Cstrip=debuginfo \
 			-Zallow-features= $(HOSTRUSTFLAGS)
 KBUILD_HOSTLDFLAGS  := $(HOST_LFS_LDFLAGS) $(HOSTLDFLAGS)
 KBUILD_HOSTLDLIBS   := $(HOST_LFS_LIBS) $(HOSTLDLIBS)
+KBUILD_PROCMACROLDFLAGS := $(or $(PROCMACROLDFLAGS),$(KBUILD_HOSTLDFLAGS))

 # Make variables (CC, etc...)
 CPP		= $(CC) -E
@@ -595,7 +596,7 @@ export HOSTRUSTC KBUILD_HOSTRUSTFLAGS
 export CPP AR NM STRIP OBJCOPY OBJDUMP READELF PAHOLE RESOLVE_BTFIDS LEX YACC AWK INSTALLKERNEL
 export PERL PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
 export KGZIP KBZIP2 KLZOP LZMA LZ4 XZ ZSTD
-export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS LDFLAGS_MODULE
+export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS KBUILD_PROCMACROLDFLAGS LDFLAGS_MODULE
 export KBUILD_USERCFLAGS KBUILD_USERLDFLAGS

 export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
diff --git a/rust/Makefile b/rust/Makefile
index f349e7b067ea..9f55c470aa2c 100644
--- a/rust/Makefile
+++ b/rust/Makefile
@@ -344,7 +344,7 @@ quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@
       cmd_rustc_procmacro = \
 	$(RUSTC_OR_CLIPPY) $(rust_common_flags) \
 		-Clinker-flavor=gcc -Clinker=$(HOSTCC) \
-		-Clink-args='$(call escsq,$(KBUILD_HOSTLDFLAGS))' \
+		-Clink-args='$(call escsq,$(KBUILD_PROCMACROLDFLAGS))' \
 		--emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \
 		--crate-type proc-macro \
 		--crate-name $(patsubst lib%.so,%,$(notdir $@)) $<

base-commit: d072acda4862f095ec9056979b654cc06a22cc68
--
2.47.0

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Alice Ryhl]

On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> From: HONG Yifan <elsk@google.com>
>
> These are flags to be passed when linking proc macros for the Rust
> toolchain. If unset, it defaults to $(KBUILD_HOSTLDFLAGS).
>
> This is needed because the list of flags to link hostprogs is not
> necessarily the same as the list of flags used to link libmacros.so.
> When we build proc macros, we need the latter, not the former (e.g. when
> using a Rust compiler binary linked to a different C library than host
> programs).
>
> To distinguish between the two, introduce this new variable to stand
> out from KBUILD_HOSTLDFLAGS used to link other host progs.
>
> Signed-off-by: HONG Yifan <elsk@google.com>
> Link: https://lore.kernel.org/r/20241017210430.2401398-2-elsk@google.com
> [ v3:
>
>   - `export`ed the variable. Otherwise it would not be visible in
>     `rust/Makefile`.

Despite the missing export, the previous version worked for us too.
I'm not sure why that is.

>   - Removed "additional" from the documentation and commit message,
>     since this actually replaces the other flags, unlike other cases.
>
>   - Added example of use case to documentation and commit message.
>     Thanks Alice for the details on what Google needs!
>
>   - Instead of `HOSTLDFLAGS`, used `KBUILD_HOSTLDFLAGS` as the fallback
>     to preserve the previous behavior as much as possible, as discussed
>     with Alice/Yifan. Thus moved the variable down too (currently we
>     do not modify `KBUILD_HOSTLDFLAGS` elsewhere) and avoided
>     mentioning `HOSTLDFLAGS` directly in the documentation.
>
>   - Fixed documentation header formatting.
>
>   - Reworded slightly.
>
>          - Miguel ]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>

Tested-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Hong, Yifan]

On Wed, Nov 13, 2024 at 5:34 AM Alice Ryhl <aliceryhl@google.com> wrote:
>
> On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@kernel.org> wrote:
> >
> > From: HONG Yifan <elsk@google.com>
> >
> > These are flags to be passed when linking proc macros for the Rust
> > toolchain. If unset, it defaults to $(KBUILD_HOSTLDFLAGS).
> >
> > This is needed because the list of flags to link hostprogs is not
> > necessarily the same as the list of flags used to link libmacros.so.
> > When we build proc macros, we need the latter, not the former (e.g. when
> > using a Rust compiler binary linked to a different C library than host
> > programs).
> >
> > To distinguish between the two, introduce this new variable to stand
> > out from KBUILD_HOSTLDFLAGS used to link other host progs.
> >
> > Signed-off-by: HONG Yifan <elsk@google.com>
> > Link: https://lore.kernel.org/r/20241017210430.2401398-2-elsk@google.com
> > [ v3:
> >
> >   - `export`ed the variable. Otherwise it would not be visible in
> >     `rust/Makefile`.
>
> Despite the missing export, the previous version worked for us too.
> I'm not sure why that is.

It happened to pass the build when KBUILD_HOSTLDFLAGS was empty, which
was the case when it was not exported. But that was definitely not the
original intention of this patch. Thanks for catching it! I have
verified that v3 correctly exports the variable to sub-make and still
works for our case.

>
> >   - Removed "additional" from the documentation and commit message,
> >     since this actually replaces the other flags, unlike other cases.
> >
> >   - Added example of use case to documentation and commit message.
> >     Thanks Alice for the details on what Google needs!
> >
> >   - Instead of `HOSTLDFLAGS`, used `KBUILD_HOSTLDFLAGS` as the fallback
> >     to preserve the previous behavior as much as possible, as discussed
> >     with Alice/Yifan. Thus moved the variable down too (currently we
> >     do not modify `KBUILD_HOSTLDFLAGS` elsewhere) and avoided
> >     mentioning `HOSTLDFLAGS` directly in the documentation.
> >
> >   - Fixed documentation header formatting.
> >
> >   - Reworded slightly.
> >
> >          - Miguel ]
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
>
> Tested-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: HONG Yifan <elsk@google.com>

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Hong, Yifan]

On Tue, Nov 12, 2024 at 10:45 AM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> From: HONG Yifan <elsk@google.com>
>
> These are flags to be passed when linking proc macros for the Rust
> toolchain. If unset, it defaults to $(KBUILD_HOSTLDFLAGS).
>
> This is needed because the list of flags to link hostprogs is not
> necessarily the same as the list of flags used to link libmacros.so.
> When we build proc macros, we need the latter, not the former (e.g. when
> using a Rust compiler binary linked to a different C library than host
> programs).
>
> To distinguish between the two, introduce this new variable to stand
> out from KBUILD_HOSTLDFLAGS used to link other host progs.
>
> Signed-off-by: HONG Yifan <elsk@google.com>
> Link: https://lore.kernel.org/r/20241017210430.2401398-2-elsk@google.com
> [ v3:
>
>   - `export`ed the variable. Otherwise it would not be visible in
>     `rust/Makefile`.
>
>   - Removed "additional" from the documentation and commit message,
>     since this actually replaces the other flags, unlike other cases.
>
>   - Added example of use case to documentation and commit message.
>     Thanks Alice for the details on what Google needs!
>
>   - Instead of `HOSTLDFLAGS`, used `KBUILD_HOSTLDFLAGS` as the fallback
>     to preserve the previous behavior as much as possible, as discussed
>     with Alice/Yifan. Thus moved the variable down too (currently we
>     do not modify `KBUILD_HOSTLDFLAGS` elsewhere) and avoided
>     mentioning `HOSTLDFLAGS` directly in the documentation.
>
>   - Fixed documentation header formatting.
>
>   - Reworded slightly.
>
>          - Miguel ]
> Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> ---
> Masahiro: if Kbuild wants to pick this up, that is great. Otherwise, I am happy
> picking this up early next cycle, if you give an `Acked-by` since this is
> changing the interface for Kbuild users given we are introducing a new
> environment variable. Thanks!
>
> Note that the `or` means if the string is empty, we will use the default rather
> than nothing. I didn't change that from Yifan's version, but maybe we want to do
> otherwise. Users can still provide e.g. an empty space to avoid any flag.

I am not sure if I understand the implications here.
https://www.gnu.org/software/make/manual/html_node/Conditional-Functions.html
says:

The or function provides a “short-circuiting” OR operation. Each
argument is expanded, in order. If an argument expands to a non-empty
string the processing stops and the result of the expansion is that
string. If, after all arguments are expanded, all of them are false
(empty), then the result of the expansion is the empty string.

I am assuming that this means:
- If PROCMACROLDFLAGS is not empty, KBUILD_PROCMACROLDFLAGS evaluates
to PROCMACROLDFLAGS
- Otherwise if KBUILD_HOSTLDFLAGS is not empty,
KBUILD_PROCMACROLDFLAGS evaluates to KBUILD_HOSTLDFLAGS
- Otherwise KBUILD_PROCMACROLDFLAGS is set to empty.

What do you mean by "use the default"?


>
> Yifan/Alice: please double-check the changes. Thanks!
>
> v3: see changes above.
> v2: https://lore.kernel.org/rust-for-linux/20241017210430.2401398-2-elsk@google.com/
> v1: https://lore.kernel.org/rust-for-linux/20241017200138.2390077-2-elsk@google.com/
>
>  Documentation/kbuild/kbuild.rst | 11 +++++++++++
>  Makefile                        |  3 ++-
>  rust/Makefile                   |  2 +-
>  3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/kbuild/kbuild.rst b/Documentation/kbuild/kbuild.rst
> index 1796b3eba37b..9cb876ccc363 100644
> --- a/Documentation/kbuild/kbuild.rst
> +++ b/Documentation/kbuild/kbuild.rst
> @@ -91,6 +91,17 @@ HOSTRUSTFLAGS
>  -------------
>  Additional flags to be passed to $(HOSTRUSTC) when building host programs.
>
> +PROCMACROLDFLAGS
> +----------------
> +Flags to be passed when linking Rust proc macros. Since proc macros are loaded
> +by rustc at build time, they must be linked in a way that is compatible with
> +the rustc toolchain being used.
> +
> +For instance, it can be useful when rustc uses a different C library than
> +the one the user wants to use for host programs.
> +
> +If unset, it defaults to the flags passed when linking host programs.
> +
>  HOSTLDFLAGS
>  -----------
>  Additional flags to be passed when linking host programs.
> diff --git a/Makefile b/Makefile
> index a9e723cb0596..3efb001bada5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -471,6 +471,7 @@ KBUILD_HOSTRUSTFLAGS := $(rust_common_flags) -O -Cstrip=debuginfo \
>                         -Zallow-features= $(HOSTRUSTFLAGS)
>  KBUILD_HOSTLDFLAGS  := $(HOST_LFS_LDFLAGS) $(HOSTLDFLAGS)
>  KBUILD_HOSTLDLIBS   := $(HOST_LFS_LIBS) $(HOSTLDLIBS)
> +KBUILD_PROCMACROLDFLAGS := $(or $(PROCMACROLDFLAGS),$(KBUILD_HOSTLDFLAGS))
>
>  # Make variables (CC, etc...)
>  CPP            = $(CC) -E
> @@ -595,7 +596,7 @@ export HOSTRUSTC KBUILD_HOSTRUSTFLAGS
>  export CPP AR NM STRIP OBJCOPY OBJDUMP READELF PAHOLE RESOLVE_BTFIDS LEX YACC AWK INSTALLKERNEL
>  export PERL PYTHON3 CHECK CHECKFLAGS MAKE UTS_MACHINE HOSTCXX
>  export KGZIP KBZIP2 KLZOP LZMA LZ4 XZ ZSTD
> -export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS LDFLAGS_MODULE
> +export KBUILD_HOSTCXXFLAGS KBUILD_HOSTLDFLAGS KBUILD_HOSTLDLIBS KBUILD_PROCMACROLDFLAGS LDFLAGS_MODULE
>  export KBUILD_USERCFLAGS KBUILD_USERLDFLAGS
>
>  export KBUILD_CPPFLAGS NOSTDINC_FLAGS LINUXINCLUDE OBJCOPYFLAGS KBUILD_LDFLAGS
> diff --git a/rust/Makefile b/rust/Makefile
> index f349e7b067ea..9f55c470aa2c 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -344,7 +344,7 @@ quiet_cmd_rustc_procmacro = $(RUSTC_OR_CLIPPY_QUIET) P $@
>        cmd_rustc_procmacro = \
>         $(RUSTC_OR_CLIPPY) $(rust_common_flags) \
>                 -Clinker-flavor=gcc -Clinker=$(HOSTCC) \
> -               -Clink-args='$(call escsq,$(KBUILD_HOSTLDFLAGS))' \
> +               -Clink-args='$(call escsq,$(KBUILD_PROCMACROLDFLAGS))' \
>                 --emit=dep-info=$(depfile) --emit=link=$@ --extern proc_macro \
>                 --crate-type proc-macro \
>                 --crate-name $(patsubst lib%.so,%,$(notdir $@)) $<
>
> base-commit: d072acda4862f095ec9056979b654cc06a22cc68
> --
> 2.47.0

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Masahiro Yamada]

On Fri, Dec 6, 2024 at 8:40 AM Hong, Yifan <elsk@google.com> wrote:
>
> On Tue, Nov 12, 2024 at 10:45 AM Miguel Ojeda <ojeda@kernel.org> wrote:
> >
> > From: HONG Yifan <elsk@google.com>
> >
> > These are flags to be passed when linking proc macros for the Rust
> > toolchain. If unset, it defaults to $(KBUILD_HOSTLDFLAGS).
> >
> > This is needed because the list of flags to link hostprogs is not
> > necessarily the same as the list of flags used to link libmacros.so.
> > When we build proc macros, we need the latter, not the former (e.g. when
> > using a Rust compiler binary linked to a different C library than host
> > programs).
> >
> > To distinguish between the two, introduce this new variable to stand
> > out from KBUILD_HOSTLDFLAGS used to link other host progs.
> >
> > Signed-off-by: HONG Yifan <elsk@google.com>
> > Link: https://lore.kernel.org/r/20241017210430.2401398-2-elsk@google.com
> > [ v3:
> >
> >   - `export`ed the variable. Otherwise it would not be visible in
> >     `rust/Makefile`.
> >
> >   - Removed "additional" from the documentation and commit message,
> >     since this actually replaces the other flags, unlike other cases.
> >
> >   - Added example of use case to documentation and commit message.
> >     Thanks Alice for the details on what Google needs!
> >
> >   - Instead of `HOSTLDFLAGS`, used `KBUILD_HOSTLDFLAGS` as the fallback
> >     to preserve the previous behavior as much as possible, as discussed
> >     with Alice/Yifan. Thus moved the variable down too (currently we
> >     do not modify `KBUILD_HOSTLDFLAGS` elsewhere) and avoided
> >     mentioning `HOSTLDFLAGS` directly in the documentation.
> >
> >   - Fixed documentation header formatting.
> >
> >   - Reworded slightly.
> >
> >          - Miguel ]
> > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > ---
> > Masahiro: if Kbuild wants to pick this up, that is great. Otherwise, I am happy
> > picking this up early next cycle, if you give an `Acked-by` since this is
> > changing the interface for Kbuild users given we are introducing a new
> > environment variable. Thanks!
> >
> > Note that the `or` means if the string is empty, we will use the default rather
> > than nothing. I didn't change that from Yifan's version, but maybe we want to do
> > otherwise. Users can still provide e.g. an empty space to avoid any flag.
>
> I am not sure if I understand the implications here.
> https://www.gnu.org/software/make/manual/html_node/Conditional-Functions.html
> says:
>
> The or function provides a “short-circuiting” OR operation. Each
> argument is expanded, in order. If an argument expands to a non-empty
> string the processing stops and the result of the expansion is that
> string. If, after all arguments are expanded, all of them are false
> (empty), then the result of the expansion is the empty string.
>
> I am assuming that this means:
> - If PROCMACROLDFLAGS is not empty, KBUILD_PROCMACROLDFLAGS evaluates
> to PROCMACROLDFLAGS
> - Otherwise if KBUILD_HOSTLDFLAGS is not empty,
> KBUILD_PROCMACROLDFLAGS evaluates to KBUILD_HOSTLDFLAGS
> - Otherwise KBUILD_PROCMACROLDFLAGS is set to empty.

I think your understanding is correct.

$(or A,B) works like $(if A,A,B)

Commit 5c8166419acf shorten the code.



> What do you mean by "use the default"?


"use the default" means,
"use $(KBUILD_HOSTLDFLAGS)"


-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Hong, Yifan]

On Thu, Dec 5, 2024 at 5:28 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Fri, Dec 6, 2024 at 8:40 AM Hong, Yifan <elsk@google.com> wrote:
> >
> > On Tue, Nov 12, 2024 at 10:45 AM Miguel Ojeda <ojeda@kernel.org> wrote:
> > >
> > > From: HONG Yifan <elsk@google.com>
> > >
> > > These are flags to be passed when linking proc macros for the Rust
> > > toolchain. If unset, it defaults to $(KBUILD_HOSTLDFLAGS).
> > >
> > > This is needed because the list of flags to link hostprogs is not
> > > necessarily the same as the list of flags used to link libmacros.so.
> > > When we build proc macros, we need the latter, not the former (e.g. when
> > > using a Rust compiler binary linked to a different C library than host
> > > programs).
> > >
> > > To distinguish between the two, introduce this new variable to stand
> > > out from KBUILD_HOSTLDFLAGS used to link other host progs.
> > >
> > > Signed-off-by: HONG Yifan <elsk@google.com>
> > > Link: https://lore.kernel.org/r/20241017210430.2401398-2-elsk@google.com
> > > [ v3:
> > >
> > >   - `export`ed the variable. Otherwise it would not be visible in
> > >     `rust/Makefile`.
> > >
> > >   - Removed "additional" from the documentation and commit message,
> > >     since this actually replaces the other flags, unlike other cases.
> > >
> > >   - Added example of use case to documentation and commit message.
> > >     Thanks Alice for the details on what Google needs!
> > >
> > >   - Instead of `HOSTLDFLAGS`, used `KBUILD_HOSTLDFLAGS` as the fallback
> > >     to preserve the previous behavior as much as possible, as discussed
> > >     with Alice/Yifan. Thus moved the variable down too (currently we
> > >     do not modify `KBUILD_HOSTLDFLAGS` elsewhere) and avoided
> > >     mentioning `HOSTLDFLAGS` directly in the documentation.
> > >
> > >   - Fixed documentation header formatting.
> > >
> > >   - Reworded slightly.
> > >
> > >          - Miguel ]
> > > Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
> > > ---
> > > Masahiro: if Kbuild wants to pick this up, that is great. Otherwise, I am happy
> > > picking this up early next cycle, if you give an `Acked-by` since this is
> > > changing the interface for Kbuild users given we are introducing a new
> > > environment variable. Thanks!
> > >
> > > Note that the `or` means if the string is empty, we will use the default rather
> > > than nothing. I didn't change that from Yifan's version, but maybe we want to do
> > > otherwise. Users can still provide e.g. an empty space to avoid any flag.
> >
> > I am not sure if I understand the implications here.
> > https://www.gnu.org/software/make/manual/html_node/Conditional-Functions.html
> > says:
> >
> > The or function provides a “short-circuiting” OR operation. Each
> > argument is expanded, in order. If an argument expands to a non-empty
> > string the processing stops and the result of the expansion is that
> > string. If, after all arguments are expanded, all of them are false
> > (empty), then the result of the expansion is the empty string.
> >
> > I am assuming that this means:
> > - If PROCMACROLDFLAGS is not empty, KBUILD_PROCMACROLDFLAGS evaluates
> > to PROCMACROLDFLAGS
> > - Otherwise if KBUILD_HOSTLDFLAGS is not empty,
> > KBUILD_PROCMACROLDFLAGS evaluates to KBUILD_HOSTLDFLAGS
> > - Otherwise KBUILD_PROCMACROLDFLAGS is set to empty.
>
> I think your understanding is correct.
>
> $(or A,B) works like $(if A,A,B)
>
> Commit 5c8166419acf shorten the code.
>
>
>
> > What do you mean by "use the default"?
>
>
> "use the default" means,
> "use $(KBUILD_HOSTLDFLAGS)"

Thank you for confirming. I think this is my original intention. If
PROCMACROLDFLAGS (a new API) is not set, the code should have the same
behavior as before this patch, i.e. cmd_rustc_procmacro uses
-Clink-args KBUILD_HOSTLDFLAGS. This minimizes surprises for existing
users.


>
>
> --
> Best Regards
> Masahiro Yamada

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Miguel Ojeda]

On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@kernel.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.

I will send a v4 unless someone thinks it is a bad idea.

Cheers,
Miguel

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Hong, Yifan]

On Mon, Dec 9, 2024 at 10:30 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@kernel.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

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Miguel Ojeda]

On Tue, Dec 10, 2024 at 1:51 AM Hong, Yifan <elsk@google.com> wrote:
>
> 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.

On second thought, #731 probably only applies to `rustc` native flags
only, rather than those that are passed to the linker (e.g. if I pass
a dummy flag, it is passed as-is, so I doubt they will start checking,
and probably they shouldn't), so it may not help here after all.

> 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?)

Yeah, we would need a variable to provide the filters, but it would be
more complex and possibly less flexible. I think it may be best to
keep things simple and use the v3 here, which already works for your
use case.

Cheers,
Miguel

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Miguel Ojeda]

On Tue, Jan 14, 2025 at 11:46 PM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On second thought, #731 probably only applies to `rustc` native flags
> only, rather than those that are passed to the linker (e.g. if I pass
> a dummy flag, it is passed as-is, so I doubt they will start checking,
> and probably they shouldn't), so it may not help here after all.

For completeness: `-Clink-args=` appends (and is allowed to be passed
several times), rather than override. Moreover, the docs say so
explicitly. So it is unlikely that one would change or that #731
applies to it (i.e. as a whole vs. individual options inside it).

Cheers,
Miguel

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Miguel Ojeda]

On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@kernel.org> wrote:
>
> To distinguish between the two, introduce this new variable to stand
> out from KBUILD_HOSTLDFLAGS used to link other host progs.

Applied to `rust-next` -- thanks everyone!

Masahiro: if you wanted to pick this up through Kbuild or to give an
Acked-by, please let me know!

Cheers,
Miguel

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Masahiro Yamada]

On Wed, Jan 15, 2025 at 8:05 AM Miguel Ojeda
<miguel.ojeda.sandonis@gmail.com> wrote:
>
> On Tue, Nov 12, 2024 at 7:45 PM Miguel Ojeda <ojeda@kernel.org> wrote:
> >
> > To distinguish between the two, introduce this new variable to stand
> > out from KBUILD_HOSTLDFLAGS used to link other host progs.
>
> Applied to `rust-next` -- thanks everyone!
>
> Masahiro: if you wanted to pick this up through Kbuild or to give an
> Acked-by, please let me know!

Acked-by: Masahiro Yamada <masahiroy@kernel.org>

-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3] kbuild: rust: add PROCMACROLDFLAGS

[Miguel Ojeda]

On Wed, Jan 15, 2025 at 3:56 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Acked-by: Masahiro Yamada <masahiroy@kernel.org>

Added, thanks a lot!

Cheers,
Miguel