[PATCH] Make mips-linux signal frame unwinding more robust.

[PATCH] Make mips-linux signal frame unwinding more robust.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

Greetings,

The current signal frame unwinding code in mips-linux-tdep.c assumes a 
constant offset from the signal return trampoline to the signal frame. 
The assumption does not hold for all kernels.  Specifically those that 
have to be compiled with ICACHE_REFILLS_WORKAROUND_WAR set (SGI O2 for 
example).  In the near future, it is likely that the assumption will 
cease to hold universally, as we are attempting to move the signal 
return trampoline off the stack entirely.

The libgcc unwinder already gets this right by using the signal frame's 
SP to locate the sigcontext.

This patch makes gdb follow suit and find the sigcontext_base using the 
signal frame's SP rather than an offset from the trampoline.

Tested on mips64-linux with no regressions (and more than 100 improvements).

OK to commit?

How about on the 7.1 branch?

2010-02-22  David Daney  <ddaney@caviumnetworks.com>

	* mips-linux-tdep.c: Update struct sigframe comments.
	(mips_linux_o32_sigframe_init): Set sigcontext_base using
	this_frame's sp.
	(mips_linux_n32n64_sigframe_init): Same.

[-- Attachment #2: gdb.patch --]
[-- Type: text/plain, Size: 1691 bytes --]

Index: gdb/mips-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-tdep.c,v
retrieving revision 1.81
diff -u -p -r1.81 mips-linux-tdep.c
--- gdb/mips-linux-tdep.c	1 Jan 2010 07:31:37 -0000	1.81
+++ gdb/mips-linux-tdep.c	19 Feb 2010 21:58:32 -0000
@@ -797,7 +797,7 @@ static const struct tramp_frame mips_lin
 
    struct sigframe {
      u32 sf_ass[4];            [argument save space for o32]
-     u32 sf_code[2];           [signal trampoline]
+     u32 sf_code[2];           [signal trampoline or fill]
      struct sigcontext sf_sc;
      sigset_t sf_mask;
    };
@@ -827,7 +827,7 @@ static const struct tramp_frame mips_lin
 
    struct rt_sigframe {
      u32 rs_ass[4];            [argument save space for o32]
-     u32 rs_code[2]            [signal trampoline]
+     u32 rs_code[2]            [signal trampoline or fill]
      struct siginfo rs_info;
      struct ucontext rs_uc;
    };
@@ -871,7 +871,7 @@ mips_linux_o32_sigframe_init (const stru
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int ireg, reg_position;
-  CORE_ADDR sigcontext_base = func - SIGFRAME_CODE_OFFSET;
+  CORE_ADDR sigcontext_base = get_frame_sp (this_frame);
   const struct mips_regnum *regs = mips_regnum (gdbarch);
   CORE_ADDR regs_base;
 
@@ -1038,7 +1038,7 @@ mips_linux_n32n64_sigframe_init (const s
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int ireg, reg_position;
-  CORE_ADDR sigcontext_base = func - SIGFRAME_CODE_OFFSET;
+  CORE_ADDR sigcontext_base =  get_frame_sp (this_frame);
   const struct mips_regnum *regs = mips_regnum (gdbarch);
 
   if (self == &mips_linux_n32_rt_sigframe)

Re: [PATCH] Make mips-linux signal frame unwinding more robust.

[Joel Brobecker]

> This patch makes gdb follow suit and find the sigcontext_base using
> the signal frame's SP rather than an offset from the trampoline.

Is there a document that explains that the sigcontext structure is
always going to be at the frame's SP?

I don't know mips-linux, but something looked funny to me: You avoid
the use of SIGFRAME_CODE_OFFSET to compute the address where the sigcontext
structure is located, but you still use it to compute the frame base
address (used when building the frame ID).  Is the frame base address
still a constant offset from FUNC, or does the frame ID base address
also needs to be changed.

I believe that Daniel J has a good knowledge of mips-linux, and would be
an ideal reviewer.  If he doesn't have time, though, I'm OK with approving
a patch for the HEAD branch.  For the 7.1 branch, though, I'd rather have
a more knowledgeable opinion.

-- 
Joel

Re: [PATCH] Make mips-linux signal frame unwinding more robust.

[David Daney]

On 02/25/2010 09:47 AM, Joel Brobecker wrote:
>> This patch makes gdb follow suit and find the sigcontext_base using
>> the signal frame's SP rather than an offset from the trampoline.
>
> Is there a document that explains that the sigcontext structure is
> always going to be at the frame's SP?

No official document, however the principle of maintaining binary 
compatibility is important, and recognized by the kernel maintainers.

There are several things that constrain the the changes that can be made 
in the kernel:

1) The glibc setcontext API as discussed here:

http://www.linux-mips.org/cgi-bin/mesg.cgi?a=linux-mips&i=alpine.DEB.1.10.0902282326580.4064%40tp.orcam.me.uk

2) libgcc's unwinder:

http://gcc.gnu.org/viewcvs/trunk/gcc/config/mips/linux-unwind.h?revision=145841&view=markup

>
> I don't know mips-linux, but something looked funny to me: You avoid
> the use of SIGFRAME_CODE_OFFSET to compute the address where the sigcontext
> structure is located, but you still use it to compute the frame base
> address (used when building the frame ID).  Is the frame base address
> still a constant offset from FUNC, or does the frame ID base address
> also needs to be changed.

Right, I missed that part.  When it started working, I stopped patching. 
  I will take another look at that part.


>
> I believe that Daniel J has a good knowledge of mips-linux, and would be
> an ideal reviewer.  If he doesn't have time, though, I'm OK with approving
> a patch for the HEAD branch.  For the 7.1 branch, though, I'd rather have
> a more knowledgeable opinion.
>

Thanks,
David Daney

Re: [PATCH] Make mips-linux signal frame unwinding more robust.

[David Daney]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

On 02/25/2010 10:48 AM, David Daney wrote:
> On 02/25/2010 09:47 AM, Joel Brobecker wrote:
[...]
>>
>> I don't know mips-linux, but something looked funny to me: You avoid
>> the use of SIGFRAME_CODE_OFFSET to compute the address where the
>> sigcontext
>> structure is located, but you still use it to compute the frame base
>> address (used when building the frame ID). Is the frame base address
>> still a constant offset from FUNC, or does the frame ID base address
>> also needs to be changed.
>
> Right, I missed that part. When it started working, I stopped patching.
> I will take another look at that part.
>
>

Here is the revised patch fixing the issue Joel noted.

 From the original message:

   The current signal frame unwinding code in mips-linux-tdep.c assumes
   a constant offset from the signal return trampoline to the signal
   frame. The assumption does not hold for all kernels.  Specifically
   those that have to be compiled with ICACHE_REFILLS_WORKAROUND_WAR
   set (SGI O2 for example).  In the near future, it is likely that the
   assumption will cease to hold universally, as we are attempting to
   move the signal return trampoline off the stack entirely.

   The libgcc unwinder already gets this right by using the signal
   frame's SP to locate the sigcontext.

   This patch makes gdb follow suit and find the sigcontext_base using
   the signal frame's SP rather than an offset from the trampoline.

Tested on mips64-linux with no regressions (and more than 100
improvements).

OK to commit?

How about on the 7.1 branch?


2010-02-26  David Daney  <ddaney.caviumnetworks.com>

	* mips-linux-tdep.c: Update struct sigframe comments.
	(SIGFRAME_CODE_OFFSET): Delete macro.
	(mips_linux_o32_sigframe_init): Calculate sigcontext_base using
	this_frame's sp.
	(mips_linux_n32n64_sigframe_init): Same.

[-- Attachment #2: gdb.patch --]
[-- Type: text/plain, Size: 3821 bytes --]

Index: gdb/mips-linux-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mips-linux-tdep.c,v
retrieving revision 1.81
diff -u -p -r1.81 mips-linux-tdep.c
--- gdb/mips-linux-tdep.c	1 Jan 2010 07:31:37 -0000	1.81
+++ gdb/mips-linux-tdep.c	25 Feb 2010 20:27:05 -0000
@@ -797,7 +797,7 @@ static const struct tramp_frame mips_lin
 
    struct sigframe {
      u32 sf_ass[4];            [argument save space for o32]
-     u32 sf_code[2];           [signal trampoline]
+     u32 sf_code[2];           [signal trampoline or fill]
      struct sigcontext sf_sc;
      sigset_t sf_mask;
    };
@@ -827,7 +827,7 @@ static const struct tramp_frame mips_lin
 
    struct rt_sigframe {
      u32 rs_ass[4];            [argument save space for o32]
-     u32 rs_code[2]            [signal trampoline]
+     u32 rs_code[2]            [signal trampoline or fill]
      struct siginfo rs_info;
      struct ucontext rs_uc;
    };
@@ -842,7 +842,6 @@ static const struct tramp_frame mips_lin
    };  */
 /* *INDENT-ON* */
 
-#define SIGFRAME_CODE_OFFSET         (4 * 4)
 #define SIGFRAME_SIGCONTEXT_OFFSET   (6 * 4)
 
 #define RTSIGFRAME_SIGINFO_SIZE      128
@@ -871,14 +870,15 @@ mips_linux_o32_sigframe_init (const stru
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int ireg, reg_position;
-  CORE_ADDR sigcontext_base = func - SIGFRAME_CODE_OFFSET;
+  CORE_ADDR frame_sp = get_frame_sp (this_frame);
+  CORE_ADDR sigcontext_base;
   const struct mips_regnum *regs = mips_regnum (gdbarch);
   CORE_ADDR regs_base;
 
   if (self == &mips_linux_o32_sigframe)
-    sigcontext_base += SIGFRAME_SIGCONTEXT_OFFSET;
+    sigcontext_base = frame_sp + SIGFRAME_SIGCONTEXT_OFFSET;
   else
-    sigcontext_base += RTSIGFRAME_SIGCONTEXT_OFFSET;
+    sigcontext_base = frame_sp + RTSIGFRAME_SIGCONTEXT_OFFSET;
 
   /* I'm not proud of this hack.  Eventually we will have the
      infrastructure to indicate the size of saved registers on a
@@ -947,9 +947,7 @@ mips_linux_o32_sigframe_init (const stru
 			   sigcontext_base + SIGCONTEXT_BADVADDR);
 
   /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
-  trad_frame_set_id (this_cache,
-		     frame_id_build (func - SIGFRAME_CODE_OFFSET,
-				     func));
+  trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
 }
 
 /* *INDENT-OFF* */
@@ -957,7 +955,7 @@ mips_linux_o32_sigframe_init (const stru
 
   struct rt_sigframe_n32 {
     u32 rs_ass[4];                  [ argument save space for o32 ]
-    u32 rs_code[2];                 [ signal trampoline ]
+    u32 rs_code[2];                 [ signal trampoline or fill ]
     struct siginfo rs_info;
     struct ucontextn32 rs_uc;
   };
@@ -1038,13 +1036,14 @@ mips_linux_n32n64_sigframe_init (const s
 {
   struct gdbarch *gdbarch = get_frame_arch (this_frame);
   int ireg, reg_position;
-  CORE_ADDR sigcontext_base = func - SIGFRAME_CODE_OFFSET;
+  CORE_ADDR frame_sp = get_frame_sp (this_frame);
+  CORE_ADDR sigcontext_base;
   const struct mips_regnum *regs = mips_regnum (gdbarch);
 
   if (self == &mips_linux_n32_rt_sigframe)
-    sigcontext_base += N32_SIGFRAME_SIGCONTEXT_OFFSET;
+    sigcontext_base = frame_sp + N32_SIGFRAME_SIGCONTEXT_OFFSET;
   else
-    sigcontext_base += N64_SIGFRAME_SIGCONTEXT_OFFSET;
+    sigcontext_base = frame_sp + N64_SIGFRAME_SIGCONTEXT_OFFSET;
 
   if (mips_linux_restart_reg_p (gdbarch))
     trad_frame_set_reg_addr (this_cache,
@@ -1082,9 +1081,7 @@ mips_linux_n32n64_sigframe_init (const s
 			   sigcontext_base + N64_SIGCONTEXT_LO);
 
   /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
-  trad_frame_set_id (this_cache,
-		     frame_id_build (func - SIGFRAME_CODE_OFFSET,
-				     func));
+  trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
 }
 
 static void

Re: [PATCH] Make mips-linux signal frame unwinding more robust.

[Daniel Jacobowitz]

On Fri, Feb 26, 2010 at 10:22:09AM -0800, David Daney wrote:
>   The current signal frame unwinding code in mips-linux-tdep.c assumes
>   a constant offset from the signal return trampoline to the signal
>   frame. The assumption does not hold for all kernels.  Specifically
>   those that have to be compiled with ICACHE_REFILLS_WORKAROUND_WAR
>   set (SGI O2 for example).  In the near future, it is likely that the
>   assumption will cease to hold universally, as we are attempting to
>   move the signal return trampoline off the stack entirely.

It's funny, I thought I'd already taught GDB about the WAR workaround,
but there's no hint of it.  Your patch looks good to me.

> OK to commit?
> 
> How about on the 7.1 branch?

OK both.

-- 
Daniel Jacobowitz
CodeSourcery