Re: [PATCH v15 3/6] arm64: Make the unwind loop in unwind() similar to other architectures

[Madhavan T. Venkataraman]

On 6/26/22 03:21, Mark Rutland wrote:
> On Fri, Jun 17, 2022 at 04:07:14PM -0500, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>>
>> Change the loop in unwind()
>> ===========================
>>
>> Change the unwind loop in unwind() to:
>>
>> 	while (unwind_continue(state, consume_entry, cookie))
>> 		unwind_next(state);
>>
>> This is easy to understand and maintain.
>> New function unwind_continue()
>> ==============================
>>
>> Define a new function unwind_continue() that is used in the unwind loop
>> to check for conditions that terminate a stack trace.
>>
>> The conditions checked are:
>>
>> 	- If the bottom of the stack (final frame) has been reached,
>> 	  terminate.
>>
>> 	- If the consume_entry() function returns false, the caller of
>> 	  unwind has asked to terminate the stack trace. So, terminate.
>>
>> 	- If unwind_next() failed for some reason (like stack corruption),
>> 	  terminate.
> 
> I'm a bit confused as to why this structure, since AFAICT this doesn't match
> other architectures (looking at x86, powerpc, and s390). I note that x86 has:
> 
> * In arch_stack_walk():
> 
>         for (unwind_start(&state, task, regs, NULL); !unwind_done(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...))
> 			break;
> 		...
> 	}
> 
> * In arch_stack_walk_reliable():
> 
>         for (unwind_start(&state, task, NULL, NULL);
>              !unwind_done(&state) && !unwind_error(&state);
>              unwind_next_frame(&state)) {
> 		...
> 		if (!consume_entry(...)
> 			return -EINVAL;
> 	}
> 
> ... and back in v6 I suggeted exactly that shape:
> 
>   https://lore.kernel.org/linux-arm-kernel/20210728165635.GA47345@C02TD0UTHF1T.local/
> 

OK. I will take a look at your suggestion and resend this patch.

>>
>> Do not return an error value from unwind_next()
>> ===============================================
>>
>> We want to check for terminating conditions only in unwind_continue() from
>> the unwinder loop. So, do not return an error value from unwind_next().
>> Simply set a flag in unwind_state and check the flag in unwind_continue().
> 
> I'm fine with the concept of moving ghe return value out of unwind_next() (e.g.
> if we go with an x86-like structure), but I don't think that we should
> centralize the other checks *and* the consumption within unwind_continue(), as
> I think those are two separate things.
> 

OK. I will address this in the next version.

>>
>> Final FP
>> ========
>>
>> Introduce a new field "final_fp" in "struct unwind_state". Initialize this
>> to the final frame of the stack trace:
>>
>> 	task_pt_regs(task)->stackframe
>>
>> This is where the stacktrace must terminate if it is successful. Add an
>> explicit comment to that effect.
> 
> Can we please make this change as a preparatory step, as with the 'task' field?
> 
> We can wrap this in a helper like:
> 
> static bool is_final_frame(struct unwind state *state)
> {
> 	return state->fp == state->final_fp;
> }
> 
> ... and use that in the main loop.
> 

OK. I will make these changes.

Thanks.

Madhavan