diff mbox

[8/8] powerpc: Fix signal handling in backtrace

Message ID 1494257212-524-8-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella Netto May 8, 2017, 3:26 p.m. UTC
Now with read consolidation which uses SYSCALL_CANCEL macro, a frame
pointer is created in the syscall code and this makes the powerpc
backtrace obtain a bogus entry for the signal handling patch.

It is because it does not setup the correct frame pointer register
(r1) based on the saved value from the kernel sigreturn.  It was not
failing because the syscall frame pointer register was the same one
for the next frame (the function that actually called the syscall).

This patch fixes it by setup the next stack frame using the saved
one by the kernel sigreturn.  It fixes tst-backtrace{5,6} after
the read consolidation patch.

Checked on powerpc-linux-gnu and powerpc64le-linux-gnu.

	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
	void* for argument type and use VDSO_SYMBOL macro.
	(is_sigtramp_address_rt): Likewise.
	(__backtrace): Setup expected frame pointer address for signal
	handling.
	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
	void* for argumetn type and use VSDO_SYMBOL macro.
	(__backtrace): Setup expected frame pointer address for signal
	handling.
---
 ChangeLog                             | 10 ++++++++++
 sysdeps/powerpc/powerpc32/backtrace.c | 17 ++++++++++-------
 sysdeps/powerpc/powerpc64/backtrace.c | 17 ++++++++++-------
 3 files changed, 30 insertions(+), 14 deletions(-)

Comments

Tulio Magno Quites Machado Filho May 9, 2017, 12:11 p.m. UTC | #1
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> Now with read consolidation which uses SYSCALL_CANCEL macro, a frame
> pointer is created in the syscall code and this makes the powerpc
> backtrace obtain a bogus entry for the signal handling patch.
>
> It is because it does not setup the correct frame pointer register
> (r1) based on the saved value from the kernel sigreturn.  It was not
> failing because the syscall frame pointer register was the same one
> for the next frame (the function that actually called the syscall).
>
> This patch fixes it by setup the next stack frame using the saved
> one by the kernel sigreturn.  It fixes tst-backtrace{5,6} after
> the read consolidation patch.
>
> Checked on powerpc-linux-gnu and powerpc64le-linux-gnu.
>
> 	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
> 	void* for argument type and use VDSO_SYMBOL macro.
> 	(is_sigtramp_address_rt): Likewise.
> 	(__backtrace): Setup expected frame pointer address for signal
> 	handling.
> 	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use

The files are duplicated in the ChangeLog.

Looks good to me with that fix as soon as patch #4 is integrated.

Thanks!
Adhemerval Zanella Netto May 9, 2017, 1:13 p.m. UTC | #2
On 09/05/2017 09:11, Tulio Magno Quites Machado Filho wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> Now with read consolidation which uses SYSCALL_CANCEL macro, a frame
>> pointer is created in the syscall code and this makes the powerpc
>> backtrace obtain a bogus entry for the signal handling patch.
>>
>> It is because it does not setup the correct frame pointer register
>> (r1) based on the saved value from the kernel sigreturn.  It was not
>> failing because the syscall frame pointer register was the same one
>> for the next frame (the function that actually called the syscall).
>>
>> This patch fixes it by setup the next stack frame using the saved
>> one by the kernel sigreturn.  It fixes tst-backtrace{5,6} after
>> the read consolidation patch.
>>
>> Checked on powerpc-linux-gnu and powerpc64le-linux-gnu.
>>
>> 	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
>> 	void* for argument type and use VDSO_SYMBOL macro.
>> 	(is_sigtramp_address_rt): Likewise.
>> 	(__backtrace): Setup expected frame pointer address for signal
>> 	handling.
>> 	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
> 
> The files are duplicated in the ChangeLog.
> 
> Looks good to me with that fix as soon as patch #4 is integrated.
> 
> Thanks!
> 

Second item should be sysdeps/powerpc/powerpc64/backtrace.c, I will fix.
Thanks.
diff mbox

Patch

diff --git a/ChangeLog b/ChangeLog
index bc97d16..dcda6c6 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,15 @@ 
 2016-05-08  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
 
+	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
+	void* for argument type and use VDSO_SYMBOL macro.
+	(is_sigtramp_address_rt): Likewise.
+	(__backtrace): Setup expected frame pointer address for signal
+	handling.
+	* sysdeps/powerpc/powerpc32/backtrace.c (is_sigtramp_address): Use
+	void* for argumetn type and use VSDO_SYMBOL macro.
+	(__backtrace): Setup expected frame pointer address for signal
+	handling.
+
 	* sysdeps/unix/sysv/linux/writev.c: New file.
 
 	* sysdeps/unix/sysv/linux/readv.c: New file.
