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

Howard Chu posted 10 patches 1 year, 5 months ago
There is a newer version of this series
[PATCH v2 06/10] perf trace: Pretty print struct data
Posted by Howard Chu 1 year, 5 months ago
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@gmail.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;
+	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));
+
+	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;
+
+	/* 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);
+}
+
 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
Re: [PATCH v2 06/10] perf trace: Pretty print struct data
Posted by Arnaldo Carvalho de Melo 1 year, 5 months ago
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@gmail.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
Re: [PATCH v2 06/10] perf trace: Pretty print struct data
Posted by Arnaldo Carvalho de Melo 1 year, 5 months ago
On Fri, Aug 23, 2024 at 09:41:41AM -0300, Arnaldo Carvalho de Melo wrote:
> 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.

So I did that, need more polishing I have now:

⬢[acme@toolbox perf-tools-next]$ git log --oneline -10
78bb4b917b942f7d (HEAD -> perf-tools-next) perf trace: Pretty print struct data
027ae076fa7bc068 perf trace: Pass the richer 'struct syscall_arg' pointer to trace__btf_scnprintf()
4f2ac4e8a8d9488a perf trace: Add trace__bpf_sys_enter_beauty_map() to prepare for fetching data in BPF
545008dcff9e06f6 perf trace: Collect augmented data using BPF
0b5a34a3cf98843a perf trace: Fix perf trace -p <PID>
23da4ec3640538fa perf evlist: Introduce method to find if there is a bpf-output event
00dc514612fe98cf perf python: Disable -Wno-cast-function-type-mismatch if present on clang
b81162302001f411 perf python: Allow checking for the existence of warning options in clang
1cfd01eb602d73b9 perf annotate-data: Copy back variable types after move
895891dad7353d60 perf annotate-data: Update stack slot for the store
⬢[acme@toolbox perf-tools-next]$

And the comment on the struct pretty printer, added to the patch where
it is introduced.

Committer testing:

After moving the changes to the BPF skel to _before_ this patch, we're
able to test this patch at this point in the series:

  root@number:~# perf trace -e connect ssh localhost
       0.000 ( 0.008 ms): ssh/762249 connect(fd: 3, uservaddr: {2,}, addrlen: 16)                          = 0
       0.014 ( 0.005 ms): ssh/762249 connect(fd: 3, uservaddr: {10,}, addrlen: 28)                         = 0
       0.030 ( 0.032 ms): ssh/762249 connect(fd: 3, uservaddr: {10,}, addrlen: 28)                         = 0
  root@localhost's password:     63.031 ( 0.035 ms): ssh/762249 connect(fd: 4, uservaddr: {1,{['/','v','a','r','/','r','u','n','/','.','h','e','i','m',],},}, addrlen: 110) = 0
      64.037 ( 0.024 ms): ssh/762249 connect(fd: 4, uservaddr: {1,{['/','v','a','r','/','r','u','n','/','.','h','e','i','m',],},}, addrlen: 110) = 0
 
  root@number:~# strace -e connect ssh localhost
  connect(3, {sa_family=AF_INET, sin_port=htons(22), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
  connect(3, {sa_family=AF_INET6, sin6_port=htons(22), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_scope_id=0}, 28) = 0
  connect(3, {sa_family=AF_INET6, sin6_port=htons(22), sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "::1", &sin6_addr), sin6_scope_id=0}, 28) = 0
  connect(4, {sa_family=AF_UNIX, sun_path="/var/run/.heim_org.h5l.kcm-socket"}, 110) = 0
  connect(4, {sa_family=AF_UNIX, sun_path="/var/run/.heim_org.h5l.kcm-socket"}, 110) = 0
  root@localhost's password:

Which while getting the struct contents produces an end result that 
conveys less info than the specilized connect BPF program we have, so we
need to only use this generic BTF dumper approach when we _don't_ have
an specialized one, at least at this point.

In the future we really should get the BTF dumper to use the more
specialized pretty printers that knows about how to pretty print network
specific addresses based on the network family, etc.