Re: [PATCH perf-tools-next v13 09/10] perf ftrace: Support omitting execution duration in function graph tracer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] perf tests ftrace: tautological stack check and leak in test_column_layouts()
- [Low] perf tests ftrace: leak of out_buf on error return in test_column_layouts()
--

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

perf ftrace: Support omitting execution duration in function graph tracer

This commit adds the noduration sub-option to --graph-opts in perf ftrace
to omit function execution duration from function_graph tracer output by
setting the kernel's funcgraph-duration tracing option to 0. It also
updates the inline decoder and tests to handle trace lines without the
duration column and rejects combining noduration with the profile
subcommand.

> diff --git a/tools/perf/tests/ftrace.c b/tools/perf/tests/ftrace.c
> index 900f5390141ff..ec3c26a01ef94 100644
> --- a/tools/perf/tests/ftrace.c
> +++ b/tools/perf/tests/ftrace.c

[ ... ]

> @@ -875,6 +951,28 @@ static int test_column_layouts(void)
>  		out_buf = NULL;
>  	}
>  
> +	/* Test noduration IRQ markers do not pop active frames from tracked stack */
> +	memset(&ftrace, 0, sizeof(ftrace));
> +	ftrace.use_inline = true;
> +	ftrace.graph_noduration = true;
> +	ftrace_cpu_states__clear();
> +
> +	out = open_memstream(&out_buf, &out_len);
> +	if (!out)
> +		return TEST_FAIL;
> +
> +	ftrace_process_fgraph_line(&ftrace, " 0)  root_func() {", out);
> +	ftrace_process_fgraph_line(&ftrace, " 0) ==========>", out);
> +	ftrace_process_fgraph_line(&ftrace, " 0) <==========", out);
> +	ftrace_process_fgraph_line(&ftrace, " 0)  } /* root_func */", out);
> +
> +	fclose(out);
> +	ftrace_cpu_states__clear();
> +
> +	TEST_ASSERT_VAL("noduration irq markers preserved stack",
> +			strstr(out_buf, " 0)  } /* root_func */\n") != NULL);

[Severity: Low]
Can this check in test_column_layouts() actually detect whether the
interrupt markers popped root_func from ts->stack?

Because ftrace is zero-initialized with no machine set and no inlined
frames are pushed onto ts->stack, ftrace_task_state__inlined_depth(ts) is
0. When ftrace_process_fgraph_line() processes the exit line for
root_func, extra_indent evaluates to 0 whether match_idx is found on
ts->stack or not:

tools/perf/util/ftrace.c:ftrace_process_fgraph_line() {
	...
		if (match_idx >= 0) {
			...
			extra_indent = ftrace_task_state__inlined_depth(ts) * 2;
		} else {
			extra_indent = 0;
		}

		fprintf(out, "%.*s%*s%s\n",
			(int)(pipe_char - line + 1), line,
			leading_spaces + extra_indent, "", func_start);
	...
}

As a result, the output line " 0)  } /* root_func */\n" is emitted
identically even if ts->stack was cleared by the interrupt markers.

> +	free(out_buf);

[Severity: Low]
Does test_column_layouts() leak out_buf if the assertion above fails?

open_memstream(&out_buf, &out_len) allocates out_buf on the heap, and if
the assertion condition evaluates to false, the macro returns immediately
before reaching free(out_buf), unlike the earlier loop in
test_column_layouts() which frees out_buf before returning.

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