Re: [PATCH v6 5/7] riscv: stacktrace: switch to frame-pointer based unwinder

[Shuai Xue <xueshuai@linux.alibaba.com>]

On 10/10/26 2:59 AM, Karl Mehltretter wrote:
> On Mon, Sep 14, 2026 at 05:26:46PM +0800, Shuai Xue wrote:
>> +noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
>> +				      void *cookie, struct task_struct *task,
>> +				      struct pt_regs *regs)
>> +{
>> +	struct kunwind_consume_entry_data data = {
>> +		.consume_entry = consume_entry,
>> +		.cookie = cookie,
>> +	};
>> +
>> +	kunwind_stack_walk(arch_kunwind_consume_entry, &data, task, regs);
>> +}
> 
> Hi Shuai Xue,
> 
> With this patch and FRAME_POINTER enabled, return_address() returns the
> caller one level further up than asked for.
> 
> return_address() in arch/riscv/kernel/return_address.c skips level + 3
> entries from arch_stack_walk(). The arm64 original skips level + 2. The
> third entry exists on mainline because the walk starts inside
> walk_stackframe() and first reports the return into arch_stack_walk().
> With 5/7 the walk starts at the caller of arch_stack_walk(), so
> level + 3 now skips a real caller. CALLER_ADDR1 and up are built on
> return_address(). The irqsoff tracer and the preemptirq tracepoints use
> them.
> 
> A KUnit test that calls ftrace_return_address() from
> leaf() <- middle() <- outer() <- check() returns:
> 
>               af32da41b032   with series   with series, level + 2
>    level 0    middle         outer         middle
>    level 1    outer          check         outer
> 
> The change to level + 2 has to depend on FRAME_POINTER. Without it the
> series keeps walk_stackframe() and the result is the same as on
> mainline. There, level + 2 makes return_address(0) return an address
> inside return_address().

Hi, Karl,

Nice point. Thank you for the thorough review and the KUnit test.
The analysis is spot on, and your table matches my reading of the code
exactly.

> 
> The same extra entry makes stack_trace_save() start one frame too early
> on mainline. 5/7 fixes that. I have a small fix for it on mainline,
> meant for stable kernels, which conflicts with 5/7. I would post it with
> you in Cc. Tell me if you would rather take the return_address.c change
> into 5/7 first.

Please go ahead with your mainline/stable fix for the
stack_trace_save() off-by-one as an independent patch. You are right
that this is a separate mainline bug: stack_trace_save() hardcodes
.skip = skipnr + 1, i.e. it expects the first entry reported by
arch_stack_walk() to be the return into stack_trace_save() itself, and
mainline's walk_stackframe() breaks that contract by reporting the
return into arch_stack_walk() first. A 7-patch series is not a vehicle
for stable, so the minimal fix is the right approach there.

Please keep me in Cc when you post it, I will review and test it.

> 
> Tested with clang 22 and the series applied on 52318cf0fa6e. The
> "with series" column is the same on QEMU's virt machine and on a
> BeagleV Ahead (TH1520). Everything else ran on QEMU only.

Thanks again for testing on real hardware (BeagleV Ahead) in addition
to QEMU, that is much appreciated.

Best regards,
Shuai Xue