[PATCH 0/3] kbuild: propagate CONFIG_WERROR to resolve_btfids

[PATCH 0/3] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Thomas Weißschuh]

Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids.
Allow the CI bots to prevent the introduction of new warnings.

This series currently depends on
"[PATCH] bpf, lsm: Fix getlsmprop hooks BTF IDs" [0]

[0] https://lore.kernel.org/lkml/20241123-bpf_lsm_task_getsecid_obj-v1-1-0d0f94649e05@weissschuh.net/

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Thomas Weißschuh (3):
      kbuild: add dependency from vmlinux to resolve_btfids
      tools/resolve_btfids: Add --fatal-warnings option
      kbuild: propagate CONFIG_WERROR to resolve_btfids

 scripts/Makefile.vmlinux        |  3 +++
 scripts/link-vmlinux.sh         |  6 +++++-
 tools/bpf/resolve_btfids/main.c | 12 ++++++++++--
 3 files changed, 18 insertions(+), 3 deletions(-)
---
base-commit: 228a1157fb9fec47eb135b51c0202b574e079ebf
change-id: 20241123-resolve_btfids-eb95c9b42d00

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH 1/3] kbuild: add dependency from vmlinux to resolve_btfids

[Thomas Weißschuh]

resolve_btfids is used by link-vmlinux.sh.
In contrast to other configuration options and targets no transitive
dependency between resolve_btfids and vmlinux.
Add an explicit one.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 scripts/Makefile.vmlinux | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
index 1284f05555b97f726c6d167a09f6b92f20e120a2..599b486adb31cfb653e54707b7d77052d372b7c1 100644
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -32,6 +32,9 @@ cmd_link_vmlinux =							\
 targets += vmlinux
 vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
 	+$(call if_changed_dep,link_vmlinux)
+ifdef CONFIG_DEBUG_INFO_BTF
+vmlinux: $(RESOLVE_BTFIDS)
+endif
 
 # module.builtin.ranges
 # ---------------------------------------------------------------------------

-- 
2.47.0

Re: [PATCH 1/3] kbuild: add dependency from vmlinux to resolve_btfids

[Jiri Olsa]

On Sat, Nov 23, 2024 at 02:33:37PM +0100, Thomas Weißschuh wrote:
> resolve_btfids is used by link-vmlinux.sh.
> In contrast to other configuration options and targets no transitive
> dependency between resolve_btfids and vmlinux.
> Add an explicit one.

hi,
there's prepare dependency in root Makefile, isn't it enough?

ifdef CONFIG_BPF
ifdef CONFIG_DEBUG_INFO_BTF
prepare: tools/bpf/resolve_btfids
endif
endif

thanks,
jirka

> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  scripts/Makefile.vmlinux | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index 1284f05555b97f726c6d167a09f6b92f20e120a2..599b486adb31cfb653e54707b7d77052d372b7c1 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -32,6 +32,9 @@ cmd_link_vmlinux =							\
>  targets += vmlinux
>  vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
>  	+$(call if_changed_dep,link_vmlinux)
> +ifdef CONFIG_DEBUG_INFO_BTF
> +vmlinux: $(RESOLVE_BTFIDS)
> +endif
>  
>  # module.builtin.ranges
>  # ---------------------------------------------------------------------------
> 
> -- 
> 2.47.0
> 

Re: [PATCH 1/3] kbuild: add dependency from vmlinux to resolve_btfids

[Thomas Weißschuh]

On 2024-11-24 21:33:34+0100, Jiri Olsa wrote:
> On Sat, Nov 23, 2024 at 02:33:37PM +0100, Thomas Weißschuh wrote:
> > resolve_btfids is used by link-vmlinux.sh.
> > In contrast to other configuration options and targets no transitive
> > dependency between resolve_btfids and vmlinux.
> > Add an explicit one.
> 
> hi,
> there's prepare dependency in root Makefile, isn't it enough?

It doesn't seem for me.
If the source of resolve_btfids is changed, it itself is recompiled as
per the current Makefile, but vmlinux is not relinked/BTFID'd.

