[v2] Kbuild: fix issues with rustc-option

Commit Message

Alice Ryhl Oct. 8, 2024, 5:32 p.m. UTC

Fix a few different compiler errors that cause rustc-option to give
wrong results.

If KBUILD_RUSTFLAGS or the flags being tested contain any -Z flags, then
the error below is generated. The RUSTC_BOOTSTRAP environment variable
is added to fix this error.

	error: the option `Z` is only accepted on the nightly compiler
	help: consider switching to a nightly toolchain: `rustup default nightly`
	note: selecting a toolchain with `+toolchain` arguments require a rustup proxy;
	      see <https://rust-lang.github.io/rustup/concepts/index.html>
	note: for more information about Rust's stability policy, see
	      <https://doc.rust-lang.org/book/appendix-07-nightly-rust.html#unstable-features>
	error: 1 nightly option were parsed

The probe may also fail incorrectly with the below error message. To fix
it, the /dev/null argument is replaced with a new rust/probe.rs file
that doesn't need even the core part of the standard library.

error[E0463]: can't find crate for `std`
  |
  = note: the `aarch64-unknown-none` target may not be installed
  = help: consider downloading the target with `rustup target add aarch64-unknown-none`
  = help: consider building the standard library from source with `cargo build -Zbuild-std`

The -o and --out-dir parameters are altered to fix this warning:

	warning: ignoring --out-dir flag due to -o flag

I verified that the Kconfig version of rustc-option doesn't have the
same issues.

Fixes: c42297438aee ("kbuild: rust: Define probing macros for rustc")
Signed-off-by: Alice Ryhl <[e-mail]>
---
Changes in v2:
- Add `export` to RUSTC_BOOTSTRAP.
- Fix error about core being missing.
- Fix warning about -o flag.
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 rust/probe.rs             | 7 +++++++
 scripts/Makefile.compiler | 5 +++--
 2 files changed, 10 insertions(+), 2 deletions(-)


---
base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
change-id: 20241008-rustc-option-bootstrap-607e5bf3114c

Best regards,

Comments

Matthew Maurer Oct. 8, 2024, 5:43 p.m. UTC | #1

Thanks for catching this - I had a specialized toolchain enabled which
didn't exercise the `RUSTC_BOOTSTRAP` issue, and without an unexpected
failure, didn't see the `--out-dir` flag conflict after I got the
request to add it.

Reviewed-By: Matthew Maurer <[e-mail]>


