linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm: Add unwinding annotations for 64bit division functions
@ 2011-09-19 22:11 Laura Abbott
  2011-09-19 23:22 ` Nicolas Pitre
  0 siblings, 1 reply; 15+ messages in thread
From: Laura Abbott @ 2011-09-19 22:11 UTC (permalink / raw)
  To: linux-arm-kernel

The 64bit division functions never had unwinding annotations
added. This prevents a backtrace from being printed within
the function and if a division by 0 occurs. Add the annotations.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm/lib/div64.S |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/arch/arm/lib/div64.S b/arch/arm/lib/div64.S
index faa7748..e55c484 100644
--- a/arch/arm/lib/div64.S
+++ b/arch/arm/lib/div64.S
@@ -13,6 +13,7 @@
  */
 
 #include <linux/linkage.h>
+#include <asm/unwind.h>
 
 #ifdef __ARMEB__
 #define xh r0
@@ -44,6 +45,7 @@
  */
 
 ENTRY(__do_div64)
+UNWIND(.fnstart)
 
 	@ Test for easy paths first.
 	subs	ip, r4, #1
@@ -189,7 +191,12 @@ ENTRY(__do_div64)
 	moveq	yh, xh
 	moveq	xh, #0
 	moveq	pc, lr
+UNWIND(.fnend)
 
+UNWIND(.fnstart)
+UNWIND(.pad #4)
+UNWIND(.save {lr})
+Ldiv0_64:
 	@ Division by 0:
 	str	lr, [sp, #-8]!
 	bl	__div0
@@ -200,4 +207,5 @@ ENTRY(__do_div64)
 	mov	xh, #0
 	ldr	pc, [sp], #8
 
+UNWIND(.fnend)
 ENDPROC(__do_div64)
-- 
1.7.3.3

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-19 22:11 [PATCH] arm: Add unwinding annotations for 64bit division functions Laura Abbott
@ 2011-09-19 23:22 ` Nicolas Pitre
  2011-09-20  1:55   ` Laura Abbott
  0 siblings, 1 reply; 15+ messages in thread
From: Nicolas Pitre @ 2011-09-19 23:22 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 19 Sep 2011, Laura Abbott wrote:

> The 64bit division functions never had unwinding annotations
> added. This prevents a backtrace from being printed within
> the function and if a division by 0 occurs. Add the annotations.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm/lib/div64.S |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/lib/div64.S b/arch/arm/lib/div64.S
> index faa7748..e55c484 100644
> --- a/arch/arm/lib/div64.S
> +++ b/arch/arm/lib/div64.S
> @@ -13,6 +13,7 @@
>   */
>  
>  #include <linux/linkage.h>
> +#include <asm/unwind.h>
>  
>  #ifdef __ARMEB__
>  #define xh r0
> @@ -44,6 +45,7 @@
>   */
>  
>  ENTRY(__do_div64)
> +UNWIND(.fnstart)
>  
>  	@ Test for easy paths first.
>  	subs	ip, r4, #1
> @@ -189,7 +191,12 @@ ENTRY(__do_div64)
>  	moveq	yh, xh
>  	moveq	xh, #0
>  	moveq	pc, lr
> +UNWIND(.fnend)
>  
> +UNWIND(.fnstart)
> +UNWIND(.pad #4)
> +UNWIND(.save {lr})
> +Ldiv0_64:

Why this phony fnend+fnstart here?


Nicolas

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-19 23:22 ` Nicolas Pitre
@ 2011-09-20  1:55   ` Laura Abbott
  2011-09-20 13:27     ` Nicolas Pitre
  2011-09-21 11:39     ` Dave Martin
  0 siblings, 2 replies; 15+ messages in thread
From: Laura Abbott @ 2011-09-20  1:55 UTC (permalink / raw)
  To: linux-arm-kernel


On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
> On Mon, 19 Sep 2011, Laura Abbott wrote:
>
>> The 64bit division functions never had unwinding annotations
>> added. This prevents a backtrace from being printed within
>> the function and if a division by 0 occurs. Add the annotations.
>>
>> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
>> ---
>>  arch/arm/lib/div64.S |    8 ++++++++
>>  1 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/lib/div64.S b/arch/arm/lib/div64.S
>> index faa7748..e55c484 100644
>> --- a/arch/arm/lib/div64.S
>> +++ b/arch/arm/lib/div64.S
>> @@ -13,6 +13,7 @@
>>   */
>>
>>  #include <linux/linkage.h>
>> +#include <asm/unwind.h>
>>
>>  #ifdef __ARMEB__
>>  #define xh r0
>> @@ -44,6 +45,7 @@
>>   */
>>
>>  ENTRY(__do_div64)
>> +UNWIND(.fnstart)
>>
>>  	@ Test for easy paths first.
>>  	subs	ip, r4, #1
>> @@ -189,7 +191,12 @@ ENTRY(__do_div64)
>>  	moveq	yh, xh
>>  	moveq	xh, #0
>>  	moveq	pc, lr
>> +UNWIND(.fnend)
>>
>> +UNWIND(.fnstart)
>> +UNWIND(.pad #4)
>> +UNWIND(.save {lr})
>> +Ldiv0_64:
>
> Why this phony fnend+fnstart here?
>
If a division by 0 occurs, we need to be able to access the saved LR on
the stack which is setup right before calling the __div0 function. This
can't go at the top of __do_div64 because if we try to do a backtrace from
within __do_div64 the annotation won't be correct as the LR was never
saved on the stack. (yes, __do_div64 is all register math but it's still
possible to take a prefetch abort in the middle of that function. Taking a
prefetch abort in the middle of __do_div64 is what found this issue in the
first place.) This setup handles both cases correctly.

>
> Nicolas
>

Laura

> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-20  1:55   ` Laura Abbott
@ 2011-09-20 13:27     ` Nicolas Pitre
  2011-09-20 14:57       ` Catalin Marinas
  2011-09-21 11:39     ` Dave Martin
  1 sibling, 1 reply; 15+ messages in thread
From: Nicolas Pitre @ 2011-09-20 13:27 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 19 Sep 2011, Laura Abbott wrote:

> 
> On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
> > On Mon, 19 Sep 2011, Laura Abbott wrote:
> >
> >> The 64bit division functions never had unwinding annotations
> >> added. This prevents a backtrace from being printed within
> >> the function and if a division by 0 occurs. Add the annotations.
> >>
> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> >> ---
> >>  arch/arm/lib/div64.S |    8 ++++++++
> >>  1 files changed, 8 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/lib/div64.S b/arch/arm/lib/div64.S
> >> index faa7748..e55c484 100644
> >> --- a/arch/arm/lib/div64.S
> >> +++ b/arch/arm/lib/div64.S
> >> @@ -13,6 +13,7 @@
> >>   */
> >>
> >>  #include <linux/linkage.h>
> >> +#include <asm/unwind.h>
> >>
> >>  #ifdef __ARMEB__
> >>  #define xh r0
> >> @@ -44,6 +45,7 @@
> >>   */
> >>
> >>  ENTRY(__do_div64)
> >> +UNWIND(.fnstart)
> >>
> >>  	@ Test for easy paths first.
> >>  	subs	ip, r4, #1
> >> @@ -189,7 +191,12 @@ ENTRY(__do_div64)
> >>  	moveq	yh, xh
> >>  	moveq	xh, #0
> >>  	moveq	pc, lr
> >> +UNWIND(.fnend)
> >>
> >> +UNWIND(.fnstart)
> >> +UNWIND(.pad #4)
> >> +UNWIND(.save {lr})
> >> +Ldiv0_64:
> >
> > Why this phony fnend+fnstart here?
> >
> If a division by 0 occurs, we need to be able to access the saved LR on
> the stack which is setup right before calling the __div0 function. This
> can't go at the top of __do_div64 because if we try to do a backtrace from
> within __do_div64 the annotation won't be correct as the LR was never
> saved on the stack.

I suppose the debug infrastructure always assume the same stack frame 
for the whole function, hence you can't put that .pad and .save right 
before the call to __div0 without the .fnstart and have it effective 
only there?

If so then:

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>


Nicolas

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-20 13:27     ` Nicolas Pitre
@ 2011-09-20 14:57       ` Catalin Marinas
  0 siblings, 0 replies; 15+ messages in thread
From: Catalin Marinas @ 2011-09-20 14:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Tue, Sep 20, 2011 at 02:27:01PM +0100, Nicolas Pitre wrote:
> On Mon, 19 Sep 2011, Laura Abbott wrote:
> > On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
> > > On Mon, 19 Sep 2011, Laura Abbott wrote:
> > >> @@ -189,7 +191,12 @@ ENTRY(__do_div64)
> > >>  	moveq	yh, xh
> > >>  	moveq	xh, #0
> > >>  	moveq	pc, lr
> > >> +UNWIND(.fnend)
> > >>
> > >> +UNWIND(.fnstart)
> > >> +UNWIND(.pad #4)
> > >> +UNWIND(.save {lr})
> > >> +Ldiv0_64:
> > >
> > > Why this phony fnend+fnstart here?
> >
> > If a division by 0 occurs, we need to be able to access the saved LR on
> > the stack which is setup right before calling the __div0 function. This
> > can't go at the top of __do_div64 because if we try to do a backtrace from
> > within __do_div64 the annotation won't be correct as the LR was never
> > saved on the stack.
> 
> I suppose the debug infrastructure always assume the same stack frame 
> for the whole function, hence you can't put that .pad and .save right 
> before the call to __div0 without the .fnstart and have it effective 
> only there?

An element in the unwinding table contains a pointer to the .fnstart
location and a set of instruction bytecodes (like .save) to the next
.fnend statement. If more 4 bytecodes (to fit in an int), it contains an
index to another table with more bytecodes. The elements in the
unwinding table are sorted by the .fnstart address.

So I think in the above case if we don't have the .fnend/.fnstart for
Ldiv0_64 we would get all the instructions in the __do_div64 unwinding
information. I don't think the unwinding bytecode is that smart to allow
other tricks like checking some conditions.

Unless anyone has a better idea to specify this:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-20  1:55   ` Laura Abbott
  2011-09-20 13:27     ` Nicolas Pitre
@ 2011-09-21 11:39     ` Dave Martin
  2011-09-21 11:55       ` Russell King - ARM Linux
  1 sibling, 1 reply; 15+ messages in thread
From: Dave Martin @ 2011-09-21 11:39 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Sep 19, 2011 at 06:55:39PM -0700, Laura Abbott wrote:
> 
> On Mon, September 19, 2011 4:22 pm, Nicolas Pitre wrote:
> > On Mon, 19 Sep 2011, Laura Abbott wrote:
> >
> >> The 64bit division functions never had unwinding annotations
> >> added. This prevents a backtrace from being printed within
> >> the function and if a division by 0 occurs. Add the annotations.
> >>
> >> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> >> ---
> >>  arch/arm/lib/div64.S |    8 ++++++++
> >>  1 files changed, 8 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/arch/arm/lib/div64.S b/arch/arm/lib/div64.S
> >> index faa7748..e55c484 100644
> >> --- a/arch/arm/lib/div64.S
> >> +++ b/arch/arm/lib/div64.S
> >> @@ -13,6 +13,7 @@
> >>   */
> >>
> >>  #include <linux/linkage.h>
> >> +#include <asm/unwind.h>
> >>
> >>  #ifdef __ARMEB__
> >>  #define xh r0
> >> @@ -44,6 +45,7 @@
> >>   */
> >>
> >>  ENTRY(__do_div64)
> >> +UNWIND(.fnstart)
> >>
> >>  	@ Test for easy paths first.
> >>  	subs	ip, r4, #1
> >> @@ -189,7 +191,12 @@ ENTRY(__do_div64)
> >>  	moveq	yh, xh
> >>  	moveq	xh, #0
> >>  	moveq	pc, lr
> >> +UNWIND(.fnend)
> >>
> >> +UNWIND(.fnstart)
> >> +UNWIND(.pad #4)
> >> +UNWIND(.save {lr})
> >> +Ldiv0_64:
> >
> > Why this phony fnend+fnstart here?
> >
> If a division by 0 occurs, we need to be able to access the saved LR on
> the stack which is setup right before calling the __div0 function. This
> can't go at the top of __do_div64 because if we try to do a backtrace from
> within __do_div64 the annotation won't be correct as the LR was never
> saved on the stack. (yes, __do_div64 is all register math but it's still
> possible to take a prefetch abort in the middle of that function. Taking a
> prefetch abort in the middle of __do_div64 is what found this issue in the
> first place.) This setup handles both cases correctly.

Talking to Catalin a bit more, it sounds like prefetch aborts should not
happen in kernel code, and data aborts should not happen when accessing
the kernel stack.  If either of these happens, there's no guarantee that
the kernel will be able to recover from the situation fully, so
backtracing may not be the primary concern.

The kinds of situation that the backtracer _can_ cope with are:

a) explicit, synchonous backtrace requests (BUG() etc. within C
   functions)

b) aborts when accessing random memory (but not the stack) outside the
   function progolgue/epilogue sequences (i.e., so that the state the
   unwind info described actually matches reality)

If you get a data abort when accessing the kernel stack, the backtracer
probably won't work at all, because it relies on the same stack.  Faults
during prologue/epilogue sequences can't be handled correctly because
the unwind annotations don't descirbe the interim state during these
sequences.  There is no general way to use the unwinder annotations to
describe fully how the state evolves during prologue/epilogue sequences.


So, having thought about it, it may be that having the extra .fnstart/
.fnend pair doesn't really help that much after all.

For this code, we do want the unwind state to be properly recorded for
the "bl __div0" call site after Ldiv0_64; but we probably don't need to
worry about backtracing from any other point in this code -- partly
becuase the probably of needing to do it is very low, and partly because
we will likely fail to produce a backtrace in this situation.  If this
is a reasonable assumption, then a single unwind annotation block should
be sufficient:

ENTRY(__do_div64)
UNWIND(.fnstart)
	subs	ip, r4, #1
	@ ...

	moveq	pc, lr

UNWIND(.pad #4)
UNWIND(.save {lr}
	stmfd	sp!, {r0, lr}
	@ ...

	bl	__div0

	@ ...
UNWIND(.fnend)

The location of the .pad and .save is of course not meaningful for the
unwind data: only their respective order, and the fact that they appear
between the .fnstart/.fnend directives.  Putting them next to the stmfd
just makes the code easier to read.


The extra annotations in the original patch are harmless, though.

Cheers
---Dave

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-21 11:39     ` Dave Martin
@ 2011-09-21 11:55       ` Russell King - ARM Linux
  2011-09-21 13:33         ` Dave Martin
  2011-09-22  7:28         ` Jon Medhurst (Tixy)
  0 siblings, 2 replies; 15+ messages in thread
From: Russell King - ARM Linux @ 2011-09-21 11:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Sep 21, 2011 at 12:39:09PM +0100, Dave Martin wrote:
> Talking to Catalin a bit more, it sounds like prefetch aborts should not
> happen in kernel code, and data aborts should not happen when accessing
> the kernel stack.

No faults should happen in kernel code, except for:

1. instructions specifically marked in the exception table, which are used
   to access user memory.
2. instructions causing an 'undefined instruction' exception.

Standard ARM instructions like 'add', 'mov' etc should _never_ fault,
and if they do that means your core isn't executing ARM instructions
correctly (eg, the hardware design is faulty.)

Instructions such as VFP, kprobes tracing, etc are expected fault
locations, and those are fairly well controlled where they can be placed.
With things like ftrace, it certainly is the case that the unwinder can
theoretically be called from almost anywhere in a function.

So I suggest that this does need to be fixed, and you can't rely on
"prefetch aborts should not happen".  That's true of prefetch aborts
but not of other aborts.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-21 11:55       ` Russell King - ARM Linux
@ 2011-09-21 13:33         ` Dave Martin
  2011-09-22  7:28         ` Jon Medhurst (Tixy)
  1 sibling, 0 replies; 15+ messages in thread
