| Message ID | 20210111180609.713998-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Hoise pahole version checks into Kconfig | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > copy of pahole results in a kernel that will fully compile but fail to > link. The user then has to either install pahole or disable > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > has failed, which could have been a significant amount of time depending > on the hardware. > > Avoid a poor user experience and require pahole to be installed with an > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > standard for options that require a specific tools version. > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> I am not sure if this is the right direction. I used to believe moving any tool test to the Kconfig was the right thing to do. For example, I tried to move the libelf test to Kconfig, and make STACK_VALIDATION depend on it. https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/ It was rejected. In my understanding, it is good to test target toolchains in Kconfig (e.g. cc-option, ld-option, etc). As for host tools, in contrast, it is better to _intentionally_ break the build in order to let users know that something needed is missing. Then, they will install necessary tools or libraries. It is just a one-time setup, in most cases, just running 'apt install' or 'dnf install'. Recently, a similar thing happened to GCC_PLUGINS https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673 Following this pattern, if a new pahole is not installed, it might be better to break the build instead of hiding the CONFIG option. In my case, it is just a matter of 'apt install pahole'. On some distributions, the bundled pahole is not new enough, and people may end up with building pahole from the source code. > --- > MAINTAINERS | 1 + > init/Kconfig | 4 ++++ > lib/Kconfig.debug | 6 ++---- > scripts/link-vmlinux.sh | 13 ------------- > scripts/pahole-version.sh | 16 ++++++++++++++++ > 5 files changed, 23 insertions(+), 17 deletions(-) > create mode 100755 scripts/pahole-version.sh > > diff --git a/MAINTAINERS b/MAINTAINERS > index b8db7637263a..6f6e24285a94 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3282,6 +3282,7 @@ F: net/core/filter.c > F: net/sched/act_bpf.c > F: net/sched/cls_bpf.c > F: samples/bpf/ > +F: scripts/pahole-version.sh > F: tools/bpf/ > F: tools/lib/bpf/ > F: tools/testing/selftests/bpf/ > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..872c61b5d204 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -74,6 +74,10 @@ config TOOLS_SUPPORT_RELR > config CC_HAS_ASM_INLINE > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > +config PAHOLE_VERSION > + int > + default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) > + > config CONSTRUCTORS > bool > depends on !UML > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 7937265ef879..70c446af9664 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -267,6 +267,7 @@ config DEBUG_INFO_DWARF4 > > config DEBUG_INFO_BTF > bool "Generate BTF typeinfo" > + depends on PAHOLE_VERSION >= 116 > depends on !DEBUG_INFO_SPLIT && !DEBUG_INFO_REDUCED > depends on !GCC_PLUGIN_RANDSTRUCT || COMPILE_TEST > help > @@ -274,12 +275,9 @@ config DEBUG_INFO_BTF > Turning this on expects presence of pahole tool, which will convert > DWARF type info into equivalent deduplicated BTF type info. > > -config PAHOLE_HAS_SPLIT_BTF > - def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119") > - > config DEBUG_INFO_BTF_MODULES > def_bool y > - depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF > + depends on DEBUG_INFO_BTF && MODULES && PAHOLE_VERSION >= 119 > help > Generate compact split BTF type information for kernel modules. > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index 6eded325c837..eef40fa9485d 100755 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -139,19 +139,6 @@ vmlinux_link() > # ${2} - file to dump raw BTF data into > gen_btf() > { > - local pahole_ver > - > - if ! [ -x "$(command -v ${PAHOLE})" ]; then > - echo >&2 "BTF: ${1}: pahole (${PAHOLE}) is not available" > - return 1 > - fi > - > - pahole_ver=$(${PAHOLE} --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/') > - if [ "${pahole_ver}" -lt "116" ]; then > - echo >&2 "BTF: ${1}: pahole version $(${PAHOLE} --version) is too old, need at least v1.16" > - return 1 > - fi > - > vmlinux_link ${1} > > info "BTF" ${2} > diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh > new file mode 100755 > index 000000000000..6de6f734a345 > --- /dev/null > +++ b/scripts/pahole-version.sh > @@ -0,0 +1,16 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Usage: $ ./scripts/pahole-version.sh pahole > +# > +# Print the pahole version as a three digit string > +# such as `119' for pahole v1.19 etc. > + > +pahole="$*" > + > +if ! [ -x "$(command -v $pahole)" ]; then > + echo 0 > + exit 1 > +fi > + > +$pahole --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > > base-commit: e22d7f05e445165e58feddb4e40cc9c0f94453bc > -- > 2.30.0 >
On Tue, Jan 12, 2021 at 04:19:01AM +0900, Masahiro Yamada wrote: > On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > > copy of pahole results in a kernel that will fully compile but fail to > > link. The user then has to either install pahole or disable > > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > > has failed, which could have been a significant amount of time depending > > on the hardware. > > > > Avoid a poor user experience and require pahole to be installed with an > > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > > standard for options that require a specific tools version. > > > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > I am not sure if this is the right direction. > > > I used to believe moving any tool test to the Kconfig > was the right thing to do. > > For example, I tried to move the libelf test to Kconfig, > and make STACK_VALIDATION depend on it. > > https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/ > > It was rejected. > > > In my understanding, it is good to test target toolchains > in Kconfig (e.g. cc-option, ld-option, etc). > > As for host tools, in contrast, it is better to _intentionally_ > break the build in order to let users know that something needed is missing. > Then, they will install necessary tools or libraries. > It is just a one-time setup, in most cases, > just running 'apt install' or 'dnf install'. > > > > Recently, a similar thing happened to GCC_PLUGINS > https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673 > > > > > Following this pattern, if a new pahole is not installed, > it might be better to break the build instead of hiding > the CONFIG option. > > In my case, it is just a matter of 'apt install pahole'. > On some distributions, the bundled pahole is not new enough, > and people may end up with building pahole from the source code. This is fair enough. However, I think that parts of this patch could still be salvaged into something that fits this by making it so that if pahole is not installed (CONFIG_PAHOLE_VERSION=0) or too old, the build errors at the beginning, rather at the end. I am not sure where the best place to put that check would be though. Cheers, Nathan
On Tue, Jan 12, 2021 at 4:34 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 04:19:01AM +0900, Masahiro Yamada wrote: > > On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > > > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > > > copy of pahole results in a kernel that will fully compile but fail to > > > link. The user then has to either install pahole or disable > > > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > > > has failed, which could have been a significant amount of time depending > > > on the hardware. > > > > > > Avoid a poor user experience and require pahole to be installed with an > > > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > > > standard for options that require a specific tools version. > > > > > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > I am not sure if this is the right direction. > > > > > > I used to believe moving any tool test to the Kconfig > > was the right thing to do. > > > > For example, I tried to move the libelf test to Kconfig, > > and make STACK_VALIDATION depend on it. > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/ > > > > It was rejected. > > > > > > In my understanding, it is good to test target toolchains > > in Kconfig (e.g. cc-option, ld-option, etc). > > > > As for host tools, in contrast, it is better to _intentionally_ > > break the build in order to let users know that something needed is missing. > > Then, they will install necessary tools or libraries. > > It is just a one-time setup, in most cases, > > just running 'apt install' or 'dnf install'. > > > > > > > > Recently, a similar thing happened to GCC_PLUGINS > > https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673 > > > > > > > > > > Following this pattern, if a new pahole is not installed, > > it might be better to break the build instead of hiding > > the CONFIG option. > > > > In my case, it is just a matter of 'apt install pahole'. > > On some distributions, the bundled pahole is not new enough, > > and people may end up with building pahole from the source code. > > This is fair enough. However, I think that parts of this patch could > still be salvaged into something that fits this by making it so that if > pahole is not installed (CONFIG_PAHOLE_VERSION=0) or too old, the build > errors at the beginning, rather at the end. I am not sure where the best > place to put that check would be though. Me neither. Collecting tool checks to the beginning would be user-friendly. However, scattering the related code to multiple places is not nice from the developer point of view. How big is it a problem if the build fails at the very last stage? You can install pahole, then resume "make". Kbuild skips unneeded building, then you will be able to come back to the last build stage shortly.
On Tue, Jan 12, 2021 at 04:50:50AM +0900, Masahiro Yamada wrote: > On Tue, Jan 12, 2021 at 4:34 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Tue, Jan 12, 2021 at 04:19:01AM +0900, Masahiro Yamada wrote: > > > On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > > > > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > > > > copy of pahole results in a kernel that will fully compile but fail to > > > > link. The user then has to either install pahole or disable > > > > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > > > > has failed, which could have been a significant amount of time depending > > > > on the hardware. > > > > > > > > Avoid a poor user experience and require pahole to be installed with an > > > > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > > > > standard for options that require a specific tools version. > > > > > > > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > > > > > I am not sure if this is the right direction. > > > > > > > > > I used to believe moving any tool test to the Kconfig > > > was the right thing to do. > > > > > > For example, I tried to move the libelf test to Kconfig, > > > and make STACK_VALIDATION depend on it. > > > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/ > > > > > > It was rejected. > > > > > > > > > In my understanding, it is good to test target toolchains > > > in Kconfig (e.g. cc-option, ld-option, etc). > > > > > > As for host tools, in contrast, it is better to _intentionally_ > > > break the build in order to let users know that something needed is missing. > > > Then, they will install necessary tools or libraries. > > > It is just a one-time setup, in most cases, > > > just running 'apt install' or 'dnf install'. > > > > > > > > > > > > Recently, a similar thing happened to GCC_PLUGINS > > > https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673 > > > > > > > > > > > > > > > Following this pattern, if a new pahole is not installed, > > > it might be better to break the build instead of hiding > > > the CONFIG option. > > > > > > In my case, it is just a matter of 'apt install pahole'. > > > On some distributions, the bundled pahole is not new enough, > > > and people may end up with building pahole from the source code. > > > > This is fair enough. However, I think that parts of this patch could > > still be salvaged into something that fits this by making it so that if > > pahole is not installed (CONFIG_PAHOLE_VERSION=0) or too old, the build > > errors at the beginning, rather at the end. I am not sure where the best > > place to put that check would be though. > > Me neither. > > > Collecting tool checks to the beginning would be user-friendly. > However, scattering the related code to multiple places is not > nice from the developer point of view. > > How big is it a problem if the build fails > at the very last stage? > > You can install pahole, then resume "make". > > Kbuild skips unneeded building, then you will > be able to come back to the last build stage shortly. There will often be times where I am testing multiple configurations in a row serially and the longer that a build takes to fail, the longer it takes for me to get a "real" result. That is my motivation behind this change. If people are happy with the current state of things, I will just stick with universally disabling CONFIG_DEBUG_INFO_BTF in my test framework. Cheers, Nathan
On Mon, Jan 11, 2021 at 12:00 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 04:50:50AM +0900, Masahiro Yamada wrote: > > On Tue, Jan 12, 2021 at 4:34 AM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > On Tue, Jan 12, 2021 at 04:19:01AM +0900, Masahiro Yamada wrote: > > > > On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > > > > > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > > > > > copy of pahole results in a kernel that will fully compile but fail to > > > > > link. The user then has to either install pahole or disable > > > > > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > > > > > has failed, which could have been a significant amount of time depending > > > > > on the hardware. > > > > > > > > > > Avoid a poor user experience and require pahole to be installed with an > > > > > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > > > > > standard for options that require a specific tools version. > > > > > > > > > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > > > > > > > > > I am not sure if this is the right direction. > > > > > > > > > > > > I used to believe moving any tool test to the Kconfig > > > > was the right thing to do. > > > > > > > > For example, I tried to move the libelf test to Kconfig, > > > > and make STACK_VALIDATION depend on it. > > > > > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/ > > > > > > > > It was rejected. > > > > > > > > > > > > In my understanding, it is good to test target toolchains > > > > in Kconfig (e.g. cc-option, ld-option, etc). > > > > > > > > As for host tools, in contrast, it is better to _intentionally_ > > > > break the build in order to let users know that something needed is missing. > > > > Then, they will install necessary tools or libraries. > > > > It is just a one-time setup, in most cases, > > > > just running 'apt install' or 'dnf install'. > > > > > > > > > > > > > > > > Recently, a similar thing happened to GCC_PLUGINS > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673 > > > > > > > > > > > > > > > > > > > > Following this pattern, if a new pahole is not installed, > > > > it might be better to break the build instead of hiding > > > > the CONFIG option. > > > > > > > > In my case, it is just a matter of 'apt install pahole'. > > > > On some distributions, the bundled pahole is not new enough, > > > > and people may end up with building pahole from the source code. > > > > > > This is fair enough. However, I think that parts of this patch could > > > still be salvaged into something that fits this by making it so that if > > > pahole is not installed (CONFIG_PAHOLE_VERSION=0) or too old, the build > > > errors at the beginning, rather at the end. I am not sure where the best > > > place to put that check would be though. > > > > Me neither. > > > > > > Collecting tool checks to the beginning would be user-friendly. > > However, scattering the related code to multiple places is not > > nice from the developer point of view. > > > > How big is it a problem if the build fails > > at the very last stage? > > > > You can install pahole, then resume "make". > > > > Kbuild skips unneeded building, then you will > > be able to come back to the last build stage shortly. > > There will often be times where I am testing multiple configurations in > a row serially and the longer that a build takes to fail, the longer it > takes for me to get a "real" result. That is my motivation behind this > change. If people are happy with the current state of things, I will > just stick with universally disabling CONFIG_DEBUG_INFO_BTF in my test > framework. > I see where Masahiro is coming from. Not seeing CONFIG_DEBUG_INFO_BTF option because pahole is not installed (or is not new enough) is, I believe, for the majority of users, a much bigger confusion. Currently they will get a specific and helpful message at the link time, which is much more actionable, IMO. Once you fix pahole dependency, running make again would skip all the already compiled code and would start linking almost immediately, so if you are doing build locally there is a very little downside. I understand your situation is a bit different in that you are building from scratch every single time (probably some sort of CI setup, right?). But it's a rarer and more power-user use case. And fixing pahole dependency is a one-time fix, so it's frustrating, but fixable on your side. As for disabling CONFIG_DEBUG_INFO_BTF. It's up to you and depends on what you are after, but major distros now enable it by default, so if you want to resemble common kernel configs, it's probably better to stick with it. Ideally, I'd love for Kconfig to have a way to express tool dependencies in such a way that it's still possible to choose desired options and if the build environment is lacking dependencies then it would be communicated early on. I have no idea if that's doable and how much effort it'd take, though. > Cheers, > Nathan
On Mon, Jan 11, 2021 at 7:06 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > copy of pahole results in a kernel that will fully compile but fail to > link. The user then has to either install pahole or disable > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > has failed, which could have been a significant amount of time depending > on the hardware. > > Avoid a poor user experience and require pahole to be installed with an > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > standard for options that require a specific tools version. > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Thanks for the patch, Nathan, Might be good to gave a hint to the user if pahole version does not match requirements? Feel free to add my: Tested-by: Sedat Dilek <sedat.dilek@gmail.com> - Sedat - > --- > MAINTAINERS | 1 + > init/Kconfig | 4 ++++ > lib/Kconfig.debug | 6 ++---- > scripts/link-vmlinux.sh | 13 ------------- > scripts/pahole-version.sh | 16 ++++++++++++++++ > 5 files changed, 23 insertions(+), 17 deletions(-) > create mode 100755 scripts/pahole-version.sh > > diff --git a/MAINTAINERS b/MAINTAINERS > index b8db7637263a..6f6e24285a94 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3282,6 +3282,7 @@ F: net/core/filter.c > F: net/sched/act_bpf.c > F: net/sched/cls_bpf.c > F: samples/bpf/ > +F: scripts/pahole-version.sh > F: tools/bpf/ > F: tools/lib/bpf/ > F: tools/testing/selftests/bpf/ > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..872c61b5d204 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -74,6 +74,10 @@ config TOOLS_SUPPORT_RELR > config CC_HAS_ASM_INLINE > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > +config PAHOLE_VERSION > + int > + default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) > + > config CONSTRUCTORS > bool > depends on !UML > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 7937265ef879..70c446af9664 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -267,6 +267,7 @@ config DEBUG_INFO_DWARF4 > > config DEBUG_INFO_BTF > bool "Generate BTF typeinfo" > + depends on PAHOLE_VERSION >= 116 > depends on !DEBUG_INFO_SPLIT && !DEBUG_INFO_REDUCED > depends on !GCC_PLUGIN_RANDSTRUCT || COMPILE_TEST > help > @@ -274,12 +275,9 @@ config DEBUG_INFO_BTF > Turning this on expects presence of pahole tool, which will convert > DWARF type info into equivalent deduplicated BTF type info. > > -config PAHOLE_HAS_SPLIT_BTF > - def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119") > - > config DEBUG_INFO_BTF_MODULES > def_bool y > - depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF > + depends on DEBUG_INFO_BTF && MODULES && PAHOLE_VERSION >= 119 > help > Generate compact split BTF type information for kernel modules. > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index 6eded325c837..eef40fa9485d 100755 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -139,19 +139,6 @@ vmlinux_link() > # ${2} - file to dump raw BTF data into > gen_btf() > { > - local pahole_ver > - > - if ! [ -x "$(command -v ${PAHOLE})" ]; then > - echo >&2 "BTF: ${1}: pahole (${PAHOLE}) is not available" > - return 1 > - fi > - > - pahole_ver=$(${PAHOLE} --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/') > - if [ "${pahole_ver}" -lt "116" ]; then > - echo >&2 "BTF: ${1}: pahole version $(${PAHOLE} --version) is too old, need at least v1.16" > - return 1 > - fi > - > vmlinux_link ${1} > > info "BTF" ${2} > diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh > new file mode 100755 > index 000000000000..6de6f734a345 > --- /dev/null > +++ b/scripts/pahole-version.sh > @@ -0,0 +1,16 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Usage: $ ./scripts/pahole-version.sh pahole > +# > +# Print the pahole version as a three digit string > +# such as `119' for pahole v1.19 etc. > + > +pahole="$*" > + > +if ! [ -x "$(command -v $pahole)" ]; then > + echo 0 > + exit 1 > +fi > + > +$pahole --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > > base-commit: e22d7f05e445165e58feddb4e40cc9c0f94453bc > -- > 2.30.0 >
On Mon, Jan 11, 2021 at 1:24 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Mon, Jan 11, 2021 at 12:00 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Tue, Jan 12, 2021 at 04:50:50AM +0900, Masahiro Yamada wrote: > > > On Tue, Jan 12, 2021 at 4:34 AM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > On Tue, Jan 12, 2021 at 04:19:01AM +0900, Masahiro Yamada wrote: > > > > > On Tue, Jan 12, 2021 at 3:06 AM Nathan Chancellor > > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > > > > > > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > > > > > > copy of pahole results in a kernel that will fully compile but fail to > > > > > > link. The user then has to either install pahole or disable > > > > > > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > > > > > > has failed, which could have been a significant amount of time depending > > > > > > on the hardware. > > > > > > > > > > > > Avoid a poor user experience and require pahole to be installed with an > > > > > > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > > > > > > standard for options that require a specific tools version. > > > > > > > > > > > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > > > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > > > > > > > > > > > > > > > > > I am not sure if this is the right direction. > > > > > > > > > > > > > > > I used to believe moving any tool test to the Kconfig > > > > > was the right thing to do. > > > > > > > > > > For example, I tried to move the libelf test to Kconfig, > > > > > and make STACK_VALIDATION depend on it. > > > > > > > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/1531186516-15764-1-git-send-email-yamada.masahiro@socionext.com/ > > > > > > > > > > It was rejected. > > > > > > > > > > > > > > > In my understanding, it is good to test target toolchains > > > > > in Kconfig (e.g. cc-option, ld-option, etc). > > > > > > > > > > As for host tools, in contrast, it is better to _intentionally_ > > > > > break the build in order to let users know that something needed is missing. > > > > > Then, they will install necessary tools or libraries. > > > > > It is just a one-time setup, in most cases, > > > > > just running 'apt install' or 'dnf install'. > > > > > > > > > > > > > > > > > > > > Recently, a similar thing happened to GCC_PLUGINS > > > > > https://patchwork.kernel.org/project/linux-kbuild/patch/20201203125700.161354-1-masahiroy@kernel.org/#23855673 > > > > > > > > > > > > > > > > > > > > > > > > > Following this pattern, if a new pahole is not installed, > > > > > it might be better to break the build instead of hiding > > > > > the CONFIG option. > > > > > > > > > > In my case, it is just a matter of 'apt install pahole'. > > > > > On some distributions, the bundled pahole is not new enough, > > > > > and people may end up with building pahole from the source code. > > > > > > > > This is fair enough. However, I think that parts of this patch could > > > > still be salvaged into something that fits this by making it so that if > > > > pahole is not installed (CONFIG_PAHOLE_VERSION=0) or too old, the build > > > > errors at the beginning, rather at the end. I am not sure where the best > > > > place to put that check would be though. > > > > > > Me neither. > > > > > > > > > Collecting tool checks to the beginning would be user-friendly. > > > However, scattering the related code to multiple places is not > > > nice from the developer point of view. > > > > > > How big is it a problem if the build fails > > > at the very last stage? > > > > > > You can install pahole, then resume "make". > > > > > > Kbuild skips unneeded building, then you will > > > be able to come back to the last build stage shortly. > > > > There will often be times where I am testing multiple configurations in > > a row serially and the longer that a build takes to fail, the longer it > > takes for me to get a "real" result. That is my motivation behind this > > change. If people are happy with the current state of things, I will > > just stick with universally disabling CONFIG_DEBUG_INFO_BTF in my test > > framework. > > > > I see where Masahiro is coming from. Not seeing CONFIG_DEBUG_INFO_BTF > option because pahole is not installed (or is not new enough) is, I > believe, for the majority of users, a much bigger confusion. Currently > they will get a specific and helpful message at the link time, which > is much more actionable, IMO. Once you fix pahole dependency, running > make again would skip all the already compiled code and would start > linking almost immediately, so if you are doing build locally there is > a very little downside. Hm.. Just saw Linus proposing using $(error-if) in Kconfig for an unrelated issue ([0]). If we can make this work, then it would catch such issue early on, yet won't have any downsides of hiding CONFIG_DEBUG_INFO_BTF if pahole is too old. WDYT? [0] https://lore.kernel.org/lkml/CAHk-=wh-+TMHPTFo1qs-MYyK7tZh-OQovA=pP3=e06aCVp6_kA@mail.gmail.com/ > > I understand your situation is a bit different in that you are > building from scratch every single time (probably some sort of CI > setup, right?). But it's a rarer and more power-user use case. And > fixing pahole dependency is a one-time fix, so it's frustrating, but > fixable on your side. > > As for disabling CONFIG_DEBUG_INFO_BTF. It's up to you and depends on > what you are after, but major distros now enable it by default, so if > you want to resemble common kernel configs, it's probably better to > stick with it. > > Ideally, I'd love for Kconfig to have a way to express tool > dependencies in such a way that it's still possible to choose desired > options and if the build environment is lacking dependencies then it > would be communicated early on. I have no idea if that's doable and > how much effort it'd take, though. > > > > Cheers, > > Nathan
On Wed, Jan 13, 2021 at 02:38:27PM -0800, Andrii Nakryiko wrote: > Hm.. Just saw Linus proposing using $(error-if) in Kconfig for an > unrelated issue ([0]). If we can make this work, then it would catch > such issue early on, yet won't have any downsides of hiding > CONFIG_DEBUG_INFO_BTF if pahole is too old. WDYT? > > [0] https://lore.kernel.org/lkml/CAHk-=wh-+TMHPTFo1qs-MYyK7tZh-OQovA=pP3=e06aCVp6_kA@mail.gmail.com/ Yes, I think that would be exactly what we want because DEBUG_INFO_BTF could cause the build to error if PAHOLE_VERSION is not >= 116. I will try to keep an eye on that thread to see how it goes then respin this based on anything that comes from it. Cheers, Nathan
On Thu, Jan 14, 2021 at 12:07 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Wed, Jan 13, 2021 at 02:38:27PM -0800, Andrii Nakryiko wrote: > > Hm.. Just saw Linus proposing using $(error-if) in Kconfig for an > > unrelated issue ([0]). If we can make this work, then it would catch > > such issue early on, yet won't have any downsides of hiding > > CONFIG_DEBUG_INFO_BTF if pahole is too old. WDYT? > > > > [0] https://lore.kernel.org/lkml/CAHk-=wh-+TMHPTFo1qs-MYyK7tZh-OQovA=pP3=e06aCVp6_kA@mail.gmail.com/ > > Yes, I think that would be exactly what we want because DEBUG_INFO_BTF > could cause the build to error if PAHOLE_VERSION is not >= 116. I will > try to keep an eye on that thread to see how it goes then respin this > based on anything that comes from it. > For BPF/pahole testing (see [1]) with CONFIG_DEBUG_INFO_DWARF5=y I did: $ git diff diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index b0840d192e95..f15b37143165 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -272,7 +272,7 @@ config DEBUG_INFO_DWARF5 bool "Generate DWARF Version 5 debuginfo" depends on GCC_VERSION >= 50000 || CC_IS_CLANG depends on CC_IS_GCC || $(success,$(srctree)/scripts/test_dwarf5_support.sh $(CC) $(CLANG_FLAGS)) - depends on !DEBUG_INFO_BTF + depends on !DEBUG_INFO_BTF || (DEBUG_INFO_BTF && PAHOLE_VERSION >= 120) help Generate DWARF v5 debug info. Requires binutils 2.35.2, gcc 5.0+ (gcc 5.0+ accepts the -gdwarf-5 flag but only had partial support for some Thanks again for that patch. - Sedat - [1] https://git.kernel.org/pub/scm/devel/pahole/pahole.git/log/?h=tmp.1.20
On Mon, Jan 11, 2021 at 7:06 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for > vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid > copy of pahole results in a kernel that will fully compile but fail to > link. The user then has to either install pahole or disable > CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build > has failed, which could have been a significant amount of time depending > on the hardware. > > Avoid a poor user experience and require pahole to be installed with an > appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is > standard for options that require a specific tools version. > > Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > MAINTAINERS | 1 + > init/Kconfig | 4 ++++ > lib/Kconfig.debug | 6 ++---- > scripts/link-vmlinux.sh | 13 ------------- > scripts/pahole-version.sh | 16 ++++++++++++++++ > 5 files changed, 23 insertions(+), 17 deletions(-) > create mode 100755 scripts/pahole-version.sh > > diff --git a/MAINTAINERS b/MAINTAINERS > index b8db7637263a..6f6e24285a94 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -3282,6 +3282,7 @@ F: net/core/filter.c > F: net/sched/act_bpf.c > F: net/sched/cls_bpf.c > F: samples/bpf/ > +F: scripts/pahole-version.sh > F: tools/bpf/ > F: tools/lib/bpf/ > F: tools/testing/selftests/bpf/ > diff --git a/init/Kconfig b/init/Kconfig > index b77c60f8b963..872c61b5d204 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -74,6 +74,10 @@ config TOOLS_SUPPORT_RELR > config CC_HAS_ASM_INLINE > def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) > > +config PAHOLE_VERSION > + int > + default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) > + > config CONSTRUCTORS > bool > depends on !UML > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index 7937265ef879..70c446af9664 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -267,6 +267,7 @@ config DEBUG_INFO_DWARF4 > > config DEBUG_INFO_BTF > bool "Generate BTF typeinfo" > + depends on PAHOLE_VERSION >= 116 > depends on !DEBUG_INFO_SPLIT && !DEBUG_INFO_REDUCED > depends on !GCC_PLUGIN_RANDSTRUCT || COMPILE_TEST > help > @@ -274,12 +275,9 @@ config DEBUG_INFO_BTF > Turning this on expects presence of pahole tool, which will convert > DWARF type info into equivalent deduplicated BTF type info. > > -config PAHOLE_HAS_SPLIT_BTF > - def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119") > - > config DEBUG_INFO_BTF_MODULES > def_bool y > - depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF > + depends on DEBUG_INFO_BTF && MODULES && PAHOLE_VERSION >= 119 > help > Generate compact split BTF type information for kernel modules. > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh > index 6eded325c837..eef40fa9485d 100755 > --- a/scripts/link-vmlinux.sh > +++ b/scripts/link-vmlinux.sh > @@ -139,19 +139,6 @@ vmlinux_link() > # ${2} - file to dump raw BTF data into > gen_btf() > { > - local pahole_ver > - > - if ! [ -x "$(command -v ${PAHOLE})" ]; then > - echo >&2 "BTF: ${1}: pahole (${PAHOLE}) is not available" > - return 1 > - fi > - > - pahole_ver=$(${PAHOLE} --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/') > - if [ "${pahole_ver}" -lt "116" ]; then > - echo >&2 "BTF: ${1}: pahole version $(${PAHOLE} --version) is too old, need at least v1.16" > - return 1 > - fi > - > vmlinux_link ${1} > > info "BTF" ${2} > diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh > new file mode 100755 > index 000000000000..6de6f734a345 > --- /dev/null > +++ b/scripts/pahole-version.sh > @@ -0,0 +1,16 @@ > +#!/bin/sh > +# SPDX-License-Identifier: GPL-2.0 > +# > +# Usage: $ ./scripts/pahole-version.sh pahole > +# > +# Print the pahole version as a three digit string > +# such as `119' for pahole v1.19 etc. > + > +pahole="$*" > + > +if ! [ -x "$(command -v $pahole)" ]; then > + echo 0 > + exit 1 > +fi > + > +$pahole --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/' > Cannot say if all supported pahole in the Linux kernel have that feature/option: $ /opt/pahole/bin/pahole --numeric_version 120 - Sedat - > base-commit: e22d7f05e445165e58feddb4e40cc9c0f94453bc > -- > 2.30.0 >
diff --git a/MAINTAINERS b/MAINTAINERS index b8db7637263a..6f6e24285a94 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3282,6 +3282,7 @@ F: net/core/filter.c F: net/sched/act_bpf.c F: net/sched/cls_bpf.c F: samples/bpf/ +F: scripts/pahole-version.sh F: tools/bpf/ F: tools/lib/bpf/ F: tools/testing/selftests/bpf/ diff --git a/init/Kconfig b/init/Kconfig index b77c60f8b963..872c61b5d204 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -74,6 +74,10 @@ config TOOLS_SUPPORT_RELR config CC_HAS_ASM_INLINE def_bool $(success,echo 'void foo(void) { asm inline (""); }' | $(CC) -x c - -c -o /dev/null) +config PAHOLE_VERSION + int + default $(shell,$(srctree)/scripts/pahole-version.sh $(PAHOLE)) + config CONSTRUCTORS bool depends on !UML diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7937265ef879..70c446af9664 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -267,6 +267,7 @@ config DEBUG_INFO_DWARF4 config DEBUG_INFO_BTF bool "Generate BTF typeinfo" + depends on PAHOLE_VERSION >= 116 depends on !DEBUG_INFO_SPLIT && !DEBUG_INFO_REDUCED depends on !GCC_PLUGIN_RANDSTRUCT || COMPILE_TEST help @@ -274,12 +275,9 @@ config DEBUG_INFO_BTF Turning this on expects presence of pahole tool, which will convert DWARF type info into equivalent deduplicated BTF type info. -config PAHOLE_HAS_SPLIT_BTF - def_bool $(success, test `$(PAHOLE) --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'` -ge "119") - config DEBUG_INFO_BTF_MODULES def_bool y - depends on DEBUG_INFO_BTF && MODULES && PAHOLE_HAS_SPLIT_BTF + depends on DEBUG_INFO_BTF && MODULES && PAHOLE_VERSION >= 119 help Generate compact split BTF type information for kernel modules. diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh index 6eded325c837..eef40fa9485d 100755 --- a/scripts/link-vmlinux.sh +++ b/scripts/link-vmlinux.sh @@ -139,19 +139,6 @@ vmlinux_link() # ${2} - file to dump raw BTF data into gen_btf() { - local pahole_ver - - if ! [ -x "$(command -v ${PAHOLE})" ]; then - echo >&2 "BTF: ${1}: pahole (${PAHOLE}) is not available" - return 1 - fi - - pahole_ver=$(${PAHOLE} --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/') - if [ "${pahole_ver}" -lt "116" ]; then - echo >&2 "BTF: ${1}: pahole version $(${PAHOLE} --version) is too old, need at least v1.16" - return 1 - fi - vmlinux_link ${1} info "BTF" ${2} diff --git a/scripts/pahole-version.sh b/scripts/pahole-version.sh new file mode 100755 index 000000000000..6de6f734a345 --- /dev/null +++ b/scripts/pahole-version.sh @@ -0,0 +1,16 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 +# +# Usage: $ ./scripts/pahole-version.sh pahole +# +# Print the pahole version as a three digit string +# such as `119' for pahole v1.19 etc. + +pahole="$*" + +if ! [ -x "$(command -v $pahole)" ]; then + echo 0 + exit 1 +fi + +$pahole --version | sed -E 's/v([0-9]+)\.([0-9]+)/\1\2/'
After commit da5fb18225b4 ("bpf: Support pre-2.25-binutils objcopy for vmlinux BTF"), having CONFIG_DEBUG_INFO_BTF enabled but lacking a valid copy of pahole results in a kernel that will fully compile but fail to link. The user then has to either install pahole or disable CONFIG_DEBUG_INFO_BTF and rebuild the kernel but only after their build has failed, which could have been a significant amount of time depending on the hardware. Avoid a poor user experience and require pahole to be installed with an appropriate version to select and use CONFIG_DEBUG_INFO_BTF, which is standard for options that require a specific tools version. Suggested-by: Sedat Dilek <sedat.dilek@gmail.com> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- MAINTAINERS | 1 + init/Kconfig | 4 ++++ lib/Kconfig.debug | 6 ++---- scripts/link-vmlinux.sh | 13 ------------- scripts/pahole-version.sh | 16 ++++++++++++++++ 5 files changed, 23 insertions(+), 17 deletions(-) create mode 100755 scripts/pahole-version.sh base-commit: e22d7f05e445165e58feddb4e40cc9c0f94453bc