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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] perf: build failure in cmd_ftrace() due to extra OPT_CALLBACK argument
- [Low] perf: ftrace_parse_retaddr() is dead code and never used for filtering
- [Low] perf: missing document title underline breaks asciidoc generation
--

commit 5dab431981f96da9cbf4caa8e862a4c0e461a1ff
Author: Aaron Tomlin <atomlin@atomlin.com>

perf ftrace: Support filtering return address comments

This commit introduces support for filtering out return address comments from
function graph trace output via a standalone --filter-retaddr flag. It adds
parsing and filtering logic, updates the command-line interface, and includes
a dedicated unit test suite.

> diff --git a/tools/perf/Documentation/perf-ftrace.txt b/tools/perf/Documentation/perf-ftrace.txt
> index 3f3808e513fe8..3dbb7e8e783e9 100644
> --- a/tools/perf/Documentation/perf-ftrace.txt
> +++ b/tools/perf/Documentation/perf-ftrace.txt
> @@ -1,5 +1,4 @@
>  perf-ftrace(1)
> -==============

[Severity: Low]
Does removing the document title underline break the asciidoc generation?

Asciidoctor strictly requires the document title to be underlined with equal
signs of at least the same length as the text. Removing it renders the
document malformed.

[ ... ]

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> index d6604be593a34..f3221089a4793 100644
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
[ ... ]
> @@ -1827,11 +1833,14 @@ int cmd_ftrace(int argc, const char **argv)
>  		     "Set nograph filter on given functions", parse_filter_func),
>  	OPT_CALLBACK(0, "graph-opts", &ftrace, "options",
>  		     "Graph tracer options, available options: args,retval,retval-hex,retaddr,nosleep-time,noirqs,verbose,thresh=<n>,depth=<n>",
> +		     "Graph tracer options, available options: args,retval,retval-hex,retaddr,nosleep-time,noirqs,noduration,verbose,thresh=<n>,depth=<n>,tail",
>  		     parse_graph_tracer_opts),

[Severity: Low]
Does this extra string literal cause a build failure?

The OPT_CALLBACK macro for graph-opts was modified by adding a new help
string literal without removing the old one. Because they are separated by a
comma, the C preprocessor treats them as two distinct arguments, causing the
macro to be invoked with 7 arguments instead of 6.

[ ... ]

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> new file mode 100644
> index 0000000000000..344d36293f5ca
> --- /dev/null
> +++ b/tools/perf/util/ftrace.c
[ ... ]
> +bool ftrace_parse_retaddr(const char *str, char *sym_name, size_t sym_len, u64 *offset)
> +{

[Severity: Low]
Is ftrace_parse_retaddr() ever used outside of the test suite?

The commit message claims this function is implemented to robustly match
return address comments before stripping them. However, looking at the
implementation of ftrace_filter_retaddr() below:

[ ... ]

> +void ftrace_filter_retaddr(char *str)
> +{
> +	char *start = strstr(str, "/* <-");
> +	char *end;
> +
> +	if (!start)
> +		return;
> +
> +	end = strstr(start, "*/");

It appears ftrace_filter_retaddr() performs string replacement using strstr
and memmove directly, bypassing ftrace_parse_retaddr() entirely. This leaves
the parsing function unused in the actual execution path.

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