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

[Segher Boessenkool <segher@kernel.crashing.org>]

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"?

> 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 :-)

>   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).

> --- 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@kernel.crashing.org>


Segher