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

Eduard Zingerman
2023-03-29T16:05:58Z

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.
>