Alan, Thanks for the patch... A few minor comments...
> Re: [PATCH] Correct PowerPC VDSO call frame info Our convention is to add powerpc: to the start. ie [PATCH] powerpc: Correct VDSO call frame info or even: [PATCH] powerpc/vdso: Correct call frame info On Fri, 2018-09-14 at 08:57 +0930, Alan Modra wrote: > There is control flow in __kernel_clock_gettime that reaches label 99 > without saving lr in r12. CFI info however is interpreted by the > unwinder without reference to control flow: It's a simple matter of > "Execute all the CFI opcodes up to the current address". That means > the unwinder thinks r12 contains the return address at label 99. > Disabuse it of that notion by resetting CFI for the return address at > label 99. Can you expand on CFI == Call Frame Information in the commit message. Not a common term for us mere kernel developers? > Note that the ".cfi_restore lr" could have gone anywhere from the > "mtlr r12" a few instructions earlier to the instruction at label 99. > I put the CFI as late as possible, because in general that's best > practice (and if possible grouped with other CFI in order to reduce > the number of CFI opcodes executed when unwinding). Using r12 as the > return address is perfectly fine after the "mtlr r12" since r12 on > that code path still contains the return address. > > __get_datapage also has a CFI error. That function temporarily saves > lr in r0, and reflects that fact with ".cfi_register lr,r0". A later > use of r0 means the CFI at that point isn't correct, as r0 no longer > contains the return address. Fix that too. Can you describe the problem this fixes at a high level? ie This fixes doing a stack unwind with gdb when in __kernel_clock_gettime. Mikey > Signed-off-by: Alan Modra <amo...@gmail.com> > > diff --git a/arch/powerpc/kernel/vdso32/datapage.S > b/arch/powerpc/kernel/vdso32/datapage.S > index 3745113fcc65..2a7eb5452aba 100644 > --- a/arch/powerpc/kernel/vdso32/datapage.S > +++ b/arch/powerpc/kernel/vdso32/datapage.S > @@ -37,6 +37,7 @@ data_page_branch: > mtlr r0 > addi r3, r3, __kernel_datapage_offset-data_page_branch > lwz r0,0(r3) > + .cfi_restore lr > add r3,r0,r3 > blr > .cfi_endproc > diff --git a/arch/powerpc/kernel/vdso32/gettimeofday.S > b/arch/powerpc/kernel/vdso32/gettimeofday.S > index 769c2624e0a6..1e0bc5955a40 100644 > --- a/arch/powerpc/kernel/vdso32/gettimeofday.S > +++ b/arch/powerpc/kernel/vdso32/gettimeofday.S > @@ -139,6 +139,7 @@ V_FUNCTION_BEGIN(__kernel_clock_gettime) > */ > 99: > li r0,__NR_clock_gettime > + .cfi_restore lr > sc > blr > .cfi_endproc > diff --git a/arch/powerpc/kernel/vdso64/datapage.S > b/arch/powerpc/kernel/vdso64/datapage.S > index abf17feffe40..bf9668691511 100644 > --- a/arch/powerpc/kernel/vdso64/datapage.S > +++ b/arch/powerpc/kernel/vdso64/datapage.S > @@ -37,6 +37,7 @@ data_page_branch: > mtlr r0 > addi r3, r3, __kernel_datapage_offset-data_page_branch > lwz r0,0(r3) > + .cfi_restore lr > add r3,r0,r3 > blr > .cfi_endproc > diff --git a/arch/powerpc/kernel/vdso64/gettimeofday.S > b/arch/powerpc/kernel/vdso64/gettimeofday.S > index c002adcc694c..a4ed9edfd5f0 100644 > --- a/arch/powerpc/kernel/vdso64/gettimeofday.S > +++ b/arch/powerpc/kernel/vdso64/gettimeofday.S > @@ -169,6 +169,7 @@ V_FUNCTION_BEGIN(__kernel_clock_gettime) > */ > 99: > li r0,__NR_clock_gettime > + .cfi_restore lr > sc > blr > .cfi_endproc >