From: Dave Martin @ 2011-09-21 13:33 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Sep 21, 2011 at 12:55:53PM +0100, Russell King - ARM Linux wrote:
> On Wed, Sep 21, 2011 at 12:39:09PM +0100, Dave Martin wrote:
> > Talking to Catalin a bit more, it sounds like prefetch aborts should not
> > happen in kernel code, and data aborts should not happen when accessing
> > the kernel stack.
> 
> No faults should happen in kernel code, except for:
> 
> 1. instructions specifically marked in the exception table, which are used
>    to access user memory.
> 2. instructions causing an 'undefined instruction' exception.
> 
> Standard ARM instructions like 'add', 'mov' etc should _never_ fault,
> and if they do that means your core isn't executing ARM instructions
> correctly (eg, the hardware design is faulty.)
> 
> Instructions such as VFP, kprobes tracing, etc are expected fault
> locations, and those are fairly well controlled where they can be placed.
> With things like ftrace, it certainly is the case that the unwinder can
> theoretically be called from almost anywhere in a function.
> 
> So I suggest that this does need to be fixed, and you can't rely on
> "prefetch aborts should not happen".  That's true of prefetch aborts
> but not of other aborts.

The important thing for the unwinder is that it can't cope well with faults
happening in the save/restore sequences at function entry and exit, and
we may not cope well with functions which don't have a simple SAVE,
EXECUTE, RESTORE, RETURN structure.

