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

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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