diff mbox

[3/3,nios2] fixes for new implementation of signal handler trampolines

Message ID 5539412B.2020101@codesourcery.com
State New
Headers show

Commit Message

Sandra Loosemore April 23, 2015, 6:59 p.m. UTC
Earlier versions of the nios2 kernel used to allocate code for signal 
handler trampolines on the stack, but when the port was accepted 
upstream it was changed to instead put the trampoline at a fixed address 
in low memory (0x1044).

Moving the code off the stack changed the layout of the stack frame, so 
the first part of this fix involves updating the offset to the register 
save area.  This is not an exported interface from the kernel; I noticed 
e.g. the existing aarch64 gdb support includes a huge block of comments 
explaining the kernel's signal handler stack frame layout but ultimately 
also relies on using magic numbers to access the register save area.  I 
used a somewhat smaller block of comments for nios2 but I think now it 
is clear where the magic numbers come from and what kernel code this 
corresponds to.

The second problem is that the trampoline is not writable by user 
processes so GDB cannot set software breakpoints there.  I've tried to 
deal with that in the single-step hook by having it effectively step 
over the trampoline by setting the breakpoint on its return address, but 
for operations like "finish" or "advance" that use the stack unwinder to 
get the location to set the breakpoint, it seems like there is nothing 
to do but kfail the tests.

OK to commit?

-Sandra

Comments

Sandra Loosemore April 23, 2015, 7:10 p.m. UTC | #1
On 04/23/2015 12:59 PM, Sandra Loosemore wrote:

> @@ -204,14 +251,8 @@ extern initialize_file_ftype _initialize_nios2_linux_tdep;
>  void
>  _initialize_nios2_linux_tdep (void)
>  {
> -
> -  const struct bfd_arch_info *arch_info;
> -
> -  for (arch_info = bfd_lookup_arch (bfd_arch_nios2, 0);
> -       arch_info != NULL;
> -       arch_info = arch_info->next)
> -    gdbarch_register_osabi (bfd_arch_nios2, arch_info->mach,
> -			    GDB_OSABI_LINUX, nios2_linux_init_abi);
> +  gdbarch_register_osabi (bfd_arch_nios2, 0, GDB_OSABI_LINUX,
> +                          nios2_linux_init_abi);
>
>    initialize_tdesc_nios2_linux ();
>  }

Ooops, please ignore this patch hunk -- I generated this patch against 
the wrong base after reverting a change that turned out to be 
unnecessary on mainline head.

-Sandra
Yao Qi April 28, 2015, 11:56 a.m. UTC | #2
Sandra Loosemore <sandra@codesourcery.com> writes:

> Earlier versions of the nios2 kernel used to allocate code for signal
> handler trampolines on the stack, but when the port was accepted
> upstream it was changed to instead put the trampoline at a fixed
> address in low memory (0x1044).
>
> Moving the code off the stack changed the layout of the stack frame,
> so the first part of this fix involves updating the offset to the
> register save area.  This is not an exported interface from the
> kernel; I noticed e.g. the existing aarch64 gdb support includes a
> huge block of comments explaining the kernel's signal handler stack
> frame layout but ultimately also relies on using magic numbers to
> access the register save area.  I used a somewhat smaller block of
> comments for nios2 but I think now it is clear where the magic numbers
> come from and what kernel code this corresponds to.

We can make this magic number less magic by documenting how it is
calculated.  We did something similar in
tic6x-linux-tdep.c:tic6x_linux_rt_sigreturn_init,

  /* The base of struct sigcontext is computed by examining the definition of
     struct rt_sigframe in linux kernel source arch/c6x/kernel/signal.c.  */
  CORE_ADDR base = (sp + TIC6X_SP_RT_SIGFRAME
		    /* Pointer type *pinfo and *puc in struct rt_sigframe.  */
		    + 4 + 4
		    + TIC6X_SIGINFO_SIZE
		    + 4 + 4 /* uc_flags and *uc_link in struct ucontext.  */
		    + TIC6X_STACK_T_SIZE);

>
> The second problem is that the trampoline is not writable by user
> processes so GDB cannot set software breakpoints there.  I've tried to
> deal with that in the single-step hook by having it effectively step
> over the trampoline by setting the breakpoint on its return address,
> but for operations like "finish" or "advance" that use the stack
> unwinder to get the location to set the breakpoint, it seems like
> there is nothing to do but kfail the tests.