On Tue, Oct 8, 2024 at 10:32 AM Alice Ryhl <[e-mail]> wrote:
>
> Fix a few different compiler errors that cause rustc-option to give
> wrong results.
>
> If KBUILD_RUSTFLAGS or the flags being tested contain any -Z flags, then
> the error below is generated. The RUSTC_BOOTSTRAP environment variable
> is added to fix this error.
>
>         error: the option `Z` is only accepted on the nightly compiler
>         help: consider switching to a nightly toolchain: `rustup default nightly`
>         note: selecting a toolchain with `+toolchain` arguments require a rustup proxy;
>               see <https://rust-lang.github.io/rustup/concepts/index.html>
>         note: for more information about Rust's stability policy, see
>               <https://doc.rust-lang.org/book/appendix-07-nightly-rust.html#unstable-features>
>         error: 1 nightly option were parsed
>
> The probe may also fail incorrectly with the below error message. To fix
> it, the /dev/null argument is replaced with a new rust/probe.rs file
> that doesn't need even the core part of the standard library.
>
> error[E0463]: can't find crate for `std`
>   |
>   = note: the `aarch64-unknown-none` target may not be installed
>   = help: consider downloading the target with `rustup target add aarch64-unknown-none`
>   = help: consider building the standard library from source with `cargo build -Zbuild-std`
>
> The -o and --out-dir parameters are altered to fix this warning:
>
>         warning: ignoring --out-dir flag due to -o flag
>
> I verified that the Kconfig version of rustc-option doesn't have the
> same issues.
>
> Fixes: c42297438aee ("kbuild: rust: Define probing macros for rustc")
> Signed-off-by: Alice Ryhl <[e-mail]>
> ---
> Changes in v2:
> - Add `export` to RUSTC_BOOTSTRAP.
> - Fix error about core being missing.
> - Fix warning about -o flag.
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  rust/probe.rs             | 7 +++++++
>  scripts/Makefile.compiler | 5 +++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/rust/probe.rs b/rust/probe.rs
> new file mode 100644
> index 000000000000..bf024e394408
> --- /dev/null
> +++ b/rust/probe.rs
> @@ -0,0 +1,7 @@
> +//! Nearly empty file passed to rustc-option by Make.
> +//!
> +//! The no_core attribute is needed because rustc-option otherwise fails due to
> +//! not being able to find the core part of the standard library.
> +
> +#![feature(no_core)]
> +#![no_core]
> diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> index 057305eae85c..08d5b7177ea8 100644
> --- a/scripts/Makefile.compiler
> +++ b/scripts/Makefile.compiler
> @@ -21,6 +21,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
>  # automatically cleaned up.
>  try-run = $(shell set -e;              \
>         TMP=$(TMPOUT)/tmp;              \
> +       export RUSTC_BOOTSTRAP=1;       \
>         trap "rm -rf $(TMPOUT)" EXIT;   \
>         mkdir -p $(TMPOUT);             \
>         if ($(1)) >/dev/null 2>&1;      \
> @@ -76,7 +77,7 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3))
>  # __rustc-option
>  # Usage: MY_RUSTFLAGS += $(call __rustc-option,$(RUSTC),$(MY_RUSTFLAGS),-Cinstrument-coverage,-Zinstrument-coverage)
>  __rustc-option = $(call try-run,\
> -       $(1) $(2) $(3) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",$(3),$(4))
> +       $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))
>
>  # rustc-option
>  # Usage: rustflags-y += $(call rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)
> @@ -86,4 +87,4 @@ rustc-option = $(call __rustc-option, $(RUSTC),\
>  # rustc-option-yn
>  # Usage: flag := $(call rustc-option-yn,-Cinstrument-coverage)
>  rustc-option-yn = $(call try-run,\
> -       $(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",y,n)
> +       $(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,y,n)
>
> ---
> base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
> change-id: 20241008-rustc-option-bootstrap-607e5bf3114c
>
> Best regards,
> --
> Alice Ryhl <[e-mail]>
>

Matthew Maurer Oct. 8, 2024, 6:25 p.m. UTC | #2

Err, slight amendment - I think you want `-o $$TMP`, and not
`--out-dir $$TMPOUT`

You definitely don't want `--out-dir $$TMP`, but the other two
settings above would be defensible.


On Tue, Oct 8, 2024 at 10:43 AM Matthew Maurer <[e-mail]> wrote:
>
> Thanks for catching this - I had a specialized toolchain enabled which
> didn't exercise the `RUSTC_BOOTSTRAP` issue, and without an unexpected
> failure, didn't see the `--out-dir` flag conflict after I got the
> request to add it.
>
> Reviewed-By: Matthew Maurer <[e-mail]>
>
>
> On Tue, Oct 8, 2024 at 10:32 AM Alice Ryhl <[e-mail]> wrote:
> >
> > Fix a few different compiler errors that cause rustc-option to give
> > wrong results.
> >
> > If KBUILD_RUSTFLAGS or the flags being tested contain any -Z flags, then
> > the error below is generated. The RUSTC_BOOTSTRAP environment variable
> > is added to fix this error.
> >
> >         error: the option `Z` is only accepted on the nightly compiler
> >         help: consider switching to a nightly toolchain: `rustup default nightly`
> >         note: selecting a toolchain with `+toolchain` arguments require a rustup proxy;
> >               see <https://rust-lang.github.io/rustup/concepts/index.html>
> >         note: for more information about Rust's stability policy, see
> >               <https://doc.rust-lang.org/book/appendix-07-nightly-rust.html#unstable-features>
> >         error: 1 nightly option were parsed
> >
> > The probe may also fail incorrectly with the below error message. To fix
> > it, the /dev/null argument is replaced with a new rust/probe.rs file
> > that doesn't need even the core part of the standard library.
> >
> > error[E0463]: can't find crate for `std`
> >   |
> >   = note: the `aarch64-unknown-none` target may not be installed
> >   = help: consider downloading the target with `rustup target add aarch64-unknown-none`
> >   = help: consider building the standard library from source with `cargo build -Zbuild-std`
> >
> > The -o and --out-dir parameters are altered to fix this warning:
> >
> >         warning: ignoring --out-dir flag due to -o flag
> >
> > I verified that the Kconfig version of rustc-option doesn't have the
> > same issues.
> >
> > Fixes: c42297438aee ("kbuild: rust: Define probing macros for rustc")
> > Signed-off-by: Alice Ryhl <[e-mail]>
> > ---
> > Changes in v2:
> > - Add `export` to RUSTC_BOOTSTRAP.
> > - Fix error about core being missing.
> > - Fix warning about -o flag.
> > - Link to v1: https://lore.kernel.org/r/[e-mail]
> > ---
> >  rust/probe.rs             | 7 +++++++
> >  scripts/Makefile.compiler | 5 +++--
> >  2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust/probe.rs b/rust/probe.rs
> > new file mode 100644
> > index 000000000000..bf024e394408
> > --- /dev/null
> > +++ b/rust/probe.rs
> > @@ -0,0 +1,7 @@
> > +//! Nearly empty file passed to rustc-option by Make.
> > +//!
> > +//! The no_core attribute is needed because rustc-option otherwise fails due to
> > +//! not being able to find the core part of the standard library.
> > +
> > +#![feature(no_core)]
> > +#![no_core]
> > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> > index 057305eae85c..08d5b7177ea8 100644
> > --- a/scripts/Makefile.compiler
> > +++ b/scripts/Makefile.compiler
> > @@ -21,6 +21,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
> >  # automatically cleaned up.
> >  try-run = $(shell set -e;              \
> >         TMP=$(TMPOUT)/tmp;              \
> > +       export RUSTC_BOOTSTRAP=1;       \
> >         trap "rm -rf $(TMPOUT)" EXIT;   \
> >         mkdir -p $(TMPOUT);             \
> >         if ($(1)) >/dev/null 2>&1;      \
> > @@ -76,7 +77,7 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3))
> >  # __rustc-option
> >  # Usage: MY_RUSTFLAGS += $(call __rustc-option,$(RUSTC),$(MY_RUSTFLAGS),-Cinstrument-coverage,-Zinstrument-coverage)
> >  __rustc-option = $(call try-run,\
> > -       $(1) $(2) $(3) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",$(3),$(4))
> > +       $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))
> >
> >  # rustc-option
> >  # Usage: rustflags-y += $(call rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)
> > @@ -86,4 +87,4 @@ rustc-option = $(call __rustc-option, $(RUSTC),\
> >  # rustc-option-yn
> >  # Usage: flag := $(call rustc-option-yn,-Cinstrument-coverage)
> >  rustc-option-yn = $(call try-run,\
> > -       $(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",y,n)
> > +       $(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,y,n)
> >
> > ---
> > base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
> > change-id: 20241008-rustc-option-bootstrap-607e5bf3114c
> >
> > Best regards,
> > --
> > Alice Ryhl <[e-mail]>
> >

Masahiro Yamada Oct. 8, 2024, 6:59 p.m. UTC | #3

On Wed, Oct 9, 2024 at 2:32 AM Alice Ryhl <[e-mail]> wrote:
>
> Fix a few different compiler errors that cause rustc-option to give
> wrong results.
>
> If KBUILD_RUSTFLAGS or the flags being tested contain any -Z flags, then
> the error below is generated. The RUSTC_BOOTSTRAP environment variable
> is added to fix this error.
>
>         error: the option `Z` is only accepted on the nightly compiler
>         help: consider switching to a nightly toolchain: `rustup default nightly`
>         note: selecting a toolchain with `+toolchain` arguments require a rustup proxy;
>               see <https://rust-lang.github.io/rustup/concepts/index.html>
>         note: for more information about Rust's stability policy, see
>               <https://doc.rust-lang.org/book/appendix-07-nightly-rust.html#unstable-features>
>         error: 1 nightly option were parsed
>
> The probe may also fail incorrectly with the below error message. To fix
> it, the /dev/null argument is replaced with a new rust/probe.rs file
> that doesn't need even the core part of the standard library.
>
> error[E0463]: can't find crate for `std`
>   |
>   = note: the `aarch64-unknown-none` target may not be installed
>   = help: consider downloading the target with `rustup target add aarch64-unknown-none`
>   = help: consider building the standard library from source with `cargo build -Zbuild-std`
>
> The -o and --out-dir parameters are altered to fix this warning:
>
>         warning: ignoring --out-dir flag due to -o flag
>
> I verified that the Kconfig version of rustc-option doesn't have the
> same issues.
>
> Fixes: c42297438aee ("kbuild: rust: Define probing macros for rustc")
> Signed-off-by: Alice Ryhl <[e-mail]>
> ---
> Changes in v2:
> - Add `export` to RUSTC_BOOTSTRAP.
> - Fix error about core being missing.
> - Fix warning about -o flag.
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  rust/probe.rs             | 7 +++++++
>  scripts/Makefile.compiler | 5 +++--
>  2 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/rust/probe.rs b/rust/probe.rs
> new file mode 100644
> index 000000000000..bf024e394408
> --- /dev/null
> +++ b/rust/probe.rs
> @@ -0,0 +1,7 @@
> +//! Nearly empty file passed to rustc-option by Make.
> +//!
> +//! The no_core attribute is needed because rustc-option otherwise fails due to
> +//! not being able to find the core part of the standard library.
> +
> +#![feature(no_core)]
> +#![no_core]
> diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> index 057305eae85c..08d5b7177ea8 100644
> --- a/scripts/Makefile.compiler
> +++ b/scripts/Makefile.compiler
> @@ -21,6 +21,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
>  # automatically cleaned up.
>  try-run = $(shell set -e;              \
>         TMP=$(TMPOUT)/tmp;              \
> +       export RUSTC_BOOTSTRAP=1;       \


try-run is not Rust-specific.

Is there any reason why you did not add it
to __rustc-option?


__rustc-option = $(call try-run,\
       RUSTC_BOOTSTRAP=1 $(1) $(2) $(3) --crate-type=rlib
$(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))



I guess it is still suspicious because the top-level Makefile
exports RUCTC_BOOTSTRAP.










>         trap "rm -rf $(TMPOUT)" EXIT;   \
>         mkdir -p $(TMPOUT);             \
>         if ($(1)) >/dev/null 2>&1;      \
> @@ -76,7 +77,7 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3))
>  # __rustc-option
>  # Usage: MY_RUSTFLAGS += $(call __rustc-option,$(RUSTC),$(MY_RUSTFLAGS),-Cinstrument-coverage,-Zinstrument-coverage)
>  __rustc-option = $(call try-run,\
> -       $(1) $(2) $(3) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",$(3),$(4))
> +       $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))
>
>  # rustc-option
>  # Usage: rustflags-y += $(call rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)
> @@ -86,4 +87,4 @@ rustc-option = $(call __rustc-option, $(RUSTC),\
>  # rustc-option-yn
>  # Usage: flag := $(call rustc-option-yn,-Cinstrument-coverage)
>  rustc-option-yn = $(call try-run,\
> -       $(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",y,n)
> +       $(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,y,n)
>
> ---
> base-commit: 8cf0b93919e13d1e8d4466eb4080a4c4d9d66d7b
> change-id: 20241008-rustc-option-bootstrap-607e5bf3114c
>
> Best regards,
> --
> Alice Ryhl <[e-mail]>
>
>

Alice Ryhl Oct. 8, 2024, 7:25 p.m. UTC | #4

On Tue, Oct 8, 2024 at 8:25 PM Matthew Maurer <[e-mail]> wrote:
>
> Err, slight amendment - I think you want `-o $$TMP`, and not
> `--out-dir $$TMPOUT`
>
> You definitely don't want `--out-dir $$TMP`, but the other two
> settings above would be defensible.

Ah good point. It seems like the reason that `--out-dir $$TMP` still
works is that it creates a sub-directory. But I agree that this is not
what we want.

Alice

Alice Ryhl Oct. 8, 2024, 7:42 p.m. UTC | #5

On Tue, Oct 8, 2024 at 9:00 PM Masahiro Yamada <[e-mail]> wrote:
>
> On Wed, Oct 9, 2024 at 2:32 AM Alice Ryhl <[e-mail]> wrote:
> > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> > index 057305eae85c..08d5b7177ea8 100644
> > --- a/scripts/Makefile.compiler
> > +++ b/scripts/Makefile.compiler
> > @@ -21,6 +21,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
> >  # automatically cleaned up.
> >  try-run = $(shell set -e;              \
> >         TMP=$(TMPOUT)/tmp;              \
> > +       export RUSTC_BOOTSTRAP=1;       \
>
>
> try-run is not Rust-specific.
>
> Is there any reason why you did not add it
> to __rustc-option?
>
>
> __rustc-option = $(call try-run,\
>        RUSTC_BOOTSTRAP=1 $(1) $(2) $(3) --crate-type=rlib
> $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))

I had an explanation for this in the commit message, but it looks like
it got lost when I rewrote it for v2. Anyway, the reason is that I'd
have to modify both __rustc-option and rustc-option-yn to do that, and
putting it here seemed more future-proof against making the same
mistake in any rustc-* commands added in the future.

But I realize that it's not clear-cut. I'm happy to move it if you prefer,
or perhaps add a try-run-rust. Let me know what you think.

> I guess it is still suspicious because the top-level Makefile
> exports RUCTC_BOOTSTRAP.

Moving the declaration of RUSTC_BOOTSTRAP to the top of the Makefile
seems to fix it. I guess moving it is probably a better solution than
adding it in scripts/Makefile.compiler.

Not that I really understand why that is. The existing invocations are
in scripts/Makefile.kasan which is invoked after RUSTC_BOOTSTRAP is
declared.


Alice

Miguel Ojeda Oct. 8, 2024, 8:21 p.m. UTC | #6

On Tue, Oct 8, 2024 at 7:32 PM Alice Ryhl <[e-mail]> wrote:
>
> diff --git a/rust/probe.rs b/rust/probe.rs
> new file mode 100644
> index 000000000000..bf024e394408
> --- /dev/null
> +++ b/rust/probe.rs
> @@ -0,0 +1,7 @@
> +//! Nearly empty file passed to rustc-option by Make.
> +//!
> +//! The no_core attribute is needed because rustc-option otherwise fails due to
> +//! not being able to find the core part of the standard library.
> +
> +#![feature(no_core)]
> +#![no_core]

What I did in the GitHub issue was:

    echo '#![feature(no_core)]#![no_core]' | rustc ... -

to avoid a file just for this and so that things were closer (i.e. the
comment would be then in the `Makefile`).

Not sure what Masahiro prefers.

Cheers,
Miguel

Miguel Ojeda Oct. 8, 2024, 8:21 p.m. UTC | #7

On Tue, Oct 8, 2024 at 8:25 PM Matthew Maurer <[e-mail]> wrote:
>
> Err, slight amendment - I think you want `-o $$TMP`, and not
> `--out-dir $$TMPOUT`

We need the `--out-dir` to avoid temporary files being created in the
current working directory.

Cheers,
Miguel

Matthew Maurer Oct. 8, 2024, 8:22 p.m. UTC | #8

On Tue, Oct 8, 2024 at 1:22 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Tue, Oct 8, 2024 at 8:25 PM Matthew Maurer <[e-mail]> wrote:
> >
> > Err, slight amendment - I think you want `-o $$TMP`, and not
> > `--out-dir $$TMPOUT`
>
> We need the `--out-dir` to avoid temporary files being created in the
> current working directory.
In that case we should omit `-o $$TMP`, because `rustc` is emitting a
warning that it's ignoring `--out-dir` because `-o` is set.
>
> Cheers,
> Miguel

Miguel Ojeda Oct. 8, 2024, 8:52 p.m. UTC | #9

On Tue, Oct 8, 2024 at 10:23 PM Matthew Maurer <[e-mail]> wrote:
>
> In that case we should omit `-o $$TMP`, because `rustc` is emitting a
> warning that it's ignoring `--out-dir` because `-o` is set.

Yes, we need to omit `-o` or we could use `--out-dir` together with
one of the `--emit=X=...` types to be more explicit and avoid building
the `.rlib`, which is what I did in the GitHub issue.

IIRC I used `obj` to run most of the compiler stages just in case that
mattered, but perhaps an even simpler one is good enough, e.g.
`metadata`.

I think just `--out-dir` should be fine and is simpler for this use
case. However, apparently outputting to stdout works too, so we could
do:

    --emit=obj=- - >/dev/null

and avoid the output file altogether. We still most likely need the
`--out-dir` in case temporaries are created.

Cheers,
Miguel

Alice Ryhl Oct. 9, 2024, 9:23 a.m. UTC | #10

On Tue, Oct 8, 2024 at 10:53 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Tue, Oct 8, 2024 at 10:23 PM Matthew Maurer <[e-mail]> wrote:
> >
> > In that case we should omit `-o $$TMP`, because `rustc` is emitting a
> > warning that it's ignoring `--out-dir` because `-o` is set.
>
> Yes, we need to omit `-o` or we could use `--out-dir` together with
> one of the `--emit=X=...` types to be more explicit and avoid building
> the `.rlib`, which is what I did in the GitHub issue.

Miguel, can you link this issue? I don't think I saw it.

> IIRC I used `obj` to run most of the compiler stages just in case that
> mattered, but perhaps an even simpler one is good enough, e.g.
> `metadata`.
>
> I think just `--out-dir` should be fine and is simpler for this use
> case. However, apparently outputting to stdout works too, so we could
> do:
>
>     --emit=obj=- - >/dev/null
>
> and avoid the output file altogether. We still most likely need the
> `--out-dir` in case temporaries are created.

Masahiro, are you able to clarify how to pass TMPOUT to rustc?

__rustc-option = $(call try-run2,\
       $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs
--out-dir=$(TMPOUT),$(3),$(4))

Should I use $(TMPOUT) or $$TMPOUT for this case? Right now, only TMP
is defined inside try-run. I am assuming that there is a reason for
having TMP be defined in try-run, rather than just using $(TMP)
everywhere. Does the same reason apply to TMPOUT? Should I add a
TMPOUT=$(TMPOUT) inside try-run?

Alice

Miguel Ojeda Oct. 9, 2024, 10:01 a.m. UTC | #11

On Wed, Oct 9, 2024 at 11:23 AM Alice Ryhl <[e-mail]> wrote:
>
> Miguel, can you link this issue? I don't think I saw it.

https://github.com/Rust-for-Linux/linux/pull/1087#issuecomment-2218445303

(It was in the Lore message I linked yesterday, sorry, I should have
been more explicit)

> Masahiro, are you able to clarify how to pass TMPOUT to rustc?
>
> __rustc-option = $(call try-run2,\
>        $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs
> --out-dir=$(TMPOUT),$(3),$(4))
>
> Should I use $(TMPOUT) or $$TMPOUT for this case? Right now, only TMP
> is defined inside try-run. I am assuming that there is a reason for
> having TMP be defined in try-run, rather than just using $(TMP)
> everywhere. Does the same reason apply to TMPOUT? Should I add a
> TMPOUT=$(TMPOUT) inside try-run?

`TMPOUT` is defined already in that `Makefile`, thus you can directly
expand it. However, `TMP` is defined inside the `shell` function, and
thus `$$TMP` is used so that that script (inside the `shell`) expands
it instead.

This is why Masahiro was saying that the `TMPOUT=$(TMPOUT)` was
unnecessary, i.e. it would work, but we can just expand it directly.

Something like this, combining everything [1] seems to work for me.

i.e. passing the file inline, `RUSTC_BOOTSTRAP=1`, avoiding an output
file, keeping `--out-dir` for intermediates files. I added using a
null sysroot too (and skipping if already given, since that is an
error).

I will test it a bit more with KASAN etc.

Cheers,
Miguel

[1]

diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
index 057305eae85c..3ce6a808764a 100644
--- a/scripts/Makefile.compiler
+++ b/scripts/Makefile.compiler
@@ -76,7 +76,9 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS)
$(1) -v,$(1),$(2),$(3))
 # __rustc-option
 # Usage: MY_RUSTFLAGS += $(call
__rustc-option,$(RUSTC),$(MY_RUSTFLAGS),-Cinstrument-coverage,-Zinstrument-coverage)
 __rustc-option = $(call try-run,\
-       $(1) $(2) $(3) --crate-type=rlib /dev/null --out-dir=$$TMPOUT
-o "$$TMP",$(3),$(4))
+       echo '//!\n#![feature(no_core)]#![no_core]' | RUSTC_BOOTSTRAP=1\
+               $(1) --sysroot=/dev/null $(filter-out
--sysroot=/dev/null,$(2)) $(3)\
+               --crate-type=rlib --out-dir=$(TMPOUT) --emit=obj=- -
>/dev/null,$(3),$(4))

 # rustc-option
 # Usage: rustflags-y += $(call
rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)

