Re: [PATCH v7 6/8] perf cs-etm: Support call indentation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] cs_etm__packet_swap improperly bypasses packet swapping when only thread_stack is requested, breaking call indentation.

Pre-existing issues:
- [High] Incorrect CPU used to fetch the branch stack in `cs_etm__synth_instruction_sample`.
--

commit 75bb53812475ad4dde7bad4012f3997de1cd7ce0
Author: Leo Yan <leo.yan@linux.dev>

perf cs-etm: Support call indentation

This commit enables thread-stack support when either itrace thread-stack
support or last branch entries are requested. It allocates the branch stack
storage accordingly and feeds taken branches to thread_stack__event() to
maintain call depth for branch samples.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index 7069b4990e610..830618763d8b1 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c
[ ... ]
> @@ -1545,7 +1547,7 @@ static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
>  	if (!cs_etm__packet_has_taken_branch(tidq->prev_packet))
>  		return;
>  
> -	if (etmq->etm->synth_opts.last_branch) {
> +	if (etmq->etm->use_thread_stack) {

[Severity: High]
If a user runs perf script -F +callindent and disables other sample types
(for example, using --itrace=e), etm->synth_opts.thread_stack becomes true
while branches, last_branch, and instructions remain false.

When this happens, does cs_etm__packet_swap() bypass packet swapping?
Looking at cs_etm__packet_swap() in tools/perf/util/cs-etm.c:

	if (etm->synth_opts.branches || etm->synth_opts.last_branch ||
	    etm->synth_opts.instructions) {
		/* Swap PACKET with PREV_PACKET ... */
		tmp = tidq->packet;
		tidq->packet = tidq->prev_packet;
		...

If use_thread_stack isn't checked here, tidq->prev_packet remains
zero-initialized, which means cs_etm__packet_has_taken_branch() will always
evaluate to false. Does cs_etm__packet_swap() need to be updated to check
etm->use_thread_stack so that thread_stack__event() can be called properly?

>  		from = cs_etm__last_executed_instr(tidq->prev_packet);
>  		to = cs_etm__first_executed_instr(tidq->packet);
>  
[ ... ]
> @@ -1955,7 +1958,7 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
>  	cs_etm__packet_swap(etm, tidq);
>  
>  	/* Reset last branches after flush the trace */
> -	if (etm->synth_opts.last_branch)
> +	if (etm->use_thread_stack)
>  		thread_stack__flush(tidq->frontend_thread);
>  
>  	return err;

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with how the
branch stack is fetched during a trace flush.

In cs_etm__synth_instruction_sample(), the code hardcodes tidq->packet->cpu
instead of using the local packet parameter:

	if (etm->synth_opts.last_branch) {
		thread_stack__br_sample(tidq->frontend_thread,
					tidq->packet->cpu,
					tidq->last_branch, tidq->br_stack_sz);
		...

If cs_etm__flush() calls cs_etm__synth_instruction_sample() with
tidq->prev_packet as the packet argument, and exactly one packet was
processed before the flush, tidq->packet will still hold the initial
zero-allocated prev_packet struct.

Would this cause tidq->packet->cpu to evaluate to 0, resulting in
thread_stack__br_sample() fetching the branch stack for CPU 0 instead of
the actual CPU of the packet?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-b4-arm_cs_callchain_support_v1-v7-0-1ba770c862ae@arm.com?part=6