Re: [PATCH perf-tools-next v6 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:
- [High] perf/ftrace: Stack tracking corrupts across context switches and task migrations
- [Medium] perf/ftrace: Stack overflow_depth is not reset when stale frames are popped
- [Low] perf/ftrace: Missing <stdio.h> inclusion for FILE type in ftrace.h
--

commit ffcafe3751978ca1760ca39e8d576173ee6402ee
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 callchain
and synthesizes the intermediate inlined frames directly into the streamed call
graph.

> diff --git a/tools/perf/util/ftrace.c b/tools/perf/util/ftrace.c
> --- a/tools/perf/util/ftrace.c
> +++ b/tools/perf/util/ftrace.c

[ ... ]

> +int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FILE *out)
> +{

[ ... ]

> +	if (!ftrace->use_inline)
> +		return print_line(ftrace, line, out);
> +
> +	pipe_char = find_func_column(ftrace, line, &cpu);
> +	if (!pipe_char)
> +		return print_line(ftrace, line, out);
> +
> +	cs = get_cpu_state(cpu);

[Severity: High]
Does this code handle context switches appropriately?

When trace_pipe output is read, it contains context switch banners when tasks
change on a CPU. These banners cannot be parsed by find_func_column(), which
will return NULL, skipping the line without clearing the stack state.

Because the state is tracked strictly per-CPU without task awareness, could
this cause the newly scheduled task to erroneously inherit the preempted
task's call stack? Furthermore, an unmatched closing brace from the newly
scheduled task could incorrectly match and pop the stranded frames belonging
to the preempted task:

[ ... ]

> +		/* Find matching frame on stack */
> +		match_idx = -1;
> +		for (i = cs->depth - 1; i >= 0; i--) {
> +			if (!cs->stack[i].inlined) {
> +				if (exit_name[0] == '\0' ||
> +				    (cs->stack[i].name &&
> +				     strcmp(cs->stack[i].name, exit_name) == 0)) {
> +					match_idx = i;
> +					break;
> +				}
> +			}
> +		}

[ ... ]

> +	/* Close stale callee frames and inactive inlined frames above caller */
> +	while (caller_idx >= 0 && cs->depth > caller_idx + 1 + common) {
> +		struct ftrace_stack_frame *top = &cs->stack[cs->depth - 1];

[Severity: Medium]
When the function call depth exceeds FTRACE_MAX_STACK, the logic starts
incrementing cs->overflow_depth. If trace events are dropped, this recovery
loop pops stale tracked frames above the caller.

However, since the caller was found in the tracked stack, does this mean all
overflow frames logically must have exited? By only decrementing cs->depth
and failing to reset cs->overflow_depth to 0, could subsequent valid exit
events mistakenly decrement overflow_depth instead of popping their actual
frame from the tracked stack?

> diff --git a/tools/perf/util/ftrace.h b/tools/perf/util/ftrace.h
> --- a/tools/perf/util/ftrace.h
> +++ b/tools/perf/util/ftrace.h

[ ... ]

> +struct machine;
> +
>  bool ftrace_parse_retaddr(const char *str, char *sym_name, size_t sym_len, u64 *offset);
>  void ftrace_filter_retaddr(char *str);
> +int ftrace_resolve_inlines(struct machine *machine,
> +			   const char *sym_name, u64 offset,
> +			   const char **inlined_names, int max_inlines);
> +int ftrace_process_fgraph_line(struct perf_ftrace *ftrace, const char *line, FILE *out);

[Severity: Low]
Does this header file need to explicitly include <stdio.h> to provide the
declaration for the FILE type? Relying on implicit header inclusion can break
compilation under musl libc.

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