Re: [PATCH dwarves] fprintf: Fix `*` not being printed for pointers with btf_type_tag

[Arnaldo Carvalho de Melo <acme@kernel.org>]

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@gmail.com/T/#md82b04f66867434524beec746138951f26a3977e
> 
> Fixes: e7fb771f2649 ("fprintf: Correct names for types with btf_type_tag attribute")
> Reported-by: Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
> Link: https://lore.kernel.org/dwarves/20230314230417.1507266-1-eddyz87@gmail.com/T/#mc630bcd474ddd64c70d237edd4e0590dc048d63d
> Signed-off-by: Eduard Zingerman <eddyz87@gmail.com>
> ---
>  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?

- 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