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

From: Arnaldo Carvalho de Melo <acme@kernel.org>
To: Eduard Zingerman <eddyz87@gmail.com>
Cc: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>,
	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: Thu, 30 Mar 2023 08:29:04 -0300	[thread overview]
Message-ID: <ZCVygOn0+zKFEqW2@kernel.org> (raw)
In-Reply-To: <f9664121426c5665ff0fc8cb61c466689beadd36.camel@gmail.com>

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.