Masahiro Yamada Oct. 9, 2024, 10:06 a.m. UTC | #12

On Wed, Oct 9, 2024 at 7:01 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Wed, Oct 9, 2024 at 11:23 AM Alice Ryhl <[e-mail]> wrote:
> >
> > Miguel, can you link this issue? I don't think I saw it.
>
> https://github.com/Rust-for-Linux/linux/pull/1087#issuecomment-2218445303
>
> (It was in the Lore message I linked yesterday, sorry, I should have
> been more explicit)
>
> > Masahiro, are you able to clarify how to pass TMPOUT to rustc?
> >
> > __rustc-option = $(call try-run2,\
> >        $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs
> > --out-dir=$(TMPOUT),$(3),$(4))
> >
> > Should I use $(TMPOUT) or $$TMPOUT for this case? Right now, only TMP
> > is defined inside try-run. I am assuming that there is a reason for
> > having TMP be defined in try-run, rather than just using $(TMP)
> > everywhere. Does the same reason apply to TMPOUT? Should I add a
> > TMPOUT=$(TMPOUT) inside try-run?
>
> `TMPOUT` is defined already in that `Makefile`, thus you can directly
> expand it. However, `TMP` is defined inside the `shell` function, and
> thus `$$TMP` is used so that that script (inside the `shell`) expands
> it instead.
>
> This is why Masahiro was saying that the `TMPOUT=$(TMPOUT)` was
> unnecessary, i.e. it would work, but we can just expand it directly.


Yes. I like --out-dir=$(TMPOUT)

Masahiro Yamada Oct. 9, 2024, 10:32 a.m. UTC | #13

On Wed, Oct 9, 2024 at 4:42 AM Alice Ryhl <[e-mail]> wrote:
>
> On Tue, Oct 8, 2024 at 9:00 PM Masahiro Yamada <[e-mail]> wrote:
> >
> > On Wed, Oct 9, 2024 at 2:32 AM Alice Ryhl <[e-mail]> wrote:
> > > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> > > index 057305eae85c..08d5b7177ea8 100644
> > > --- a/scripts/Makefile.compiler
> > > +++ b/scripts/Makefile.compiler
> > > @@ -21,6 +21,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
> > >  # automatically cleaned up.
> > >  try-run = $(shell set -e;              \
> > >         TMP=$(TMPOUT)/tmp;              \
> > > +       export RUSTC_BOOTSTRAP=1;       \
> >
> >
> > try-run is not Rust-specific.
> >
> > Is there any reason why you did not add it
> > to __rustc-option?
> >
> >
> > __rustc-option = $(call try-run,\
> >        RUSTC_BOOTSTRAP=1 $(1) $(2) $(3) --crate-type=rlib
> > $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))
>
> I had an explanation for this in the commit message, but it looks like
> it got lost when I rewrote it for v2. Anyway, the reason is that I'd
> have to modify both __rustc-option and rustc-option-yn to do that, and
> putting it here seemed more future-proof against making the same
> mistake in any rustc-* commands added in the future.