> ifdef CONFIG_BPF
> ifdef CONFIG_DEBUG_INFO_BTF
> prepare: tools/bpf/resolve_btfids
> endif
> endif
> 
> thanks,
> jirka
> 
> > 
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  scripts/Makefile.vmlinux | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > index 1284f05555b97f726c6d167a09f6b92f20e120a2..599b486adb31cfb653e54707b7d77052d372b7c1 100644
> > --- a/scripts/Makefile.vmlinux
> > +++ b/scripts/Makefile.vmlinux
> > @@ -32,6 +32,9 @@ cmd_link_vmlinux =							\
> >  targets += vmlinux
> >  vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
> >  	+$(call if_changed_dep,link_vmlinux)
> > +ifdef CONFIG_DEBUG_INFO_BTF
> > +vmlinux: $(RESOLVE_BTFIDS)
> > +endif
> >  
> >  # module.builtin.ranges
> >  # ---------------------------------------------------------------------------
> > 
> > -- 
> > 2.47.0
> > 

Re: [PATCH 1/3] kbuild: add dependency from vmlinux to resolve_btfids

[Masahiro Yamada]

On Mon, Nov 25, 2024 at 5:58 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2024-11-24 21:33:34+0100, Jiri Olsa wrote:
> > On Sat, Nov 23, 2024 at 02:33:37PM +0100, Thomas Weißschuh wrote:
> > > resolve_btfids is used by link-vmlinux.sh.
> > > In contrast to other configuration options and targets no transitive
> > > dependency between resolve_btfids and vmlinux.
> > > Add an explicit one.
> >
> > hi,
> > there's prepare dependency in root Makefile, isn't it enough?
>
> It doesn't seem for me.
> If the source of resolve_btfids is changed, it itself is recompiled as
> per the current Makefile, but vmlinux is not relinked/BTFID'd.


If we need rebuilding vmlinux, this seems correct

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


I can pick up this during the current MW.






> > ifdef CONFIG_BPF
> > ifdef CONFIG_DEBUG_INFO_BTF
> > prepare: tools/bpf/resolve_btfids
> > endif
> > endif
> >
> > thanks,
> > jirka
> >
> > >
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > >  scripts/Makefile.vmlinux | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > > index 1284f05555b97f726c6d167a09f6b92f20e120a2..599b486adb31cfb653e54707b7d77052d372b7c1 100644
> > > --- a/scripts/Makefile.vmlinux
> > > +++ b/scripts/Makefile.vmlinux
> > > @@ -32,6 +32,9 @@ cmd_link_vmlinux =                                                        \
> > >  targets += vmlinux
> > >  vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
> > >     +$(call if_changed_dep,link_vmlinux)
> > > +ifdef CONFIG_DEBUG_INFO_BTF
> > > +vmlinux: $(RESOLVE_BTFIDS)
> > > +endif
> > >
> > >  # module.builtin.ranges
> > >  # ---------------------------------------------------------------------------
> > >
> > > --
> > > 2.47.0
> > >



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/3] kbuild: add dependency from vmlinux to resolve_btfids

[Masahiro Yamada]

On Sat, Nov 23, 2024 at 10:33 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> resolve_btfids is used by link-vmlinux.sh.
> In contrast to other configuration options and targets no transitive
> dependency between resolve_btfids and vmlinux.
> Add an explicit one.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---

1/3, applied to linux-kbuild.
Thanks.



>  scripts/Makefile.vmlinux | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index 1284f05555b97f726c6d167a09f6b92f20e120a2..599b486adb31cfb653e54707b7d77052d372b7c1 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -32,6 +32,9 @@ cmd_link_vmlinux =                                                    \
>  targets += vmlinux
>  vmlinux: scripts/link-vmlinux.sh vmlinux.o $(KBUILD_LDS) FORCE
>         +$(call if_changed_dep,link_vmlinux)
> +ifdef CONFIG_DEBUG_INFO_BTF
> +vmlinux: $(RESOLVE_BTFIDS)
> +endif
>
>  # module.builtin.ranges
>  # ---------------------------------------------------------------------------
>
> --
> 2.47.0
>


-- 
Best Regards
Masahiro Yamada