My gut feeling is that neither (1) or (2) should happen in those sequences,
and VFP faults should not happen in these sequences because the kernel
should not contain VFP code except in particular controlled locations.

For things like kprobes which allow a trap to be set at a function's entry
point we do have a problem: if we try to backtrace from this point, the
backtracer will see we are in that function and will assume that the
function's state saving code has already executed.  It might be simple
to work around this particular case by making the unwinder intelligent
enough to realise that if backtracing from the first instruction of a
function, none of the function's state save code can have executed yet.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-21 11:55       ` Russell King - ARM Linux
  2011-09-21 13:33         ` Dave Martin
@ 2011-09-22  7:28         ` Jon Medhurst (Tixy)
  2011-09-22  9:48           ` Catalin Marinas
  1 sibling, 1 reply; 15+ messages in thread
From: Jon Medhurst (Tixy) @ 2011-09-22  7:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, 2011-09-21 at 12:55 +0100, Russell King - ARM Linux wrote:
> Instructions such as VFP, kprobes tracing, etc are expected fault
> locations, and those are fairly well controlled where they can be placed.
> With things like ftrace, it certainly is the case that the unwinder can
> theoretically be called from almost anywhere in a function.

Actually, kprobes can be places on any instruction in the kernel that
isn't in the section .kprobes.text.

I also strongly suspect that stack unwinding won't happen correctly
across the boundary between the kprobes handling code and the function
which was probed - there's an awful lot of stack jiggery pokery going on
there.

-- 
Tixy

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-22  7:28         ` Jon Medhurst (Tixy)
@ 2011-09-22  9:48           ` Catalin Marinas
  2011-09-22 11:06             ` Jon Medhurst (Tixy)
  0 siblings, 1 reply; 15+ messages in thread
