[PATCH RESEND 0/7] Fix backtrace support in THUMB2 mode

[PATCH RESEND 0/7] Fix backtrace support in THUMB2 mode

[Nikolay Borisov]

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.


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      | 3 +++
 arch/arm/include/asm/stacktrace.h  | 5 +++++
 arch/arm/include/asm/thread_info.h | 6 ++++++
 arch/arm/kernel/perf_event.c       | 5 +----
 arch/arm/kernel/stacktrace.c       | 9 +++++++++
 arch/arm/kernel/time.c             | 5 +----
 arch/arm/kernel/traps.c            | 6 ++++--
 arch/arm/kernel/unwind.c           | 8 +++-----
 arch/arm/oprofile/common.c         | 5 +----
 9 files changed, 33 insertions(+), 19 deletions(-)

-- 
1.8.1.5

[PATCH RESEND 1/7] ARM: Make thread_save_fp macro aware of THUMB2 mode

[Nikolay Borisov]

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 <Nikolay.Borisov@arm.com>
Reviewed-by: Anurag Aggarwal <anurag19aggarwal@gmail.com>
---
 arch/arm/include/asm/thread_info.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
index f989d7c..e4e4208 100644
--- a/arch/arm/include/asm/thread_info.h
+++ b/arch/arm/include/asm/thread_info.h
@@ -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 *);
-- 
1.8.1.5

[PATCH RESEND 1/7] ARM: Make thread_save_fp macro aware of THUMB2 mode

[Uwe Kleine-König]

Hello,

On Fri, May 23, 2014 at 10:26:30AM +0100, Nikolay Borisov wrote:
> 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 <Nikolay.Borisov@arm.com>
> Reviewed-by: Anurag Aggarwal <anurag19aggarwal@gmail.com>
> ---
>  arch/arm/include/asm/thread_info.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/include/asm/thread_info.h b/arch/arm/include/asm/thread_info.h
> index f989d7c..e4e4208 100644
> --- a/arch/arm/include/asm/thread_info.h
> +++ b/arch/arm/include/asm/thread_info.h
> @@ -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
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/  |

[PATCH RESEND 1/7] ARM: Make thread_save_fp macro aware of THUMB2 mode

[Nikolay Borisov]

> -----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,
> 
> On Fri, May 23, 2014 at 10:26:30AM +0100, Nikolay Borisov wrote:
> > 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 <Nikolay.Borisov@arm.com>
> > Reviewed-by: Anurag Aggarwal <anurag19aggarwal@gmail.com>
> > ---
> >  arch/arm/include/asm/thread_info.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/arm/include/asm/thread_info.h
> b/arch/arm/include/asm/thread_info.h
> > index f989d7c..e4e4208 100644
> > --- a/arch/arm/include/asm/thread_info.h
> > +++ b/arch/arm/include/asm/thread_info.h
> > @@ -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
> 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?
> 

Just checked with one of the LLVM compiler engineers and he said that 
LLVM also conforms to this behaviour e.g. R7 is used as a frame pointer 
in THUMB2 mode. In addition to that, mainline doesn't compile with LLVM 
(yet). 

> Uwe
> 

Regards, 
Nikolay 

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

[PATCH RESEND 2/7] ARM: Introduce arm_get_current_stack_frame()

[Nikolay Borisov]

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

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
---
 arch/arm/include/asm/ptrace.h     | 3 +++
 arch/arm/include/asm/stacktrace.h | 5 +++++
 arch/arm/kernel/stacktrace.c      | 9 +++++++++
 3 files changed, 17 insertions(+)

diff --git a/arch/arm/include/asm/ptrace.h b/arch/arm/include/asm/ptrace.h
index c877654..863acec 100644
--- a/arch/arm/include/asm/ptrace.h
+++ b/arch/arm/include/asm/ptrace.h
@@ -83,6 +83,9 @@ static inline long regs_return_value(struct pt_regs *regs)
 }
 
 #define instruction_pointer(regs)	(regs)->ARM_pc
+#define frame_pointer(regs)				  \
+	IS_ENABLED(CONFIG_THUMB2_KERNEL) ? (regs)->ARM_r7 \
+	: (regs)->ARM_fp				  \
 
 static inline void instruction_pointer_set(struct pt_regs *regs,
 					   unsigned long val)
diff --git a/arch/arm/include/asm/stacktrace.h b/arch/arm/include/asm/stacktrace.h
index 4d0a164..9cbdf36 100644
--- a/arch/arm/include/asm/stacktrace.h
+++ b/arch/arm/include/asm/stacktrace.h
@@ -2,12 +2,17 @@
 #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;
 };
 
+extern void arm_get_current_stackframe(struct pt_regs *regs, struct stackframe
+		*frame);
 extern int unwind_frame(struct stackframe *frame);
 extern void walk_stackframe(struct stackframe *frame,
 			    int (*fn)(struct stackframe *, void *), void *data);
diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
index af4e8c8..5d6ac4d 100644
--- a/arch/arm/kernel/stacktrace.c
+++ b/arch/arm/kernel/stacktrace.c
@@ -43,6 +43,15 @@ int notrace unwind_frame(struct stackframe *frame)
 }
 #endif
 
+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;
+}
+EXPORT_SYMBOL_GPL(arm_get_current_stackframe);
+
 void notrace walk_stackframe(struct stackframe *frame,
 		     int (*fn)(struct stackframe *, void *), void *data)
 {
-- 
1.8.1.5

[PATCH RESEND 2/7] ARM: Introduce arm_get_current_stack_frame()

[Robert Richter]

On 23.05.14 10:26:31, Nikolay Borisov wrote:

> +#define frame_pointer(regs)				  \
> +	IS_ENABLED(CONFIG_THUMB2_KERNEL) ? (regs)->ARM_r7 \
> +	: (regs)->ARM_fp				  \

#ifdef CONFIG_THUMB2_KERNEL

... is better here, this is booleon, not tristate, better to read, and
no mixed c and macro statements either.

> +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;
> +}
> +EXPORT_SYMBOL_GPL(arm_get_current_stackframe);

I am wondering if this should be better implemented as inline function
as this is in the hot path.

-Robert

[PATCH RESEND 3/7] ARM: perf: Make perf use arm_get_current_stackframe

[Nikolay Borisov]

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

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
---
 arch/arm/kernel/perf_event.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index a6bc431..55353fd 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -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);
 }
 
-- 
1.8.1.5

[PATCH RESEND 4/7] ARM: time: Make use of arm_get_current_stackframe

[Nikolay Borisov]

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

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
---
 arch/arm/kernel/time.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/kernel/time.c b/arch/arm/kernel/time.c
index 829a96d..0cc7e58 100644
--- a/arch/arm/kernel/time.c
+++ b/arch/arm/kernel/time.c
@@ -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)
-- 
1.8.1.5

[PATCH RESEND 5/7] ARM: unwind: Use arm_get_current_stackframe

[Nikolay Borisov]

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

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
---
 arch/arm/kernel/unwind.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/unwind.c b/arch/arm/kernel/unwind.c
index 3c21769..7aaec44 100644
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -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;
-- 
1.8.1.5

[PATCH RESEND 6/7] ARM: traps: Make use of the frame_pointer macro

[Nikolay Borisov]

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

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
---
 arch/arm/kernel/traps.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index abd2fc0..c8e4bb7 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -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
-- 
1.8.1.5

[PATCH RESEND 7/7] ARM: oprofile: Use of arm_get_current_stackframe

[Nikolay Borisov]

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.

Signed-off-by: Nikolay Borisov <Nikolay.Borisov@arm.com>
---
 arch/arm/oprofile/common.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/arm/oprofile/common.c b/arch/arm/oprofile/common.c
index 99c63d4b..e6a3c4c 100644
--- a/arch/arm/oprofile/common.c
+++ b/arch/arm/oprofile/common.c
@@ -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;
 	}
-- 
1.8.1.5

[PATCH RESEND 0/7] Fix backtrace support in THUMB2 mode

[Will Deacon]

On Fri, May 23, 2014 at 10:26:29AM +0100, Nikolay Borisov wrote:
> 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.

For everything apart from patch 2 (where Robert pointed out that
arm_get_current_stackframe would be better inline):

  Acked-by: Will Deacon <will.deacon@arm.com>

Will

[PATCH RESEND 0/7] Fix backtrace support in THUMB2 mode

[Russell King - ARM Linux]

On Fri, May 23, 2014 at 10:26:29AM +0100, Nikolay Borisov wrote:
> 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.

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.

[PATCH RESEND 0/7] Fix backtrace support in THUMB2 mode

[Nikolay Borisov]

> -----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
> 
> On Fri, May 23, 2014 at 10:26:29AM +0100, Nikolay Borisov wrote:
> > 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.
> 
> Can someone please explain to me what the point of this churn is?
> 

During my testing with a THUMB2 kernel it turned out that no stacktrace
information was being printed when either magic-sysrq was used
or the 'ps -Al' command executed. The commit message of patch 8069/1 does
list those uses cases as failures resulting from this bug. 

Are you able to obtain backtrace support from a THUMB2 kernel using the 
the correct magic sysrq to dump the current threads stacktrace?

This thread started by Jed David also sheds some light on the issue: 
https://lkml.org/lkml/2013/7/12/469


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