Re: [PATCH] arm: Add unwinding support for division functions

[Laura Abbott]

On Tue, May 17, 2011 8:07 am, Dave Martin wrote:
> On Thu, May 12, 2011 at 06:24:53PM -0700, Laura Abbott wrote:
>> The software division functions never had unwinding annotations
>> added. Currently, when a division by zero occurs the backtrace shown
>> will stop at Ldiv0 or some completely unrelated function. Add
>> unwinding annotations in hopes of getting a more useful backtrace
>> when a division by zero occurs.
>
> Definitely a good idea.
>
>>
>> Signed-off-by: Laura Abbott <lauraa@xxxxxxxxxxxxxx>
>> ---
>>  arch/arm/lib/lib1funcs.S |   27 +++++++++++++++++++++------
>>  1 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/arm/lib/lib1funcs.S b/arch/arm/lib/lib1funcs.S
>> index 6dc0648..63b75df 100644
>> --- a/arch/arm/lib/lib1funcs.S
>> +++ b/arch/arm/lib/lib1funcs.S
>> @@ -35,7 +35,7 @@ Boston, MA 02111-1307, USA.  */
>>
>>  #include <linux/linkage.h>
>>  #include <asm/assembler.h>
>> -
>> +#include <asm/unwind.h>
>>
>>  .macro ARM_DIV_BODY dividend, divisor, result, curbit
>>
>> @@ -207,6 +207,7 @@ Boston, MA 02111-1307, USA.  */
>>
>>  ENTRY(__udivsi3)
>>  ENTRY(__aeabi_uidiv)
>> +UNWIND(.fnstart)
>>
>>  	subs	r2, r1, #1
>>  	moveq	pc, lr
>> @@ -230,10 +231,12 @@ ENTRY(__aeabi_uidiv)
>>  	mov	r0, r0, lsr r2
>>  	mov	pc, lr
>>
>> +UNWIND(.fnend)
>>  ENDPROC(__udivsi3)
>>  ENDPROC(__aeabi_uidiv)
>>
>>  ENTRY(__umodsi3)
>> +UNWIND(.fnstart)
>>
>>  	subs	r2, r1, #1			@ compare divisor with 1
>>  	bcc	Ldiv0
>> @@ -247,10 +250,12 @@ ENTRY(__umodsi3)
>>
>>  	mov	pc, lr
>>
>> +UNWIND(.fnend)
>>  ENDPROC(__umodsi3)
>>
>>  ENTRY(__divsi3)
>>  ENTRY(__aeabi_idiv)
>> +UNWIND(.fnstart)
>>
>>  	cmp	r1, #0
>>  	eor	ip, r0, r1			@ save the sign of the result.
>> @@ -287,10 +292,12 @@ ENTRY(__aeabi_idiv)
>>  	rsbmi	r0, r0, #0
>>  	mov	pc, lr
>>
>> +UNWIND(.fnend)
>>  ENDPROC(__divsi3)
>>  ENDPROC(__aeabi_idiv)
>>
>>  ENTRY(__modsi3)
>> +UNWIND(.fnstart)
>>
>>  	cmp	r1, #0
>>  	beq	Ldiv0
>> @@ -310,11 +317,14 @@ ENTRY(__modsi3)
>>  	rsbmi	r0, r0, #0
>>  	mov	pc, lr
>>
>> +UNWIND(.fnend)
>>  ENDPROC(__modsi3)
>>
>>  #ifdef CONFIG_AEABI
>>
>>  ENTRY(__aeabi_uidivmod)
>> +UNWIND(.fnstart)
>> +UNWIND(.save {r0, r1, ip, lr}	)
>>
>>  	stmfd	sp!, {r0, r1, ip, lr}
>>  	bl	__aeabi_uidiv
>> @@ -323,10 +333,12 @@ ENTRY(__aeabi_uidivmod)
>>  	sub	r1, r1, r3
>>  	mov	pc, lr
>>
>> +UNWIND(.fnend)
>>  ENDPROC(__aeabi_uidivmod)
>>
>>  ENTRY(__aeabi_idivmod)
>> -
>> +UNWIND(.fnstart)
>> +UNWIND(.save {r0, r1, ip, lr}	)
>>  	stmfd	sp!, {r0, r1, ip, lr}
>>  	bl	__aeabi_idiv
>>  	ldmfd	sp!, {r1, r2, ip, lr}
>> @@ -334,15 +346,18 @@ ENTRY(__aeabi_idivmod)
>>  	sub	r1, r1, r3
>>  	mov	pc, lr
>>
>> +UNWIND(.fnend)
>>  ENDPROC(__aeabi_idivmod)
>>
>>  #endif
>>
>> -Ldiv0:
>> -
>> +ENTRY(Ldiv0)
>
> There's no reason to make Ldiv0 global and pollute the global namespace.
> I suggest you remove ENTRY() here, but keep the ENDPROC() so that the
> symbol type and size information is correct.
>
> If CONFIG_KALLSYMS is enabled, local symbols are included in the
> kallsyms table, so you still get a sensibly-named backtrace entry without
> needing to make the symbol global.
>
Okay, I'll get rid of the ENTRY annotation for Ldiv0.

>> +UNWIND(.fnstart)
>> +UNWIND(.pad #4)
>> +UNWIND(.save {lr})
>>  	str	lr, [sp, #-8]!
>>  	bl	__div0
>>  	mov	r0, #0			@ About as wrong as it could be.
>>  	ldr	pc, [sp], #8
>> -
>> -
>> +UNWIND(.fnend)
>> +ENDPROC(Ldiv0)
>> --
>> 1.7.3.3
>
> Otherwise, the patch looks sound to me.
>
> With it, I get plausible backtraces, e.g.:
> [ 1376.909088] Division by zero in kernel.
> [ 1376.909149] [<c0073551>] (unwind_backtrace+0x1/0xa0) from [<c02b83a3>]
> (Ldiv0+0x9/0x12)
> [ 1376.909149] [<c02b83a3>] (Ldiv0+0x9/0x12) from [<bf88c033>]
> (init+0x32/0x6f [div0])
> [ 1376.909179] [<bf88c033>] (init+0x32/0x6f [div0]) from [<c006951b>]
> (do_one_initcall+0x2b/0x11c)
> [ 1376.909210] [<c006951b>] (do_one_initcall+0x2b/0x11c) from [<c00c8c4b>]
> (sys_init_module+0xc7/0x144c)
> [ 1376.909240] [<c00c8c4b>] (sys_init_module+0xc7/0x144c) from
> [<c006df81>] (ret_fast_syscall+0x1/0x44)
>
> (Tested using a trivial test module which just calls a function which
> divides by zero from its init function.)
>
> Tested-by: Dave Martin <dave.martin@xxxxxxxxxx>
>
> Cheers
> ---Dave
>

Thanks for reviewing and testing

Laura

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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/