From: Catalin Marinas @ 2011-09-22  9:48 UTC (permalink / raw)
  To: linux-arm-kernel

On 22 September 2011 08:28, Jon Medhurst (Tixy) <jon.medhurst@linaro.org> wrote:
> On Wed, 2011-09-21 at 12:55 +0100, Russell King - ARM Linux wrote:
>> Instructions such as VFP, kprobes tracing, etc are expected fault
>> locations, and those are fairly well controlled where they can be placed.
>> With things like ftrace, it certainly is the case that the unwinder can
>> theoretically be called from almost anywhere in a function.
>
> Actually, kprobes can be places on any instruction in the kernel that
> isn't in the section .kprobes.text.
>
> I also strongly suspect that stack unwinding won't happen correctly
> across the boundary between the kprobes handling code and the function
> which was probed - there's an awful lot of stack jiggery pokery going on
> there.

Are people most likely to place kprobes on the first instruction of a
function? We could improve things a bit in the unwinder and assume
that if the fault address is the same as the .fnstart address, the
return value is always in LR and the SP not affected (that's unwinding
bytecode 0xb0). For a few instructions into the function prologue we
can't reliably get the unwinding information.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-22  9:48           ` Catalin Marinas
@ 2011-09-22 11:06             ` Jon Medhurst (Tixy)
  2011-09-22 11:57               ` Catalin Marinas
  0 siblings, 1 reply; 15+ messages in thread
