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

Arnaldo Carvalho de Melo
2023-03-28T15:28:14Z

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