Skip to contentSkip to conversation

Fix backtrace support in THUMB2 mode

PATCH Superseded Nikolay Borisov 4 participants 13 messages May 30, 2014 – Jun 2, 2014

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

v2 cover Fix backtrace support in THUMB2 mode

From: Nikolay Borisov <[email protected]>

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.

Changes since v1:
* Nothing major, just made arg_get_current_stack frame an inline routine.

Nikolay Borisov (7):
ARM: Make thread_save_fp macro aware of THUMB2 mode
ARM: Introduce arm_get_current_stack_frame()
ARM: perf: Make perf use arm_get_current_stackframe
ARM: time: Make use of arm_get_current_stackframe
ARM: unwind: Use arm_get_current_stackframe
ARM: traps: Make use of the frame_pointer macro
ARM: oprofile: Use of arm_get_current_stackframe

 arch/arm/include/asm/ptrace.h      |  6 ++++++
 arch/arm/include/asm/stacktrace.h  | 12 ++++++++++++
 arch/arm/include/asm/thread_info.h |  6 ++++++
 arch/arm/kernel/perf_event.c       |  5 +----
 arch/arm/kernel/time.c             |  5 +----
 arch/arm/kernel/traps.c            |  6 ++++--
 arch/arm/kernel/unwind.c           |  8 +++-----
 arch/arm/oprofile/common.c         |  5 +----
 8 files changed, 34 insertions(+), 19 deletions(-)
Signature · 1 line
1.8.1.5
#1 lore raw

PATCH v2 1/7 ARM: Make thread_save_fp macro aware of THUMB2 mode

From: Nikolay Borisov <[email protected]>

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.

---
 arch/arm/include/asm/thread_info.h | 6 ++++++
 1 file changed, 6 insertions(+)
arch/arm/include/asm/thread_info.h +6 -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))
 #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
 
 extern void crunch_task_disable(struct thread_info *);
 extern void crunch_task_copy(struct thread_info *, void *);
Signature · 1 line
1.8.1.5
#2 lore raw

PATCH v2 2/7 ARM: Introduce arm_get_current_stack_frame()

In reply to Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Currently there are numerous places where "struct pt_regs" are used to populate "struct stackframe", however all of those location do not consider the situation where the kernel might be compiled in THUMB2 mode, in which case the framepointer member of pt_regs become ARM_r7 instead of ARM_fp (r11). Document this idiosyncracy in the definition of "struct stackframe"