From: Jon Medhurst (Tixy) @ 2011-09-22 11:06 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
> On 22 September 2011 08:28, Jon Medhurst (Tixy) <jon.medhurst@linaro.org> wrote:
> > On Wed, 2011-09-21 at 12:55 +0100, Russell King - ARM Linux wrote:
> >> Instructions such as VFP, kprobes tracing, etc are expected fault
> >> locations, and those are fairly well controlled where they can be placed.
> >> With things like ftrace, it certainly is the case that the unwinder can
> >> theoretically be called from almost anywhere in a function.
> >
> > Actually, kprobes can be places on any instruction in the kernel that
> > isn't in the section .kprobes.text.
> >
> > I also strongly suspect that stack unwinding won't happen correctly
> > across the boundary between the kprobes handling code and the function
> > which was probed - there's an awful lot of stack jiggery pokery going on
> > there.
> 
> Are people most likely to place kprobes on the first instruction of a
> function? 

I believe that is the usual case.

> We could improve things a bit in the unwinder and assume
> that if the fault address is the same as the .fnstart address, the
> return value is always in LR and the SP not affected (that's unwinding
> bytecode 0xb0). For a few instructions into the function prologue we
> can't reliably get the unwinding information.

That would help make it possible to unwind out of kprobes handlers to
the probed function. The kprobes code itself would need work as well,
and possibly the undef handler. Do we think it is worthwhile to do
this? 

