[patch 0/3, nios2] unbreak nios2-linux-gnu GDB

[patch 0/3, nios2] unbreak nios2-linux-gnu GDB

[Sandra Loosemore]

GDB support for nios2-linux-gnu became broken during the time when glibc 
and kernel support for that target was transitioning to the upstream 
repositories.  Both ports were accepted around the first of the year, 
but then the initial 3.19 kernel was not usable for testing due to some 
unrelated problems with the ethernet support, it took a long time to 
track down a bug that caused a kernel hang, and then we ran into some 
additional bugs with kernel cache flushing.  The icache flush issues 
were fixed in time for the 4.0 kernel release and GDB is working pretty 
well with that kernel version now for manual use, but to get consistent 
results from automated testing I had to apply an additional local patch 
to flush the dcache more aggressively.  I expect some version of that 
will make it into future kernel releases since Altera is aware of the 
trouble now.

There are 3 patches:

(1) revert to using "trap 31" for breakpoints
(2) use PTRACE_GETREGSET/SETREGSET in gdbserver
(3) fixes for new implementation of signal handler trampolines

Since the old out-of-tree kernel/glibc ports are now obsolete, we 
haven't tried to maintain backward compatibility in these patches.

Yao, I noticed that you haven't updated your e-mail address in 
MAINTAINERS; are you still acting as nios2 maintainer?

-Sandra

[patch 1/3, nios2] revert to using "trap 31" for breakpoints

[Sandra Loosemore]

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

This patch fixes a bug I introduced in commit 
d53c26c753a39b80a338fb85bd41f75a49374842, when I was refactoring code to 
prepare for the addition of new Nios II ISA variants.  One of the new 
variants we were testing did not have an appropriate "trap" encoding 
usable for all breakpoints, so we had to tweak GDB to use "break 31" 
instead of "trap 31".  While that works fine for bare-metal, it's 
incompatible with the published Nios II ABI documentation for Linux 
systems, which explicitly requires "trap 31".  Moreover, using "break 
31" causes a kernel hang.  Oops!  So, this patch puts it back to using 
"trap 31", the way it was before.  We've discussed this with Altera and 
they are going to fix the new ISA instead of changing the ABI.

OK to commit?

-Sandra


[-- Attachment #2: gdb-breakpoint.log --]
[-- Type: text/x-log, Size: 180 bytes --]

2015-04-23  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/
	* nios2-tdep.c (nios2_breakpoint_from_pc): Revert to using
	"trap 31" as the breakpoint instruction on all targets.

[-- Attachment #3: gdb-breakpoint.patch --]
[-- Type: text/x-patch, Size: 1481 bytes --]

diff --git a/gdb/nios2-tdep.c b/gdb/nios2-tdep.c
index 08f2034..882c263 100644
--- a/gdb/nios2-tdep.c
+++ b/gdb/nios2-tdep.c
@@ -1189,7 +1189,9 @@ nios2_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR start_pc)
   return nios2_analyze_prologue (gdbarch, start_pc, start_pc, &cache, NULL);
 }
 
-/* Implement the breakpoint_from_pc gdbarch hook.  */
+/* Implement the breakpoint_from_pc gdbarch hook.
+   Note that the Nios II ABI for Linux requires "trap 31"
+   as the breakpoint, and we use that consistently on all targets.  */
 
 static const gdb_byte*
 nios2_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *bp_addr,
@@ -1198,11 +1200,11 @@ nios2_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *bp_addr,
   enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
   unsigned long mach = gdbarch_bfd_arch_info (gdbarch)->mach;
 
-  /* R1 break encoding:
-     ((0x1e << 17) | (0x34 << 11) | (0x1f << 6) | (0x3a << 0))
-     0x003da7fa */
-  static const gdb_byte r1_breakpoint_le[] = {0xfa, 0xa7, 0x3d, 0x0};
-  static const gdb_byte r1_breakpoint_be[] = {0x0, 0x3d, 0xa7, 0xfa};
+  /* R1 trap encoding:
+     ((0x1d << 17) | (0x2d << 11) | (0x1f << 6) | (0x3a << 0))
+     0x003b6ffa */
+  static const gdb_byte r1_breakpoint_le[] = {0xfa, 0x6f, 0x3b, 0x0};
+  static const gdb_byte r1_breakpoint_be[] = {0x0, 0x3b, 0x6f, 0xfa};
   *bp_size = NIOS2_OPCODE_SIZE;
   if (byte_order_for_code == BFD_ENDIAN_BIG)
     return r1_breakpoint_be;

