[PATCH v2 0/2] kbuild: propagate CONFIG_WERROR to resolve_btfids

[PATCH v2 0/2] 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>
---
Changes in v2:
- Avoid uninitialized read of fatal_warnings
- Use OPT_BOOLEAN over OPT_INCR
- Drop dependency patch, which went in via the kbuild tree
- Link to v1: https://lore.kernel.org/r/20241123-resolve_btfids-v1-0-927700b641d1@weissschuh.net

---
Thomas Weißschuh (2):
      tools/resolve_btfids: Add --fatal-warnings option
      kbuild: propagate CONFIG_WERROR to resolve_btfids

 scripts/link-vmlinux.sh         |  6 +++++-
 tools/bpf/resolve_btfids/main.c | 12 ++++++++++--
 2 files changed, 15 insertions(+), 3 deletions(-)
---
base-commit: 1518b7a61299cf3737728d4fbf7e29cf2db497c7
change-id: 20241123-resolve_btfids-eb95c9b42d00

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

[PATCH v2 1/2] 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>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 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..571d29d2da97fea75e5f9c544a95b9ac65f9e579 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,6 +786,7 @@ int main(int argc, const char **argv)
 		.funcs    = RB_ROOT,
 		.sets     = RB_ROOT,
 	};
+	bool fatal_warnings = false;
 	struct option btfid_options[] = {
 		OPT_INCR('v', "verbose", &verbose,
 			 "be more verbose (show errors, etc)"),
@@ -789,6 +794,8 @@ int main(int argc, const char **argv)
 			   "BTF data"),
 		OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
 			   "path of file providing base BTF"),
+		OPT_BOOLEAN(0, "fatal-warnings", &fatal_warnings,
+			    "turn warnings into errors"),
 		OPT_END()
 	};
 	int err = -1;
@@ -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.1

[PATCH v2 2/2] 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>
Acked-by: Jiri Olsa <jolsa@kernel.org>
---
 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 a3c634b2f348f9c976cff9693caf290db7f31666..7288fe501b57e29003d922441c8d5e2f48dee6ad 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -279,7 +279,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.1

Re: [PATCH v2 0/2] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Daniel Borkmann]

Hi Thomas,

On 11/26/24 10:17 PM, Thomas Weißschuh wrote:
> 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/

Given this is a dependency, do you plan to follow up on [1]?

Thanks,
Daniel

   [1] https://lore.kernel.org/lkml/Z0TRc0A6Q8QUxNAe@google.com/

Re: [PATCH v2 0/2] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Thomas Weißschuh]

Hi Daniel,

On 2024-12-02 16:28:07+0100, Daniel Borkmann wrote:
> On 11/26/24 10:17 PM, Thomas Weißschuh wrote:
> > 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/
> 
> Given this is a dependency, do you plan to follow up on [1]?

>   [1] https://lore.kernel.org/lkml/Z0TRc0A6Q8QUxNAe@google.com/

I did so in [2], which is already part of the BPF tree.

[2] https://lore.kernel.org/lkml/20241125-bpf_lsm_task_getsecid_obj-v2-1-c8395bde84e0@weissschuh.net/

Re: [PATCH v2 0/2] kbuild: propagate CONFIG_WERROR to resolve_btfids

[Daniel Borkmann]

On 12/2/24 4:32 PM, Thomas Weißschuh wrote:
> On 2024-12-02 16:28:07+0100, Daniel Borkmann wrote:
>> On 11/26/24 10:17 PM, Thomas Weißschuh wrote:
>>> 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/
>>
>> Given this is a dependency, do you plan to follow up on [1]?
> 
>>    [1] https://lore.kernel.org/lkml/Z0TRc0A6Q8QUxNAe@google.com/
> 
> I did so in [2], which is already part of the BPF tree.
> 
> [2] https://lore.kernel.org/lkml/20241125-bpf_lsm_task_getsecid_obj-v2-1-c8395bde84e0@weissschuh.net/

