[PATCH] powerpc/vdso: Fix incorrect CFI in gettimeofday.S

[Michael Ellerman]

Segher Boessenkool <segher at kernel.crashing.org> writes:
> Hi!
>
> On Mon, May 02, 2022 at 10:50:10PM +1000, Michael Ellerman wrote:
>> As reported by Alan, the CFI (Call Frame Information) in the VDSO time
>> routines is incorrect since commit ce7d8056e38b ("powerpc/vdso: Prepare
>> for switching VDSO to generic C implementation.").
>> 
>> In particular the changes to the frame address register (r1) are not
>> properly described, which prevents gdb from being able to generate a
>> backtrace from inside VDSO functions, eg:
>
> Note that r1 is not the same as the CFA: r1 is the stack pointer, while
> the CFA is a DWARF concept.  Often (but not always) they point to the
> same thing, for us.  "When we change the stack pointer we should change
> the DWARF CFA as well"?
 
Thanks, I reworded it a bit:

    DWARF has a concept called the CFA (Canonical Frame Address), which on
    powerpc is calculated as an offset from the stack pointer (r1). That
    means when the stack pointer is changed there must be a corresponding
    CFI directive to update the calculation of the CFA.

    The current code is missing those directives for the changes to r1,
    which prevents gdb from being able to generate a backtrace from inside
    VDSO functions, eg

>> Alan helpfully describes some rules for correctly maintaining the CFI information:
>> 
>>   1) Every adjustment to the current frame address reg (ie. r1) must be
>>      described, and exactly at the instruction where r1 changes. Why?
>>      Because stack unwinding might want to access previous frames.
>>   2) If a function changes LR or any non-volatile register, the save
>>      location for those regs must be given. The cfi can be at any
>>      instruction after the saves up to the point that the reg is
>>      changed. (Exception: LR save should be described before a bl.)
>
> That isn't an exception?  bl changes the current LR after all :-)
 
As Alan mentioned the exception is the the CFI has to appear before the
bl not after, I noted that in the change log.

>>   3) If asychronous unwind info is needed then restores of LR and
>>      non-volatile regs must also be described. The cfi can be at any
>>      instruction after the reg is restored up to the point where the
>>      save location is (potentially) trashed.
>
> The general rule is that your CFI must enable a debugger to reconstruct
> the state at function entry (or it can explicitly say something has been
> clobbered), using only data available at any point in the program we are
> at now.  If something is available in multiple places (in some
> registers, somewhere in memory) either place can be used; only one such
> place is described in the CFI.  A store or even a restore does not have
> to be described at the exact spot it happens (but that is by far the
> most readable / least confusing way to do it).

Ack.

I moved the LR restore down one line, which saves one CFA_advance_loc and
is not hard on the reader.

>> --- a/arch/powerpc/kernel/vdso/gettimeofday.S
>> +++ b/arch/powerpc/kernel/vdso/gettimeofday.S
>> @@ -22,12 +22,15 @@
>>  .macro cvdso_call funct call_time=0
>>    .cfi_startproc
>>  	PPC_STLU	r1, -PPC_MIN_STKFRM(r1)
>> +  .cfi_adjust_cfa_offset PPC_MIN_STKFRM
>>  	mflr		r0
>> -  .cfi_register lr, r0
>>  	PPC_STLU	r1, -PPC_MIN_STKFRM(r1)
>> +  .cfi_adjust_cfa_offset PPC_MIN_STKFRM
>>  	PPC_STL		r0, PPC_MIN_STKFRM + PPC_LR_STKOFF(r1)
>> +  .cfi_rel_offset lr, PPC_MIN_STKFRM + PPC_LR_STKOFF
>
> So you don't need to describe lr being saved in r0, because at all times
> it is available elsewhere, namely in the lr reg still, or on the stack.
> If lr could be clobbered before r0 is saved to the stack slot you would
> still need to describe r0 containing the value of lr at function entry,
> because that value then isn't available elsewhere.
>
> The patch looks good to me :-)
>
> Reviewed-by: Segher Boessenkool <segher at kernel.crashing.org>

Thanks.

cheers