Could you address this in a separated patch?
Sandra Loosemore April 28, 2015, 3:26 p.m. UTC | #3
On 04/28/2015 05:56 AM, Yao Qi wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
>
>> Earlier versions of the nios2 kernel used to allocate code for signal
>> handler trampolines on the stack, but when the port was accepted
>> upstream it was changed to instead put the trampoline at a fixed
>> address in low memory (0x1044).
>>
>> Moving the code off the stack changed the layout of the stack frame,
>> so the first part of this fix involves updating the offset to the
>> register save area.  This is not an exported interface from the
>> kernel; I noticed e.g. the existing aarch64 gdb support includes a
>> huge block of comments explaining the kernel's signal handler stack
>> frame layout but ultimately also relies on using magic numbers to
>> access the register save area.  I used a somewhat smaller block of
>> comments for nios2 but I think now it is clear where the magic numbers
>> come from and what kernel code this corresponds to.
>
> We can make this magic number less magic by documenting how it is
> calculated.  We did something similar in
> tic6x-linux-tdep.c:tic6x_linux_rt_sigreturn_init,
>
>    /* The base of struct sigcontext is computed by examining the definition of
>       struct rt_sigframe in linux kernel source arch/c6x/kernel/signal.c.  */
>    CORE_ADDR base = (sp + TIC6X_SP_RT_SIGFRAME
> 		    /* Pointer type *pinfo and *puc in struct rt_sigframe.  */
> 		    + 4 + 4
> 		    + TIC6X_SIGINFO_SIZE
> 		    + 4 + 4 /* uc_flags and *uc_link in struct ucontext.  */
> 		    + TIC6X_STACK_T_SIZE);

Well, ahem, the magic number was actually calculated by inspection of 
the stack from the debugger.  :-)  I got lost trying to calculate the 
sizes of the data structures (struct siginfo, etc) from the kernel code 
by hand, and what purpose would it serve to have more magic numbers that 
are harder to compute than the current one?

>> The second problem is that the trampoline is not writable by user
>> processes so GDB cannot set software breakpoints there.  I've tried to
>> deal with that in the single-step hook by having it effectively step
>> over the trampoline by setting the breakpoint on its return address,
>> but for operations like "finish" or "advance" that use the stack
>> unwinder to get the location to set the breakpoint, it seems like
>> there is nothing to do but kfail the tests.
>
> Could you address this in a separated patch?

Yes, I can split the patch.

-Sandra
diff mbox

Patch

diff --git a/gdb/nios2-tdep.c b/gdb/nios2-tdep.c
index 882c263..0de2f54 100644
--- a/gdb/nios2-tdep.c
+++ b/gdb/nios2-tdep.c
@@ -1570,7 +1570,7 @@  static const struct frame_unwind nios2_stub_frame_unwind =
 /* Determine where to set a single step breakpoint while considering
    branch prediction.  */
 
