Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute

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

Em Tue, Mar 28, 2023 at 05:08:48PM +0300, Eduard Zingerman escreveu:
> On Tue, 2023-03-28 at 10:59 -0300, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Mar 15, 2023 at 01:04:13AM +0200, Eduard Zingerman escreveu:
> > > The following example contains a structure field annotated with
> > > btf_type_tag attribute:
> > > 
> > >     #define __tag1 __attribute__((btf_type_tag("tag1")))
> > > 
> > >     struct st {
> > >       int __tag1 *a;
> > >     } g;
> > > 
> > > It is not printed correctly by `pahole -F dwarf` command:
> > > 
> > >     $ clang -g -c test.c -o test.o
> > >     pahole -F dwarf test.o
> > >     struct st {
> > >     	tag1 *                     a;                    /*     0     8 */
> > >     	...
> > >     };
> > > 
> > > Note the type for variable `a`: `tag1` is printed instead of `int`.
> > > This commit teaches `type__fprintf()` and `__tag_name()` logic to skip
> > > `DW_TAG_LLVM_annotation` objects that are used to encode type tags.
> > 
> > I noticed this:
> > 
> > ⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-clang-pahole-1.25+rust > /tmp/clang
> > ⬢[acme@toolbox pahole]$ pahole --sort -F btf ../vmlinux-gcc-pahole-1.25+rust > /tmp/gcc
> > 
> > 
> > --- /tmp/gcc    2023-03-28 10:55:37.075999474 -0300
> > +++ /tmp/clang  2023-03-28 10:55:53.324436319 -0300
> > @@ -70,21 +70,21 @@
> >  struct Qdisc {
> >         int                        (*enqueue)(struct sk_buff *, struct Qdisc *, struct sk_buff * *); /*     0     8 */
> >         struct sk_buff *           (*dequeue)(struct Qdisc *); /*     8     8 */
> >         unsigned int               flags;                /*    16     4 */
> >         u32                        limit;                /*    20     4 */
> >         const struct Qdisc_ops  *  ops;                  /*    24     8 */
> > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > +       struct qdisc_size_table    stab;                 /*    32     8 */
> >         struct hlist_node          hash;                 /*    40    16 */
> >         u32                        handle;               /*    56     4 */
> >         u32                        parent;               /*    60     4 */
> >         /* --- cacheline 1 boundary (64 bytes) --- */
> >         struct netdev_queue *      dev_queue;            /*    64     8 */
> > -       struct net_rate_estimator * rate_est;            /*    72     8 */
> > -       struct gnet_stats_basic_sync * cpu_bstats;       /*    80     8 */
> > -       struct gnet_stats_queue *  cpu_qstats;           /*    88     8 */
> > +       struct net_rate_estimator  rate_est;             /*    72     8 */
> > +       struct gnet_stats_basic_sync cpu_bstats;         /*    80     8 */
> > +       struct gnet_stats_queue    cpu_qstats;           /*    88     8 */
> >         int                        pad;                  /*    96     4 */
> >         refcount_t                 refcnt;               /*   100     4 */
> > 
> >         /* XXX 24 bytes hole, try to pack */
> > 
> >         /* --- cacheline 2 boundary (128 bytes) --- */
> > 
> > And:
> > 
> > struct Qdisc {
> >         int                     (*enqueue)(struct sk_buff *skb,
> >                                            struct Qdisc *sch,
> >                                            struct sk_buff **to_free);
> >         struct sk_buff *        (*dequeue)(struct Qdisc *sch);
> >         unsigned int            flags;
> > #define TCQ_F_BUILTIN           1
> > #define TCQ_F_INGRESS           2
> > #define TCQ_F_CAN_BYPASS        4
> > #define TCQ_F_MQROOT            8
> > #define TCQ_F_ONETXQUEUE        0x10 /* dequeue_skb() can assume all skbs are for
> >                                       * q->dev_queue : It can test
> >                                       * netif_xmit_frozen_or_stopped() before
> >                                       * dequeueing next packet.
> >                                       * Its true for MQ/MQPRIO slaves, or non
> >                                       * multiqueue device.
> >                                       */
> > #define TCQ_F_WARN_NONWC        (1 << 16)
> > #define TCQ_F_CPUSTATS          0x20 /* run using percpu statistics */
> > #define TCQ_F_NOPARENT          0x40 /* root of its hierarchy :
> >                                       * qdisc_tree_decrease_qlen() should stop.
> >                                       */
> > #define TCQ_F_INVISIBLE         0x80 /* invisible by default in dump */
> > #define TCQ_F_NOLOCK            0x100 /* qdisc does not require locking */
> > #define TCQ_F_OFFLOADED         0x200 /* qdisc is offloaded to HW */
> >         u32                     limit;
> >         const struct Qdisc_ops  *ops;
> >         struct qdisc_size_table __rcu *stab;
> >         struct hlist_node       hash;
> >         u32                     handle;
> >         u32                     parent;
> > 
> >         struct netdev_queue     *dev_queue;
> > 
> >         struct net_rate_estimator __rcu *rate_est;
> >         struct gnet_stats_basic_sync __percpu *cpu_bstats;
> >         struct gnet_stats_queue __percpu *cpu_qstats;
> >         int                     pad;
> >         refcount_t              refcnt;
> > 
> > 
> > I'll try to fix now.
> 
> Ouch. The fields are annotated with type tags, which are ignored by gcc.
> If this is not urgent I can debug it either later today or tomorrow.

Sure, but look:

> > -       struct qdisc_size_table *  stab;                 /*    32     8 */
> > +       struct qdisc_size_table    stab;                 /*    32     8 */

Its the DW_TAG_pointer_type that is getting lost somehow:

 <1><b0af32>: Abbrev Number: 81 (DW_TAG_structure_type)
    <b0af33>   DW_AT_name        : (indirect string, offset: 0xe825): Qdisc
    <b0af37>   DW_AT_byte_size   : 384
    <b0af39>   DW_AT_decl_file   : 223
    <b0af3a>   DW_AT_decl_line   : 72

<SNIP>

 <2><b0af77>: Abbrev Number: 65 (DW_TAG_member)
    <b0af78>   DW_AT_name        : (indirect string, offset: 0x4745ff): stab
    <b0af7c>   DW_AT_type        : <0xb0b76b>
    <b0af80>   DW_AT_decl_file   : 223
    <b0af81>   DW_AT_decl_line   : 99
    <b0af82>   DW_AT_data_member_location: 32

<SNIP>

<1><b0b76b>: Abbrev Number: 61 (DW_TAG_pointer_type)
    <b0b76c>   DW_AT_type        : <0xb0b77a>
 <2><b0b770>: Abbrev Number: 62 (User TAG value: 0x6000)
    <b0b771>   DW_AT_name        : (indirect string, offset: 0x378): btf_type_tag
    <b0b775>   DW_AT_const_value : (indirect string, offset: 0x6e93): rcu
 <2><b0b779>: Abbrev Number: 0
 <1><b0b77a>: Abbrev Number: 69 (DW_TAG_structure_type)
    <b0b77b>   DW_AT_name        : (indirect string, offset: 0x6e5ed): qdisc_size_table
    <b0b77f>   DW_AT_byte_size   : 64
    <b0b780>   DW_AT_decl_file   : 223
    <b0b781>   DW_AT_decl_line   : 56

 
So it's all there in the DWARF info:

   b0af77 has type 0xb0b76b which is a DW_TAG_pointer_type that has type
   0xb0b77a, that is DW_TAG_structure_type.

Now lets see how this was encoded into BTF:

[4725] STRUCT 'Qdisc' size=384 vlen=28
<SNIP>
        'stab' type_id=4790 bits_offset=256
<SNIP>
[4790] PTR '(anon)' type_id=4789
<SNIP>
[4789] TYPE_TAG 'rcu' type_id=4791
<SNIP>
[4791] STRUCT 'qdisc_size_table' size=64 vlen=5
        'rcu' type_id=320 bits_offset=0
        'list' type_id=87 bits_offset=128
        'szopts' type_id=4792 bits_offset=256
        'refcnt' type_id=16 bits_offset=448
        'data' type_id=4659 bits_offset=480

So it all seems ok, we should keep all the info and teach fprintf to
handle TYPE_TAG.

Which you tried, but somehow the '*' link is missing.

- Arnaldo