| Message ID | 20230314230417.1507266-2-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | Support for new btf_type_tag encoding | expand |
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'm applying this now to make progress on this front, but longer term we should printf it too, as we want the output to match the original source code as much as possible from the type information. - Arnaldo > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > dwarves_fprintf.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > index e8399e7..1e6147a 100644 > --- a/dwarves_fprintf.c > +++ b/dwarves_fprintf.c > @@ -572,6 +572,7 @@ 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); > @@ -786,6 +787,10 @@ next_type: > 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, > @@ -880,6 +885,14 @@ print_modifier: { > else > printed += enumeration__fprintf(type, &tconf, fp); > break; > + case DW_TAG_LLVM_annotation: { > + struct tag *ttype = cu__type(cu, type->type); > + if (ttype) { > + type = ttype; > + goto next_type; > + } > + goto out_type_not_found; > + } > } > out: > if (type_expanded) > -- > 2.39.1 >
On Mon, 2023-03-27 at 08:46 -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'm applying this now to make progress on this front, but longer term we > should printf it too, as we want the output to match the original source > code as much as possible from the type information. Understood, thank you. Also, I want to give a heads-up about ongoing discussion in: https://reviews.llvm.org/D143967 The gist of the discussion is that for the code like: volatile __tag("foo") int; Kernel expects BTF to be: __tag("foo") -> volatile -> int And I encode it in DWARF as: volatile -> int __tag("foo") But GCC guys argue that DWARF should be like this: volatile -> int __tag("foo") So, to get the BTF to a form acceptable for kernel some additional pahole modifications might be necessary. (I will work on a prototype for such modifications this week). Maybe put this patch-set on-hold until that is resolved? Thanks, Eduard > > - Arnaldo > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > > --- > > dwarves_fprintf.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > > index e8399e7..1e6147a 100644 > > --- a/dwarves_fprintf.c > > +++ b/dwarves_fprintf.c > > @@ -572,6 +572,7 @@ 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); > > @@ -786,6 +787,10 @@ next_type: > > 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, > > @@ -880,6 +885,14 @@ print_modifier: { > > else > > printed += enumeration__fprintf(type, &tconf, fp); > > break; > > + case DW_TAG_LLVM_annotation: { > > + struct tag *ttype = cu__type(cu, type->type); > > + if (ttype) { > > + type = ttype; > > + goto next_type; > > + } > > + goto out_type_not_found; > > + } > > } > > out: > > if (type_expanded) > > -- > > 2.39.1 > > >
On March 27, 2023 9:10:22 AM GMT-03:00, Eduard Zingerman <eddyz87@gmail.com> wrote: >On Mon, 2023-03-27 at 08:46 -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'm applying this now to make progress on this front, but longer term we >> should printf it too, as we want the output to match the original source >> code as much as possible from the type information. > >Understood, thank you. > >Also, I want to give a heads-up about ongoing discussion in: >https://reviews.llvm.org/D143967 > >The gist of the discussion is that for the code like: > > volatile __tag("foo") int; > >Kernel expects BTF to be: > > __tag("foo") -> volatile -> int > >And I encode it in DWARF as: > > volatile -> int > __tag("foo") > >But GCC guys argue that DWARF should be like this: > > volatile -> int > __tag("foo") > >So, to get the BTF to a form acceptable for kernel some additional >pahole modifications might be necessary. (I will work on a prototype >for such modifications this week). > >Maybe put this patch-set on-hold until that is resolved? Ok, will read the discussion and wait, - Arnaldo > >Thanks, >Eduard > >> >> - Arnaldo >> >> > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> >> > --- >> > dwarves_fprintf.c | 13 +++++++++++++ >> > 1 file changed, 13 insertions(+) >> > >> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c >> > index e8399e7..1e6147a 100644 >> > --- a/dwarves_fprintf.c >> > +++ b/dwarves_fprintf.c >> > @@ -572,6 +572,7 @@ 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); >> > @@ -786,6 +787,10 @@ next_type: >> > 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, >> > @@ -880,6 +885,14 @@ print_modifier: { >> > else >> > printed += enumeration__fprintf(type, &tconf, fp); >> > break; >> > + case DW_TAG_LLVM_annotation: { >> > + struct tag *ttype = cu__type(cu, type->type); >> > + if (ttype) { >> > + type = ttype; >> > + goto next_type; >> > + } >> > + goto out_type_not_found; >> > + } >> > } >> > out: >> > if (type_expanded) >> > -- >> > 2.39.1 >> > >> >
Em Mon, Mar 27, 2023 at 03:10:22PM +0300, Eduard Zingerman escreveu: > On Mon, 2023-03-27 at 08:46 -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'm applying this now to make progress on this front, but longer term we > > should printf it too, as we want the output to match the original source > > code as much as possible from the type information. > > Understood, thank you. > > Also, I want to give a heads-up about ongoing discussion in: > https://reviews.llvm.org/D143967 > > The gist of the discussion is that for the code like: > > volatile __tag("foo") int; > > Kernel expects BTF to be: > > __tag("foo") -> volatile -> int > > And I encode it in DWARF as: > > volatile -> int > __tag("foo") > > But GCC guys argue that DWARF should be like this: > > volatile -> int > __tag("foo") > > So, to get the BTF to a form acceptable for kernel some additional > pahole modifications might be necessary. (I will work on a prototype > for such modifications this week). > > Maybe put this patch-set on-hold until that is resolved? Ok, so I'll apply just the first two, to get btfdiff a down to those zero sized arrays when processing clang generated DWARF for a recent kernel, see below. Ok? - A rnaldo ⬢[acme@toolbox pahole]$ git log --oneline -5 b43651673676c1dc (HEAD -> master) btf_loader: A hack for BTF import of btf_type_tag attributes e7fb771f2649fc1b fprintf: Correct names for types with btf_type_tag attribute 4d17096076b2351f (quaco/master, quaco/HEAD, github/tmp.master, github/next, acme/tmp.master, acme/next) btf_encoder: Compare functions via prototypes not parameter names 82730394195276ac fprintf: Support skipping modifier d184aaa125ea40ff fprintf: Generalize function prototype print to support passing conf ⬢[acme@toolbox pahole]$ ⬢[acme@toolbox pahole]$ btfdiff ../vmlinux-clang-pahole-1.25+rust die__process_class: tag not supported 0x2f (template_type_parameter)! die__process_class: tag not supported 0x33 (variant_part)! --- /tmp/btfdiff.dwarf.EiDOTz 2023-03-28 09:38:09.283942846 -0300 +++ /tmp/btfdiff.btf.rWM9v6 2023-03-28 09:38:09.624952028 -0300 @@ -14496,7 +14496,7 @@ struct bpf_cand_cache { struct { const struct btf * btf; /* 16 8 */ u32 id; /* 24 4 */ - } cands[0]; /* 16 0 */ + } cands[]; /* 16 0 */ /* size: 16, cachelines: 1, members: 5 */ /* last cacheline: 16 bytes */ @@ -18310,7 +18310,7 @@ struct btf_id_set8 { struct { u32 id; /* 8 4 */ u32 flags; /* 12 4 */ - } pairs[0]; /* 8 0 */ + } pairs[]; /* 8 0 */ /* size: 8, cachelines: 1, members: 3 */ /* last cacheline: 8 bytes */ @@ -27765,7 +27765,7 @@ struct cpu_rmap { struct { u16 index; /* 16 2 */ u16 dist; /* 18 2 */ - } near[0]; /* 16 0 */ + } near[]; /* 16 0 */ /* size: 16, cachelines: 1, members: 5 */ /* last cacheline: 16 bytes */ @@ -73931,7 +73931,7 @@ struct linux_efi_memreserve { struct { phys_addr_t base; /* 16 8 */ phys_addr_t size; /* 24 8 */ - } entry[0]; /* 16 0 */ + } entry[]; /* 16 0 */ /* size: 16, cachelines: 1, members: 4 */ /* last cacheline: 16 bytes */ @@ -84345,7 +84345,7 @@ struct netlink_policy_dump_state { struct { const struct nla_policy * policy; /* 16 8 */ unsigned int maxtype; /* 24 4 */ - } policies[0]; /* 16 0 */ + } policies[]; /* 16 0 */ /* size: 16, cachelines: 1, members: 4 */ /* sum members: 12, holes: 1, sum holes: 4 */ @@ -139178,7 +139178,7 @@ struct uv_rtc_timer_head { /* XXX 4 bytes hole, try to pack */ u64 expires; /* 24 8 */ - } cpu[0]; /* 16 0 */ + } cpu[]; /* 16 0 */ /* size: 16, cachelines: 1, members: 4 */ /* sum members: 12, holes: 1, sum holes: 4 */ ⬢[acme@toolbox pahole]$
On Tue, 2023-03-28 at 09:40 -0300, Arnaldo Carvalho de Melo wrote: [...] > > Maybe put this patch-set on-hold until that is resolved? > > Ok, so I'll apply just the first two, to get btfdiff a down to those > zero sized arrays when processing clang generated DWARF for a recent > kernel, see below. > > Ok? Sure, thank you! [...]
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. - Arnaldo > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > --- > dwarves_fprintf.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > index e8399e7..1e6147a 100644 > --- a/dwarves_fprintf.c > +++ b/dwarves_fprintf.c > @@ -572,6 +572,7 @@ 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); > @@ -786,6 +787,10 @@ next_type: > 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, > @@ -880,6 +885,14 @@ print_modifier: { > else > printed += enumeration__fprintf(type, &tconf, fp); > break; > + case DW_TAG_LLVM_annotation: { > + struct tag *ttype = cu__type(cu, type->type); > + if (ttype) { > + type = ttype; > + goto next_type; > + } > + goto out_type_not_found; > + } > } > out: > if (type_expanded) > -- > 2.39.1 >
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. > > - Arnaldo > > > > Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> > > --- > > dwarves_fprintf.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > > index e8399e7..1e6147a 100644 > > --- a/dwarves_fprintf.c > > +++ b/dwarves_fprintf.c > > @@ -572,6 +572,7 @@ 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); > > @@ -786,6 +787,10 @@ next_type: > > 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, > > @@ -880,6 +885,14 @@ print_modifier: { > > else > > printed += enumeration__fprintf(type, &tconf, fp); > > break; > > + case DW_TAG_LLVM_annotation: { > > + struct tag *ttype = cu__type(cu, type->type); > > + if (ttype) { > > + type = ttype; > > + goto next_type; > > + } > > + goto out_type_not_found; > > + } > > } > > out: > > if (type_expanded) > > -- > > 2.39.1 > > >
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
On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote: [...] > 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. Yep, thanks a lot for the analysis, I will take a look. Hopefully will send v2 tomorrow. > > - Arnaldo
Em Tue, Mar 28, 2023 at 06:30:05PM +0300, Eduard Zingerman escreveu: > On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote: > [...] > > 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. > > Yep, thanks a lot for the analysis, I will take a look. > Hopefully will send v2 tomorrow. So, with the patch below it gets equivalent, but some more tweaking is needed to make the output completely match (spaces, "long usigned" -> "unsigned long", which seems to be all equivalent): --- /tmp/gcc 2023-03-28 18:13:42.022385428 -0300 +++ /tmp/clang 2023-03-28 18:13:45.854486885 -0300 @@ -73,15 +73,15 @@ 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 */ @@ -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 */ diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c index 1e6147a82056c188..1ecc24321bf8f975 100644 --- a/dwarves_fprintf.c +++ b/dwarves_fprintf.c @@ -788,8 +788,15 @@ next_type: if (n) return printed + n; if (ptype->tag == DW_TAG_LLVM_annotation) { - type = ptype; - goto next_type; + // FIXME: Just skip this for now, we need to print this annotation + // to match the original source code. + + if (ptype->type == 0) { + printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name); + break; + } + + ptype = cu__type(cu, ptype->type); } if (ptype->tag == DW_TAG_subroutine_type) { printed += ftype__fprintf(tag__ftype(ptype),
On Tue, 2023-03-28 at 18:17 -0300, Arnaldo Carvalho de Melo wrote: > Em Tue, Mar 28, 2023 at 06:30:05PM +0300, Eduard Zingerman escreveu: > > On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote: > > [...] > > > 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. > > > > Yep, thanks a lot for the analysis, I will take a look. > > Hopefully will send v2 tomorrow. > > So, with the patch below it gets equivalent, but some more tweaking is > needed to make the output completely match (spaces, "long usigned" -> > "unsigned long", which seems to be all equivalent): > > --- /tmp/gcc 2023-03-28 18:13:42.022385428 -0300 > +++ /tmp/clang 2023-03-28 18:13:45.854486885 -0300 > @@ -73,15 +73,15 @@ > 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 */ > > @@ -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 */ > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > index 1e6147a82056c188..1ecc24321bf8f975 100644 > --- a/dwarves_fprintf.c > +++ b/dwarves_fprintf.c > @@ -788,8 +788,15 @@ next_type: > if (n) > return printed + n; > if (ptype->tag == DW_TAG_LLVM_annotation) { > - type = ptype; > - goto next_type; > + // FIXME: Just skip this for now, we need to print this annotation > + // to match the original source code. > + > + if (ptype->type == 0) { > + printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name); > + break; > + } > + > + ptype = cu__type(cu, ptype->type); > } > if (ptype->tag == DW_TAG_subroutine_type) { > printed += ftype__fprintf(tag__ftype(ptype), This explains why '*' was missing, but unfortunately it breaks apart when there are multiple type tags, e.g.: $ cat tag-test.c #define __t __attribute__((btf_type_tag("t1"))) struct foo { int (__t __t *a)(void); } 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 ()(void) * a; /* 0 8 */ /* size: 8, cachelines: 1, members: 1 */ /* last cacheline: 8 bytes */ }; $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o struct foo { int ()(void) * a; /* 0 8 */ /* size: 8, cachelines: 1, members: 1 */ /* last cacheline: 8 bytes */ }; What I don't understand is why pointer's type is LLVM_annotation. Probably, the cleanest solution would be to make DWARF and BTF loaders work in a similar way and attach LLVM_annotation as `annots` field of the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's in the middle of type chains. I'll work on this.
Em Wed, Mar 29, 2023 at 06:36:34PM +0300, Eduard Zingerman escreveu: > On Tue, 2023-03-28 at 18:17 -0300, Arnaldo Carvalho de Melo wrote: > > Em Tue, Mar 28, 2023 at 06:30:05PM +0300, Eduard Zingerman escreveu: > > > On Tue, 2023-03-28 at 12:26 -0300, Arnaldo Carvalho de Melo wrote: > > > [...] > > > > 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. > > > > > > Yep, thanks a lot for the analysis, I will take a look. > > > Hopefully will send v2 tomorrow. > > > > So, with the patch below it gets equivalent, but some more tweaking is > > needed to make the output completely match (spaces, "long usigned" -> > > "unsigned long", which seems to be all equivalent): > > > > --- /tmp/gcc 2023-03-28 18:13:42.022385428 -0300 > > +++ /tmp/clang 2023-03-28 18:13:45.854486885 -0300 > > @@ -73,15 +73,15 @@ > > 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 */ > > > > @@ -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 */ > > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > > index 1e6147a82056c188..1ecc24321bf8f975 100644 > > --- a/dwarves_fprintf.c > > +++ b/dwarves_fprintf.c > > @@ -788,8 +788,15 @@ next_type: > > if (n) > > return printed + n; > > if (ptype->tag == DW_TAG_LLVM_annotation) { > > - type = ptype; > > - goto next_type; > > + // FIXME: Just skip this for now, we need to print this annotation > > + // to match the original source code. > > + > > + if (ptype->type == 0) { > > + printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name); > > + break; > > + } > > + > > + ptype = cu__type(cu, ptype->type); > > } > > if (ptype->tag == DW_TAG_subroutine_type) { > > printed += ftype__fprintf(tag__ftype(ptype), > > This explains why '*' was missing, but unfortunately it breaks apart > when there are multiple type tags, e.g.: > > > $ cat tag-test.c > #define __t __attribute__((btf_type_tag("t1"))) > > struct foo { > int (__t __t *a)(void); > } 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 ()(void) * a; /* 0 8 */ > > /* size: 8, cachelines: 1, members: 1 */ > /* last cacheline: 8 bytes */ > }; > $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o > struct foo { > int ()(void) * a; /* 0 8 */ > > /* size: 8, cachelines: 1, members: 1 */ > /* last cacheline: 8 bytes */ > }; > > What I don't understand is why pointer's type is LLVM_annotation. Well, that is how it is encoded in BTF and then you supported it in: Author: Eduard Zingerman <eddyz87@gmail.com> Date: Wed Mar 15 01:04:14 2023 +0200 btf_loader: A hack for BTF import of btf_type_tag attributes` I find it natural, and think that annots thing is a variation on how to store modifiers for types, see, this DW_TAG_LLVM_annotation is in the same class as 'restrict', 'const', 'volatile', "atomic", etc I understand that for encoding _DWARF_ people preferred to make it as a child DIE to avoid breaking existing DWARF consumers, but in pahole's dwarf_loader.c we can just make it work like BTF and insert the btf_type_tag in the chain, just like 'const', etc, no? pahole wants to print those annotation just like it prints 'const', 'volatile', etc. > Probably, the cleanest solution would be to make DWARF and BTF loaders > work in a similar way and attach LLVM_annotation as `annots` field of > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's > in the middle of type chains. I'll work on this.
On Wed, 2023-03-29 at 12:43 -0300, Arnaldo Carvalho de Melo wrote: [...] > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > > > index 1e6147a82056c188..1ecc24321bf8f975 100644 > > > --- a/dwarves_fprintf.c > > > +++ b/dwarves_fprintf.c > > > @@ -788,8 +788,15 @@ next_type: > > > if (n) > > > return printed + n; > > > if (ptype->tag == DW_TAG_LLVM_annotation) { > > > - type = ptype; > > > - goto next_type; > > > + // FIXME: Just skip this for now, we need to print this annotation > > > + // to match the original source code. > > > + > > > + if (ptype->type == 0) { > > > + printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name); > > > + break; > > > + } > > > + > > > + ptype = cu__type(cu, ptype->type); > > > } > > > if (ptype->tag == DW_TAG_subroutine_type) { > > > printed += ftype__fprintf(tag__ftype(ptype), > > > > This explains why '*' was missing, but unfortunately it breaks apart > > when there are multiple type tags, e.g.: > > > > > > $ cat tag-test.c > > #define __t __attribute__((btf_type_tag("t1"))) > > > > struct foo { > > int (__t __t *a)(void); > > } 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 ()(void) * a; /* 0 8 */ > > > > /* size: 8, cachelines: 1, members: 1 */ > > /* last cacheline: 8 bytes */ > > }; > > $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o > > struct foo { > > int ()(void) * a; /* 0 8 */ > > > > /* size: 8, cachelines: 1, members: 1 */ > > /* last cacheline: 8 bytes */ > > }; > > > > What I don't understand is why pointer's type is LLVM_annotation. > > Well, that is how it is encoded in BTF and then you supported it in: > > Author: Eduard Zingerman <eddyz87@gmail.com> > Date: Wed Mar 15 01:04:14 2023 +0200 > > btf_loader: A hack for BTF import of btf_type_tag attributes` To be honest, I was under impression that I add a workaround and the preferred way is to do what dwarf loader does with btf_type_tag_ptr_type::annots. > I find it natural, and think that annots thing is a variation on how to > store modifiers for types, see, this DW_TAG_LLVM_annotation is in the > same class as 'restrict', 'const', 'volatile', "atomic", etc > > I understand that for encoding _DWARF_ people preferred to make it as a > child DIE to avoid breaking existing DWARF consumers, but in > pahole's dwarf_loader.c we can just make it work like BTF and insert the > btf_type_tag in the chain, just like 'const', etc, no? > > pahole wants to print those annotation just like it prints 'const', > 'volatile', etc. Actually, if reflecting physical structure of the DWARF is not a goal, forgoing "annots" fields altogether and treating type tags as derived types should make support for btf:type_tag (the v2 version) simpler. Then, getting back to the current issue, I need to add skip_llvm_annotations function with a loop inside, right? > > Probably, the cleanest solution would be to make DWARF and BTF loaders > > work in a similar way and attach LLVM_annotation as `annots` field of > > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's > > in the middle of type chains. I'll work on this. >
Em Wed, Mar 29, 2023 at 07:02:45PM +0300, Eduard Zingerman escreveu: > On Wed, 2023-03-29 at 12:43 -0300, Arnaldo Carvalho de Melo wrote: > [...] > > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > > > > index 1e6147a82056c188..1ecc24321bf8f975 100644 > > > > --- a/dwarves_fprintf.c > > > > +++ b/dwarves_fprintf.c > > > > @@ -788,8 +788,15 @@ next_type: > > > > if (n) > > > > return printed + n; > > > > if (ptype->tag == DW_TAG_LLVM_annotation) { > > > > - type = ptype; > > > > - goto next_type; > > > > + // FIXME: Just skip this for now, we need to print this annotation > > > > + // to match the original source code. > > > > + > > > > + if (ptype->type == 0) { > > > > + printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name); > > > > + break; > > > > + } > > > > + > > > > + ptype = cu__type(cu, ptype->type); > > > > } > > > > if (ptype->tag == DW_TAG_subroutine_type) { > > > > printed += ftype__fprintf(tag__ftype(ptype), > > > > > > This explains why '*' was missing, but unfortunately it breaks apart > > > when there are multiple type tags, e.g.: > > > > > > > > > $ cat tag-test.c > > > #define __t __attribute__((btf_type_tag("t1"))) > > > > > > struct foo { > > > int (__t __t *a)(void); > > > } 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 ()(void) * a; /* 0 8 */ > > > > > > /* size: 8, cachelines: 1, members: 1 */ > > > /* last cacheline: 8 bytes */ > > > }; > > > $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o > > > struct foo { > > > int ()(void) * a; /* 0 8 */ > > > > > > /* size: 8, cachelines: 1, members: 1 */ > > > /* last cacheline: 8 bytes */ > > > }; > > > > > > What I don't understand is why pointer's type is LLVM_annotation. > > > > Well, that is how it is encoded in BTF and then you supported it in: > > > > Author: Eduard Zingerman <eddyz87@gmail.com> > > Date: Wed Mar 15 01:04:14 2023 +0200 > > > > btf_loader: A hack for BTF import of btf_type_tag attributes` > > To be honest, I was under impression that I add a workaround and the > preferred way is to do what dwarf loader does with > btf_type_tag_ptr_type::annots. > > > I find it natural, and think that annots thing is a variation on how to > > store modifiers for types, see, this DW_TAG_LLVM_annotation is in the > > same class as 'restrict', 'const', 'volatile', "atomic", etc > > > > I understand that for encoding _DWARF_ people preferred to make it as a > > child DIE to avoid breaking existing DWARF consumers, but in > > pahole's dwarf_loader.c we can just make it work like BTF and insert the > > btf_type_tag in the chain, just like 'const', etc, no? > > > > pahole wants to print those annotation just like it prints 'const', > > 'volatile', etc. > > Actually, if reflecting physical structure of the DWARF is not a goal, Well reflecting the physical structure of DWARF _pre_ DW_TAG_llvm_annotation was the goal, but now, since this was done differently of DW_TAG_const_type, DW_TAG_pointer_type, etc, as one link in the chain, to avoid breaking existing DWARF consumers, we ended up with this annot thing, but the internal representation in pahole can continue being as a chain, as BTF does, right? > forgoing "annots" fields altogether and treating type tags as derived > types should make support for btf:type_tag (the v2 version) simpler. I think it should simplify as we're used to these chains. > Then, getting back to the current issue, I need to add > skip_llvm_annotations function with a loop inside, right? You can, to remove them completely and its like they don't exist for dwarf_fprintf.c, but what I think should be done is to actually print them, to reconstruct the original source code. You can start removing them and we can work later on properly printing it, so that we can get 1.25 out of the door as there are multiple requests for it to be released to solve other problems with using 1.24. - Arnaldo > > > Probably, the cleanest solution would be to make DWARF and BTF loaders > > > work in a similar way and attach LLVM_annotation as `annots` field of > > > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's > > > in the middle of type chains. I'll work on this.
On Thu, 2023-03-30 at 08:29 -0300, Arnaldo Carvalho de Melo wrote: > Em Wed, Mar 29, 2023 at 07:02:45PM +0300, Eduard Zingerman escreveu: > > On Wed, 2023-03-29 at 12:43 -0300, Arnaldo Carvalho de Melo wrote: > > [...] > > > > > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c > > > > > index 1e6147a82056c188..1ecc24321bf8f975 100644 > > > > > --- a/dwarves_fprintf.c > > > > > +++ b/dwarves_fprintf.c > > > > > @@ -788,8 +788,15 @@ next_type: > > > > > if (n) > > > > > return printed + n; > > > > > if (ptype->tag == DW_TAG_LLVM_annotation) { > > > > > - type = ptype; > > > > > - goto next_type; > > > > > + // FIXME: Just skip this for now, we need to print this annotation > > > > > + // to match the original source code. > > > > > + > > > > > + if (ptype->type == 0) { > > > > > + printed += fprintf(fp, "%-*s %s", tconf.type_spacing, "void *", name); > > > > > + break; > > > > > + } > > > > > + > > > > > + ptype = cu__type(cu, ptype->type); > > > > > } > > > > > if (ptype->tag == DW_TAG_subroutine_type) { > > > > > printed += ftype__fprintf(tag__ftype(ptype), > > > > > > > > This explains why '*' was missing, but unfortunately it breaks apart > > > > when there are multiple type tags, e.g.: > > > > > > > > > > > > $ cat tag-test.c > > > > #define __t __attribute__((btf_type_tag("t1"))) > > > > > > > > struct foo { > > > > int (__t __t *a)(void); > > > > } 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 ()(void) * a; /* 0 8 */ > > > > > > > > /* size: 8, cachelines: 1, members: 1 */ > > > > /* last cacheline: 8 bytes */ > > > > }; > > > > $ clang -g -c tag-test.c -o tag-test.o && pahole -J tag-test.o && pahole --sort -F btf tag-test.o > > > > struct foo { > > > > int ()(void) * a; /* 0 8 */ > > > > > > > > /* size: 8, cachelines: 1, members: 1 */ > > > > /* last cacheline: 8 bytes */ > > > > }; > > > > > > > > What I don't understand is why pointer's type is LLVM_annotation. > > > > > > Well, that is how it is encoded in BTF and then you supported it in: > > > > > > Author: Eduard Zingerman <eddyz87@gmail.com> > > > Date: Wed Mar 15 01:04:14 2023 +0200 > > > > > > btf_loader: A hack for BTF import of btf_type_tag attributes` > > > > To be honest, I was under impression that I add a workaround and the > > preferred way is to do what dwarf loader does with > > btf_type_tag_ptr_type::annots. > > > > > I find it natural, and think that annots thing is a variation on how to > > > store modifiers for types, see, this DW_TAG_LLVM_annotation is in the > > > same class as 'restrict', 'const', 'volatile', "atomic", etc > > > > > > I understand that for encoding _DWARF_ people preferred to make it as a > > > child DIE to avoid breaking existing DWARF consumers, but in > > > pahole's dwarf_loader.c we can just make it work like BTF and insert the > > > btf_type_tag in the chain, just like 'const', etc, no? > > > > > > pahole wants to print those annotation just like it prints 'const', > > > 'volatile', etc. > > > > Actually, if reflecting physical structure of the DWARF is not a goal, > > Well reflecting the physical structure of DWARF _pre_ > DW_TAG_llvm_annotation was the goal, but now, since this was done > differently of DW_TAG_const_type, DW_TAG_pointer_type, etc, as one link > in the chain, to avoid breaking existing DWARF consumers, we ended up > with this annot thing, but the internal representation in pahole can > continue being as a chain, as BTF does, right? Ok, I'll rework the patch-set and we'll see, my expectation is that working with BTF style structure would be simpler. > > forgoing "annots" fields altogether and treating type tags as derived > > types should make support for btf:type_tag (the v2 version) simpler. > > I think it should simplify as we're used to these chains. > > > Then, getting back to the current issue, I need to add > > skip_llvm_annotations function with a loop inside, right? > > You can, to remove them completely and its like they don't exist for > dwarf_fprintf.c, but what I think should be done is to actually print > them, to reconstruct the original source code. > > You can start removing them and we can work later on properly printing > it, so that we can get 1.25 out of the door as there are multiple > requests for it to be released to solve other problems with using 1.24. Understood, I'll send an update that ignores type tags properly today, after some testing, and add printing as a follow-up. > > - Arnaldo > > > > > Probably, the cleanest solution would be to make DWARF and BTF loaders > > > > work in a similar way and attach LLVM_annotation as `annots` field of > > > > the `struct btf_type_tag_ptr_type`. Thus, avoiding 'LLVM_annotation's > > > > in the middle of type chains. I'll work on this.
diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c index e8399e7..1e6147a 100644 --- a/dwarves_fprintf.c +++ b/dwarves_fprintf.c @@ -572,6 +572,7 @@ 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); @@ -786,6 +787,10 @@ next_type: 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, @@ -880,6 +885,14 @@ print_modifier: { else printed += enumeration__fprintf(type, &tconf, fp); break; + case DW_TAG_LLVM_annotation: { + struct tag *ttype = cu__type(cu, type->type); + if (ttype) { + type = ttype; + goto next_type; + } + goto out_type_not_found; + } } out: if (type_expanded)
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. Signed-off-by: Eduard Zingerman <eddyz87@gmail.com> --- dwarves_fprintf.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)