| Message ID | 20230108021450.120791-1-yakoyoku@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [v2] scripts: Exclude Rust CUs with pahole | expand |
On Sun, Jan 8, 2023 at 3:14 AM Martin Rodriguez Reboredo <yakoyoku@gmail.com> 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 <ecurtin@redhat.com> > Tested-by: Eric Curtin <ecurtin@redhat.com> > Reviewed-by: Neal Gompa <neal@gompa.dev> > Tested-by: Neal Gompa <neal@gompa.dev> 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 <ojeda@kernel.org> Cheers, Miguel
On Sun, 8 Jan 2023 at 02:15, Martin Rodriguez Reboredo <yakoyoku@gmail.com> 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 <ecurtin@redhat.com> > Tested-by: Eric Curtin <ecurtin@redhat.com> > Reviewed-by: Neal Gompa <neal@gompa.dev> > Tested-by: Neal Gompa <neal@gompa.dev> > Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > --- > 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 <ecurtin@redhat.com> 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 <ecurtin@redhat.com> wrote: > > On Sun, 8 Jan 2023 at 02:15, Martin Rodriguez Reboredo > <yakoyoku@gmail.com> 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 <ecurtin@redhat.com> > > Tested-by: Eric Curtin <ecurtin@redhat.com> > > Reviewed-by: Neal Gompa <neal@gompa.dev> > > Tested-by: Neal Gompa <neal@gompa.dev> > > Signed-off-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com> > > --- > > 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 <ecurtin@redhat.com> > > 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 <ecurtin@redhat.com> wrote: >> >> On Sun, 8 Jan 2023 at 02:15, Martin Rodriguez Reboredo >> <yakoyoku@gmail.com> 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 <ecurtin@redhat.com> >> >> 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 <yakoyoku@gmail.com> 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
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}