[PATCH 2/3] tools/resolve_btfids: Add --fatal-warnings option

[Thomas Weißschuh]

Currently warnings emitted by resolve_btfids are buried in the build log
and are slipping into mainline frequently.
Add an option to elevate warnings to hard errors so the CI bots can
catch any new warnings.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 tools/bpf/resolve_btfids/main.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index bd9f960bce3d5b74dc34159b35af1e0b33524d2d..a6ffd6b5fc3ebe9e0983556bfb178642a1c6639d 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -141,6 +141,7 @@ struct object {
 };
 
 static int verbose;
+static int warnings;
 
 static int eprintf(int level, int var, const char *fmt, ...)
 {
@@ -604,6 +605,7 @@ static int symbols_resolve(struct object *obj)
 			if (id->id) {
 				pr_info("WARN: multiple IDs found for '%s': %d, %d - using %d\n",
 					str, id->id, type_id, id->id);
+				warnings++;
 			} else {
 				id->id = type_id;
 				(*nr)--;
@@ -625,8 +627,10 @@ static int id_patch(struct object *obj, struct btf_id *id)
 	int i;
 
 	/* For set, set8, id->id may be 0 */
-	if (!id->id && !id->is_set && !id->is_set8)
+	if (!id->id && !id->is_set && !id->is_set8) {
 		pr_err("WARN: resolve_btfids: unresolved symbol %s\n", id->name);
+		warnings++;
+	}
 
 	for (i = 0; i < id->addr_cnt; i++) {
 		unsigned long addr = id->addr[i];
@@ -782,9 +786,12 @@ int main(int argc, const char **argv)
 		.funcs    = RB_ROOT,
 		.sets     = RB_ROOT,
 	};
+	int fatal_warnings;
 	struct option btfid_options[] = {
 		OPT_INCR('v', "verbose", &verbose,
 			 "be more verbose (show errors, etc)"),
+		OPT_INCR(0, "fatal-warnings", &fatal_warnings,
+			 "turn warnings into errors"),
 		OPT_STRING(0, "btf", &obj.btf, "BTF data",
 			   "BTF data"),
 		OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
@@ -823,7 +830,8 @@ int main(int argc, const char **argv)
 	if (symbols_patch(&obj))
 		goto out;
 
-	err = 0;
+	if (!(fatal_warnings && warnings))
+		err = 0;
 out:
 	if (obj.efile.elf) {
 		elf_end(obj.efile.elf);

-- 
2.47.0

[PATCH 3/3] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Thomas Weißschuh]

Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids.
Allow the CI bots to prevent the introduction of new warnings.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 scripts/link-vmlinux.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -274,7 +274,11 @@ vmlinux_link vmlinux
 # fill in BTF IDs
 if is_enabled CONFIG_DEBUG_INFO_BTF; then
 	info BTFIDS vmlinux
-	${RESOLVE_BTFIDS} vmlinux
+	RESOLVE_BTFIDS_ARGS=""
+	if is_enabled CONFIG_WERROR; then
+		RESOLVE_BTFIDS_ARGS=" --fatal-warnings "
+	fi
+	${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux
 fi
 
 mksysmap vmlinux System.map

-- 
2.47.0

Re: [PATCH 3/3] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Alexei Starovoitov]

On Sat, Nov 23, 2024 at 5:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids.
> Allow the CI bots to prevent the introduction of new warnings.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  scripts/link-vmlinux.sh | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -274,7 +274,11 @@ vmlinux_link vmlinux
>  # fill in BTF IDs
>  if is_enabled CONFIG_DEBUG_INFO_BTF; then
>         info BTFIDS vmlinux
> -       ${RESOLVE_BTFIDS} vmlinux
> +       RESOLVE_BTFIDS_ARGS=""
> +       if is_enabled CONFIG_WERROR; then
> +               RESOLVE_BTFIDS_ARGS=" --fatal-warnings "
> +       fi
> +       ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux

I'm not convinced we need to fail the build when functions are renamed.
These warns are eventually found and fixed.

Re: [PATCH 3/3] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Thomas Weißschuh]

On 2024-11-24 15:38:40-0800, Alexei Starovoitov wrote:
> On Sat, Nov 23, 2024 at 5:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids.
> > Allow the CI bots to prevent the introduction of new warnings.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> >  scripts/link-vmlinux.sh | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755
> > --- a/scripts/link-vmlinux.sh
> > +++ b/scripts/link-vmlinux.sh
> > @@ -274,7 +274,11 @@ vmlinux_link vmlinux
> >  # fill in BTF IDs
> >  if is_enabled CONFIG_DEBUG_INFO_BTF; then
> >         info BTFIDS vmlinux
> > -       ${RESOLVE_BTFIDS} vmlinux
> > +       RESOLVE_BTFIDS_ARGS=""
> > +       if is_enabled CONFIG_WERROR; then
> > +               RESOLVE_BTFIDS_ARGS=" --fatal-warnings "
> > +       fi
> > +       ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux
> 
> I'm not convinced we need to fail the build when functions are renamed.
> These warns are eventually found and fixed.

The same could be said for most other build warnings.
CONFIG_WERROR is a well-known opt-in switch for exactly this behavior.

Fixing these warnings before they hit mainline has various
advantages. The author introducing the warning knows about the full
impact of their change, discussions can be had when everybody still
has the topic fresh on their mind and other unrelated people don't get
confused, like me or [0].

The "eventually fixed" part seems to have been me the last two times :-)

Given the fairly simple implementation, in my opinion this is worth doing.

Please note that I have two fairly trivial changes for a v2 and would
also like to get some feedback from Masahiro, especially for patch 1.


Thomas

[0] https://lore.kernel.org/lkml/20241113093703.9936-1-laura.nao@collabora.com/

Re: [PATCH 3/3] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Jiri Olsa]

