Re: [PATCH] perf ftrace: Support display of inlined functions in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf: infinite spinloop on trace EOF in ftrace
- [Medium] perf: infinite parser state growth without funcgraph-tail
--

commit 8997c170e283406995c2dcd46938bbd3757d6735
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 resolves return addresses to their inlined callchains and synthesizes
intermediate inlined frames directly into the streamed call graph.

> diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
> --- a/tools/perf/builtin-ftrace.c
> +++ b/tools/perf/builtin-ftrace.c
> [ ... ]
> @@ -810,11 +829,24 @@ static int __cmd_ftrace(struct perf_ftrace *ftrace)
>  			break;
>  
>  		if (pollfd.revents & POLLIN) {
> -			int n = read(trace_fd, buf, sizeof(buf));
> +			int n = read(trace_fd, buf, TRACE_BUF_SIZE);
>  			if (n < 0)
>  				break;
> -			if (fwrite(buf, n, 1, stdout) != 1)
> -				break;
> +			if (ftrace->use_inline || ftrace->filter_retaddr) {
> +				for (int i = 0; i < n; i++) {

[Severity: Medium]
When the trace_pipe reaches EOF, read() will return 0. The old code handled
this via fwrite() != 1 which broke the poll loop.

With the new logic under --inline or --filter-retaddr, if n == 0, the for loop
is skipped and the outer loop doesn't break. Does this cause an infinite
spinloop on poll() when ftrace reaches EOF?

> +					if (buf[i] == '\n') {
> +						if (ftrace_process_fgraph_line(ftrace, linebuf.buf, stdout) < 0)
> +							goto out_close_fd;
> +						strbuf_setlen(&linebuf, 0);
> +					} else {
> +						if (strbuf_addch(&linebuf, buf[i]) < 0)
> +							goto out_close_fd;
> +					}
> +				}
> +			} else {
> +				if (fwrite(buf, n, 1, stdout) != 1)
> +					break;
> +			}
>  			/* flush output since stdout is in full buffering mode due to pager */
>  			fflush(stdout);
>  		}
> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> --- /dev/null
> +++ b/tools/perf/util/ftrace.c
> [ ... ]
> +int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FILE *out)
> +{
> [ ... ]
> +	if (is_exit) {
> +		char exit_name[128];
> +
> +		extract_exit_func_name(func_start, exit_name, sizeof(exit_name));
> +
> [ ... ]
> +		/* Close all inlined frames above match_idx */
> +		while (cs->depth > 0 && (match_idx < 0 || cs->depth - 1 > match_idx)) {
> +			if (cs->stack[cs->depth - 1].inlined) {
> [ ... ]
> +				cs->inlined_depth--;
> [ ... ]
> +			} else {
> +				break;
> +			}
> +		}
> +
> +		/* Pop the matched real frame */
> +		if (cs->depth > 0 && !cs->stack[cs->depth - 1].inlined &&
> +		    cs->stack[cs->depth - 1].name &&
> +		    strcmp(cs->stack[cs->depth - 1].name, exit_name) == 0) {
> +			free(cs->stack[cs->depth - 1].name);
> +			cs->depth--;
> +		}

[Severity: Medium]
If funcgraph-tail is disabled (the default setting), function exit events lack
the trailing comment containing the function name. This means
extract_exit_func_name() will yield an empty string.

Because exit_name is empty, will strcmp() against the stack frame fail,
preventing the real frame from ever being popped?

> [ ... ]
> +	/* Open new inlined frames */
> +	for (i = common; i < num_inlines; i++) {
> +		int entry_indent = leading_spaces + cs->inlined_depth * 2;
> +
> +		if (!inlined_names[i]) {
> +			pr_err("Not enough memory\n");
> +			for (int j = 0; j < num_inlines; j++)
> +				free((void *)inlined_names[j]);
> +			return -ENOMEM;
> +		}
> +		fprintf(out, "%s%*s%s() { /* (inline) */\n", blank_prefix, entry_indent, "", inlined_names[i]);
> +		if (cs->depth < FTRACE_MAX_STACK) {
> +			cs->stack[cs->depth].name = strdup(inlined_names[i]);
> +			if (!cs->stack[cs->depth].name) {
> +				pr_err("Not enough memory\n");
> +				for (int j = 0; j < num_inlines; j++)
> +					free((void *)inlined_names[j]);
> +				return -ENOMEM;
> +			}
> +			cs->stack[cs->depth].inlined = true;
> +			cs->stack[cs->depth].indent = entry_indent;
> +			cs->depth++;
> +		}
> +		cs->inlined_depth++;
> +	}

[Severity: Medium]
If real frames are never popped, FTRACE_MAX_STACK will eventually saturate. Once
cs->depth reaches FTRACE_MAX_STACK, new frames are no longer pushed to the
stack, but cs->inlined_depth continues to unconditionally increment here.

Does this cause infinite indentation growth, since cs->inlined_depth will never
decrement on exit due to the stuck real frames blocking the popping loop above?

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