-- 
Tixy

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-22 11:06             ` Jon Medhurst (Tixy)
@ 2011-09-22 11:57               ` Catalin Marinas
  2011-09-22 12:13                 ` Jon Medhurst (Tixy)
  0 siblings, 1 reply; 15+ messages in thread
From: Catalin Marinas @ 2011-09-22 11:57 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 22, 2011 at 12:06:46PM +0100, Jon Medhurst (Tixy) wrote:
> On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
> > We could improve things a bit in the unwinder and assume
> > that if the fault address is the same as the .fnstart address, the
> > return value is always in LR and the SP not affected (that's unwinding
> > bytecode 0xb0). For a few instructions into the function prologue we
> > can't reliably get the unwinding information.
> 
> That would help make it possible to unwind out of kprobes handlers to
> the probed function. The kprobes code itself would need work as well,
> and possibly the undef handler. Do we think it is worthwhile to do
> this? 

Does kprobes need to trace beyond the probed function? If not, you get
the address of the probed function via pt_regs anyway, so no need for
unwinding beyond that.

-- 
Catalin

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-22 11:57               ` Catalin Marinas
@ 2011-09-22 12:13                 ` Jon Medhurst (Tixy)
  2011-09-22 13:00                   ` Catalin Marinas
  0 siblings, 1 reply; 15+ messages in thread
From: Jon Medhurst (Tixy) @ 2011-09-22 12:13 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 2011-09-22 at 12:57 +0100, Catalin Marinas wrote:
> On Thu, Sep 22, 2011 at 12:06:46PM +0100, Jon Medhurst (Tixy) wrote:
> > On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
> > > We could improve things a bit in the unwinder and assume
> > > that if the fault address is the same as the .fnstart address, the
> > > return value is always in LR and the SP not affected (that's unwinding
> > > bytecode 0xb0). For a few instructions into the function prologue we
> > > can't reliably get the unwinding information.
> > 
> > That would help make it possible to unwind out of kprobes handlers to
> > the probed function. The kprobes code itself would need work as well,
> > and possibly the undef handler. Do we think it is worthwhile to do
> > this? 
> 
> Does kprobes need to trace beyond the probed function? If not, you get
> the address of the probed function via pt_regs anyway, so no need for
> unwinding beyond that.

To be honest, I'm not very sure how kprobes get used in the real world.
Though, if stack unwinding from their handlers currently doesn't work
and people had a usecase for it, we would expect them to complain.

