diff mbox series

[dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag

Message ID 20230330212700.697124-1-eddyz87@gmail.com (mailing list archive)
State Not Applicable
Delegated to: BPF
Headers show
Series [dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch
bpf/vmtest-bpf-PR success PR summary
bpf/vmtest-bpf-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-VM_Test-8 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-9 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-10 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-VM_Test-11 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-12 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-13 fail Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-14 fail Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-15 fail Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-VM_Test-16 fail Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-17 fail Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-18 fail Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-19 fail Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-20 fail Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-VM_Test-21 fail Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-22 fail Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-VM_Test-32 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-VM_Test-33 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-VM_Test-34 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-VM_Test-35 success Logs for test_verifier on x86_64 with llvm-16

Commit Message

Eduard Zingerman March 30, 2023, 9:27 p.m. UTC 
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(-)

Comments

Arnaldo Carvalho de Melo March 31, 2023, 12:13 p.m. UTC | #1 
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 Carvalho de Melo March 31, 2023, 12:14 p.m. UTC | #2 
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 Carvalho de Melo March 31, 2023, 12:20 p.m. UTC | #3 
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
Eduard Zingerman March 31, 2023, 1:35 p.m. UTC | #4 
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.

> [...]
Arnaldo Carvalho de Melo March 31, 2023, 2:05 p.m. UTC | #5 
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
>
>> [...]
Eduard Zingerman March 31, 2023, 2:12 p.m. UTC | #6 
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
Arnaldo Carvalho de Melo March 31, 2023, 6:33 p.m. UTC | #7 
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
diff mbox series

Patch

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,