Version 1.24 of pahole has the capability to exclude compilation units
(CUs) of specific languages. Rust, as of writing, is not currently
supported by pahole and if it's used with a build that has BTF debugging
enabled it results in malformed kernel and module binaries (see
Rust-for-Linux/linux#735). So it's better for pahole to exclude Rust
CUs until support for it arrives.
Reviewed-by: Eric Curtin <[email protected]>
Tested-by: Eric Curtin <[email protected]>
Reviewed-by: Neal Gompa <[email protected]>
Tested-by: Neal Gompa <[email protected]>
Signed-off-by: Martin Rodriguez Reboredo <[email protected]>
---
V1 -> V2: Removed dependency on auto.conf
init/Kconfig | 2 +-
lib/Kconfig.debug | 9 +++++++++
scripts/Makefile.modfinal | 4 ++++
scripts/link-vmlinux.sh | 4 ++++
4 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/init/Kconfig b/init/Kconfig
index 694f7c160c9c..360aef8d7292 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1913,7 +1913,7 @@ config RUST
depends on !MODVERSIONS
depends on !GCC_PLUGINS
depends on !RANDSTRUCT
- depends on !DEBUG_INFO_BTF
+ depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
select CONSTRUCTORS
help
Enables Rust support in the kernel.
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index ea4c903c9868..d473d491e709 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -364,6 +364,15 @@ config PAHOLE_HAS_BTF_TAG
btf_decl_tag) or not. Currently only clang compiler implements
these attributes, so make the config depend on CC_IS_CLANG.
+config PAHOLE_HAS_LANG_EXCLUDE
+ def_bool PAHOLE_VERSION >= 124
+ help
+ Support for the --lang_exclude flag which makes pahole exclude
+ compilation units from the supplied language. Used in Kbuild to
+ omit Rust CUs which are not supported in version 1.24 of pahole,
+ otherwise it would emit malformed kernel and module binaries when
+ using DEBUG_INFO_BTF_MODULES.
+
config DEBUG_INFO_BTF_MODULES
def_bool y
depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF
diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
index 25bedd83644b..a880f2d6918f 100644
--- a/scripts/Makefile.modfinal
+++ b/scripts/Makefile.modfinal
@@ -30,6 +30,10 @@ quiet_cmd_cc_o_c = CC [M] $@
ARCH_POSTLINK := $(wildcard $(srctree)/arch/$(SRCARCH)/Makefile.postlink)
+ifdef CONFIG_RUST
+PAHOLE_FLAGS += --lang_exclude=rust
+endif
+
quiet_cmd_ld_ko_o = LD [M] $@
cmd_ld_ko_o += \
$(LD) -r $(KBUILD_LDFLAGS) \
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index 918470d768e9..69eb0bea89bf 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -122,6 +122,10 @@ gen_btf()
return 1
fi
+ if is_enabled CONFIG_RUST; then
+ PAHOLE_FLAGS="${PAHOLE_FLAGS} --lang_exclude=rust"
+ fi
+
vmlinux_link ${1}
info "BTF" ${2}
--
2.39.0
On Sun, Jan 8, 2023 at 3:14 AM Martin Rodriguez Reboredo
<[email protected]> wrote:
>
> Version 1.24 of pahole has the capability to exclude compilation units
It would be nice to have a reference ("...of pahole [1][2]") to the
commits that introduced it in `pahole` -- especially the second one
below has a nice commit message about the use case for this patch:
Link: https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?id=49358dfe2aaae4e90b072332c3e324019826783f
[1]
Link: https://git.kernel.org/pub/scm/devel/pahole/pahole.git/commit/?id=8ee363790b7437283c53090a85a9fec2f0b0fbc4
[2]
> (CUs) of specific languages. Rust, as of writing, is not currently
> supported by pahole and if it's used with a build that has BTF debugging
> enabled it results in malformed kernel and module binaries (see
> Rust-for-Linux/linux#735). So it's better for pahole to exclude Rust
I think this came from a rendered link from GitHub, so please
transform it into a link since otherwise people may be confused where
to find it:
Link: https://github.com/Rust-for-Linux/linux/issues/735 [3]
> Reviewed-by: Eric Curtin <[email protected]>
> Tested-by: Eric Curtin <[email protected]>
> Reviewed-by: Neal Gompa <[email protected]>
> Tested-by: Neal Gompa <[email protected]>
Given the patch has had non-trivial changes (in fact, in this case, it
fundamentally changed the implementation), these tags should be
removed and requested again.
Thanks for this!
Acked-by: Miguel Ojeda <[email protected]>
Cheers,
Miguel
On Sun, 8 Jan 2023 at 02:15, Martin Rodriguez Reboredo
<[email protected]> wrote:
>
> Version 1.24 of pahole has the capability to exclude compilation units
> (CUs) of specific languages. Rust, as of writing, is not currently
> supported by pahole and if it's used with a build that has BTF debugging
> enabled it results in malformed kernel and module binaries (see
> Rust-for-Linux/linux#735). So it's better for pahole to exclude Rust
> CUs until support for it arrives.
>
> Reviewed-by: Eric Curtin <[email protected]>
> Tested-by: Eric Curtin <[email protected]>
> Reviewed-by: Neal Gompa <[email protected]>
> Tested-by: Neal Gompa <[email protected]>
> Signed-off-by: Martin Rodriguez Reboredo <[email protected]>
> ---
> V1 -> V2: Removed dependency on auto.conf
>
> init/Kconfig | 2 +-
> lib/Kconfig.debug | 9 +++++++++
> scripts/Makefile.modfinal | 4 ++++
> scripts/link-vmlinux.sh | 4 ++++
> 4 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/init/Kconfig b/init/Kconfig
> index 694f7c160c9c..360aef8d7292 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -1913,7 +1913,7 @@ config RUST
> depends on !MODVERSIONS
> depends on !GCC_PLUGINS
> depends on !RANDSTRUCT
> - depends on !DEBUG_INFO_BTF
> + depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
> select CONSTRUCTORS
> help
> Enables Rust support in the kernel.
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index ea4c903c9868..d473d491e709 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -364,6 +364,15 @@ config PAHOLE_HAS_BTF_TAG
> btf_decl_tag) or not. Currently only clang compiler implements
> these attributes, so make the config depend on CC_IS_CLANG.
>
> +config PAHOLE_HAS_LANG_EXCLUDE
> + def_bool PAHOLE_VERSION >= 124
> + help
> + Support for the --lang_exclude flag which makes pahole exclude
> + compilation units from the supplied language. Used in Kbuild to
> + omit Rust CUs which are not supported in version 1.24 of pahole,
> + otherwise it would emit malformed kernel and module binaries when
> + using DEBUG_INFO_BTF_MODULES.
> +
> config DEBUG_INFO_BTF_MODULES
> def_bool y
> depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF
> diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> index 25bedd83644b..a880f2d6918f 100644
> --- a/scripts/Makefile.modfinal
> +++ b/scripts/Makefile.modfinal
> @@ -30,6 +30,10 @@ quiet_cmd_cc_o_c = CC [M] $@
>
> ARCH_POSTLINK := $(wildcard $(srctree)/arch/$(SRCARCH)/Makefile.postlink)
>
> +ifdef CONFIG_RUST
> +PAHOLE_FLAGS += --lang_exclude=rust
> +endif
> +
> quiet_cmd_ld_ko_o = LD [M] $@
> cmd_ld_ko_o += \
> $(LD) -r $(KBUILD_LDFLAGS) \
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index 918470d768e9..69eb0bea89bf 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -122,6 +122,10 @@ gen_btf()
> return 1
> fi
>
> + if is_enabled CONFIG_RUST; then
> + PAHOLE_FLAGS="${PAHOLE_FLAGS} --lang_exclude=rust"
> + fi
If it was me, I would do things more like v1 of the patch (instead
just checking pahole version), because this is the only flag set in
scripts/Makefile.modfinal, which is a little confusing and
inconsistent. It's ok to set --lang_exclude=rust in all cases, as long
as pahole_ver is recent enough.
+if [ "${pahole_ver}" -ge "124" ]; then
+ # see PAHOLE_HAS_LANG_EXCLUDE
+ extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
+fi
But I'm not too opinionated either on this so...
Reviewed-by: Eric Curtin <[email protected]>
can be reapplied. I'm gonna test this again to see if it works in a
Fedora Asahi rpm build.
> +
> vmlinux_link ${1}
>
> info "BTF" ${2}
> --
> 2.39.0
>
On Sun, 8 Jan 2023 at 15:18, Eric Curtin <[email protected]> wrote:
>
> On Sun, 8 Jan 2023 at 02:15, Martin Rodriguez Reboredo
> <[email protected]> wrote:
> >
> > Version 1.24 of pahole has the capability to exclude compilation units
> > (CUs) of specific languages. Rust, as of writing, is not currently
> > supported by pahole and if it's used with a build that has BTF debugging
> > enabled it results in malformed kernel and module binaries (see
> > Rust-for-Linux/linux#735). So it's better for pahole to exclude Rust
> > CUs until support for it arrives.
> >
> > Reviewed-by: Eric Curtin <[email protected]>
> > Tested-by: Eric Curtin <[email protected]>
> > Reviewed-by: Neal Gompa <[email protected]>
> > Tested-by: Neal Gompa <[email protected]>
> > Signed-off-by: Martin Rodriguez Reboredo <[email protected]>
> > ---
> > V1 -> V2: Removed dependency on auto.conf
> >
> > init/Kconfig | 2 +-
> > lib/Kconfig.debug | 9 +++++++++
> > scripts/Makefile.modfinal | 4 ++++
> > scripts/link-vmlinux.sh | 4 ++++
> > 4 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/init/Kconfig b/init/Kconfig
> > index 694f7c160c9c..360aef8d7292 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -1913,7 +1913,7 @@ config RUST
> > depends on !MODVERSIONS
> > depends on !GCC_PLUGINS
> > depends on !RANDSTRUCT
> > - depends on !DEBUG_INFO_BTF
> > + depends on !DEBUG_INFO_BTF || PAHOLE_HAS_LANG_EXCLUDE
> > select CONSTRUCTORS
> > help
> > Enables Rust support in the kernel.
> > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> > index ea4c903c9868..d473d491e709 100644
> > --- a/lib/Kconfig.debug
> > +++ b/lib/Kconfig.debug
> > @@ -364,6 +364,15 @@ config PAHOLE_HAS_BTF_TAG
> > btf_decl_tag) or not. Currently only clang compiler implements
> > these attributes, so make the config depend on CC_IS_CLANG.
> >
> > +config PAHOLE_HAS_LANG_EXCLUDE
> > + def_bool PAHOLE_VERSION >= 124
> > + help
> > + Support for the --lang_exclude flag which makes pahole exclude
> > + compilation units from the supplied language. Used in Kbuild to
> > + omit Rust CUs which are not supported in version 1.24 of pahole,
> > + otherwise it would emit malformed kernel and module binaries when
> > + using DEBUG_INFO_BTF_MODULES.
> > +
> > config DEBUG_INFO_BTF_MODULES
> > def_bool y
> > depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF
> > diff --git a/scripts/Makefile.modfinal b/scripts/Makefile.modfinal
> > index 25bedd83644b..a880f2d6918f 100644
> > --- a/scripts/Makefile.modfinal
> > +++ b/scripts/Makefile.modfinal
> > @@ -30,6 +30,10 @@ quiet_cmd_cc_o_c = CC [M] $@
> >
> > ARCH_POSTLINK := $(wildcard $(srctree)/arch/$(SRCARCH)/Makefile.postlink)
> >
> > +ifdef CONFIG_RUST
> > +PAHOLE_FLAGS += --lang_exclude=rust
> > +endif
> > +
> > quiet_cmd_ld_ko_o = LD [M] $@
> > cmd_ld_ko_o += \
> > $(LD) -r $(KBUILD_LDFLAGS) \
> > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > index 918470d768e9..69eb0bea89bf 100755
> > --- a/scripts/link-vmlinux.sh
> > +++ b/scripts/link-vmlinux.sh
> > @@ -122,6 +122,10 @@ gen_btf()
> > return 1
> > fi
> >
> > + if is_enabled CONFIG_RUST; then
> > + PAHOLE_FLAGS="${PAHOLE_FLAGS} --lang_exclude=rust"
> > + fi
>
> If it was me, I would do things more like v1 of the patch (instead
> just checking pahole version), because this is the only flag set in
> scripts/Makefile.modfinal, which is a little confusing and
> inconsistent. It's ok to set --lang_exclude=rust in all cases, as long
> as pahole_ver is recent enough.
>
> +if [ "${pahole_ver}" -ge "124" ]; then
> + # see PAHOLE_HAS_LANG_EXCLUDE
> + extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> +fi
>
> But I'm not too opinionated either on this so...
>
> Reviewed-by: Eric Curtin <[email protected]>
>
> can be reapplied. I'm gonna test this again to see if it works in a
> Fedora Asahi rpm build.
After testing I probably have to retract my Reviewed-by tag,
apologies, bpf and all that did not work with this patch when I built
in the fedora way, but, the good news is when I alter v1 of the patch
to just check pahole version like so (instead of the is_enabled
check):
+if [ "${pahole_ver}" -ge "124" ]; then
+ # see PAHOLE_HAS_LANG_EXCLUDE
+ extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
+fi
it worked just fine, and that should satisfy the testbot and all the
other ways we build too. Could we change to that @Martin Rodriguez
Reboredo ?
>
>
> > +
> > vmlinux_link ${1}
> >
> > info "BTF" ${2}
> > --
> > 2.39.0
> >
On 1/8/23 16:19, Eric Curtin wrote:
> On Sun, 8 Jan 2023 at 15:18, Eric Curtin <[email protected]> wrote:
>>
>> On Sun, 8 Jan 2023 at 02:15, Martin Rodriguez Reboredo
>> <[email protected]> wrote:
>>>
>>> [ ... ]
>>
>> If it was me, I would do things more like v1 of the patch (instead
>> just checking pahole version), because this is the only flag set in
>> scripts/Makefile.modfinal, which is a little confusing and
>> inconsistent. It's ok to set --lang_exclude=rust in all cases, as long
>> as pahole_ver is recent enough.
>>
>> +if [ "${pahole_ver}" -ge "124" ]; then
>> + # see PAHOLE_HAS_LANG_EXCLUDE
>> + extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
>> +fi
>>
>> But I'm not too opinionated either on this so...
>>
>> Reviewed-by: Eric Curtin <[email protected]>
>>
>> can be reapplied. I'm gonna test this again to see if it works in a
>> Fedora Asahi rpm build.
>
> After testing I probably have to retract my Reviewed-by tag,
> apologies, bpf and all that did not work with this patch when I built
> in the fedora way, but, the good news is when I alter v1 of the patch
> to just check pahole version like so (instead of the is_enabled
> check):
>
> +if [ "${pahole_ver}" -ge "124" ]; then
> + # see PAHOLE_HAS_LANG_EXCLUDE
> + extra_paholeopt="${extra_paholeopt} --lang_exclude=rust"
> +fi
>
> it worked just fine, and that should satisfy the testbot and all the
> other ways we build too. Could we change to that @Martin Rodriguez
> Reboredo ?
>
From my POV I don't like this way due to it being set regardless whether
or not you are building the kernel with Rust. Though, because it doesn't
affect non `CONFIG_RUST` builds, I _think_ it won't hurt if we use that
way for now. Gonna send v3.
>>
>>
>>> +
>>> vmlinux_link ${1}
>>>
>>> info "BTF" ${2}
>>> --
>>> 2.39.0
>>>
>
On Wed, Jan 11, 2023 at 4:02 PM Martin Rodriguez Reboredo
<[email protected]> wrote:
>
> From my POV I don't like this way due to it being set regardless whether
> or not you are building the kernel with Rust. Though, because it doesn't
> affect non `CONFIG_RUST` builds, I _think_ it won't hurt if we use that
> way for now. Gonna send v3.
One advantage (in general, i.e. not talking about this case in
particular) of having something always done is that it is one less
moving part, so less complexity, everyone will test it all the time,
etc.
So if it is not expected to hurt, but it does for an unknown reason,
then it would be nice to know as soon as possible, regardless of
whether it is gated under `CONFIG_RUST` or not.
Of course, it can always happen that something changes over time, and
thus it may start to hurt in the future only, and therefore breaking
everybody instead of a subset of people. But then again, the sooner we
would know about that unexpected change, the better; especially since
the goal is to eventually get to a point where `CONFIG_RUST` can start
to be routinely enabled by common configurations etc.
Cheers,
Miguel