One solution is to delete rustc-option-yn since there are no users of it.

Another solution is to refactor the code.

Either way, there is no good reason for code duplication.


If you keep rustc-option-yn, you can rebased v3 on top of this patch:
https://lore.kernel.org/lkml/[e-mail]/T/#u




>
> But I realize that it's not clear-cut. I'm happy to move it if you prefer,
> or perhaps add a try-run-rust. Let me know what you think.
>
> > I guess it is still suspicious because the top-level Makefile
> > exports RUCTC_BOOTSTRAP.
>
> Moving the declaration of RUSTC_BOOTSTRAP to the top of the Makefile
> seems to fix it. I guess moving it is probably a better solution than
> adding it in scripts/Makefile.compiler.



I prefer to keep RUSTC_BOOTSTRAP close to other compiler flags.


>
> Not that I really understand why that is. The existing invocations are
> in scripts/Makefile.kasan which is invoked after RUSTC_BOOTSTRAP is
> declared.


Miguel gave perfect explanation.
https://lore.kernel.org/linux-kbuild/[e-mail]/T/#m899bc321ae80d9c4a904680709c9a53f09e51b9e


>
>
> Alice
>
--
Best Regards
Masahiro Yamada

Masahiro Yamada Oct. 9, 2024, 10:43 a.m. UTC | #14