Perfect, nevermind then!

Re: [PATCH v2 1/2] tools/resolve_btfids: Add --fatal-warnings option

[Andrii Nakryiko]

On Tue, Nov 26, 2024 at 1:17 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> 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>
> Acked-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  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..571d29d2da97fea75e5f9c544a95b9ac65f9e579 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,6 +786,7 @@ int main(int argc, const char **argv)
>                 .funcs    = RB_ROOT,
>                 .sets     = RB_ROOT,
>         };
> +       bool fatal_warnings = false;
>         struct option btfid_options[] = {
>                 OPT_INCR('v', "verbose", &verbose,
>                          "be more verbose (show errors, etc)"),
> @@ -789,6 +794,8 @@ int main(int argc, const char **argv)
>                            "BTF data"),
>                 OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
>                            "path of file providing base BTF"),
> +               OPT_BOOLEAN(0, "fatal-warnings", &fatal_warnings,
> +                           "turn warnings into errors"),

We are mixing naming styles here: we have "btf_base" with underscore
separator, and you are adding "fatal-warnings" with dash separator. I
personally like dashes, but whichever way we should stay consistent.
So let's fix it, otherwise it looks a bit sloppy.

Please also use [PATCH bpf-next v3] subject prefix to make it explicit
that this should go through bpf-next tree.

pw-bot: cr

>                 OPT_END()
>         };
>         int err = -1;
> @@ -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;

nit: just

if (!fatal_warnings)
    err = 0;

?

>  out:
>         if (obj.efile.elf) {
>                 elf_end(obj.efile.elf);
>
> --
> 2.47.1
>

Re: [PATCH v2 1/2] tools/resolve_btfids: Add --fatal-warnings option

[Thomas Weißschuh]

On 2024-12-03 14:31:01-0800, Andrii Nakryiko wrote:
> On Tue, Nov 26, 2024 at 1:17 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > 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>
> > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  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..571d29d2da97fea75e5f9c544a95b9ac65f9e579 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,6 +786,7 @@ int main(int argc, const char **argv)
> >                 .funcs    = RB_ROOT,
> >                 .sets     = RB_ROOT,
> >         };
> > +       bool fatal_warnings = false;
> >         struct option btfid_options[] = {
> >                 OPT_INCR('v', "verbose", &verbose,
> >                          "be more verbose (show errors, etc)"),
> > @@ -789,6 +794,8 @@ int main(int argc, const char **argv)
> >                            "BTF data"),
> >                 OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
> >                            "path of file providing base BTF"),
> > +               OPT_BOOLEAN(0, "fatal-warnings", &fatal_warnings,
> > +                           "turn warnings into errors"),
> 
> We are mixing naming styles here: we have "btf_base" with underscore
> separator, and you are adding "fatal-warnings" with dash separator. I
> personally like dashes, but whichever way we should stay consistent.
> So let's fix it, otherwise it looks a bit sloppy.

Ack.

> 
> Please also use [PATCH bpf-next v3] subject prefix to make it explicit
> that this should go through bpf-next tree.

Ack.

> 
> pw-bot: cr
> 
> >                 OPT_END()
> >         };
> >         int err = -1;
> > @@ -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;
> 
> nit: just
> 
> if (!fatal_warnings)
>     err = 0;
> 
> ?

This seems wrong. Now the actual warning counter is never evaluated.
And --fatal_warnings will always lead to an error exit code.

