Fix backtrace support in THUMB2 mode

The thread_save_fp macro has been defined so that it always reads the fp member of the cpu_context_save struct. However, in the case of THUMB2 the fp is saved not in the fp (r11) member but rather in r7.

This patch changes the way the macro is defined such that FP is read from the correct place depending on whether we are a THUMB2 kernel or not. This enables the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or the function in which a process sleeping when "ps -Al" is invoked.

Signed-off-by Nikolay Borisov <[email protected]>

Reviewed-by Anurag Aggarwal <[email protected]>

---
 arch/arm/include/asm/thread_info.h | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/include/asm/thread_info.h +0 -0

index f989d7c..e4e4208 100644
@@ -114,8 +114,14 @@ static inline struct thread_info *current_thread_info(void)
 	((unsigned long)(task_thread_info(tsk)->cpu_context.pc))

-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-
bounces at lists.infradead.org] On Behalf Of Uwe Kleine-K?nig
Sent: 23 May 2014 10:54
To: Nikolay Borisov
Cc: rric at kernel.org; a.p.zijlstra at chello.nl; jld at mozilla.com; sboyd at codeaurora.org; Will Deacon; dave.long at linaro.org; Dave P Martin; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH RESEND 1/7] ARM: Make thread_save_fp macro aware of THUMB2 mode

Hello,

Quoted 1 line · Nikolay Borisov

The thread_save_fp macro has been defined so that it always reads the

fp member

Quoted 1 line

of the cpu_context_save struct. However, in the case of THUMB2 the fp

is saved

Quoted 3 lines

not in the fp (r11) member but rather in r7.

This patch changes the way the macro is defined such that FP is read

from the

Quoted 1 line

correct place depending on whether we are a THUMB2 kernel or not.

This enables

Quoted 1 line

the backtrace in sitaution such as "echo t > /proc/sysrq-trigger" or

the

Review

Quoted 9 lines

function in which a process sleeping when "ps -Al" is invoked.

Signed-off-by Nikolay Borisov <[email protected]>

Reviewed-by Anurag Aggarwal <[email protected]>

---
 arch/arm/include/asm/thread_info.h | 6 ++++++
 1 file changed, 6 insertions(+)

(quoted diff) +0 -0

b/arch/arm/include/asm/thread_info.h

arch/arm/include/asm/thread_info.h static inline struct thread_info

Quoted 4 lines

index f989d7c..e4e4208 100644

arch/arm/include/asm/thread_info.h +0 -0

@@ -114,8 +114,14 @@ static inline struct thread_info

*current_thread_info(void)

arch/arm/include/asm/thread_info.h static inline struct thread_info *current_thread_info(void) · line 124

Quoted 11 lines

arch/arm/include/asm/thread_info.h +6 -0

 	((unsigned long)(task_thread_info(tsk)->cpu_context.pc))
 #define thread_saved_sp(tsk)	\
 	((unsigned long)(task_thread_info(tsk)->cpu_context.sp))
+
+#ifndef CONFIG_THUMB2_KERNEL
 #define thread_saved_fp(tsk)	\
 	((unsigned long)(task_thread_info(tsk)->cpu_context.fp))
+#else
+#define thread_saved_fp(tsk)	\
+	((unsigned long)(task_thread_info(tsk)->cpu_context.r7))
+#endif

A comment would be nice here describing that it's gcc who forces this on
us. Did you test if the same happens with clang?

Uwe

--
Pengutronix e.K. | Uwe Kleine-K?nig
|
Industrial Linux Solutions | http://www.pengutronix.de/
|

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

Currently all the code which deals with backtrace support assumes that R11 is the frame-pointer. While this is the case for ARM mode and is explicitly documented in the AAPCS, this is not the case for THUMB2 mode.

There is no official document requiring that R11 has to be the frame pointer and GCC uses R7 as FP and given that R7's usage is so intertwined within GCC's mechanics it is unlikely to change, so fixing backtrace in THUMB2 mode seems in order.