On Wed, Oct 9, 2024 at 7:01 PM Miguel Ojeda
<[e-mail]> wrote:
>
> On Wed, Oct 9, 2024 at 11:23 AM Alice Ryhl <[e-mail]> wrote:
> >
> > Miguel, can you link this issue? I don't think I saw it.
>
> https://github.com/Rust-for-Linux/linux/pull/1087#issuecomment-2218445303
>
> (It was in the Lore message I linked yesterday, sorry, I should have
> been more explicit)
>
> > Masahiro, are you able to clarify how to pass TMPOUT to rustc?
> >
> > __rustc-option = $(call try-run2,\
> >        $(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs
> > --out-dir=$(TMPOUT),$(3),$(4))
> >
> > Should I use $(TMPOUT) or $$TMPOUT for this case? Right now, only TMP
> > is defined inside try-run. I am assuming that there is a reason for
> > having TMP be defined in try-run, rather than just using $(TMP)
> > everywhere. Does the same reason apply to TMPOUT? Should I add a
> > TMPOUT=$(TMPOUT) inside try-run?
>
> `TMPOUT` is defined already in that `Makefile`, thus you can directly
> expand it. However, `TMP` is defined inside the `shell` function, and
> thus `$$TMP` is used so that that script (inside the `shell`) expands
> it instead.
>
> This is why Masahiro was saying that the `TMPOUT=$(TMPOUT)` was
> unnecessary, i.e. it would work, but we can just expand it directly.
>
> Something like this, combining everything [1] seems to work for me.
>
> i.e. passing the file inline, `RUSTC_BOOTSTRAP=1`, avoiding an output
> file, keeping `--out-dir` for intermediates files. I added using a
> null sysroot too (and skipping if already given, since that is an
> error).
>
> I will test it a bit more with KASAN etc.
>
> Cheers,
> Miguel
>
> [1]
>
> diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> index 057305eae85c..3ce6a808764a 100644
> --- a/scripts/Makefile.compiler
> +++ b/scripts/Makefile.compiler
> @@ -76,7 +76,9 @@ ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS)
> $(1) -v,$(1),$(2),$(3))
>  # __rustc-option
>  # Usage: MY_RUSTFLAGS += $(call
> __rustc-option,$(RUSTC),$(MY_RUSTFLAGS),-Cinstrument-coverage,-Zinstrument-coverage)
>  __rustc-option = $(call try-run,\
> -       $(1) $(2) $(3) --crate-type=rlib /dev/null --out-dir=$$TMPOUT
> -o "$$TMP",$(3),$(4))
> +       echo '//!\n#![feature(no_core)]#![no_core]' | RUSTC_BOOTSTRAP=1\
> +               $(1) --sysroot=/dev/null $(filter-out
> --sysroot=/dev/null,$(2)) $(3)\
> +               --crate-type=rlib --out-dir=$(TMPOUT) --emit=obj=- -
> >/dev/null,$(3),$(4))
>
>  # rustc-option
>  # Usage: rustflags-y += $(call
> rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)
>