[patch 2/3, nios2] use PTRACE_GETREGSET/SETREGSET in gdbserver

[Sandra Loosemore]

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

When glibc support for nios2 was submitted upstream, we were asked to 
change the ptrace register query interface from 
PTRACE_GETREGS/PTRACE_SETREGS to PTRACE_GETREGSET/PTRACE_SETREGSET. 
This patch makes the corresponding change to gdbserver (there is no 
native GDB support for this target yet).

OK to commit?

-Sandra

[-- Attachment #2: gdb-regset.log --]
[-- Type: text/x-log, Size: 275 bytes --]

2015-04-23  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/gdbserver/
	* linux-nios2-low.c: Adjust includes and comments.  Remove
	HAVE_PTRACE_GETREGS conditionals.
	(nios2_regsets): Use PTRACE_GETREGSET and PTRACE_SETREGSET
	instead of PTRACE_GETREGS and PTRACE_SETREGS.

[-- Attachment #3: gdb-regset.patch --]
[-- Type: text/x-patch, Size: 1474 bytes --]

diff --git a/gdb/gdbserver/linux-nios2-low.c b/gdb/gdbserver/linux-nios2-low.c
index e2fbb89..7bd3c97 100644
--- a/gdb/gdbserver/linux-nios2-low.c
+++ b/gdb/gdbserver/linux-nios2-low.c
@@ -21,6 +21,7 @@
 
 #include "server.h"
 #include "linux-low.h"
+#include "elf/common.h"
 #include <sys/ptrace.h>
 #include <endian.h>
 #include "gdb_proc_service.h"
@@ -32,7 +33,7 @@
 
 /* The following definition must agree with the number of registers
    defined in "struct user_regs" in GLIBC
-   (ports/sysdeps/unix/sysv/linux/nios2/sys/user.h), and also with
+   (sysdeps/unix/sysv/linux/nios2/sys/user.h), and also with
    NIOS2_NUM_REGS in GDB proper.  */
 
 #define nios2_num_regs 49
@@ -163,8 +164,6 @@ ps_get_thread_area (const struct ps_prochandle *ph,
   return PS_OK;
 }
 
-#ifdef HAVE_PTRACE_GETREGS
-
 /* Helper functions to collect/supply a single register REGNO.  */
 
 static void
@@ -205,14 +204,12 @@ nios2_store_gregset (struct regcache *regcache, const void *buf)
   for (i = 0; i < nios2_num_regs; i++)
     nios2_supply_register (regcache, i, regset + i);
 }
-#endif /* HAVE_PTRACE_GETREGS */
 
 static struct regset_info nios2_regsets[] =
 {
-#ifdef HAVE_PTRACE_GETREGS
-  { PTRACE_GETREGS, PTRACE_SETREGS, 0, nios2_num_regs * 4, GENERAL_REGS,
+  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
+    nios2_num_regs * 4, GENERAL_REGS,
     nios2_fill_gregset, nios2_store_gregset },
-#endif /* HAVE_PTRACE_GETREGS */
   { 0, 0, 0, -1, -1, NULL, NULL }
 };
 

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

[Sandra Loosemore]

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

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


[-- Attachment #2: gdb-sigtramp.log --]
[-- Type: text/x-log, Size: 590 bytes --]

2015-04-23  Sandra Loosemore  <sandra@codesourcery.com>

	gdb/
	* nios2-tdep.c (nios2_get_next_pc): Make non-static.
	* nios2-tdep.h (nios2_get_next_pc): Declare as extern.
	* nios2-linux-tdep.c: Include breakpoint.h.
	(NIOS2_SIGRETURN_TRAMP_ADDR): Define.
	(NIOS2_SIGRETURN_REGSAVE_OFFSET): Define.
	(nios2_linux_rt_sigreturn_init): Adjust base address of
	register save area.
	(nios2_linux_software_single_step): New target hook function.
	(nios2_linux_init_abi): Install it.

	gdb/testsuite/
	* gdb.base/sigaltstack.exp: Add kfails for nios2-*-linux*.
	* gdb.base/sigstep.exp: Likewise.

[-- Attachment #3: gdb-sigtramp.patch --]
[-- Type: text/x-patch, Size: 6554 bytes --]

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} $" {

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

[Sandra Loosemore]

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

Re: [patch 0/3, nios2] unbreak nios2-linux-gnu GDB

[Yao Qi]

Sandra Loosemore <sandra@codesourcery.com> writes:

> GDB support for nios2-linux-gnu became broken during the time when
> glibc and kernel support for that target was transitioning to the
> upstream repositories.  Both ports were accepted around the first of
> the year, but then the initial 3.19 kernel was not usable for testing
> due to some unrelated problems with the ethernet support, it took a
> long time to track down a bug that caused a kernel hang, and then we
> ran into some additional bugs with kernel cache flushing.  The icache
> flush issues were fixed in time for the 4.0 kernel release and GDB is
> working pretty well with that kernel version now for manual use, but
> to get consistent results from automated testing I had to apply an
> additional local patch to flush the dcache more aggressively.  I
> expect some version of that will make it into future kernel releases
> since Altera is aware of the trouble now.

In short, with your patches applied, GDB/GDBserver should work well with
kernel 4.0 release (with your local patch to flush dcache), is it a
correct statement?  Do you have a GDB test summary (number of PASS and
FAIL) to show how well it does now?

>
> There are 3 patches:
>
> (1) revert to using "trap 31" for breakpoints
> (2) use PTRACE_GETREGSET/SETREGSET in gdbserver
> (3) fixes for new implementation of signal handler trampolines
>
> Since the old out-of-tree kernel/glibc ports are now obsolete, we
> haven't tried to maintain backward compatibility in these patches.

I think we need a NEWS entry for this change.

>
> Yao, I noticed that you haven't updated your e-mail address in
> MAINTAINERS; are you still acting as nios2 maintainer?

ARM and Linaro isn't against me to reviewing nios2 gdb patches with my
gmail address, so I think I can still review them.

-- 
Yao (齐尧)

Re: [patch 1/3, nios2] revert to using "trap 31" for breakpoints

[Yao Qi]

Sandra Loosemore <sandra@codesourcery.com> writes:

> -/* Implement the breakpoint_from_pc gdbarch hook.  */
> +/* Implement the breakpoint_from_pc gdbarch hook.
> +   Note that the Nios II ABI for Linux requires "trap 31"
> +   as the breakpoint, and we use that consistently on all targets.  */

Please quote the original statement from NIOS 2 ABI,

"Userspace programs should not use the break instruction and userspace
debuggers should not insert one."

and

"Userspace breakpoints are accomplished using the trap instruction with
immediate operand 31 (all ones)."

OK with this change.

-- 
Yao (齐尧)

Re: [patch 2/3, nios2] use PTRACE_GETREGSET/SETREGSET in gdbserver

[Yao Qi]

Sandra Loosemore <sandra@codesourcery.com> writes:

> When glibc support for nios2 was submitted upstream, we were asked to
> change the ptrace register query interface from
> PTRACE_GETREGS/PTRACE_SETREGS to
> PTRACE_GETREGSET/PTRACE_SETREGSET. This patch makes the corresponding
> change to gdbserver (there is no native GDB support for this target
> yet).

Hi Sandra,
Could you show me the discussion archive about ptrace interface change
request?

>
> diff --git a/gdb/gdbserver/linux-nios2-low.c b/gdb/gdbserver/linux-nios2-low.c
> index e2fbb89..7bd3c97 100644
> --- a/gdb/gdbserver/linux-nios2-low.c
> +++ b/gdb/gdbserver/linux-nios2-low.c
> @@ -21,6 +21,7 @@
>  
>  #include "server.h"
>  #include "linux-low.h"
> +#include "elf/common.h"

The new include should be mentioned in the ChangeLog entry.

-- 
Yao (齐尧)

Re: [patch 2/3, nios2] use PTRACE_GETREGSET/SETREGSET in gdbserver

[Pedro Alves]

On 04/28/2015 12:18 PM, Yao Qi wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
> 
>> When glibc support for nios2 was submitted upstream, we were asked to
>> change the ptrace register query interface from
>> PTRACE_GETREGS/PTRACE_SETREGS to
>> PTRACE_GETREGSET/PTRACE_SETREGSET. This patch makes the corresponding
>> change to gdbserver (there is no native GDB support for this target
>> yet).
> 
> Hi Sandra,
> Could you show me the discussion archive about ptrace interface change
> request?

I don't know about a glibc discussion, but I'm to blame for requesting
that on the kernel side:

  https://lkml.org/lkml/2014/4/25/683

Thanks,
Pedro Alves

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

[Yao Qi]

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?

-- 
Yao (齐尧)

Re: [patch 0/3, nios2] unbreak nios2-linux-gnu GDB

[Sandra Loosemore]

On 04/28/2015 04:59 AM, Yao Qi wrote:
> Sandra Loosemore <sandra@codesourcery.com> writes:
>
>> GDB support for nios2-linux-gnu became broken during the time when
>> glibc and kernel support for that target was transitioning to the
>> upstream repositories.  Both ports were accepted around the first of
>> the year, but then the initial 3.19 kernel was not usable for testing
>> due to some unrelated problems with the ethernet support, it took a
>> long time to track down a bug that caused a kernel hang, and then we
>> ran into some additional bugs with kernel cache flushing.  The icache
>> flush issues were fixed in time for the 4.0 kernel release and GDB is
>> working pretty well with that kernel version now for manual use, but
>> to get consistent results from automated testing I had to apply an
>> additional local patch to flush the dcache more aggressively.  I
>> expect some version of that will make it into future kernel releases
>> since Altera is aware of the trouble now.
>
> In short, with your patches applied, GDB/GDBserver should work well with
> kernel 4.0 release (with your local patch to flush dcache), is it a
> correct statement?

Yes.  GDB works well for ordinary manual debugging even without the 
dcache fix kernel patch (I couldn't reproduce the random failures I saw 
in automated testing by hand).

> Do you have a GDB test summary (number of PASS and
> FAIL) to show how well it does now?

Sure.  This is from dropping mainline GDB into a 2015.05 CodeBench build 
(GCC 4.9.2, glibc 2.21):

# of expected passes		19447
# of unexpected failures	105
# of unexpected successes	1
# of expected failures		31
# of known failures		69
# of unresolved testcases	1
# of untested testcases		67
# of unsupported tests		269

....which is a big improvement over the status quo of not being usable 
at all.  :-)

-Sandra

Re: [patch 2/3, nios2] use PTRACE_GETREGSET/SETREGSET in gdbserver

[Sandra Loosemore]

On 04/28/2015 05:24 AM, Pedro Alves wrote:
> On 04/28/2015 12:18 PM, Yao Qi wrote:
>> Sandra Loosemore <sandra@codesourcery.com> writes:
>>
>>> When glibc support for nios2 was submitted upstream, we were asked to
>>> change the ptrace register query interface from
>>> PTRACE_GETREGS/PTRACE_SETREGS to
>>> PTRACE_GETREGSET/PTRACE_SETREGSET. This patch makes the corresponding
>>> change to gdbserver (there is no native GDB support for this target
>>> yet).
>>
>> Hi Sandra,
>> Could you show me the discussion archive about ptrace interface change
>> request?
>
> I don't know about a glibc discussion, but I'm to blame for requesting
> that on the kernel side:
>
>    https://lkml.org/lkml/2014/4/25/683

Thanks, Pedro, for digging this up; I was mis-remembering in thinking 
that this change came from the glibc review.  It indeed came from the 
kernel submission instead.

-Sandra

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

[Sandra Loosemore]

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