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
