Re: [PATCH perf-tools-next v8 2/6] perf ftrace: Support filtering return address comments

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf ftrace: ftrace_filter_retaddr() strips leaf return values
--

commit bb5a10abc2299a6ae2ba0cbda70fab044e68ad36
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support filtering return address comments

This commit introduces ftrace_parse_retaddr() and ftrace_filter_retaddr()
along with a --filter-retaddr command-line option and unit tests to strip
funcgraph-retaddr return address comments from function_graph trace output.

> diff --git a/tools/perf/tests/ftrace.c b/tools/perf/tests/ftrace.c
> new file mode 100644
> index 0000000000000..596cecee844e3
> --- /dev/null
> +++ b/tools/perf/tests/ftrace.c
> @@ -0,0 +1,80 @@

[ ... ]

> +static int test_filter_retaddr(void)
> +{
> +	char line1[] = "kernel_read() { /* <-load_elf_phdrs+0x6c/0xb0 */";
> +	char line2[] = "__cond_resched(); /* <-load_elf_phdrs+0x6c/0xb0 ret=0x0 */";
> +	char line3[] = "do_filp_open();";
> +
> +	ftrace_filter_retaddr(line1);
> +	TEST_ASSERT_EQUAL("filter standard retaddr", strcmp(line1, "kernel_read() {"), 0);
> +
> +	ftrace_filter_retaddr(line2);
> +	TEST_ASSERT_EQUAL("filter retaddr with ret", strcmp(line2, "__cond_resched();"), 0);
> +
> +	ftrace_filter_retaddr(line3);
> +	TEST_ASSERT_EQUAL("filter line without retaddr unchanged",
> +			  strcmp(line3, "do_filp_open();"), 0);
> +
> +	return TEST_OK;
> +}

[ ... ]

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> new file mode 100644
> index 0000000000000..aaf108369c365
> --- /dev/null
> +++ b/tools/perf/util/ftrace.c
> @@ -0,0 +1,86 @@

[ ... ]

> +/**
> + * ftrace_filter_retaddr - Remove return address comment from trace line
> + * @str: The trace line to modify
> + *
> + * Find and strip the funcgraph-retaddr comment from the trace line
> + * in-place, removing trailing spaces where applicable.
> + */
> +void ftrace_filter_retaddr(char *str)
> +{
> +	char *start = strstr(str, "/* <-");
> +	char *end;
> +
> +	if (!start)
> +		return;
> +
> +	end = strstr(start, "*/");
> +	if (!end)
> +		return;
> +
> +	end += 2; /* skip end-of-comment delimiter */
> +
> +	/* Also backtrack any spaces before comment */
> +	while (start > str && *(start - 1) == ' ')
> +		start--;
> +
> +	memmove(start, end, strlen(end) + 1);
> +}

[Severity: Medium]
Does ftrace_filter_retaddr() unintentionally strip return values from leaf
functions when both funcgraph-retaddr and funcgraph-retval are enabled?

In kernel/trace/trace_functions_graph.c:print_graph_retval(), when both
TRACE_GRAPH_PRINT_RETADDR and TRACE_GRAPH_PRINT_RETVAL are active, the
kernel combines the return address and return value into a single comment on
leaf functions:

    __cond_resched(); /* <-load_elf_phdrs+0x6c/0xb0 ret=0x0 */

Non-leaf functions place the return address on the entry line and the return
value on the closing brace line, so their return values are unaffected.
However, on leaf functions, ftrace_filter_retaddr() removes the entire
comment from "/* <-" through "*/", discarding "ret=0x0" as well (which
test_filter_retaddr() also asserts).

When running with "--graph-opts retaddr,retval --filter-retaddr" (or when a
subsequent change such as "perf ftrace: Support display of inlined functions
in function graph tracer" automatically enables funcgraph-retaddr and
filter_retaddr alongside "--graph-opts retval"), shouldn't
ftrace_filter_retaddr() preserve the "/* ret=... */" portion of the comment
on leaf functions instead of removing the whole comment block?

([ ... ])

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