> >  out:
> >         if (obj.efile.elf) {
> >                 elf_end(obj.efile.elf);
> >
> > --
> > 2.47.1
> >

Re: [PATCH v2 1/2] tools/resolve_btfids: Add --fatal-warnings option

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 3:09 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2024-12-03 14:31:01-0800, Andrii Nakryiko wrote:
> > On Tue, Nov 26, 2024 at 1:17 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > >
> > > 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>
> > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > >  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..571d29d2da97fea75e5f9c544a95b9ac65f9e579 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,6 +786,7 @@ int main(int argc, const char **argv)
> > >                 .funcs    = RB_ROOT,
> > >                 .sets     = RB_ROOT,
> > >         };
> > > +       bool fatal_warnings = false;
> > >         struct option btfid_options[] = {
> > >                 OPT_INCR('v', "verbose", &verbose,
> > >                          "be more verbose (show errors, etc)"),
> > > @@ -789,6 +794,8 @@ int main(int argc, const char **argv)
> > >                            "BTF data"),
> > >                 OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
> > >                            "path of file providing base BTF"),
> > > +               OPT_BOOLEAN(0, "fatal-warnings", &fatal_warnings,
> > > +                           "turn warnings into errors"),
> >
> > We are mixing naming styles here: we have "btf_base" with underscore
> > separator, and you are adding "fatal-warnings" with dash separator. I
> > personally like dashes, but whichever way we should stay consistent.
> > So let's fix it, otherwise it looks a bit sloppy.
>
> Ack.
>
> >
> > Please also use [PATCH bpf-next v3] subject prefix to make it explicit
> > that this should go through bpf-next tree.
>
> Ack.
>
> >
> > pw-bot: cr
> >
> > >                 OPT_END()
> > >         };
> > >         int err = -1;
> > > @@ -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;
> >
> > nit: just
> >
> > if (!fatal_warnings)
> >     err = 0;
> >
> > ?
>
> This seems wrong. Now the actual warning counter is never evaluated.
> And --fatal_warnings will always lead to an error exit code.

Ah, I missed that you are using default -1 value here. I wonder if we
should make it a bit more explicit?

if (fatal_warnings)
    err = warnings ? -1 : 0;
else
    err = 0;

Something like that?

>
> > >  out:
> > >         if (obj.efile.elf) {
> > >                 elf_end(obj.efile.elf);
> > >
> > > --
> > > 2.47.1
> > >

Re: [PATCH v2 1/2] tools/resolve_btfids: Add --fatal-warnings option

[Thomas Weißschuh]

On 2024-12-03 18:06:26-0800, Andrii Nakryiko wrote:
> On Tue, Dec 3, 2024 at 3:09 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> >
> > On 2024-12-03 14:31:01-0800, Andrii Nakryiko wrote:
> > > On Tue, Nov 26, 2024 at 1:17 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > > >
> > > > 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>
> > > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  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..571d29d2da97fea75e5f9c544a95b9ac65f9e579 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,6 +786,7 @@ int main(int argc, const char **argv)
> > > >                 .funcs    = RB_ROOT,
> > > >                 .sets     = RB_ROOT,
> > > >         };
> > > > +       bool fatal_warnings = false;
> > > >         struct option btfid_options[] = {
> > > >                 OPT_INCR('v', "verbose", &verbose,
> > > >                          "be more verbose (show errors, etc)"),
> > > > @@ -789,6 +794,8 @@ int main(int argc, const char **argv)
> > > >                            "BTF data"),
> > > >                 OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
> > > >                            "path of file providing base BTF"),
> > > > +               OPT_BOOLEAN(0, "fatal-warnings", &fatal_warnings,
> > > > +                           "turn warnings into errors"),
> > >
> > > We are mixing naming styles here: we have "btf_base" with underscore
> > > separator, and you are adding "fatal-warnings" with dash separator. I
> > > personally like dashes, but whichever way we should stay consistent.
> > > So let's fix it, otherwise it looks a bit sloppy.
> >
> > Ack.
> >
> > >
> > > Please also use [PATCH bpf-next v3] subject prefix to make it explicit
> > > that this should go through bpf-next tree.
> >
> > Ack.
> >
> > >
> > > pw-bot: cr
> > >
> > > >                 OPT_END()
> > > >         };
> > > >         int err = -1;
> > > > @@ -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;
> > >
> > > nit: just
> > >
> > > if (!fatal_warnings)
> > >     err = 0;
> > >
> > > ?
> >
> > This seems wrong. Now the actual warning counter is never evaluated.
> > And --fatal_warnings will always lead to an error exit code.
> 
> Ah, I missed that you are using default -1 value here. I wonder if we
> should make it a bit more explicit?
> 
> if (fatal_warnings)
>     err = warnings ? -1 : 0;
> else
>     err = 0;
> 
> Something like that?

