[PATCH] arm: Add unwinding support for memset and memzero.

[PATCH] arm: Add unwinding support for memset and memzero.

[Laura Abbott]

Both memset and memzero lack unwinding annoations. If
an abort occurs trying to access the pointer, the backtrace
is incomplete. Add unwinding annotations to both functions
so we can actually get a useful backtrace.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 arch/arm/lib/memset.S  |    7 +++++--
 arch/arm/lib/memzero.S |    7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
index 650d592..4379912 100644
--- a/arch/arm/lib/memset.S
+++ b/arch/arm/lib/memset.S
@@ -11,6 +11,7 @@
  */
 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#include <asm/unwind.h>
 
 	.text
 	.align	5
@@ -29,6 +30,7 @@
  */
 
 ENTRY(memset)
+UNWIND(.fnstart)
 	ands	r3, r0, #3		@ 1 unaligned?
 	bne	1b			@ 1
 /*
@@ -41,7 +43,7 @@ ENTRY(memset)
 	blt	4f
 
 #if ! CALGN(1)+0
-
+UNWIND(.save {lr})
 /*
  * We need an extra register for this loop - save the return address and
  * use the LR
@@ -68,7 +70,7 @@ ENTRY(memset)
 	ldr	lr, [sp], #4
 
 #else
-
+UNWIND(.save {r4, r5, r6, r7, lr})
 /*
  * This version aligns the destination pointer in order to write
  * whole cache lines at once.
@@ -124,4 +126,5 @@ ENTRY(memset)
 	tst	r2, #1
 	strneb	r1, [r0], #1
 	mov	pc, lr
+UNWIND(.fnend)
 ENDPROC(memset)
diff --git a/arch/arm/lib/memzero.S b/arch/arm/lib/memzero.S
index 3fbdef5..26f9ce8 100644
--- a/arch/arm/lib/memzero.S
+++ b/arch/arm/lib/memzero.S
@@ -9,6 +9,7 @@
  */
 #include <linux/linkage.h>
 #include <asm/assembler.h>
+#include <asm/unwind.h>
 
 	.text
 	.align	5
@@ -31,6 +32,7 @@
  */
 
 ENTRY(__memzero)
+UNWIND(.fnstart)
 	mov	r2, #0			@ 1
 	ands	r3, r0, #3		@ 1 unaligned?
 	bne	1b			@ 1
@@ -41,7 +43,7 @@ ENTRY(__memzero)
 	blt	4f			@ 1 have < 16 bytes
 
 #if ! CALGN(1)+0
