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

From: Eduard Zingerman <eddyz87@gmail.com>
To: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Cc: dwarves@vger.kernel.org, bpf@vger.kernel.org, kernel-team@fb.com,
	ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
	yhs@fb.com, jose.marchesi@oracle.com, david.faust@oracle.com,
	alan.maguire@oracle.com
Subject: Re: [PATCH dwarves v2 1/5] fprintf: Correct names for types with btf_type_tag attribute
Date: Wed, 29 Mar 2023 19:02:45 +0300	[thread overview]
Message-ID: <f9664121426c5665ff0fc8cb61c466689beadd36.camel@gmail.com> (raw)
In-Reply-To: <ZCRctmB2yrwgsNMh@kernel.org>

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