Re: [PATCH perf-tools-next v3 3/5] perf ftrace: Support display of inlined functions in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] perf: machine__delete() used instead of machine__put() violating refcount API
- [Low] perf: missing explicit POSIX libc headers in tools/perf/tests/ftrace.c
- [Low] perf ftrace: make_blank_prefix clobbers comm-pid column in verbose mode
--

commit 9964df54e91214dc84e330cbac056548e3af8bc4
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support display of inlined functions in function graph tracer

This commit introduces the --inline option to perf ftrace for the function
graph tracer. It utilizes DWARF debug information to dynamically resolve and
synthesize inlined function frames directly into the streamed call graph.

> diff --git a/tools/perf/tests/ftrace.c b/tools/perf/tests/ftrace.c
> index 596cecee844e3..e856bed00700a 100644
> --- a/tools/perf/tests/ftrace.c
> +++ b/tools/perf/tests/ftrace.c
> @@ -7,6 +7,10 @@
>  #include <unistd.h>
>  #include "tests.h"
>  #include "debug.h"
> +#include "dso.h"
> +#include "machine.h"
> +#include "map.h"
> +#include "symbol.h"
>  #include "util/ftrace.h"

[ ... ]

> +static int test_inline_processing(void)
> +{
> +	static const char line_entry[] = " 0)               |  wake_up_new_task() {";

[ ... ]

> +	struct perf_ftrace ftrace;
> +	char *out_buf = NULL;
> +	size_t out_len = 0;
> +	FILE *out;
> +	const char *unmatched_pos;
> +	const char *inline_exit_pos;
> +	int ret = TEST_OK;
> +
> +	memset(&ftrace, 0, sizeof(ftrace));
> +	ftrace.use_inline = true;
> +	ftrace.filter_retaddr = 1; /* Default when --graph-opts retaddr is not specified */
> +	ftrace.tracer = "function_graph";

[ ... ]

> +		out = open_memstream(&out_buf, &out_len);

[Severity: Low]
Is it possible that test_inline_processing() and other functions in this test
file will fail to compile under musl libc?
The perf subsystem guidelines require files using POSIX standard libc functions,
constants, or variables to directly include the corresponding headers. This code
makes heavy use of string/memory functions like memset() and file streams like
open_memstream(), but the file does not include <string.h> or <stdio.h>. Could
these headers be explicitly included to avoid relying on implicit glibc
namespace pollution?

[ ... ]

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> index 344d36293f5ca..8737bb52921e1 100644
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c
> @@ -1,13 +1,82 @@

[ ... ]

> +static void make_blank_prefix(const char *line, const char *pipe_char, char *buf, size_t size)
> +{
> +	size_t len = pipe_char - line + 1;
> +	const char *paren = strchr(line, ')');
> +	char *c;
> +
> +	if (len >= size)
> +		len = size - 1;
> +
> +	memcpy(buf, line, len);
> +	buf[len] = '\0';
> +
> +	/* Replace duration between ')' and '|' with spaces */
> +	if (paren && paren < line + len) {
> +		char *end = buf + (pipe_char - line);
> +
> +		if (end > buf + len)
> +			end = buf + len;
> +		for (c = buf + (paren - line) + 1; c < end; c++)
> +			*c = ' ';
> +	}
> +}

[Severity: Low]
Does make_blank_prefix() inadvertently erase the comm-pid string when in
verbose mode?
In function_graph verbose mode (--graph-opts verbose), the trace output
includes a comm-pid column and a duration column between the CPU number and
the function name. This loop overwrites everything between the closing
parenthesis of the CPU number and the final pipe delimiter before the function
name with spaces. While this correctly blanks out the duration column, it
looks like it might unintentionally clobber the task comm-pid string and its
pipe delimiter as well, breaking visual alignment.

[ ... ]

> +void perf_ftrace__cleanup_inlines(struct perf_ftrace *ftrace)
> +{
> +	if (ftrace->machine) {
> +		machine__delete(ftrace->machine);
> +		ftrace->machine = NULL;
> +	}
> +	ftrace_cpu_states__clear();
> +}

[Severity: Medium]
Should machine__put() be used here instead of machine__delete()?
The machine reference is allocated using machine__new_host() earlier in
perf_ftrace__setup_inlines(). The perf subsystem guidelines mandate strict
reference count checking, stating that every reference handle allocated via
_new() must be strictly paired with a matching _put(). Bypassing the refcount
API by calling machine__delete() directly can lead to memory leaks,
use-after-free traps, and LSAN failures.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012003016.263063-1-atomlin@atomlin.com?part=3