The easiest solution is to introduce a new function (in the spirit of
https://groups.google.com/forum/#!topic/linux.kernel/dA2YuUcSpZ4)
which would hide the complexity of initializing the stackframe struct from pt_regs.

Also implement a macro frame_pointer(regs) that would return the correct register so that we can use it in cases where we just require the frame pointer and not a whole struct stackframe

---
 arch/arm/include/asm/ptrace.h     |  6 ++++++
 arch/arm/include/asm/stacktrace.h | 12 ++++++++++++
 2 files changed, 18 insertions(+)
arch/arm/include/asm/ptrace.h +6 -0
index c877654..601264d 100644
@@ -84,6 +84,12 @@ static inline long regs_return_value(struct pt_regs *regs)
 
 #define instruction_pointer(regs)	(regs)->ARM_pc
 
+#ifdef CONFIG_THUMB2_KERNEL
+#define frame_pointer(regs) (regs)->ARM_r7
+#else
+#define frame_pointer(regs) (regs)->ARM_fp
+#endif
+
 static inline void instruction_pointer_set(struct pt_regs *regs,
 					   unsigned long val)
 {
arch/arm/include/asm/stacktrace.h +12 -0
index 4d0a164..6a5b13e 100644
@@ -2,12 +2,24 @@
 #define __ASM_STACKTRACE_H
 
 struct stackframe {
+	/* FP member should hold R7 when CONFIG_THUMB2_KERNEL is enabled.
+	 * and R11 otherwise
+	 */
 	unsigned long fp;
 	unsigned long sp;
 	unsigned long lr;
 	unsigned long pc;
 };
 
+static __always_inline
+void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
+{
+		frame->fp = frame_pointer(regs);
+		frame->sp = regs->ARM_sp;
+		frame->lr = regs->ARM_lr;
+		frame->pc = regs->ARM_pc;
+}
+
 extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
Signature · 1 line
1.8.1.5
#3 lore raw

PATCH v2 2/7 ARM: Introduce arm_get_current_stack_frame()

Quoted 46 lines · Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Currently there are numerous places where "struct pt_regs" are used to populate "struct stackframe", however all of those location do not consider the situation where the kernel might be compiled in THUMB2 mode, in which case the framepointer member of pt_regs become ARM_r7 instead of ARM_fp (r11). Document this idiosyncracy in the definition of "struct stackframe"

The easiest solution is to introduce a new function (in the spirit of
https://groups.google.com/forum/#!topic/linux.kernel/dA2YuUcSpZ4)
which would hide the complexity of initializing the stackframe struct from pt_regs.

Also implement a macro frame_pointer(regs) that would return the correct register so that we can use it in cases where we just require the frame pointer and not a whole struct stackframe

---
 arch/arm/include/asm/ptrace.h     |  6 ++++++
 arch/arm/include/asm/stacktrace.h | 12 ++++++++++++
 2 files changed, 18 insertions(+)
arch/arm/include/asm/ptrace.h +6 -0
index c877654..601264d 100644
@@ -84,6 +84,12 @@ static inline long regs_return_value(struct pt_regs *regs)
 
 #define instruction_pointer(regs)	(regs)->ARM_pc
 
+#ifdef CONFIG_THUMB2_KERNEL
+#define frame_pointer(regs) (regs)->ARM_r7
+#else
+#define frame_pointer(regs) (regs)->ARM_fp
+#endif
+
 static inline void instruction_pointer_set(struct pt_regs *regs,
 					   unsigned long val)
 {
arch/arm/include/asm/stacktrace.h +0 -0
index 4d0a164..6a5b13e 100644

arch/arm/include/asm/stacktrace.h line 7

Quoted 6 lines
arch/arm/include/asm/stacktrace.h +3 -0
@@ -2,12 +2,24 @@
 #define __ASM_STACKTRACE_H
 
 struct stackframe {
+	/* FP member should hold R7 when CONFIG_THUMB2_KERNEL is enabled.
+	 * and R11 otherwise
+	 */

Very minor nit: please reformat the comment like:

	/*
	 * FP member should hold R7 when CONFIG_THUMB2_KERNEL is enabled
	 * and R11 otherwise.
	 */

for consistency with the rest of the kernel (I also moved the full-stop to the end of the sentence).

With that:

Will

#4 lore raw

PATCH v2 2/7 ARM: Introduce arm_get_current_stack_frame()

In reply to Nikolay Borisov
Quoted 28 lines · Nikolay Borisov
arch/arm/include/asm/ptrace.h +6 -0
index c877654..601264d 100644
@@ -84,6 +84,12 @@ static inline long regs_return_value(struct pt_regs *regs)
 
 #define instruction_pointer(regs)	(regs)->ARM_pc
 
+#ifdef CONFIG_THUMB2_KERNEL
+#define frame_pointer(regs) (regs)->ARM_r7
+#else
+#define frame_pointer(regs) (regs)->ARM_fp
+#endif
+
 static inline void instruction_pointer_set(struct pt_regs *regs,
 					   unsigned long val)
 {
arch/arm/include/asm/stacktrace.h +3 -0
index 4d0a164..6a5b13e 100644
@@ -2,12 +2,24 @@
 #define __ASM_STACKTRACE_H
 
 struct stackframe {
+	/* FP member should hold R7 when CONFIG_THUMB2_KERNEL is enabled.
+	 * and R11 otherwise
+	 */

arch/arm/include/asm/stacktrace.h line 17

Quoted 10 lines
arch/arm/include/asm/stacktrace.h +4 -0
 	unsigned long fp;
 	unsigned long sp;
 	unsigned long lr;
 	unsigned long pc;
 };
 
+static __always_inline
+void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe *frame)
+{
+		frame->fp = frame_pointer(regs);

stacktrace.h should now include asm/ptrace.h.

Otherwise

for patch 2 and 7 of this series.

-Robert

Quoted 4 lines
arch/arm/include/asm/stacktrace.h +4 -0
+		frame->sp = regs->ARM_sp;
+		frame->lr = regs->ARM_lr;
+		frame->pc = regs->ARM_pc;
+}
#5 lore raw

PATCH v3 3/7 ARM: perf: Make perf use arm_get_current_stackframe

In reply to Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Make the perf backend use the API so that it correctly references the FP when in THUMB2 mode

---
 arch/arm/kernel/perf_event.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
arch/arm/kernel/perf_event.c +1 -4
index a6bc431..55353fd 100644
@@ -621,10 +621,7 @@ perf_callchain_kernel(struct perf_callchain_entry *entry, struct pt_regs *regs)
 		return;
 	}
 
-	fr.fp = regs->ARM_fp;
-	fr.sp = regs->ARM_sp;
-	fr.lr = regs->ARM_lr;
-	fr.pc = regs->ARM_pc;
+	arm_get_current_stackframe(regs, &fr);
 	walk_stackframe(&fr, callchain_trace, entry);
 }
 
Signature · 1 line
1.8.1.5
#6 lore raw

PATCH v2 4/7 ARM: time: Make use of arm_get_current_stackframe

In reply to Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Make use of the arm_get_current_stackframe api so that the frame pointer is correctly referenced in THUMB2 mode

---
 arch/arm/kernel/time.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
arch/arm/kernel/time.c +1 -4
index 829a96d..0cc7e58 100644
@@ -50,10 +50,7 @@ unsigned long profile_pc(struct pt_regs *regs)
 	if (!in_lock_functions(regs->ARM_pc))
 		return regs->ARM_pc;
 
-	frame.fp = regs->ARM_fp;
-	frame.sp = regs->ARM_sp;
-	frame.lr = regs->ARM_lr;
-	frame.pc = regs->ARM_pc;
+	arm_get_current_stackframe(regs, &frame);
 	do {
 		int ret = unwind_frame(&frame);
 		if (ret < 0)
Signature · 1 line
1.8.1.5
#7 lore raw

PATCH v2 5/7 ARM: unwind: Use arm_get_current_stackframe

In reply to Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Make the unwind code use the correct API so that the frame pointer is extracted from the correct register.

---
 arch/arm/kernel/unwind.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
arch/arm/kernel/unwind.c +3 -5
index 3c21769..7aaec44 100644
@@ -479,12 +479,10 @@ void unwind_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		tsk = current;
 
 	if (regs) {
-		frame.fp = regs->ARM_fp;
-		frame.sp = regs->ARM_sp;
-		frame.lr = regs->ARM_lr;
+		arm_get_current_stackframe(regs, &frame);
 		/* PC might be corrupted, use LR in that case. */
-		frame.pc = kernel_text_address(regs->ARM_pc)
-			 ? regs->ARM_pc : regs->ARM_lr;
+		if (!kernel_text_address(regs->ARM_pc))
+			frame.pc = regs->ARM_lr;
 	} else if (tsk == current) {
 		frame.fp = (unsigned long)__builtin_frame_address(0);
 		frame.sp = current_sp;
Signature · 1 line
1.8.1.5
#8 lore raw

PATCH v2 6/7 ARM: traps: Make use of the frame_pointer macro

In reply to Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Use the newly-introduced frame_pointer macro to extract the correct FP based on whether we are in THUMB2 mode or not.

---
 arch/arm/kernel/traps.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
arch/arm/kernel/traps.c +4 -2
index abd2fc0..c8e4bb7 100644
@@ -31,11 +31,13 @@
 #include <asm/exception.h>
 #include <asm/unistd.h>
 #include <asm/traps.h>
+#include <asm/ptrace.h>
 #include <asm/unwind.h>
 #include <asm/tls.h>
 #include <asm/system_misc.h>
 #include <asm/opcodes.h>
 
+
 static const char *handler[]= {
 	"prefetch abort",
 	"data abort",
@@ -184,7 +186,7 @@ static void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk)
 		tsk = current;
 
 	if (regs) {
-		fp = regs->ARM_fp;
+		fp = frame_pointer(regs);
 		mode = processor_mode(regs);
 	} else if (tsk != current) {
 		fp = thread_saved_fp(tsk);
@@ -719,7 +721,7 @@ asmlinkage int arm_syscall(int no, struct pt_regs *regs)
 		dump_instr("", regs);
 		if (user_mode(regs)) {
 			__show_regs(regs);
-			c_backtrace(regs->ARM_fp, processor_mode(regs));
+			c_backtrace(frame_pointer(regs), processor_mode(regs));
 		}
 	}
 #endif
Signature · 1 line
1.8.1.5
#9 lore raw

PATCH v2 7/7 ARM: oprofile: Use of arm_get_current_stackframe

In reply to Nikolay Borisov

From: Nikolay Borisov <[email protected]>

Use the newly introduced API so that FP is correctly referenced from either R7/R11 based on whether we are running in THUMB2 mode or not.

---
 arch/arm/oprofile/common.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)
arch/arm/oprofile/common.c +1 -4
index 99c63d4b..e6a3c4c 100644
@@ -107,10 +107,7 @@ static void arm_backtrace(struct pt_regs * const regs, unsigned int depth)
 
 	if (!user_mode(regs)) {
 		struct stackframe frame;
-		frame.fp = regs->ARM_fp;
-		frame.sp = regs->ARM_sp;
-		frame.lr = regs->ARM_lr;
-		frame.pc = regs->ARM_pc;
+		arm_get_current_stackframe(regs, &frame);
 		walk_stackframe(&frame, report_trace, &depth);
 		return;
 	}
Signature · 1 line
1.8.1.5
#10 lore raw

v2 cover Fix backtrace support in THUMB2 mode

In reply to Nikolay Borisov
Quoted 18 lines · Nikolay Borisov

From: Nikolay Borisov <[email protected]>

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

Quoted 8 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.

Changes since v1:
* Nothing major, just made arg_get_current_stack frame an inline routine.

I already acked most of these, please can you add my tag?

http://www.spinics.net/lists/arm-kernel/msg334457.html

Will

#11 lore raw

v2 cover Fix backtrace support in THUMB2 mode

In reply to Nikolay Borisov
Quoted 15 lines · On Friday 30 May 2014 17:59:44 Nikolay Borisov wrote:

From: Nikolay Borisov <[email protected]>

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 :

Quoted 8 lines

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.

Sorry for the stupid question, but does your series relate to the #warning "TODO: return_address should use unwind tables" that we see all the time in arch/arm/kernel/return_address.c?

If I remember the story correctly, the problem is that so far we could never use the frame pointer in thumb2 mode, and nobody has implemented a version of that function using the arm unwinder.

Are we now able to use the frame pointer after all?

Arnd

#12 lore raw

v2 cover Fix backtrace support in THUMB2 mode

[Adding Dave Martin as CC]

Quoted 37 lines

-----Original Message-----
From: Arnd Bergmann [mailto:arnd at arndb.de]
Sent: 31 May 2014 19:32
To: linux-arm-kernel at lists.infradead.org
Cc: Nikolay Borisov; rric at kernel.org; Will Deacon; u.kleine- koenig at pengutronix.de
Subject: Re: [PATCH v2 0/7] Fix backtrace support in THUMB2 mode

Quoted 3 lines · On Friday 30 May 2014 17:59:44 Nikolay Borisov wrote:

From: Nikolay Borisov <[email protected]>

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 9 lines

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.

Quoted 7 lines
Quoted 1 line

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

routine.

Sorry for the stupid question, but does your series relate to the #warning "TODO: return_address should use unwind tables" that we see all the time in arch/arm/kernel/return_address.c?

My changes have nothing to do with the use of unwind table whatsoever, I guess this warning is a result of a completely different issue. Probably Dave Martin could shed more light on this.

Quoted 5 lines

If I remember the story correctly, the problem is that so far we could never use the frame pointer in thumb2 mode, and nobody has implemented a version of that function using the arm unwinder.

Are we now able to use the frame pointer after all?

What my changes implement, boils down to referring the correct register Which acts as a frame pointer depending on whether we are running a THUMB2 Kernel or not.

Quoted 3 lines

Arnd

Regards,
Nikolay

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782