* [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
@ 2023-03-30 21:27 Eduard Zingerman
2023-03-31 12:13 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 8+ messages in thread
From: Eduard Zingerman @ 2023-03-30 21:27 UTC (permalink / raw)
To: dwarves, arnaldo.melo
Cc: bpf, kernel-team, ast, daniel, andrii, yhs, Eduard Zingerman
Recent change to fprintf (see below) causes incorrect `type_fprintf()`
behavior for pointers annotated with btf_type_tag, for example:
$ cat tag-test.c
#define __t __attribute__((btf_type_tag("t1")))
struct foo {
int __t *a;
} g;
$ clang -g -c tag-test.c -o tag-test.o && \
pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
struct foo {
int a; /* 0 8 */
...
};
Note that `*` is missing in the pahole output.
The issue is caused by `goto next_type` that jumps over the
`tag__name()` that is responsible for pointer printing.
As agreed in [1] `type__fprintf()` is modified to skip type tags for
now and would be modified to emit type tags later.
The change in `__tag__name()` is necessary to avoid the following behavior:
$ cat tag-test.c
#define __t __attribute__((btf_type_tag("t1")))
struct foo {
int __t *a;
int __t __t *b;
} g;
$ clang -g -c tag-test.c -o tag-test.o && \
pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
struct foo {
int * a; /* 0 8 */
int * b; /* 8 8 */
...
};
Note the extra space before `*` for field `b`.
The issue was reported and tracked down to the root cause by
Arnaldo Carvalho de Melo.
Links:
[1] https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#md82b04f66867434524beec746138951f26a3977e
Fixes: e7fb771f2649 ("fprintf: Correct names for types with btf_type_tag attribute")
Reported-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Link: https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#mc630bcd474ddd64c70d237edd4e0590dc048d63d
Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
---
dwarves_fprintf.c | 37 +++++++++++++++++++++++++++++--------
1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
index 1e6147a..818db2d 100644
--- a/dwarves_fprintf.c
+++ b/dwarves_fprintf.c
@@ -572,7 +572,6 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
case DW_TAG_restrict_type:
case DW_TAG_atomic_type:
case DW_TAG_unspecified_type:
- case DW_TAG_LLVM_annotation:
type = cu__type(cu, tag->type);
if (type == NULL && tag->type != 0)
tag__id_not_found_snprintf(bf, len, tag->type);
@@ -618,6 +617,13 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
case DW_TAG_variable:
snprintf(bf, len, "%s", variable__name(tag__variable(tag)));
break;
+ case DW_TAG_LLVM_annotation:
+ type = cu__type(cu, tag->type);
+ if (type == NULL && tag->type != 0)
+ tag__id_not_found_snprintf(bf, len, tag->type);
+ else if (!tag__has_type_loop(tag, type, bf, len, NULL))
+ __tag__name(type, cu, bf, len, conf);
+ break;
default:
snprintf(bf, len, "%s%s", tag__prefix(cu, tag->tag, pconf),
type__name(tag__type(tag)) ?: "");
@@ -677,6 +683,22 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
return printed;
}
+static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
+{
+ struct tag *type;
+
+ for (;;) {
+ if (id == 0)
+ break;
+ type = cu__type(cu, id);
+ if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
+ break;
+ id = type->type;
+ }
+
+ return id;
+}
+
static size_t union__fprintf(struct type *type, const struct cu *cu,
const struct conf_fprintf *conf, FILE *fp);
@@ -778,19 +800,17 @@ inner_struct:
next_type:
switch (type->tag) {
- case DW_TAG_pointer_type:
- if (type->type != 0) {
+ case DW_TAG_pointer_type: {
+ type_id_t ptype_id = skip_llvm_annotations(cu, type->type);
+
+ if (ptype_id != 0) {
int n;
- struct tag *ptype = cu__type(cu, type->type);
+ struct tag *ptype = cu__type(cu, ptype_id);
if (ptype == NULL)
goto out_type_not_found;
n = tag__has_type_loop(type, ptype, NULL, 0, fp);
if (n)
return printed + n;
- if (ptype->tag == DW_TAG_LLVM_annotation) {
- type = ptype;
- goto next_type;
- }
if (ptype->tag == DW_TAG_subroutine_type) {
printed += ftype__fprintf(tag__ftype(ptype),
cu, name, 0, 1,
@@ -811,6 +831,7 @@ next_type:
}
}
/* Fall Thru */
+ }
default:
print_default:
printed += fprintf(fp, "%-*s %s", tconf.type_spacing,
--
2.40.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-30 21:27 [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag Eduard Zingerman
@ 2023-03-31 12:13 ` Arnaldo Carvalho de Melo
2023-03-31 12:14 ` Arnaldo Carvalho de Melo
2023-03-31 13:35 ` Eduard Zingerman
0 siblings, 2 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-31 12:13 UTC (permalink / raw)
To: Eduard Zingerman
Cc: dwarves, arnaldo.melo, bpf, kernel-team, ast, daniel, andrii, yhs
Em Fri, Mar 31, 2023 at 12:27:00AM +0300, Eduard Zingerman escreveu:
> Recent change to fprintf (see below) causes incorrect `type_fprintf()`
> behavior for pointers annotated with btf_type_tag, for example:
>
> $ cat tag-test.c
> #define __t __attribute__((btf_type_tag("t1")))
>
> struct foo {
> int __t *a;
> } g;
>
> $ clang -g -c tag-test.c -o tag-test.o && \
> pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> struct foo {
> int a; /* 0 8 */
> ...
> };
>
> Note that `*` is missing in the pahole output.
> The issue is caused by `goto next_type` that jumps over the
> `tag__name()` that is responsible for pointer printing.
>
> As agreed in [1] `type__fprintf()` is modified to skip type tags for
> now and would be modified to emit type tags later.
>
> The change in `__tag__name()` is necessary to avoid the following behavior:
>
> $ cat tag-test.c
> #define __t __attribute__((btf_type_tag("t1")))
>
> struct foo {
> int __t *a;
> int __t __t *b;
> } g;
>
> $ clang -g -c tag-test.c -o tag-test.o && \
> pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> struct foo {
> int * a; /* 0 8 */
> int * b; /* 8 8 */
> ...
> };
>
> Note the extra space before `*` for field `b`.
>
> The issue was reported and tracked down to the root cause by
> Arnaldo Carvalho de Melo.
>
> Links:
> [1] https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#md82b04f66867434524beec746138951f26a3977e
>
> Fixes: e7fb771f2649 ("fprintf: Correct names for types with btf_type_tag attribute")
> Reported-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Link: https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#mc630bcd474ddd64c70d237edd4e0590dc048d63d
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
> dwarves_fprintf.c | 37 +++++++++++++++++++++++++++++--------
> 1 file changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> index 1e6147a..818db2d 100644
> --- a/dwarves_fprintf.c
> +++ b/dwarves_fprintf.c
> @@ -572,7 +572,6 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> case DW_TAG_restrict_type:
> case DW_TAG_atomic_type:
> case DW_TAG_unspecified_type:
> - case DW_TAG_LLVM_annotation:
> type = cu__type(cu, tag->type);
> if (type == NULL && tag->type != 0)
> tag__id_not_found_snprintf(bf, len, tag->type);
> @@ -618,6 +617,13 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> case DW_TAG_variable:
> snprintf(bf, len, "%s", variable__name(tag__variable(tag)));
> break;
> + case DW_TAG_LLVM_annotation:
> + type = cu__type(cu, tag->type);
> + if (type == NULL && tag->type != 0)
> + tag__id_not_found_snprintf(bf, len, tag->type);
> + else if (!tag__has_type_loop(tag, type, bf, len, NULL))
> + __tag__name(type, cu, bf, len, conf);
> + break;
> default:
> snprintf(bf, len, "%s%s", tag__prefix(cu, tag->tag, pconf),
> type__name(tag__type(tag)) ?: "");
> @@ -677,6 +683,22 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
> return printed;
> }
>
> +static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
> +{
> + struct tag *type;
> +
> + for (;;) {
> + if (id == 0)
> + break;
> + type = cu__type(cu, id);
> + if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
> + break;
> + id = type->type;
> + }
> +
> + return id;
> +}
This part I didn't understand, do you see any possibility of a
DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
- Arnaldo
> +
> static size_t union__fprintf(struct type *type, const struct cu *cu,
> const struct conf_fprintf *conf, FILE *fp);
>
> @@ -778,19 +800,17 @@ inner_struct:
>
> next_type:
> switch (type->tag) {
> - case DW_TAG_pointer_type:
> - if (type->type != 0) {
> + case DW_TAG_pointer_type: {
> + type_id_t ptype_id = skip_llvm_annotations(cu, type->type);
> +
> + if (ptype_id != 0) {
> int n;
> - struct tag *ptype = cu__type(cu, type->type);
> + struct tag *ptype = cu__type(cu, ptype_id);
> if (ptype == NULL)
> goto out_type_not_found;
> n = tag__has_type_loop(type, ptype, NULL, 0, fp);
> if (n)
> return printed + n;
> - if (ptype->tag == DW_TAG_LLVM_annotation) {
> - type = ptype;
> - goto next_type;
> - }
> if (ptype->tag == DW_TAG_subroutine_type) {
> printed += ftype__fprintf(tag__ftype(ptype),
> cu, name, 0, 1,
> @@ -811,6 +831,7 @@ next_type:
> }
> }
> /* Fall Thru */
> + }
> default:
> print_default:
> printed += fprintf(fp, "%-*s %s", tconf.type_spacing,
> --
> 2.40.0
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-31 12:13 ` Arnaldo Carvalho de Melo
@ 2023-03-31 12:14 ` Arnaldo Carvalho de Melo
2023-03-31 12:20 ` Arnaldo Carvalho de Melo
2023-03-31 13:35 ` Eduard Zingerman
1 sibling, 1 reply; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-31 12:14 UTC (permalink / raw)
To: Eduard Zingerman
Cc: dwarves, arnaldo.melo, bpf, kernel-team, ast, daniel, andrii, yhs
Em Fri, Mar 31, 2023 at 09:13:41AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Mar 31, 2023 at 12:27:00AM +0300, Eduard Zingerman escreveu:
> > Recent change to fprintf (see below) causes incorrect `type_fprintf()`
> > behavior for pointers annotated with btf_type_tag, for example:
> >
> > $ cat tag-test.c
> > #define __t __attribute__((btf_type_tag("t1")))
> >
> > struct foo {
> > int __t *a;
> > } g;
> >
> > $ clang -g -c tag-test.c -o tag-test.o && \
> > pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> > struct foo {
> > int a; /* 0 8 */
> > ...
> > };
> >
> > Note that `*` is missing in the pahole output.
> > The issue is caused by `goto next_type` that jumps over the
> > `tag__name()` that is responsible for pointer printing.
> >
> > As agreed in [1] `type__fprintf()` is modified to skip type tags for
> > now and would be modified to emit type tags later.
> >
> > The change in `__tag__name()` is necessary to avoid the following behavior:
> >
> > $ cat tag-test.c
> > #define __t __attribute__((btf_type_tag("t1")))
> >
> > struct foo {
> > int __t *a;
> > int __t __t *b;
> > } g;
> >
> > $ clang -g -c tag-test.c -o tag-test.o && \
> > pahole -J tag-test.o && pahole --sort -F dwarf tag-test.o
> > struct foo {
> > int * a; /* 0 8 */
> > int * b; /* 8 8 */
> > ...
> > };
> >
> > Note the extra space before `*` for field `b`.
> >
> > The issue was reported and tracked down to the root cause by
> > Arnaldo Carvalho de Melo.
> >
> > Links:
> > [1] https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#md82b04f66867434524beec746138951f26a3977e
> >
> > Fixes: e7fb771f2649 ("fprintf: Correct names for types with btf_type_tag attribute")
> > Reported-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> > Link: https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#mc630bcd474ddd64c70d237edd4e0590dc048d63d
> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> > ---
> > dwarves_fprintf.c | 37 +++++++++++++++++++++++++++++--------
> > 1 file changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index 1e6147a..818db2d 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -572,7 +572,6 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> > case DW_TAG_restrict_type:
> > case DW_TAG_atomic_type:
> > case DW_TAG_unspecified_type:
> > - case DW_TAG_LLVM_annotation:
> > type = cu__type(cu, tag->type);
> > if (type == NULL && tag->type != 0)
> > tag__id_not_found_snprintf(bf, len, tag->type);
> > @@ -618,6 +617,13 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> > case DW_TAG_variable:
> > snprintf(bf, len, "%s", variable__name(tag__variable(tag)));
> > break;
> > + case DW_TAG_LLVM_annotation:
> > + type = cu__type(cu, tag->type);
> > + if (type == NULL && tag->type != 0)
> > + tag__id_not_found_snprintf(bf, len, tag->type);
> > + else if (!tag__has_type_loop(tag, type, bf, len, NULL))
> > + __tag__name(type, cu, bf, len, conf);
> > + break;
> > default:
> > snprintf(bf, len, "%s%s", tag__prefix(cu, tag->tag, pconf),
> > type__name(tag__type(tag)) ?: "");
> > @@ -677,6 +683,22 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
> > return printed;
> > }
> >
> > +static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
> > +{
> > + struct tag *type;
> > +
> > + for (;;) {
> > + if (id == 0)
> > + break;
> > + type = cu__type(cu, id);
> > + if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
> > + break;
> > + id = type->type;
> > + }
> > +
> > + return id;
> > +}
>
> This part I didn't understand, do you see any possibility of a
> DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
I _think_ its a noop, so will test your patch as-is, thanks!
- Arnaldo
> - Arnaldo
>
> > +
> > static size_t union__fprintf(struct type *type, const struct cu *cu,
> > const struct conf_fprintf *conf, FILE *fp);
> >
> > @@ -778,19 +800,17 @@ inner_struct:
> >
> > next_type:
> > switch (type->tag) {
> > - case DW_TAG_pointer_type:
> > - if (type->type != 0) {
> > + case DW_TAG_pointer_type: {
> > + type_id_t ptype_id = skip_llvm_annotations(cu, type->type);
> > +
> > + if (ptype_id != 0) {
> > int n;
> > - struct tag *ptype = cu__type(cu, type->type);
> > + struct tag *ptype = cu__type(cu, ptype_id);
> > if (ptype == NULL)
> > goto out_type_not_found;
> > n = tag__has_type_loop(type, ptype, NULL, 0, fp);
> > if (n)
> > return printed + n;
> > - if (ptype->tag == DW_TAG_LLVM_annotation) {
> > - type = ptype;
> > - goto next_type;
> > - }
> > if (ptype->tag == DW_TAG_subroutine_type) {
> > printed += ftype__fprintf(tag__ftype(ptype),
> > cu, name, 0, 1,
> > @@ -811,6 +831,7 @@ next_type:
> > }
> > }
> > /* Fall Thru */
> > + }
> > default:
> > print_default:
> > printed += fprintf(fp, "%-*s %s", tconf.type_spacing,
> > --
> > 2.40.0
> >
>
> --
>
> - Arnaldo
--
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-31 12:14 ` Arnaldo Carvalho de Melo
@ 2023-03-31 12:20 ` Arnaldo Carvalho de Melo
2023-03-31 14:12 ` Eduard Zingerman
0 siblings, 1 reply; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-31 12:20 UTC (permalink / raw)
To: Eduard Zingerman
Cc: dwarves, arnaldo.melo, bpf, kernel-team, ast, daniel, andrii, yhs
Em Fri, Mar 31, 2023 at 09:14:39AM -0300, Arnaldo Carvalho de Melo escreveu:
> > This part I didn't understand, do you see any possibility of a
> > DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
>
> I _think_ its a noop, so will test your patch as-is, thanks!
Tested, now we're left with normalizing base type names generated by
clang and gcc, things like:
--- /tmp/gcc 2023-03-31 09:16:34.100006650 -0300
+++ /tmp/clang 2023-03-31 09:16:26.211789489 -0300
@@ -96,8 +96,8 @@
/* XXX 4 bytes hole, try to pack */
- long unsigned int state; /* 216 8 */
- long unsigned int state2; /* 224 8 */
+ unsigned long state; /* 216 8 */
+ unsigned long state2; /* 224 8 */
struct Qdisc * next_sched; /* 232 8 */
struct sk_buff_head skb_bad_txq; /* 240 24 */
@@ -112,7 +112,7 @@
/* XXX 40 bytes hole, try to pack */
/* --- cacheline 6 boundary (384 bytes) --- */
- long int privdata[]; /* 384 0 */
+ long privdata[]; /* 384 0 */
/* size: 384, cachelines: 6, members: 28 */
/* sum members: 260, holes: 4, sum holes: 124 */
@@ -145,19 +145,19 @@
/* XXX 4 bytes hole, try to pack */
struct netdev_queue * (*select_queue)(struct Qdisc *, struct tcmsg *); /* 8 8 */
- int (*graft)(struct Qdisc *, long unsigned int, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /* 16 8 */
+ int (*graft)(struct Qdisc *, unsigned long, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /* 16 8 */
- struct Qdisc * (*leaf)(struct Qdisc *, long unsigned int); /* 24 8 */
+ struct Qdisc * (*leaf)(struct Qdisc *, unsigned long); /* 24 8 */
- void (*qlen_notify)(struct Qdisc *, long unsigned int); /* 32 8 */
+ void (*qlen_notify)(struct Qdisc *, unsigned long); /* 32 8 */
- long unsigned int (*find)(struct Qdisc *, u32); /* 40 8 */
+ unsigned long (*find)(struct Qdisc *, u32); /* 40 8 */
- int (*change)(struct Qdisc *, u32, u32, struct nlattr * *, long unsigned int *, struct netlink_ext_ack *); /* 48 8 */
+ int (*change)(struct Qdisc *, u32, u32, struct nlattr * *, unsigned long *, struct netlink_ext_ack *); /* 48 8 */
- int (*delete)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /* 56 8 */
+ int (*delete)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /* 56 8 */
/* --- cacheline 1 boundary (64 bytes) --- */
void (*walk)(struct Qdisc *, struct qdisc_walker *); /* 64 8 */
- struct tcf_block * (*tcf_block)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /* 72 8 */
+ struct tcf_block * (*tcf_block)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /* 72 8 */
- long unsigned int (*bind_tcf)(struct Qdisc *, long unsigned int, u32); /* 80 8 */
+ unsigned long (*bind_tcf)(struct Qdisc *, unsigned long, u32); /* 80 8 */
- void (*unbind_tcf)(struct Qdisc *, long unsigned int); /* 88 8 */
+ void (*unbind_tcf)(struct Qdisc *, unsigned long); /* 88 8 */
- int (*dump)(struct Qdisc *, long unsigned int, struct sk_buff *, struct tcmsg *); /* 96 8 */
+ int (*dump)(struct Qdisc *, unsigned long, struct sk_buff *, struct tcmsg *); /* 96 8 */
- int (*dump_stats)(struct Qdisc *, long unsigned int, struct gnet_dump *); /* 104 8 */
+ int (*dump_stats)(struct Qdisc *, unsigned long, struct gnet_dump *); /* 104 8 */
/* size: 112, cachelines: 2, members: 14 */
/* sum members: 108, holes: 1, sum holes: 4 */
@@ -227,21 +227,21 @@
This was affected somehow by these LLVM_annotation patches, I'll try to
handle this later.
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-31 12:13 ` Arnaldo Carvalho de Melo
2023-03-31 12:14 ` Arnaldo Carvalho de Melo
@ 2023-03-31 13:35 ` Eduard Zingerman
2023-03-31 14:05 ` Arnaldo Carvalho de Melo
1 sibling, 1 reply; 8+ messages in thread
From: Eduard Zingerman @ 2023-03-31 13:35 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs
On Fri, 2023-03-31 at 09:13 -0300, Arnaldo Carvalho de Melo wrote:
> [...]
> >
> > +static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
> > +{
> > + struct tag *type;
> > +
> > + for (;;) {
> > + if (id == 0)
> > + break;
> > + type = cu__type(cu, id);
> > + if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
> > + break;
> > + id = type->type;
> > + }
> > +
> > + return id;
> > +}
>
> This part I didn't understand, do you see any possibility of a
> DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
Not at the moment, but it is no illegal, it is possible to write
something like this:
#define __t1 __attribute__((btf_type_tag("t1")))
#define __t2 __attribute__((btf_type_tag("t2")))
int __t1 __t2 *g;
And to get BTF like ptr --> __t2 --> __t1 --> int.
> [...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-31 13:35 ` Eduard Zingerman
@ 2023-03-31 14:05 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-31 14:05 UTC (permalink / raw)
To: Eduard Zingerman; +Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs
On March 31, 2023 10:35:52 AM GMT-03:00, Eduard Zingerman <eddyz87@gmail.com> wrote:
>On Fri, 2023-03-31 at 09:13 -0300, Arnaldo Carvalho de Melo wrote:
>> [...]
>> >
>> > +static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
>> > +{
>> > + struct tag *type;
>> > +
>> > + for (;;) {
>> > + if (id == 0)
>> > + break;
>> > + type = cu__type(cu, id);
>> > + if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
>> > + break;
>> > + id = type->type;
>> > + }
>> > +
>> > + return id;
>> > +}
>>
>> This part I didn't understand, do you see any possibility of a
>> DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
>
>Not at the moment, but it is no illegal, it is possible to write
>something like this:
>
> #define __t1 __attribute__((btf_type_tag("t1")))
> #define __t2 __attribute__((btf_type_tag("t2")))
>
> int __t1 __t2 *g;
>
>And to get BTF like ptr --> __t2 --> __t1 --> int.
Right, thanks for clarifying, I'll add this as a comment above the skip llvm function.
This patch is already in the 'next' branch, will move to 'master' later today.
- Arnaldo
>
>> [...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-31 12:20 ` Arnaldo Carvalho de Melo
@ 2023-03-31 14:12 ` Eduard Zingerman
2023-03-31 18:33 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 8+ messages in thread
From: Eduard Zingerman @ 2023-03-31 14:12 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: dwarves, bpf, kernel-team, ast, daniel, andrii, yhs
On Fri, 2023-03-31 at 09:20 -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Mar 31, 2023 at 09:14:39AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > This part I didn't understand, do you see any possibility of a
> > > DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
> >
> > I _think_ its a noop, so will test your patch as-is, thanks!
>
> Tested, now we're left with normalizing base type names generated by
> clang and gcc, things like:
>
> --- /tmp/gcc 2023-03-31 09:16:34.100006650 -0300
> +++ /tmp/clang 2023-03-31 09:16:26.211789489 -0300
> @@ -96,8 +96,8 @@
>
> /* XXX 4 bytes hole, try to pack */
>
> - long unsigned int state; /* 216 8 */
> - long unsigned int state2; /* 224 8 */
> + unsigned long state; /* 216 8 */
> + unsigned long state2; /* 224 8 */
> struct Qdisc * next_sched; /* 232 8 */
> struct sk_buff_head skb_bad_txq; /* 240 24 */
>
> @@ -112,7 +112,7 @@
> /* XXX 40 bytes hole, try to pack */
>
> /* --- cacheline 6 boundary (384 bytes) --- */
> - long int privdata[]; /* 384 0 */
> + long privdata[]; /* 384 0 */
>
> /* size: 384, cachelines: 6, members: 28 */
> /* sum members: 260, holes: 4, sum holes: 124 */
> @@ -145,19 +145,19 @@
> /* XXX 4 bytes hole, try to pack */
>
> struct netdev_queue * (*select_queue)(struct Qdisc *, struct tcmsg *); /* 8 8 */
> - int (*graft)(struct Qdisc *, long unsigned int, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /* 16 8 */
> + int (*graft)(struct Qdisc *, unsigned long, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /* 16 8 */
> - struct Qdisc * (*leaf)(struct Qdisc *, long unsigned int); /* 24 8 */
> + struct Qdisc * (*leaf)(struct Qdisc *, unsigned long); /* 24 8 */
> - void (*qlen_notify)(struct Qdisc *, long unsigned int); /* 32 8 */
> + void (*qlen_notify)(struct Qdisc *, unsigned long); /* 32 8 */
> - long unsigned int (*find)(struct Qdisc *, u32); /* 40 8 */
> + unsigned long (*find)(struct Qdisc *, u32); /* 40 8 */
> - int (*change)(struct Qdisc *, u32, u32, struct nlattr * *, long unsigned int *, struct netlink_ext_ack *); /* 48 8 */
> + int (*change)(struct Qdisc *, u32, u32, struct nlattr * *, unsigned long *, struct netlink_ext_ack *); /* 48 8 */
> - int (*delete)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /* 56 8 */
> + int (*delete)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /* 56 8 */
> /* --- cacheline 1 boundary (64 bytes) --- */
> void (*walk)(struct Qdisc *, struct qdisc_walker *); /* 64 8 */
> - struct tcf_block * (*tcf_block)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /* 72 8 */
> + struct tcf_block * (*tcf_block)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /* 72 8 */
> - long unsigned int (*bind_tcf)(struct Qdisc *, long unsigned int, u32); /* 80 8 */
> + unsigned long (*bind_tcf)(struct Qdisc *, unsigned long, u32); /* 80 8 */
> - void (*unbind_tcf)(struct Qdisc *, long unsigned int); /* 88 8 */
> + void (*unbind_tcf)(struct Qdisc *, unsigned long); /* 88 8 */
> - int (*dump)(struct Qdisc *, long unsigned int, struct sk_buff *, struct tcmsg *); /* 96 8 */
> + int (*dump)(struct Qdisc *, unsigned long, struct sk_buff *, struct tcmsg *); /* 96 8 */
> - int (*dump_stats)(struct Qdisc *, long unsigned int, struct gnet_dump *); /* 104 8 */
> + int (*dump_stats)(struct Qdisc *, unsigned long, struct gnet_dump *); /* 104 8 */
>
> /* size: 112, cachelines: 2, members: 14 */
> /* sum members: 108, holes: 1, sum holes: 4 */
> @@ -227,21 +227,21 @@
>
> This was affected somehow by these LLVM_annotation patches, I'll try to
> handle this later.
Are you sure it is related to LLVM_annotation patches?
I tried (4d17096 "btf_encoder: Compare functions via prototypes not parameter names")
and see the same behavior.
Looking at the DWARF, itself gcc and clang use different names for these types:
gcc:
0x0000002b: DW_TAG_base_type
DW_AT_byte_size (0x08)
DW_AT_encoding (DW_ATE_unsigned)
DW_AT_name ("long unsigned int")
clang:
0x000000f7: DW_TAG_base_type
DW_AT_name ("unsigned long")
DW_AT_encoding (DW_ATE_unsigned)
DW_AT_byte_size (0x08)
And the base type names are copied to BTF as is. Looks like some
normalization is necessary either in dwarf_loader.c:base_type__new()
or in fprintf.
Thanks,
Eduard
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag
2023-03-31 14:12 ` Eduard Zingerman
@ 2023-03-31 18:33 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 8+ messages in thread
From: Arnaldo Carvalho de Melo @ 2023-03-31 18:33 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Arnaldo Carvalho de Melo, dwarves, bpf, kernel-team, ast, daniel,
andrii, yhs
Em Fri, Mar 31, 2023 at 05:12:31PM +0300, Eduard Zingerman escreveu:
> On Fri, 2023-03-31 at 09:20 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Mar 31, 2023 at 09:14:39AM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > This part I didn't understand, do you see any possibility of a
> > > > DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
> > >
> > > I _think_ its a noop, so will test your patch as-is, thanks!
> >
> > Tested, now we're left with normalizing base type names generated by
> > clang and gcc, things like:
> >
> > --- /tmp/gcc 2023-03-31 09:16:34.100006650 -0300
> > +++ /tmp/clang 2023-03-31 09:16:26.211789489 -0300
> > @@ -96,8 +96,8 @@
> >
> > /* XXX 4 bytes hole, try to pack */
> >
> > - long unsigned int state; /* 216 8 */
> > - long unsigned int state2; /* 224 8 */
> > + unsigned long state; /* 216 8 */
> > + unsigned long state2; /* 224 8 */
> > struct Qdisc * next_sched; /* 232 8 */
> > struct sk_buff_head skb_bad_txq; /* 240 24 */
> >
> > @@ -112,7 +112,7 @@
> > /* XXX 40 bytes hole, try to pack */
> >
> > /* --- cacheline 6 boundary (384 bytes) --- */
> > - long int privdata[]; /* 384 0 */
> > + long privdata[]; /* 384 0 */
> >
> > /* size: 384, cachelines: 6, members: 28 */
> > /* sum members: 260, holes: 4, sum holes: 124 */
> > @@ -145,19 +145,19 @@
> > /* XXX 4 bytes hole, try to pack */
> >
> > struct netdev_queue * (*select_queue)(struct Qdisc *, struct tcmsg *); /* 8 8 */
> > - int (*graft)(struct Qdisc *, long unsigned int, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /* 16 8 */
> > + int (*graft)(struct Qdisc *, unsigned long, struct Qdisc *, struct Qdisc * *, struct netlink_ext_ack *); /* 16 8 */
> > - struct Qdisc * (*leaf)(struct Qdisc *, long unsigned int); /* 24 8 */
> > + struct Qdisc * (*leaf)(struct Qdisc *, unsigned long); /* 24 8 */
> > - void (*qlen_notify)(struct Qdisc *, long unsigned int); /* 32 8 */
> > + void (*qlen_notify)(struct Qdisc *, unsigned long); /* 32 8 */
> > - long unsigned int (*find)(struct Qdisc *, u32); /* 40 8 */
> > + unsigned long (*find)(struct Qdisc *, u32); /* 40 8 */
> > - int (*change)(struct Qdisc *, u32, u32, struct nlattr * *, long unsigned int *, struct netlink_ext_ack *); /* 48 8 */
> > + int (*change)(struct Qdisc *, u32, u32, struct nlattr * *, unsigned long *, struct netlink_ext_ack *); /* 48 8 */
> > - int (*delete)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /* 56 8 */
> > + int (*delete)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /* 56 8 */
> > /* --- cacheline 1 boundary (64 bytes) --- */
> > void (*walk)(struct Qdisc *, struct qdisc_walker *); /* 64 8 */
> > - struct tcf_block * (*tcf_block)(struct Qdisc *, long unsigned int, struct netlink_ext_ack *); /* 72 8 */
> > + struct tcf_block * (*tcf_block)(struct Qdisc *, unsigned long, struct netlink_ext_ack *); /* 72 8 */
> > - long unsigned int (*bind_tcf)(struct Qdisc *, long unsigned int, u32); /* 80 8 */
> > + unsigned long (*bind_tcf)(struct Qdisc *, unsigned long, u32); /* 80 8 */
> > - void (*unbind_tcf)(struct Qdisc *, long unsigned int); /* 88 8 */
> > + void (*unbind_tcf)(struct Qdisc *, unsigned long); /* 88 8 */
> > - int (*dump)(struct Qdisc *, long unsigned int, struct sk_buff *, struct tcmsg *); /* 96 8 */
> > + int (*dump)(struct Qdisc *, unsigned long, struct sk_buff *, struct tcmsg *); /* 96 8 */
> > - int (*dump_stats)(struct Qdisc *, long unsigned int, struct gnet_dump *); /* 104 8 */
> > + int (*dump_stats)(struct Qdisc *, unsigned long, struct gnet_dump *); /* 104 8 */
> >
> > /* size: 112, cachelines: 2, members: 14 */
> > /* sum members: 108, holes: 1, sum holes: 4 */
> > @@ -227,21 +227,21 @@
> >
> > This was affected somehow by these LLVM_annotation patches, I'll try to
> > handle this later.
>
> Are you sure it is related to LLVM_annotation patches?
My bad, was just a hunch, this is not btfdiff, where it uses just one
vmlinux, comparing its DWARF and BTF infos, this is a diff for two
vmlinux produced by different compilers (gcc and clang) for a mostly
equal .config file (modulo the ones that depend on being built by clang,
etc).
So a normalization or names is interesting, but has to be opt in, when
someone wants to do what I did, compare BTF or DWARF from produced by
different compilers, which is useful, like in this case, where I noticed
the problem by using this technique.
I'll queue this up for after 1.25 is produced.
- Arnaldo
> I tried (4d17096 "btf_encoder: Compare functions via prototypes not parameter names")
> and see the same behavior.
>
> Looking at the DWARF, itself gcc and clang use different names for these types:
>
> gcc:
> 0x0000002b: DW_TAG_base_type
> DW_AT_byte_size (0x08)
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_name ("long unsigned int")
>
> clang:
> 0x000000f7: DW_TAG_base_type
> DW_AT_name ("unsigned long")
> DW_AT_encoding (DW_ATE_unsigned)
> DW_AT_byte_size (0x08)
>
> And the base type names are copied to BTF as is. Looks like some
> normalization is necessary either in dwarf_loader.c:base_type__new()
> or in fprintf.
>
> Thanks,
> Eduard
--
- Arnaldo
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-03-31 18:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-30 21:27 [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag Eduard Zingerman
2023-03-31 12:13 ` Arnaldo Carvalho de Melo
2023-03-31 12:14 ` Arnaldo Carvalho de Melo
2023-03-31 12:20 ` Arnaldo Carvalho de Melo
2023-03-31 14:12 ` Eduard Zingerman
2023-03-31 18:33 ` Arnaldo Carvalho de Melo
2023-03-31 13:35 ` Eduard Zingerman
2023-03-31 14:05 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox