ARM: unwinder: Fix handling of 'fp' in thumb2 mode
PATCH v2 1/1 ARM: unwinder: Fix handling of 'fp' 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.
Another place where similar change is requried is when working with pt_reg. regs->ARM_fp is defined such that r11 is always referenced as the frame pointer, so the definition of ARM_fp needs to change depending on the type of kernel we are running.
Changes since v1:
* Added changes for the pt_reg structure.
handle the case when we've got regs present
--- arch/arm/include/asm/thread_info.h | 6 ++++++ arch/arm/include/uapi/asm/ptrace.h | 4 ++++ 2 files changed, 10 insertions(+)
arch/arm/include/asm/thread_info.h +6 -0
@@ -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 *);
arch/arm/include/uapi/asm/ptrace.h +4 -0
@@ -131,7 +131,11 @@ struct pt_regs {#define ARM_lr uregs[14]#define ARM_sp uregs[13]#define ARM_ip uregs[12] +#ifndef CONFIG_THUMB2_KERNEL #define ARM_fp uregs[11] +#else +#define ARM_fp uregs[7] +#endif #define ARM_r10 uregs[10]#define ARM_r9 uregs[9]#define ARM_r8 uregs[8]
Signature · 1 line
1.8.1.5
PATCH v2 1/1 ARM: unwinder: Fix handling of 'fp' in thumb2 mode
Seems good.
I would like to know what kind of testing you have done that for this, Other than that you can add my review statement
Quoted 14 lines · Nikolay Borisov
The
thread_save_fpmacro has been defined so that it always reads the fp member of thecpu_context_savestruct. 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.Another place where similar change is requried is when working with
pt_reg.regs->ARM_fpis defined such that r11 is always referenced as the frame pointer, so the definition ofARM_fpneeds to change depending on the type of kernel we are running.
Quoted 4 lines
Changes since v1:
* Added changes for thept_regstructure.Signed-off-by Nikolay Borisov <[email protected]>
Quoted 46 lines
handle the case when we've got regs present
--- arch/arm/include/asm/thread_info.h | 6 ++++++ arch/arm/include/uapi/asm/ptrace.h | 4 ++++ 2 files changed, 10 insertions(+)arch/arm/include/asm/thread_info.h +6
-0@@ -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 *);arch/arm/include/uapi/asm/ptrace.h +4
-1@@ -131,7 +131,11 @@ struct pt_regs {#define ARM_lr uregs[14]#define ARM_sp uregs[13]#define ARM_ip uregs[12] +#ifndef CONFIG_THUMB2_KERNEL #define ARM_fp uregs[11] +#else +#define ARM_fp uregs[7] +#endif #define ARM_r10 uregs[10]#define ARM_r9 uregs[9]#define ARM_r8 uregs[8] --1.8.1.5
PATCH v2 1/1 ARM: unwinder: Fix handling of 'fp' in thumb2 mode
Hi,
Quoted 9 lines
-----Original Message-----
From: Anurag Aggarwal [mailto:anurag19aggarwal at gmail.com]
Sent: 03 May 2014 07:57
To: Nikolay Borisov
Cc: linux-arm-kernel at lists.infradead.org; Dave P Martin; Catalin Marinas
Subject: Re: [PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 mode
Quoted 5 lines
Seems good.
I would like to know what kind of testing you have done that for this, Other than that you can add my review statement
Without this patch if you use the magic sysrq to print backtrace of all the running tasks in the system those that are sleeping would not print a backtrace. With the patch this works correctly. Another case where you can observe the change is in the invocation of the "ps -Al" which shows (among other things) which function the sleeping task is blocked in.
Quoted 80 lines
Quoted 1 line · Nikolay Borisov
The
thread_save_fpmacro has been defined so that it always reads thefp member
Quoted 1 line
of the
cpu_context_savestruct. However, in the case of THUMB2 the fpis 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" orthe
Quoted 3 lines
function in which a process sleeping when "ps -Al" is invoked.
Another place where similar change is requried is when working with
pt_reg.Quoted 1 line
regs->ARM_fpis defined such that r11 is always referenced as theframe pointer,
Quoted 1 line
so the definition of
ARM_fpneeds to change depending on the type ofkernel we
Quoted 6 lines
are running.
Changes since v1:
* Added changes for thept_regstructure.Signed-off-by Nikolay Borisov <[email protected]>Reviewed-by Anurag Aggarwal <[email protected]>Review
Quoted 8 lines
handle the case when we've got regs present
--- arch/arm/include/asm/thread_info.h | 6 ++++++ arch/arm/include/uapi/asm/ptrace.h | 4 ++++ 2 files changed, 10 insertions(+)(quoted diff) +0
-0
b/arch/arm/include/asm/thread_info.harch/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)Review
Quoted 15 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 extern void crunch_task_disable (struct thread_info *);extern void crunch_task_copy (struct thread_info *,void *);(quoted diff) +0
-0
b/arch/arm/include/uapi/asm/ptrace.hQuoted 19 lines
index 5af0ed1..92e1bae 100644
arch/arm/include/uapi/asm/ptrace.h +4
-1@@ -131,7 +131,11 @@ struct pt_regs {#define ARM_lr uregs[14]#define ARM_sp uregs[13]#define ARM_ip uregs[12] +#ifndef CONFIG_THUMB2_KERNEL #define ARM_fp uregs[11] +#else +#define ARM_fp uregs[7] +#endif #define ARM_r10 uregs[10]#define ARM_r9 uregs[9]#define ARM_r8 uregs[8] --1.8.1.5
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH v2 1/1 ARM: unwinder: Fix handling of 'fp' in thumb2 mode
The test cases seems good, although I haven't been able to verify them myself. Other than that everything seems good.
Quoted 111 lines · Nikolay Borisov
Hi,
Quoted 9 lines
-----Original Message-----
From: Anurag Aggarwal [mailto:anurag19aggarwal at gmail.com]
Sent: 03 May 2014 07:57
To: Nikolay Borisov
Cc: linux-arm-kernel at lists.infradead.org; Dave P Martin; Catalin Marinas
Subject: Re: [PATCH v2] ARM: unwinder: Fix handling of 'fp' in thumb2 modeQuoted 5 lines
Seems good.
I would like to know what kind of testing you have done that for this, Other than that you can add my review statement
Without this patch if you use the magic sysrq to print backtrace of all the running tasks in the system those that are sleeping would not print a backtrace. With the patch this works correctly. Another case where you can observe the change is in the invocation of the "ps -Al" which shows (among other things) which function the sleeping task is blocked in.
Quoted 72 lines
Quoted 1 line · Nikolay Borisov
The
thread_save_fpmacro has been defined so that it always reads thefp member
Quoted 1 line
of the
cpu_context_savestruct. However, in the case of THUMB2 the fpis 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" orthe
Quoted 3 lines
function in which a process sleeping when "ps -Al" is invoked.
Another place where similar change is requried is when working with
pt_reg.Quoted 1 line
regs->ARM_fpis defined such that r11 is always referenced as theframe pointer,
Quoted 1 line
so the definition of
ARM_fpneeds to change depending on the type ofkernel we
Quoted 6 lines
are running.
Changes since v1:
* Added changes for thept_regstructure.Signed-off-by Nikolay Borisov <[email protected]>Reviewed-by Anurag Aggarwal <[email protected]>Review
Quoted 8 lines
handle the case when we've got regs present
--- arch/arm/include/asm/thread_info.h | 6 ++++++ arch/arm/include/uapi/asm/ptrace.h | 4 ++++ 2 files changed, 10 insertions(+)(quoted diff) +0
-0
b/arch/arm/include/asm/thread_info.harch/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)Review
Quoted 15 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 extern void crunch_task_disable (struct thread_info *);extern void crunch_task_copy (struct thread_info *,void *);(quoted diff) +0
-0
b/arch/arm/include/uapi/asm/ptrace.hQuoted 11 lines
index 5af0ed1..92e1bae 100644
arch/arm/include/uapi/asm/ptrace.h +3
-0@@ -131,7 +131,11 @@ struct pt_regs {#define ARM_lr uregs[14]#define ARM_sp uregs[13]#define ARM_ip uregs[12] +#ifndef CONFIG_THUMB2_KERNEL #define ARM_fp uregs[11] +#else +#define ARM_fp uregs[7]Quoted 8 lines
Quoted 8 lines
arch/arm/include/uapi/asm/ptrace.h +1
-1+#endif #define ARM_r10 uregs[10]#define ARM_r9 uregs[9]#define ARM_r8 uregs[8] --1.8.1.5
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 1 line
Anurag Aggarwal
PATCH v2 1/1 ARM: unwinder: Fix handling of 'fp' in thumb2 mode
In reply to Nikolay BorisovQuoted 9 lines · Nikolay Borisov
The
thread_save_fpmacro has been defined so that it always reads the fp member of thecpu_context_savestruct. 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.
Quoted 4 lines
Another place where similar change is requried is when working with
pt_reg.regs->ARM_fpis defined such that r11 is always referenced as the frame pointer, so the definition ofARM_fpneeds to change depending on the type of kernel we are running.
(Keep your S-o-b line here, but move the "change log" stuff after the --- tearoff line so it doesn't form part of the commit message.)
Quoted 35 lines
Changes since v1:
* Added changes for thept_regstructure.Signed-off-by Nikolay Borisov <[email protected]>handle the case when we've got regs present
--- arch/arm/include/asm/thread_info.h | 6 ++++++ arch/arm/include/uapi/asm/ptrace.h | 4 ++++ 2 files changed, 10 insertions(+)arch/arm/include/asm/thread_info.h +6
-0@@ -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 *);arch/arm/include/uapi/asm/ptrace.h +0
-0
arch/arm/include/uapi/asm/ptrace.h struct pt_regs { · line 138
Quoted 8 lines
arch/arm/include/uapi/asm/ptrace.h +4
-0@@ -131,7 +131,11 @@ struct pt_regs {#define ARM_lr uregs[14]#define ARM_sp uregs[13]#define ARM_ip uregs[12] +#ifndef CONFIG_THUMB2_KERNEL #define ARM_fp uregs[11] +#else +#define ARM_fp uregs[7] +#endif
I'm not sure about this. There seem to be other bits of code that assume the ARM_fp name maps to the fixed register defined by the PCS, i.e. always r11 and not the "frame pointer". For example, see arch/arm/kernel/kgdb.c: I think we'll end up passing the wrong regs to gdb in this case.
It may be better to work around the usage of these #defines locally in the unwinder code instead.
Cheers
---Dave