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