diff --git a/sysdeps/powerpc/powerpc32/backtrace.c b/sysdeps/powerpc/powerpc32/backtrace.c
index b60ac32..3940621 100644
--- a/sysdeps/powerpc/powerpc32/backtrace.c
+++ b/sysdeps/powerpc/powerpc32/backtrace.c
@@ -52,10 +52,10 @@  struct signal_frame_32 {
 };
 
 static inline int
-is_sigtramp_address (unsigned int nip)
+is_sigtramp_address (void *nip)
 {
 #ifdef SHARED
-  if (nip == (unsigned int)__vdso_sigtramp32)
+  if (nip == VDSO_SYMBOL (sigtramp32))
     return 1;
 #endif
   return 0;
@@ -69,10 +69,10 @@  struct rt_signal_frame_32 {
 };
 
 static inline int
-is_sigtramp_address_rt (unsigned int nip)
+is_sigtramp_address_rt (void * nip)
 {
 #ifdef SHARED
-  if (nip == (unsigned int)__vdso_sigtramp_rt32)
+  if (nip == VDSO_SYMBOL (sigtramp_rt32))
     return 1;
 #endif
   return 0;
@@ -100,20 +100,23 @@  __backtrace (void **array, int size)
 
       /* Check if the symbol is the signal trampoline and get the interrupted
        * symbol address from the trampoline saved area.  */
-      if (is_sigtramp_address ((unsigned int)current->return_address))
+      if (is_sigtramp_address (current->return_address))
 	{
 	  struct signal_frame_32 *sigframe =
 	    (struct signal_frame_32*) current;
           gregset = &sigframe->mctx.gregs;
         }
-      else if (is_sigtramp_address_rt ((unsigned int)current->return_address))
+      else if (is_sigtramp_address_rt (current->return_address))
 	{
 	  struct rt_signal_frame_32 *sigframe =
             (struct rt_signal_frame_32*) current;
           gregset = &sigframe->uc.uc_mcontext.uc_regs->gregs;
         }
       if (gregset)
-	array[++count] = (void*)((*gregset)[PT_NIP]);
+	{
+	  array[++count] = (void*)((*gregset)[PT_NIP]);
+	  current = (void*)((*gregset)[PT_R1]);
+	}
     }
 
   /* It's possible the second-last stack frame can't return
diff --git a/sysdeps/powerpc/powerpc64/backtrace.c b/sysdeps/powerpc/powerpc64/backtrace.c
index 83b963e..723948d 100644
--- a/sysdeps/powerpc/powerpc64/backtrace.c
+++ b/sysdeps/powerpc/powerpc64/backtrace.c
@@ -16,10 +16,12 @@ 
    License along with the GNU C Library; see the file COPYING.LIB.  If
    not, see <http://www.gnu.org/licenses/>.  */
 
-#include <execinfo.h>
 #include <stddef.h>
 #include <string.h>
 #include <signal.h>
+#include <stdint.h>
+
+#include <execinfo.h>
 #include <libc-vdso.h>
 
 /* This is the stack layout we see with every stack frame.
@@ -37,7 +39,7 @@ 
 struct layout
 {
   struct layout *next;
-  long condition_register;
+  long int condition_register;
   void *return_address;
 };
 
@@ -47,16 +49,16 @@  struct layout
    dummy frame to make it look like it has a caller.  */
 struct signal_frame_64 {
 #define SIGNAL_FRAMESIZE 128
-  char            dummy[SIGNAL_FRAMESIZE];
+  char dummy[SIGNAL_FRAMESIZE];
   struct ucontext uc;
   /* We don't care about the rest, since the IP value is at 'uc' field.  */
 };
 
 static inline int
-is_sigtramp_address (unsigned long nip)
+is_sigtramp_address (void *nip)
 {
 #ifdef SHARED
-  if (nip == (unsigned long)__vdso_sigtramp_rt64)
+  if (nip == VDSO_SYMBOL (sigtramp_rt64))
     return 1;
 #endif
   return 0;
@@ -82,10 +84,11 @@  __backtrace (void **array, int size)
 
       /* Check if the symbol is the signal trampoline and get the interrupted
        * symbol address from the trampoline saved area.  */
-      if (is_sigtramp_address ((unsigned long)current->return_address))
+      if (is_sigtramp_address (current->return_address))
         {
 	  struct signal_frame_64 *sigframe = (struct signal_frame_64*) current;
-          array[++count] = (void*)sigframe->uc.uc_mcontext.gp_regs[PT_NIP];
+          array[++count] = (void*) sigframe->uc.uc_mcontext.gp_regs[PT_NIP];
+	  current = (void*) sigframe->uc.uc_mcontext.gp_regs[PT_R1];
 	}
     }