-- 
Tixy

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-22 12:13                 ` Jon Medhurst (Tixy)
@ 2011-09-22 13:00                   ` Catalin Marinas
  2011-09-22 13:19                     ` Jon Medhurst (Tixy)
  0 siblings, 1 reply; 15+ messages in thread
From: Catalin Marinas @ 2011-09-22 13:00 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, Sep 22, 2011 at 01:13:01PM +0100, Jon Medhurst (Tixy) wrote:
> On Thu, 2011-09-22 at 12:57 +0100, Catalin Marinas wrote:
> > On Thu, Sep 22, 2011 at 12:06:46PM +0100, Jon Medhurst (Tixy) wrote:
> > > On Thu, 2011-09-22 at 10:48 +0100, Catalin Marinas wrote:
> > > > We could improve things a bit in the unwinder and assume
> > > > that if the fault address is the same as the .fnstart address, the
> > > > return value is always in LR and the SP not affected (that's unwinding
> > > > bytecode 0xb0). For a few instructions into the function prologue we
> > > > can't reliably get the unwinding information.
> > > 
> > > That would help make it possible to unwind out of kprobes handlers to
> > > the probed function. The kprobes code itself would need work as well,
> > > and possibly the undef handler. Do we think it is worthwhile to do
> > > this? 
> > 
> > Does kprobes need to trace beyond the probed function? If not, you get
> > the address of the probed function via pt_regs anyway, so no need for
> > unwinding beyond that.
> 
> To be honest, I'm not very sure how kprobes get used in the real world.
> Though, if stack unwinding from their handlers currently doesn't work
> and people had a usecase for it, we would expect them to complain.

The unwinding fix should be simple (I haven't tested it yet):

8<-----------------------------
ARM: Ignore the unwinding information for the first instruction in a function

From: Catalin Marinas <catalin.marinas@arm.com>

When backtracing from the first instruction of a function, the prologue
has not been executed and the unwinding information is not valid. This
patch checks for this case and just assumes that the return address is
in LR.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm/kernel/unwind.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index d2cb0b3..946face 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -293,6 +293,16 @@ int unwind_frame(struct stackframe *frame)
 		return -URC_FAILURE;
 	}
 
+	/*
+	 * Check for backtrace on the first instruction of a function. The
+	 * prologue has not been executed yet and the unwinding information is
+	 * not valid. Assume that the return address is in LR.
+	 */
+	if (idx.addr == frame->pc) {
+		frame->pc = frame->lr;
+		return URC_OK;
+	}
+
 	ctrl.vrs[FP] = frame->fp;
 	ctrl.vrs[SP] = frame->sp;
 	ctrl.vrs[LR] = frame->lr;

-- 
Catalin

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] arm: Add unwinding annotations for 64bit division functions
  2011-09-22 13:00                   ` Catalin Marinas
@ 2011-09-22 13:19                     ` Jon Medhurst (Tixy)
  0 siblings, 0 replies; 15+ messages in thread
From: Jon Medhurst (Tixy) @ 2011-09-22 13:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 2011-09-22 at 14:00 +0100, Catalin Marinas wrote:
> The unwinding fix should be simple (I haven't tested it yet):
> 
> 8<-----------------------------
> ARM: Ignore the unwinding information for the first instruction in a function
> 
> From: Catalin Marinas <catalin.marinas@arm.com>
> 
> When backtracing from the first instruction of a function, the prologue
> has not been executed and the unwinding information is not valid. This
> patch checks for this case and just assumes that the return address is
> in LR.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> ---
>  arch/arm/kernel/unwind.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
> index d2cb0b3..946face 100644
> --- a/arch/arm/kernel/unwind.c
> +++ b/arch/arm/kernel/unwind.c
> @@ -293,6 +293,16 @@ int unwind_frame(struct stackframe *frame)
>  		return -URC_FAILURE;
>  	}
>  
> +	/*
> +	 * Check for backtrace on the first instruction of a function. The
> +	 * prologue has not been executed yet and the unwinding information is
> +	 * not valid. Assume that the return address is in LR.
> +	 */
> +	if (idx.addr == frame->pc) {
> +		frame->pc = frame->lr;
> +		return URC_OK;
> +	}
> +
>  	ctrl.vrs[FP] = frame->fp;
>  	ctrl.vrs[SP] = frame->sp;
>  	ctrl.vrs[LR] = frame->lr;
> 

I've never looked at the unwinding code before but the one comment I
would make is: does the patch work with Thumb code? I.e. does bit zero
of idx.addr, frame->pc or frame->lr ever get set to indicate Thumb
state? And if so, they had better all get set otherwise it won't
work :-)

-- 
Tixy

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-09-22 13:19 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-09-19 22:11 [PATCH] arm: Add unwinding annotations for 64bit division functions Laura Abbott
2011-09-19 23:22 ` Nicolas Pitre
2011-09-20  1:55   ` Laura Abbott
2011-09-20 13:27     ` Nicolas Pitre
2011-09-20 14:57       ` Catalin Marinas
2011-09-21 11:39     ` Dave Martin
2011-09-21 11:55       ` Russell King - ARM Linux
2011-09-21 13:33         ` Dave Martin
2011-09-22  7:28         ` Jon Medhurst (Tixy)
2011-09-22  9:48           ` Catalin Marinas
2011-09-22 11:06             ` Jon Medhurst (Tixy)
2011-09-22 11:57               ` Catalin Marinas
2011-09-22 12:13                 ` Jon Medhurst (Tixy)
2011-09-22 13:00                   ` Catalin Marinas
2011-09-22 13:19                     ` Jon Medhurst (Tixy)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).