Could you please add a comment line to remember the future clean-up?

e.g.

# TODO: remove RUSTC_BOOTSTRAP=1 when we raise the minimum GNU Make
version to 4.4



I also like the commit description to record that
RUSTC_BOOTSTRAP=1 is needed for GNU Make prier to commit 98da874c4303
("[SV 10593] Export variables to $(shell ...) commands"), i.e.,
GNU Make 4.3 or older.




--
Best Regards
Masahiro Yamada

Alice Ryhl Oct. 9, 2024, 11:32 a.m. UTC | #15

On Wed, Oct 9, 2024 at 12:32 PM Masahiro Yamada <[e-mail]> wrote:
>
> On Wed, Oct 9, 2024 at 4:42 AM Alice Ryhl <[e-mail]> wrote:
> >
> > On Tue, Oct 8, 2024 at 9:00 PM Masahiro Yamada <[e-mail]> wrote:
> > >
> > > On Wed, Oct 9, 2024 at 2:32 AM Alice Ryhl <[e-mail]> wrote:
> > > > diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
> > > > index 057305eae85c..08d5b7177ea8 100644
> > > > --- a/scripts/Makefile.compiler
> > > > +++ b/scripts/Makefile.compiler
> > > > @@ -21,6 +21,7 @@ TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
> > > >  # automatically cleaned up.
> > > >  try-run = $(shell set -e;              \
> > > >         TMP=$(TMPOUT)/tmp;              \
> > > > +       export RUSTC_BOOTSTRAP=1;       \
> > >
> > >
> > > try-run is not Rust-specific.
> > >
> > > Is there any reason why you did not add it
> > > to __rustc-option?
> > >
> > >
> > > __rustc-option = $(call try-run,\
> > >        RUSTC_BOOTSTRAP=1 $(1) $(2) $(3) --crate-type=rlib
> > > $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))
> >
> > I had an explanation for this in the commit message, but it looks like
> > it got lost when I rewrote it for v2. Anyway, the reason is that I'd
> > have to modify both __rustc-option and rustc-option-yn to do that, and
> > putting it here seemed more future-proof against making the same
> > mistake in any rustc-* commands added in the future.
>
>
> One solution is to delete rustc-option-yn since there are no users of it.
>
> Another solution is to refactor the code.
>
> Either way, there is no good reason for code duplication.
>
>
> If you keep rustc-option-yn, you can rebased v3 on top of this patch:
> https://lore.kernel.org/lkml/[e-mail]/T/#u