On Mon, Nov 25, 2024 at 09:20:37AM +0100, Thomas Weißschuh wrote:
> On 2024-11-24 15:38:40-0800, Alexei Starovoitov wrote:
> > On Sat, Nov 23, 2024 at 5:33 AM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > >
> > > Use CONFIG_WERROR to also fail on warnings emitted by resolve_btfids.
> > > Allow the CI bots to prevent the introduction of new warnings.
> > >
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > >  scripts/link-vmlinux.sh | 6 +++++-
> > >  1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> > > index a9b3f34a78d2cd4514e73a728f1a784eee891768..61f1f670291351a276221153146d66001eca556c 100755
> > > --- a/scripts/link-vmlinux.sh
> > > +++ b/scripts/link-vmlinux.sh
> > > @@ -274,7 +274,11 @@ vmlinux_link vmlinux
> > >  # fill in BTF IDs
> > >  if is_enabled CONFIG_DEBUG_INFO_BTF; then
> > >         info BTFIDS vmlinux
> > > -       ${RESOLVE_BTFIDS} vmlinux
> > > +       RESOLVE_BTFIDS_ARGS=""
> > > +       if is_enabled CONFIG_WERROR; then
> > > +               RESOLVE_BTFIDS_ARGS=" --fatal-warnings "
> > > +       fi
> > > +       ${RESOLVE_BTFIDS} ${RESOLVE_BTFIDS_ARGS} vmlinux
> > 
> > I'm not convinced we need to fail the build when functions are renamed.
> > These warns are eventually found and fixed.
> 
> The same could be said for most other build warnings.
> CONFIG_WERROR is a well-known opt-in switch for exactly this behavior.
> 
> Fixing these warnings before they hit mainline has various
> advantages. The author introducing the warning knows about the full
> impact of their change, discussions can be had when everybody still
> has the topic fresh on their mind and other unrelated people don't get
> confused, like me or [0].
> 
> The "eventually fixed" part seems to have been me the last two times :-)
> 
> Given the fairly simple implementation, in my opinion this is worth doing.
> 
> Please note that I have two fairly trivial changes for a v2 and would
> also like to get some feedback from Masahiro, especially for patch 1.

ok, I think it's fine to fail for CONFIG_WERROR option, for patchset:

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> 
> 
> Thomas
> 
> [0] https://lore.kernel.org/lkml/20241113093703.9936-1-laura.nao@collabora.com/