Message-ID: <ZsiDhSPs4XYX4VP9@x1>
Date: Fri, 23 Aug 2024 09:41:41 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Howard Chu <howardchu95@...il.com>
Cc: adrian.hunter@...el.com, irogers@...gle.com, jolsa@...nel.org,
	kan.liang@...ux.intel.com, namhyung@...nel.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 06/10] perf trace: Pretty print struct data

On Thu, Aug 15, 2024 at 09:36:22AM +0800, Howard Chu wrote:
> Use btf_dump API to pretty print augmented struct pointer.
> 
> set compact = true and skip_names = true, so that no newline character
> and argument name are printed.
> 
> Signed-off-by: Howard Chu <howardchu95@...il.com>
> ---
>  tools/perf/builtin-trace.c | 51 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 4bde40f91531..e7421128f589 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -1006,6 +1006,55 @@ static size_t btf_enum_scnprintf(const struct btf_type *type, struct btf *btf, c
>  	return 0;
>  }
>  
> +#define DUMPSIZ 1024
> +
> +static void btf_dump_snprintf(void *ctx, const char *fmt, va_list args)
> +{
> +	char *str = ctx, new[DUMPSIZ];
> +
> +	vscnprintf(new, DUMPSIZ, fmt, args);
> +
> +	if (strlen(str) + strlen(new) < DUMPSIZ)
> +		strncat(str, new, DUMPSIZ - strlen(str) - 1);
> +}
> +
> +static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf, char *bf, size_t size, struct syscall_arg *arg, int type_id)
> +{
> +	char str[DUMPSIZ];
> +	int dump_size;
> +	int consumed;


Take a look at this simplification:

struct trace_btf_dump_snprintf_ctx {
        char   *bf;
        size_t printed, size;
};

static void trace__btf_dump_snprintf(void *vctx, const char *fmt, va_list args)
{
        struct trace_btf_dump_snprintf_ctx *ctx = vctx;

        ctx->printed += vscnprintf(ctx->bf + ctx->printed, ctx->size - ctx->printed, fmt, args);
}

static size_t btf_struct_scnprintf(const struct btf_type *type, struct btf *btf, char *bf, size_t size, struct syscall_arg *arg)
{
        struct trace_btf_dump_snprintf_ctx ctx = {
                .bf   = bf,
                .size = size,
        };


I.e. use a context to pass the original buffer and size received by
btf_struct_scnprintf, and go on printing on it instead of using two
extra buffers in btf_struct_scnprintf() and trace__btf_dump_snprintf()
doing more truncation than needed with those series of strlen() calls
before using strncat in trace__btf_dump_snprintf().

Also type id can be obtained from arg->fmt->type_id, so remove that
extra argument, that way we can go on thinking about having an unified
function signature for all btf types (enum and struct at this point in
the series).

> +	struct btf_dump *btf_dump;
> +	struct augmented_arg *augmented_arg = arg->augmented.args;
> +
> +	LIBBPF_OPTS(btf_dump_opts, dump_opts);
> +	LIBBPF_OPTS(btf_dump_type_data_opts, dump_data_opts);
> +
> +	if (arg == NULL || arg->augmented.args == NULL)
> +		return 0;
> +
> +	memset(str, 0, sizeof(str));

We don't need this memset then

> +
> +	dump_data_opts.compact     = true;
> +	dump_data_opts.skip_names  = true;
> +
> +	btf_dump = btf_dump__new(btf, btf_dump_snprintf, str, &dump_opts);
> +	if (btf_dump == NULL)
> +		return 0;

I wonder if we could stop doing this new + free for the btf_dump object
at each btf_struct_scnprintf() call, but I'll leave this for later.

> +	/* pretty print the struct data here */
> +	dump_size = btf_dump__dump_type_data(btf_dump, type_id, arg->augmented.args->value, type->size, &dump_data_opts);
> +	if (dump_size == 0)
> +		return 0;
> +
> +	consumed = sizeof(*augmented_arg) + augmented_arg->size;
> +	arg->augmented.args = ((void *)arg->augmented.args) + consumed;
> +	arg->augmented.size -= consumed;
> +
> +	btf_dump__free(btf_dump);
> +
> +	return scnprintf(bf, size, "%s", str);

Here we return ctx.printed.

I'll get what I have and leave it in that tmp.perf_trace_btf branch.

One other thing I think that the skel patch should come before these,
so that at this point in the series I could _test_ struct printing.

- Arnaldo

> +}
> +
>  static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg_fmt *arg_fmt, char *bf,
>  				   size_t size, int val, struct syscall_arg *arg, char *type)
>  {
> @@ -1023,6 +1072,8 @@ static size_t trace__btf_scnprintf(struct trace *trace, struct syscall_arg_fmt *
>  
>  	if (btf_is_enum(arg_fmt->type))
>  		return btf_enum_scnprintf(arg_fmt->type, trace->btf, bf, size, val);
> +	else if (btf_is_struct(arg_fmt->type))
> +		return btf_struct_scnprintf(arg_fmt->type, trace->btf, bf, size, arg, arg_fmt->type_id);
>  
>  	return 0;
>  }
> -- 
> 2.45.2

Powered by blists - more mailing lists