-
+UNWIND(.save {lr})
 /*
  * We need an extra register for this loop - save the return address and
  * use the LR
@@ -68,7 +70,7 @@ ENTRY(__memzero)
 	ldr	lr, [sp], #4		@ 1
 
 #else
-
+UNWIND(.save{r4, r5, r6, r7})
 /*
  * This version aligns the destination pointer in order to write
  * whole cache lines at once.
@@ -122,4 +124,5 @@ ENTRY(__memzero)
 	tst	r1, #1			@ 1 a byte left over
 	strneb	r2, [r0], #1		@ 1
 	mov	pc, lr			@ 1
+UNWIND(.fnend)
 ENDPROC(__memzero)
-- 
1.7.8.3

Re: [PATCH] arm: Add unwinding support for memset and memzero.

[David Brown]

On Mon, Mar 12, 2012 at 10:40:27AM -0700, Laura Abbott wrote:
> @@ -41,7 +43,7 @@ ENTRY(memset)
>  	blt	4f
>  
>  #if ! CALGN(1)+0
> -
> +UNWIND(.save {lr})
>  /*
>   * We need an extra register for this loop - save the return address and
>   * use the LR

Did you intend to remove these blank lines?

David

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

Re: [PATCH] arm: Add unwinding support for memset and memzero.

[Dave Martin]

On Mon, Mar 12, 2012 at 10:40:27AM -0700, Laura Abbott wrote:
> Both memset and memzero lack unwinding annoations. If
> an abort occurs trying to access the pointer, the backtrace
> is incomplete. Add unwinding annotations to both functions
> so we can actually get a useful backtrace.
> 
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
>  arch/arm/lib/memset.S  |    7 +++++--
>  arch/arm/lib/memzero.S |    7 +++++--
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/lib/memset.S b/arch/arm/lib/memset.S
> index 650d592..4379912 100644
> --- a/arch/arm/lib/memset.S
> +++ b/arch/arm/lib/memset.S
> @@ -11,6 +11,7 @@
>   */
>  #include <linux/linkage.h>
>  #include <asm/assembler.h>
> +#include <asm/unwind.h>
>  
>  	.text
>  	.align	5
> @@ -29,6 +30,7 @@
>   */
>  
>  ENTRY(memset)
> +UNWIND(.fnstart)
>  	ands	r3, r0, #3		@ 1 unaligned?
>  	bne	1b			@ 1
>  /*
> @@ -41,7 +43,7 @@ ENTRY(memset)
>  	blt	4f
>  
>  #if ! CALGN(1)+0
> -
> +UNWIND(.save {lr})
>  /*
>   * We need an extra register for this loop - save the return address and
>   * use the LR
> @@ -68,7 +70,7 @@ ENTRY(memset)
>  	ldr	lr, [sp], #4
>  
>  #else
> -
> +UNWIND(.save {r4, r5, r6, r7, lr})

For functions like this, the unwinding requirements are different
depending on where we are in the function.  But the unwinder annotations
don't remember exact instruction locations; only the extent of the
whole unwind block is recorded, along with the sequence (but not
location) of unwinder directives.

As is, the unwinding may be wrong depending on which part of the function
is executing when the fault occurs.

It may be possible to split the function into multiple unwind blocks,
e.g.:

ENTRY(somefunc)
UNWIND(.fnstart)

UNWIND(.save {r4,lr})
	stfmd	sp!, {r4,lr}

	/* check something */

	blt	_the_other_way
	/* maybe carry out our job this way */

	ldmfd	sp!, {r4,lr}
UNWIND(.fnend)

_the_other_way:
UNWIND(.fnstart)
UNWIND(.save {r4,lr})
UNWIND(.save {r5-r8})
	stmfd	sp!, {r5-r8}	/* !! */

	/* carry out our job the other way */

	ldmfd	sp!, {r5-r8}
	ldmfd	sp!, {r4,pc}	/* !! */
UNWIND(.fnend)


This is still not exactly right (it's hard to be exactly right,
since the unwind tables are not meant for handling asynchronous
unwinding), but unwinding should be correct for the main bits of code
where most time is spent and/or faults are most likely (the "carry out
our job" comments).

You'd have to experiment to see whether the backtracer does something
sensible with unwind tables like this.  It might need tweaking to
find the correct function symbol if a fault occurs in the second
unwind block for example ... that perhaps it will already do the
right thing.

Cheers
---Dave

Re: [PATCH] arm: Add unwinding support for memset and memzero.

[Laura Abbott]

On 3/14/2012 4:15 AM, Dave Martin wrote:
>
> For functions like this, the unwinding requirements are different
> depending on where we are in the function.  But the unwinder annotations
> don't remember exact instruction locations; only the extent of the
> whole unwind block is recorded, along with the sequence (but not
> location) of unwinder directives.
>
> As is, the unwinding may be wrong depending on which part of the function
> is executing when the fault occurs.
>

Hmmmm, I thought I could get away with only one annotation based on the 
structure of memset/memzero but looking again you are right, it really 
requires multiple annotations to be correct.

> It may be possible to split the function into multiple unwind blocks,
> e.g.:
>
> ENTRY(somefunc)
> UNWIND(.fnstart)
>
> UNWIND(.save {r4,lr})
> 	stfmd	sp!, {r4,lr}
>
> 	/* check something */
>
> 	blt	_the_other_way
> 	/* maybe carry out our job this way */
>
> 	ldmfd	sp!, {r4,lr}
> UNWIND(.fnend)
>
> _the_other_way:
> UNWIND(.fnstart)
> UNWIND(.save {r4,lr})
> UNWIND(.save {r5-r8})
> 	stmfd	sp!, {r5-r8}	/* !! */
>
> 	/* carry out our job the other way */
>
> 	ldmfd	sp!, {r5-r8}
> 	ldmfd	sp!, {r4,pc}	/* !! */
> UNWIND(.fnend)
>
>
> This is still not exactly right (it's hard to be exactly right,
> since the unwind tables are not meant for handling asynchronous
> unwinding), but unwinding should be correct for the main bits of code
> where most time is spent and/or faults are most likely (the "carry out
> our job" comments).
>

Would a compiler be able to generate code such as this and still 
generate correct completely unwinding annotations? Or if the compiler 
knows unwinding is necessary, is the only option to generate code in 
'unwindable blocks'? (alternatively, no compiler is smart/stupid enough 
to generate this code?)

> You'd have to experiment to see whether the backtracer does something
> sensible with unwind tables like this.  It might need tweaking to
> find the correct function symbol if a fault occurs in the second
> unwind block for example ... that perhaps it will already do the
> right thing.
>

Yes, I'll look into this. memcpy is missing annotations as well but that 
code is significantly more convoluted.

> Cheers
> ---Dave
>

Thanks,
Laura

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

Re: [PATCH] arm: Add unwinding support for memset and memzero.

[Dave Martin]

On Fri, Mar 16, 2012 at 10:21:02AM -0700, Laura Abbott wrote:
> On 3/14/2012 4:15 AM, Dave Martin wrote:
> >
> >For functions like this, the unwinding requirements are different
> >depending on where we are in the function.  But the unwinder annotations
> >don't remember exact instruction locations; only the extent of the
> >whole unwind block is recorded, along with the sequence (but not
> >location) of unwinder directives.
> >
> >As is, the unwinding may be wrong depending on which part of the function
> >is executing when the fault occurs.
> >
> 
> Hmmmm, I thought I could get away with only one annotation based on
> the structure of memset/memzero but looking again you are right, it
> really requires multiple annotations to be correct.
> 
> >It may be possible to split the function into multiple unwind blocks,
> >e.g.:
> >
> >ENTRY(somefunc)
> >UNWIND(.fnstart)
> >
> >UNWIND(.save {r4,lr})
> >	stfmd	sp!, {r4,lr}
> >
> >	/* check something */
> >
> >	blt	_the_other_way
> >	/* maybe carry out our job this way */
> >
> >	ldmfd	sp!, {r4,lr}
> >UNWIND(.fnend)
> >
> >_the_other_way:
> >UNWIND(.fnstart)
> >UNWIND(.save {r4,lr})
> >UNWIND(.save {r5-r8})
> >	stmfd	sp!, {r5-r8}	/* !! */
> >
> >	/* carry out our job the other way */
> >
> >	ldmfd	sp!, {r5-r8}
> >	ldmfd	sp!, {r4,pc}	/* !! */
> >UNWIND(.fnend)
> >
> >
> >This is still not exactly right (it's hard to be exactly right,
> >since the unwind tables are not meant for handling asynchronous
> >unwinding), but unwinding should be correct for the main bits of code
> >where most time is spent and/or faults are most likely (the "carry out
> >our job" comments).
> >
> 
> Would a compiler be able to generate code such as this and still
> generate correct completely unwinding annotations? Or if the
> compiler knows unwinding is necessary, is the only option to
> generate code in 'unwindable blocks'? (alternatively, no compiler is
> smart/stupid enough to generate this code?)

I believe the compiler will typically generate a single unwind block per
function, with single save/restore sequences at the entry and exit
points which save/restore everything the function could possibly
use.  This means that unless a fault occurs in the save/restore
sequences (this would imply a stack overflow etc.) then the resulting
backtrace should be sensible.

In assembly functions, we do sometimes optimise things further, so
that if there is a fast path through the function we don't necessarily
do the full save/restore on that path.  However, deferring state save
like this doesn't fit the "single unwind block per function" model.

The unusual thing about the function being considered here is that
it has two parts where non-stack faults can reasonably occur, with
different save/restore frames in each part.

Cheers
---Dave