* [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type
@ 2021-01-05 15:39 Jiri Olsa
2021-01-05 19:37 ` Alexei Starovoitov
2021-01-05 21:15 ` Andrii Nakryiko
0 siblings, 2 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-01-05 15:39 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: Qais Yousef, netdev, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh
The kernel image can contain multiple types (structs/unions)
with the same name. This causes distinct type hierarchies in
BTF data and makes resolve_btfids fail with error like:
BTFIDS vmlinux
FAILED unresolved symbol udp6_sock
as reported by Qais Yousef [1].
This change adds warning when multiple types of the same name
are detected:
BTFIDS vmlinux
WARN: multiple IDs found for 'file' (526, 113351)
WARN: multiple IDs found for 'sk_buff' (2744, 113958)
We keep the lower ID for the given type instance and let the
build continue.
[1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/
Reported-by: Qais Yousef <qais.yousef@arm.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/bpf/resolve_btfids/main.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
index e3ea569ee125..36a3b1024cdc 100644
--- a/tools/bpf/resolve_btfids/main.c
+++ b/tools/bpf/resolve_btfids/main.c
@@ -139,6 +139,8 @@ int eprintf(int level, int var, const char *fmt, ...)
#define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
#define pr_err(fmt, ...) \
eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info(fmt, ...) \
+ eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
static bool is_btf_id(const char *name)
{
@@ -526,8 +528,13 @@ static int symbols_resolve(struct object *obj)
id = btf_id__find(root, str);
if (id) {
- id->id = type_id;
- (*nr)--;
+ if (id->id) {
+ pr_info("WARN: multiple IDs found for '%s' (%d, %d)\n",
+ str, id->id, type_id);
+ } else {
+ id->id = type_id;
+ (*nr)--;
+ }
}
}
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type
2021-01-05 15:39 [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type Jiri Olsa
@ 2021-01-05 19:37 ` Alexei Starovoitov
2021-01-05 19:50 ` Jiri Olsa
2021-01-05 21:15 ` Andrii Nakryiko
1 sibling, 1 reply; 6+ messages in thread
From: Alexei Starovoitov @ 2021-01-05 19:37 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Qais Yousef, Network Development, bpf, Martin KaFai Lau,
Song Liu, Yonghong Song, John Fastabend, KP Singh
On Tue, Jan 5, 2021 at 7:39 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The kernel image can contain multiple types (structs/unions)
> with the same name. This causes distinct type hierarchies in
> BTF data and makes resolve_btfids fail with error like:
>
> BTFIDS vmlinux
> FAILED unresolved symbol udp6_sock
>
> as reported by Qais Yousef [1].
>
> This change adds warning when multiple types of the same name
> are detected:
>
> BTFIDS vmlinux
> WARN: multiple IDs found for 'file' (526, 113351)
> WARN: multiple IDs found for 'sk_buff' (2744, 113958)
>
> We keep the lower ID for the given type instance and let the
> build continue.
I think it would make sense to mention this decision in the warning.
'WARN: multiple IDs' is ambiguous and confusing when action is not specified.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type
2021-01-05 19:37 ` Alexei Starovoitov
@ 2021-01-05 19:50 ` Jiri Olsa
2021-01-05 20:02 ` Alexei Starovoitov
0 siblings, 1 reply; 6+ messages in thread
From: Jiri Olsa @ 2021-01-05 19:50 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Qais Yousef, Network Development, bpf, Martin KaFai Lau,
Song Liu, Yonghong Song, John Fastabend, KP Singh
On Tue, Jan 05, 2021 at 11:37:09AM -0800, Alexei Starovoitov wrote:
> On Tue, Jan 5, 2021 at 7:39 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The kernel image can contain multiple types (structs/unions)
> > with the same name. This causes distinct type hierarchies in
> > BTF data and makes resolve_btfids fail with error like:
> >
> > BTFIDS vmlinux
> > FAILED unresolved symbol udp6_sock
> >
> > as reported by Qais Yousef [1].
> >
> > This change adds warning when multiple types of the same name
> > are detected:
> >
> > BTFIDS vmlinux
> > WARN: multiple IDs found for 'file' (526, 113351)
> > WARN: multiple IDs found for 'sk_buff' (2744, 113958)
> >
> > We keep the lower ID for the given type instance and let the
> > build continue.
>
> I think it would make sense to mention this decision in the warning.
> 'WARN: multiple IDs' is ambiguous and confusing when action is not specified.
ok, how about:
WARN: multiple IDs found for 'file': 526, 113351 - using 526
jirka
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type
2021-01-05 19:50 ` Jiri Olsa
@ 2021-01-05 20:02 ` Alexei Starovoitov
0 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2021-01-05 20:02 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Qais Yousef, Network Development, bpf, Martin KaFai Lau,
Song Liu, Yonghong Song, John Fastabend, KP Singh
On Tue, Jan 5, 2021 at 11:50 AM Jiri Olsa <jolsa@redhat.com> wrote:
>
> On Tue, Jan 05, 2021 at 11:37:09AM -0800, Alexei Starovoitov wrote:
> > On Tue, Jan 5, 2021 at 7:39 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > The kernel image can contain multiple types (structs/unions)
> > > with the same name. This causes distinct type hierarchies in
> > > BTF data and makes resolve_btfids fail with error like:
> > >
> > > BTFIDS vmlinux
> > > FAILED unresolved symbol udp6_sock
> > >
> > > as reported by Qais Yousef [1].
> > >
> > > This change adds warning when multiple types of the same name
> > > are detected:
> > >
> > > BTFIDS vmlinux
> > > WARN: multiple IDs found for 'file' (526, 113351)
> > > WARN: multiple IDs found for 'sk_buff' (2744, 113958)
> > >
> > > We keep the lower ID for the given type instance and let the
> > > build continue.
> >
> > I think it would make sense to mention this decision in the warning.
> > 'WARN: multiple IDs' is ambiguous and confusing when action is not specified.
>
> ok, how about:
>
> WARN: multiple IDs found for 'file': 526, 113351 - using 526
yep. much better imo.
Please add a comment to .c file with the suggestion that such types should
be renamed to avoid ambiguity.
Adding 'please rename' to WARN is probably overkill.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type
2021-01-05 15:39 [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type Jiri Olsa
2021-01-05 19:37 ` Alexei Starovoitov
@ 2021-01-05 21:15 ` Andrii Nakryiko
2021-01-05 22:47 ` Jiri Olsa
1 sibling, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2021-01-05 21:15 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Qais Yousef, Networking, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh
On Tue, Jan 5, 2021 at 7:41 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The kernel image can contain multiple types (structs/unions)
> with the same name. This causes distinct type hierarchies in
> BTF data and makes resolve_btfids fail with error like:
>
> BTFIDS vmlinux
> FAILED unresolved symbol udp6_sock
>
> as reported by Qais Yousef [1].
>
> This change adds warning when multiple types of the same name
> are detected:
>
> BTFIDS vmlinux
> WARN: multiple IDs found for 'file' (526, 113351)
> WARN: multiple IDs found for 'sk_buff' (2744, 113958)
>
> We keep the lower ID for the given type instance and let the
> build continue.
>
> [1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/
> Reported-by: Qais Yousef <qais.yousef@arm.com>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
see comments below, but otherwise lgtm
Acked-by: Andrii Nakryiko <andrii@kernel.org>
> tools/bpf/resolve_btfids/main.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> index e3ea569ee125..36a3b1024cdc 100644
> --- a/tools/bpf/resolve_btfids/main.c
> +++ b/tools/bpf/resolve_btfids/main.c
> @@ -139,6 +139,8 @@ int eprintf(int level, int var, const char *fmt, ...)
> #define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
> #define pr_err(fmt, ...) \
> eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_info(fmt, ...) \
> + eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
how is it different from pr_err? Did you forget to update verboseness
levels or it's intentional?
>
> static bool is_btf_id(const char *name)
> {
> @@ -526,8 +528,13 @@ static int symbols_resolve(struct object *obj)
>
> id = btf_id__find(root, str);
> if (id) {
> - id->id = type_id;
> - (*nr)--;
> + if (id->id) {
> + pr_info("WARN: multiple IDs found for '%s' (%d, %d)\n",
> + str, id->id, type_id);
> + } else {
> + id->id = type_id;
> + (*nr)--;
btw, there is a nasty shadowing of nr variable, which is used both for
the for() loop condition (as int) and as `int *` inside the loop body.
It's better to rename inner (or outer) nr, it's extremely confusing as
is.
> + }
> }
> }
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type
2021-01-05 21:15 ` Andrii Nakryiko
@ 2021-01-05 22:47 ` Jiri Olsa
0 siblings, 0 replies; 6+ messages in thread
From: Jiri Olsa @ 2021-01-05 22:47 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Qais Yousef, Networking, bpf, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh
On Tue, Jan 05, 2021 at 01:15:39PM -0800, Andrii Nakryiko wrote:
> On Tue, Jan 5, 2021 at 7:41 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The kernel image can contain multiple types (structs/unions)
> > with the same name. This causes distinct type hierarchies in
> > BTF data and makes resolve_btfids fail with error like:
> >
> > BTFIDS vmlinux
> > FAILED unresolved symbol udp6_sock
> >
> > as reported by Qais Yousef [1].
> >
> > This change adds warning when multiple types of the same name
> > are detected:
> >
> > BTFIDS vmlinux
> > WARN: multiple IDs found for 'file' (526, 113351)
> > WARN: multiple IDs found for 'sk_buff' (2744, 113958)
> >
> > We keep the lower ID for the given type instance and let the
> > build continue.
> >
> > [1] https://lore.kernel.org/lkml/20201229151352.6hzmjvu3qh6p2qgg@e107158-lin/
> > Reported-by: Qais Yousef <qais.yousef@arm.com>
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
>
> see comments below, but otherwise lgtm
>
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>
> > tools/bpf/resolve_btfids/main.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index e3ea569ee125..36a3b1024cdc 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -139,6 +139,8 @@ int eprintf(int level, int var, const char *fmt, ...)
> > #define pr_debug2(fmt, ...) pr_debugN(2, pr_fmt(fmt), ##__VA_ARGS__)
> > #define pr_err(fmt, ...) \
> > eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_info(fmt, ...) \
> > + eprintf(0, verbose, pr_fmt(fmt), ##__VA_ARGS__)
>
> how is it different from pr_err? Did you forget to update verboseness
> levels or it's intentional?
intentional, I'm using pr_err to print in error paths,
so I wanted to add new one for other 'info' messages without -v
>
> >
> > static bool is_btf_id(const char *name)
> > {
> > @@ -526,8 +528,13 @@ static int symbols_resolve(struct object *obj)
> >
> > id = btf_id__find(root, str);
> > if (id) {
> > - id->id = type_id;
> > - (*nr)--;
> > + if (id->id) {
> > + pr_info("WARN: multiple IDs found for '%s' (%d, %d)\n",
> > + str, id->id, type_id);
> > + } else {
> > + id->id = type_id;
> > + (*nr)--;
>
> btw, there is a nasty shadowing of nr variable, which is used both for
> the for() loop condition (as int) and as `int *` inside the loop body.
> It's better to rename inner (or outer) nr, it's extremely confusing as
> is.
nice, I'll change that
thanks,
jirka
>
> > + }
> > }
> > }
> >
> > --
> > 2.26.2
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-05 22:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-05 15:39 [PATCH bpf-next] tools/resolve_btfids: Warn when having multiple IDs for single type Jiri Olsa
2021-01-05 19:37 ` Alexei Starovoitov
2021-01-05 19:50 ` Jiri Olsa
2021-01-05 20:02 ` Alexei Starovoitov
2021-01-05 21:15 ` Andrii Nakryiko
2021-01-05 22:47 ` Jiri Olsa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.