This patch series rectifies the problem by first fixing the thread_save_fp macro to reference the correct register. Furthermore, there a lot of repetetive sequences of code such as :

stackframe.fp = pt_regs->ARM_fp
stackframe.lr = pt_regs->ARM_lr

so introducing a function arm_get_current_stack_frame which both hides this repetition and also utilizes teh frame_pointer(regs) macro to reference the correct register depending on the mode.

Finally, change all the call sites so that they utilize the new routine.

Currently all the code which deals with backtrace support assumes that R11 is the frame-pointer. While this is the case for ARM mode and is explicitly documented in the AAPCS, this is not the case for THUMB2 mode.

There is no official document requiring that R11 has to be the frame pointer and GCC uses R7 as FP and given that R7's usage is so intertwined within GCC's mechanics it is unlikely to change, so fixing backtrace in THUMB2 mode seems in order.

This patch series rectifies the problem by first fixing the thread_save_fp macro to reference the correct register. Furthermore, there a lot of repetetive sequences of code such as :

stackframe.fp = pt_regs->ARM_fp
stackframe.lr = pt_regs->ARM_lr

so introducing a function arm_get_current_stack_frame which both hides this repetition and also utilizes teh frame_pointer(regs) macro to reference the correct register depending on the mode.

Finally, change all the call sites so that they utilize the new routine.

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
Sent: 17 June 2014 15:49
To: Nikolay Borisov
Cc: linux-arm-kernel at lists.infradead.org; jld at mozilla.com; rric at kernel.org; a.p.zijlstra at chello.nl; Will Deacon; sboyd at codeaurora.org; dave.long at linaro.org; u.kleine- koenig at pengutronix.de; Dave P Martin
Subject: Re: [PATCH RESEND 0/7] Fix backtrace support in THUMB2 mode

Quoted 1 line · Nikolay Borisov

Currently all the code which deals with backtrace support assumes

that R11

Quoted 1 line

is the frame-pointer. While this is the case for ARM mode and is

explicitly

Quoted 3 lines

documented in the AAPCS, this is not the case for THUMB2 mode.

There is no official document requiring that R11 has to be the frame

pointer

Quoted 1 line

and GCC uses R7 as FP and given that R7's usage is so intertwined

within GCC's

Quoted 1 line

mechanics it is unlikely to change, so fixing backtrace in THUMB2

mode seems

Quoted 4 lines

in order.

This patch series rectifies the problem by first fixing the thread_save_fp macro to reference the correct register. Furthermore,

there

Quoted 5 lines

a lot of repetetive sequences of code such as :

stackframe.fp = pt_regs->ARM_fp
stackframe.lr = pt_regs->ARM_lr

Quoted 5 lines

so introducing a function arm_get_current_stack_frame which both hides this repetition and also utilizes teh frame_pointer(regs) macro to reference the correct register depending on the mode.

Finally, change all the call sites so that they utilize the new

routine.

Can someone please explain to me what the point of this churn is?

Let's start with a summary of Thumb2 kernel building. When a Thumb2 kernel is built, we may build it with or without frame pointers. In either case, we always require the unwinder.

When the unwinder is in use, we don't use the APCS backtracing support. The APCS backtracing support makes use of the frame pointer (and requires
frame pointer support.)

The unwinder, although it is given what would be in the frame pointer register, never reads from this value - the only registers that the unwinder cares about is the stack pointer (so it can read values off the stack), LR (in case PC is zero) and the PC value itself (so it can work out where in the unwind information to start the unwinding process.

The unwinder does write to the FP entry, but this is not really used for anything (in much the same way that it writes to the other registers.) It also prints the FP value in its debugging, though what use that is can be argued.

So, although the code /may/ look weird, and not really conform to what is expected, don't see any bug with the code as it stands today (with the exception of one c_backtrace() call in arm_syscall, which should probably be fixed - but that's an entirely separate problem.)

While we may deem that introducing arm_get_current_stackframe() is a useful cleanup, that's all it should be...

Am I missing something?

--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.