I'll rebase on top of that. If we choose to delete rustc-option-yn
then I think we should first merge the refactor and then delete it in
a follow-up. That way, when someone does need it, they will find the
refactored implementation in the git history.

Alice

Alice Ryhl Oct. 9, 2024, 11:43 a.m. UTC | #16

On Tue, Oct 8, 2024 at 7:32 PM Alice Ryhl <[e-mail]> wrote:
>
> Fix a few different compiler errors that cause rustc-option to give
> wrong results.
>
> If KBUILD_RUSTFLAGS or the flags being tested contain any -Z flags, then
> the error below is generated. The RUSTC_BOOTSTRAP environment variable
> is added to fix this error.
>
>         error: the option `Z` is only accepted on the nightly compiler
>         help: consider switching to a nightly toolchain: `rustup default nightly`
>         note: selecting a toolchain with `+toolchain` arguments require a rustup proxy;
>               see <https://rust-lang.github.io/rustup/concepts/index.html>
>         note: for more information about Rust's stability policy, see
>               <https://doc.rust-lang.org/book/appendix-07-nightly-rust.html#unstable-features>
>         error: 1 nightly option were parsed
>
> The probe may also fail incorrectly with the below error message. To fix
> it, the /dev/null argument is replaced with a new rust/probe.rs file
> that doesn't need even the core part of the standard library.
>
> error[E0463]: can't find crate for `std`
>   |
>   = note: the `aarch64-unknown-none` target may not be installed
>   = help: consider downloading the target with `rustup target add aarch64-unknown-none`
>   = help: consider building the standard library from source with `cargo build -Zbuild-std`
>
> The -o and --out-dir parameters are altered to fix this warning:
>
>         warning: ignoring --out-dir flag due to -o flag
>
> I verified that the Kconfig version of rustc-option doesn't have the
> same issues.
>
> Fixes: c42297438aee ("kbuild: rust: Define probing macros for rustc")
> Signed-off-by: Alice Ryhl <[e-mail]>
> ---
> Changes in v2:
> - Add `export` to RUSTC_BOOTSTRAP.
> - Fix error about core being missing.
> - Fix warning about -o flag.
> - Link to v1: https://lore.kernel.org/r/[e-mail]

v3 sent here:
https://lore.kernel.org/all/[e-mail]/

Alice

Patch

diff --git a/rust/probe.rs b/rust/probe.rs
new file mode 100644
index 000000000000..bf024e394408
--- /dev/null
+++ b/rust/probe.rs
@@ -0,0 +1,7 @@ 
+//! Nearly empty file passed to rustc-option by Make.
+//!
+//! The no_core attribute is needed because rustc-option otherwise fails due to
+//! not being able to find the core part of the standard library.
+
+#![feature(no_core)]
+#![no_core]
diff --git a/scripts/Makefile.compiler b/scripts/Makefile.compiler
index 057305eae85c..08d5b7177ea8 100644
--- a/scripts/Makefile.compiler
+++ b/scripts/Makefile.compiler
@@ -21,6 +21,7 @@  TMPOUT = $(if $(KBUILD_EXTMOD),$(firstword $(KBUILD_EXTMOD))/).tmp_$$$$
 # automatically cleaned up.
 try-run = $(shell set -e;		\
 	TMP=$(TMPOUT)/tmp;		\
+	export RUSTC_BOOTSTRAP=1;	\
 	trap "rm -rf $(TMPOUT)" EXIT;	\
 	mkdir -p $(TMPOUT);		\
 	if ($(1)) >/dev/null 2>&1;	\
@@ -76,7 +77,7 @@  ld-option = $(call try-run, $(LD) $(KBUILD_LDFLAGS) $(1) -v,$(1),$(2),$(3))
 # __rustc-option
 # Usage: MY_RUSTFLAGS += $(call __rustc-option,$(RUSTC),$(MY_RUSTFLAGS),-Cinstrument-coverage,-Zinstrument-coverage)
 __rustc-option = $(call try-run,\
-	$(1) $(2) $(3) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",$(3),$(4))
+	$(1) $(2) $(3) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,$(3),$(4))
 
 # rustc-option
 # Usage: rustflags-y += $(call rustc-option,-Cinstrument-coverage,-Zinstrument-coverage)
@@ -86,4 +87,4 @@  rustc-option = $(call __rustc-option, $(RUSTC),\
 # rustc-option-yn
 # Usage: flag := $(call rustc-option-yn,-Cinstrument-coverage)
 rustc-option-yn = $(call try-run,\
-	$(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib /dev/null --out-dir=$$TMPOUT -o "$$TMP",y,n)
+	$(RUSTC) $(KBUILD_RUSTFLAGS) $(1) --crate-type=rlib $(srctree)/rust/probe.rs --out-dir=$$TMP,y,n)