Re: [PATCH v2 06/10] perf trace: Pretty print struct data

From: Arnaldo Carvalho de Melo
Date: Fri Aug 23 2024 - 08:41:52 EST


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