Em Fri, Mar 31, 2023 at 09:13:41AM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Fri, Mar 31, 2023 at 12:27:00AM +0300, Eduard Zingerman escreveu:
> > Recent change to fprintf (see below) causes incorrect `type_fprintf()`
> > behavior for pointers annotated with btf_type_tag, for example:
> >
> > $ cat tag-test.c
> > #define __t __attribute__((btf_type_tag("t1")))
> >
> > struct foo {
> > int __t *a;
> > } 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 a; /* 0 8 */
> > ...
> > };
> >
> > Note that `*` is missing in the pahole output.
> > The issue is caused by `goto next_type` that jumps over the
> > `tag__name()` that is responsible for pointer printing.
> >
> > As agreed in [1] `type__fprintf()` is modified to skip type tags for
> > now and would be modified to emit type tags later.
> >
> > The change in `__tag__name()` is necessary to avoid the following behavior:
> >
> > $ cat tag-test.c
> > #define __t __attribute__((btf_type_tag("t1")))
> >
> > struct foo {
> > int __t *a;
> > int __t __t *b;
> > } 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 * a; /* 0 8 */
> > int * b; /* 8 8 */
> > ...
> > };
> >
> > Note the extra space before `*` for field `b`.
> >
> > The issue was reported and tracked down to the root cause by
> > Arnaldo Carvalho de Melo.
> >
> > Links:
> > [1] https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@xxxxxxxxx/T/#md82b04f66867434524beec746138951f26a3977e
> >
> > Fixes: e7fb771f2649 ("fprintf: Correct names for types with btf_type_tag attribute")
> > Reported-by: Arnaldo Carvalho de Melo <arnaldo.melo@xxxxxxxxx>
> > Link: https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@xxxxxxxxx/T/#mc630bcd474ddd64c70d237edd4e0590dc048d63d
> > Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
> > ---
> > dwarves_fprintf.c | 37 +++++++++++++++++++++++++++++--------
> > 1 file changed, 29 insertions(+), 8 deletions(-)
> >
> > diff --git a/dwarves_fprintf.c b/dwarves_fprintf.c
> > index 1e6147a..818db2d 100644
> > --- a/dwarves_fprintf.c
> > +++ b/dwarves_fprintf.c
> > @@ -572,7 +572,6 @@ 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);
> > @@ -618,6 +617,13 @@ static const char *__tag__name(const struct tag *tag, const struct cu *cu,
> > case DW_TAG_variable:
> > snprintf(bf, len, "%s", variable__name(tag__variable(tag)));
> > break;
> > + 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);
> > + else if (!tag__has_type_loop(tag, type, bf, len, NULL))
> > + __tag__name(type, cu, bf, len, conf);
> > + break;
> > default:
> > snprintf(bf, len, "%s%s", tag__prefix(cu, tag->tag, pconf),
> > type__name(tag__type(tag)) ?: "");
> > @@ -677,6 +683,22 @@ static size_t type__fprintf_stats(struct type *type, const struct cu *cu,
> > return printed;
> > }
> >
> > +static type_id_t skip_llvm_annotations(const struct cu *cu, type_id_t id)
> > +{
> > + struct tag *type;
> > +
> > + for (;;) {
> > + if (id == 0)
> > + break;
> > + type = cu__type(cu, id);
> > + if (type == NULL || type->tag != DW_TAG_LLVM_annotation || type->type == id)
> > + break;
> > + id = type->type;
> > + }
> > +
> > + return id;
> > +}
>
> This part I didn't understand, do you see any possibility of a
> DW_TAG_LLVM_annotation pointing to another DW_TAG_LLVM_annotation?
I _think_ its a noop, so will test your patch as-is, thanks!
- Arnaldo
> - Arnaldo
>
> > +
> > static size_t union__fprintf(struct type *type, const struct cu *cu,
> > const struct conf_fprintf *conf, FILE *fp);
> >
> > @@ -778,19 +800,17 @@ inner_struct:
> >
> > next_type:
> > switch (type->tag) {
> > - case DW_TAG_pointer_type:
> > - if (type->type != 0) {
> > + case DW_TAG_pointer_type: {
> > + type_id_t ptype_id = skip_llvm_annotations(cu, type->type);
> > +
> > + if (ptype_id != 0) {
> > int n;
> > - struct tag *ptype = cu__type(cu, type->type);
> > + struct tag *ptype = cu__type(cu, ptype_id);
> > if (ptype == NULL)
> > goto out_type_not_found;
> > 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,
> > @@ -811,6 +831,7 @@ next_type:
> > }
> > }
> > /* Fall Thru */
> > + }
> > default:
> > print_default:
> > printed += fprintf(fp, "%-*s %s", tconf.type_spacing,
> > --
> > 2.40.0
> >
>
> --
>
> - Arnaldo
--
- Arnaldo