The existing code was the same. Also the rest of the function
relies on this. IMO the pattern is clear when looking at the resulting
code and not the diff.
But if you prefer I can change it of course.

> > > >  out:
> > > >         if (obj.efile.elf) {
> > > >                 elf_end(obj.efile.elf);
> > > >
> > > > --
> > > > 2.47.1
> > > >

Re: [PATCH v2 1/2] tools/resolve_btfids: Add --fatal-warnings option

[Andrii Nakryiko]

On Tue, Dec 3, 2024 at 10:19 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2024-12-03 18:06:26-0800, Andrii Nakryiko wrote:
> > On Tue, Dec 3, 2024 at 3:09 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > >
> > > On 2024-12-03 14:31:01-0800, Andrii Nakryiko wrote:
> > > > On Tue, Nov 26, 2024 at 1:17 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
> > > > >
> > > > > 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>
> > > > > Acked-by: Jiri Olsa <jolsa@kernel.org>
> > > > > ---
> > > > >  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..571d29d2da97fea75e5f9c544a95b9ac65f9e579 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,6 +786,7 @@ int main(int argc, const char **argv)
> > > > >                 .funcs    = RB_ROOT,
> > > > >                 .sets     = RB_ROOT,
> > > > >         };
> > > > > +       bool fatal_warnings = false;
> > > > >         struct option btfid_options[] = {
> > > > >                 OPT_INCR('v', "verbose", &verbose,
> > > > >                          "be more verbose (show errors, etc)"),
> > > > > @@ -789,6 +794,8 @@ int main(int argc, const char **argv)
> > > > >                            "BTF data"),
> > > > >                 OPT_STRING('b', "btf_base", &obj.base_btf_path, "file",
> > > > >                            "path of file providing base BTF"),
> > > > > +               OPT_BOOLEAN(0, "fatal-warnings", &fatal_warnings,
> > > > > +                           "turn warnings into errors"),
> > > >
> > > > We are mixing naming styles here: we have "btf_base" with underscore
> > > > separator, and you are adding "fatal-warnings" with dash separator. I
> > > > personally like dashes, but whichever way we should stay consistent.
> > > > So let's fix it, otherwise it looks a bit sloppy.
> > >
> > > Ack.
> > >
> > > >
> > > > Please also use [PATCH bpf-next v3] subject prefix to make it explicit
> > > > that this should go through bpf-next tree.
> > >
> > > Ack.
> > >
> > > >
> > > > pw-bot: cr
> > > >
> > > > >                 OPT_END()
> > > > >         };
> > > > >         int err = -1;
> > > > > @@ -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;
> > > >
> > > > nit: just
> > > >
> > > > if (!fatal_warnings)
> > > >     err = 0;
> > > >
> > > > ?
> > >
> > > This seems wrong. Now the actual warning counter is never evaluated.
> > > And --fatal_warnings will always lead to an error exit code.
> >
> > Ah, I missed that you are using default -1 value here. I wonder if we
> > should make it a bit more explicit?
> >
> > if (fatal_warnings)
> >     err = warnings ? -1 : 0;
> > else
> >     err = 0;
> >
> > Something like that?
>
> The existing code was the same. Also the rest of the function
> relies on this. IMO the pattern is clear when looking at the resulting
> code and not the diff.
> But if you prefer I can change it of course.

That new condition breaks my brain, but luckily I don't have to look
at it often, so I don't care all that much. Feel free to leave it as
is.

>
> > > > >  out:
> > > > >         if (obj.efile.elf) {
> > > > >                 elf_end(obj.efile.elf);
> > > > >
> > > > > --
> > > > > 2.47.1
> > > > >