-static CORE_ADDR
+CORE_ADDR
 nios2_get_next_pc (struct frame_info *frame, CORE_ADDR pc)
 {
   struct gdbarch *gdbarch = get_frame_arch (frame);
diff --git a/gdb/nios2-tdep.h b/gdb/nios2-tdep.h
index af36c41..46eb3e2 100644
--- a/gdb/nios2-tdep.h
+++ b/gdb/nios2-tdep.h
@@ -77,4 +77,6 @@  struct gdbarch_tdep
 extern struct target_desc *tdesc_nios2_linux;
 extern struct target_desc *tdesc_nios2;
 
+extern CORE_ADDR nios2_get_next_pc (struct frame_info *, CORE_ADDR);
+
 #endif /* NIOS2_TDEP_H */
diff --git a/gdb/nios2-linux-tdep.c b/gdb/nios2-linux-tdep.c
index dff1603..336faff 100644
--- a/gdb/nios2-linux-tdep.c
+++ b/gdb/nios2-linux-tdep.c
@@ -29,6 +29,7 @@ 
 #include "linux-tdep.h"
 #include "glibc-tdep.h"
 #include "nios2-tdep.h"
+#include "breakpoint.h"
 
 #include "features/nios2-linux.c"
 
@@ -114,7 +115,24 @@  nios2_iterate_over_regset_sections (struct gdbarch *gdbarch,
 }
 
 /* Initialize a trad-frame cache corresponding to the tramp-frame.
-   FUNC is the address of the instruction TRAMP[0] in memory.  */
+   FUNC is the address of the instruction TRAMP[0] in memory.
+
+   This ABI is not documented.  It corresponds to rt_setup_ucontext in
+   the kernel arch/nios2/kernel/signal.c file.
+
+   The key points are:
+   - The kernel creates a trampoline at the hard-wired address 0x1044.
+   - The stack pointer points to an object of type struct rt_sigframe.
+     The definition of this structure is not exported from the kernel.
+     The register save area is located at offset 152 bytes, and the
+     registers are saved as r1-r23, ra, fp, gp, ea, sp.
+
+   This interface was implemented with kernel version 3.19 (the first
+   official mainline kernel).  Older unofficial kernel versions used
+   incompatible conventions; we do not support those here.  */
+
+#define NIOS2_SIGRETURN_TRAMP_ADDR 0x1044
+#define NIOS2_SIGRETURN_REGSAVE_OFFSET 152
 
 static void
 nios2_linux_rt_sigreturn_init (const struct tramp_frame *self,
@@ -122,7 +140,8 @@  nios2_linux_rt_sigreturn_init (const struct tramp_frame *self,
 			       struct trad_frame_cache *this_cache,
 			       CORE_ADDR func)
 {
-  CORE_ADDR base = func + 41 * 4;
+  CORE_ADDR sp = get_frame_register_unsigned (next_frame, NIOS2_SP_REGNUM);
+  CORE_ADDR base = sp + NIOS2_SIGRETURN_REGSAVE_OFFSET;
   int i;
 
   for (i = 0; i < 23; i++)
@@ -166,6 +185,32 @@  nios2_linux_syscall_next_pc (struct frame_info *frame)
   return pc + NIOS2_OPCODE_SIZE;
 }
 
+/* Override the bare-metal software_single_step gdbarch method.
+   If the PC where we'd ordinarily want to set the breakpoint is
+   the signal trampoline at 0x1044, the kernel will not let us set a
+   breakpoint at that location.  So, treat it as if it were a function call
+   and set the breakpoint on its return address instead.  */
+
+static int
+nios2_linux_software_single_step (struct frame_info *frame)
+{
+  struct gdbarch *gdbarch = get_frame_arch (frame);
+  struct address_space *aspace;
+  CORE_ADDR next_pc = nios2_get_next_pc (frame, get_frame_pc (frame));
+
+  if (next_pc == NIOS2_SIGRETURN_TRAMP_ADDR)
+    {
+      frame = get_prev_frame (frame);
+      next_pc = frame_unwind_caller_pc (frame);
+    }
+
+  aspace = get_frame_address_space (frame);
+  insert_single_step_breakpoint (gdbarch, aspace, next_pc);
+
+  return 1;
+}
+
+
 /* Hook function for gdbarch_register_osabi.  */
 
 static void
@@ -191,6 +236,8 @@  nios2_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   tramp_frame_prepend_unwinder (gdbarch,
                                 &nios2_linux_rt_sigreturn_tramp_frame);
 
+  /* Single stepping.  */
+  set_gdbarch_software_single_step (gdbarch, nios2_linux_software_single_step);
   tdep->syscall_next_pc = nios2_linux_syscall_next_pc;
 
   /* Index of target address word in glibc jmp_buf.  */
@@ -204,14 +251,8 @@  extern initialize_file_ftype _initialize_nios2_linux_tdep;
 void
 _initialize_nios2_linux_tdep (void)
 {
-
-  const struct bfd_arch_info *arch_info;
-
-  for (arch_info = bfd_lookup_arch (bfd_arch_nios2, 0);
-       arch_info != NULL;
-       arch_info = arch_info->next)
-    gdbarch_register_osabi (bfd_arch_nios2, arch_info->mach,
-			    GDB_OSABI_LINUX, nios2_linux_init_abi);
+  gdbarch_register_osabi (bfd_arch_nios2, 0, GDB_OSABI_LINUX,
+                          nios2_linux_init_abi);
 
   initialize_tdesc_nios2_linux ();
 }
diff --git a/gdb/testsuite/gdb.base/sigaltstack.exp b/gdb/testsuite/gdb.base/sigaltstack.exp
index b65ea48..59a6c17 100644
--- a/gdb/testsuite/gdb.base/sigaltstack.exp
+++ b/gdb/testsuite/gdb.base/sigaltstack.exp
@@ -76,6 +76,7 @@  proc finish_test { pattern msg } {
 	    # don't gracefully fall back to single-stepping.
 	    setup_kfail gdb/8841 "i?86-*-linux*"
 	    setup_kfail gdb/8841 "*-*-openbsd*"
+	    setup_kfail gdb/8841 "nios2-*-linux*"
 	    fail "$msg (could not set breakpoint)"
 	}
 	-re "$pattern.*${gdb_prompt} $" {
diff --git a/gdb/testsuite/gdb.base/sigstep.exp b/gdb/testsuite/gdb.base/sigstep.exp
index 3c9454c..800fa08 100644
--- a/gdb/testsuite/gdb.base/sigstep.exp
+++ b/gdb/testsuite/gdb.base/sigstep.exp
@@ -161,6 +161,14 @@  set in_handler_map {
 		fail "$test (spurious SIGTRAP)"
 		return
 	    }
+	    -re "Cannot insert breakpoint 0.*${gdb_prompt} $" {
+		# Some platforms use a special read-only page for signal
+		# trampolines.  We can't set a breakpoint there, and we
+		# don't gracefully fall back to single-stepping.
+		setup_kfail gdb/8841 "nios2-*-linux*"
+		fail "$test (could not set breakpoint)"
+		return
+	    }
 	    -re "other handler location.*$gdb_prompt $" {
 		pass $test
 	    }
@@ -203,6 +211,7 @@  proc advancei { cmd } {
 		# don't gracefully fall back to single-stepping.
 		setup_kfail gdb/8841 "i?86-*-linux*"
 		setup_kfail gdb/8841 "*-*-openbsd*"
+		setup_kfail gdb/8841 "nios2-*-linux*"
 		fail "$test (could not set breakpoint)"
 		return
 	    }
@@ -225,6 +234,10 @@  proc advancei { cmd } {
 		pass "$test"
 	    }
 	    -re "main .*${gdb_prompt} $" {
+	    # Some targets cannot set single-step breakpoints on a
+	    # signal trampoline and step over the trampoline
+	    # instead of through it.
+	    setup_kfail gdb/8841 "nios2-*-linux*"
 		fail "$test (in main)"
 	    }
 	    -re "$inferior_exited_re normally.*${gdb_prompt} $" {