[PATCH 0/2] RISC-V: Linux signal frame support.

[PATCH 0/2] RISC-V: Linux signal frame support.

[Jim Wilson]

This adds initial support for unwinding through signal trampolines.
I've tested it by hand and can see correct register values for the
frame where the signal occurred.  I've tested it with the gdb
testsuite on an unleashed board with fc29 and a patched kernel, and I
get 37 more passes and 35 fewer failures.

The result isn't as good as I hoped, as I'm getting a lot of failures
from the fact that stepi into handler doesn't work, but that
apparently is not expected to work with software single step which is
all I have implemented so far.

This also fixes some duplicate comments the way that Simon Marchi
suggested back in August, by moving them to a .h file and modifying
the .c file to refer to the comments in the .h file.

Jim

[PATCH 1/2] RISC-V: Linux signal frame support.

[Jim Wilson]

Make riscv_isa_flen available to the linux native code, and clean up duplicate
comments.

	gdb/
	* riscv-tdep.c (riscv_isa_xlen): Refer to riscv-tdep.h comment.
	(riscv_isa_flen): Likewise.  Drop static.
	* riscv-tdep.h (riscv_isa_xlen): Move riscv-tdep.c comment to here.
	(riscv_isa_flen): Likewise.
---
 gdb/riscv-tdep.c | 11 +++--------
 gdb/riscv-tdep.h | 11 ++++++++++-
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
index f02420dfe5..a58c59765e 100644
--- a/gdb/riscv-tdep.c
+++ b/gdb/riscv-tdep.c
@@ -342,9 +342,7 @@ riscv_has_feature (struct gdbarch *gdbarch, char feature)
   return (misa & (1 << (feature - 'A'))) != 0;
 }
 
-/* Return the width in bytes  of the general purpose registers for GDBARCH.
-   Possible return values are 4, 8, or 16 for RiscV variants RV32, RV64, or
-   RV128.  */
+/* See riscv-tdep.h.  */
 
 int
 riscv_isa_xlen (struct gdbarch *gdbarch)
@@ -363,12 +361,9 @@ riscv_isa_xlen (struct gdbarch *gdbarch)
     }
 }
 
-/* Return the width in bytes of the floating point registers for GDBARCH.
-   If this architecture has no floating point registers, then return 0.
-   Possible values are 4, 8, or 16 for depending on which of single, double
-   or quad floating point support is available.  */
+/* See riscv-tdep.h.  */
 
-static int
+int
 riscv_isa_flen (struct gdbarch *gdbarch)
 {
   if (riscv_has_feature (gdbarch, 'Q'))
diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
index e04e728f32..2cb51b16c5 100644
--- a/gdb/riscv-tdep.h
+++ b/gdb/riscv-tdep.h
@@ -84,9 +84,18 @@ struct gdbarch_tdep
   struct type *riscv_fpreg_q_type;
 };
 
-/* Return the width in bytes of the general purpose registers for GDBARCH.  */
+
+/* Return the width in bytes  of the general purpose registers for GDBARCH.
+   Possible return values are 4, 8, or 16 for RiscV variants RV32, RV64, or
+   RV128.  */
 extern int riscv_isa_xlen (struct gdbarch *gdbarch);
 
+/* Return the width in bytes of the floating point registers for GDBARCH.
+   If this architecture has no floating point registers, then return 0.
+   Possible values are 4, 8, or 16 for depending on which of single, double
+   or quad floating point support is available.  */
+extern int riscv_isa_flen (struct gdbarch *gdbarch);
+
 /* Single step based on where the current instruction will take us.  */
 extern std::vector<CORE_ADDR> riscv_software_single_step
   (struct regcache *regcache);
-- 
2.17.1

Re: [PATCH 1/2] RISC-V: Linux signal frame support.

[Kevin Buettner]

On Thu, 25 Oct 2018 16:59:35 -0700
Jim Wilson <jimw@sifive.com> wrote:

> Make riscv_isa_flen available to the linux native code, and clean up duplicate
> comments.
> 
> 	gdb/
> 	* riscv-tdep.c (riscv_isa_xlen): Refer to riscv-tdep.h comment.
> 	(riscv_isa_flen): Likewise.  Drop static.
> 	* riscv-tdep.h (riscv_isa_xlen): Move riscv-tdep.c comment to here.
> 	(riscv_isa_flen): Likewise.

Maybe:

	(riscv_isa_flen): Likewise; declare.

?

Otherwise, LGTM.

Kevin

Re: [PATCH 1/2] RISC-V: Linux signal frame support.

[Jim Wilson]

On Thu, Oct 25, 2018 at 8:25 PM Kevin Buettner <kevinb@redhat.com> wrote:
>
> On Thu, 25 Oct 2018 16:59:35 -0700
> Jim Wilson <jimw@sifive.com> wrote:
>
> > Make riscv_isa_flen available to the linux native code, and clean up duplicate
> > comments.
> >
> >       gdb/
> >       * riscv-tdep.c (riscv_isa_xlen): Refer to riscv-tdep.h comment.
> >       (riscv_isa_flen): Likewise.  Drop static.
> >       * riscv-tdep.h (riscv_isa_xlen): Move riscv-tdep.c comment to here.
> >       (riscv_isa_flen): Likewise.
>
> Maybe:
>
>         (riscv_isa_flen): Likewise; declare.
>
> ?
>
> Otherwise, LGTM.

Thanks.  Committed with the corrected ChangeLog entry.

Jim

[PATCH 2/2] RISC-V: Linux signal frame support.

[Jim Wilson]

Add support for recognizing signal trampolines, parsing the signal frame,
and reading register values from it.

	gdb/
	* riscv-linux-tdep.c: Include tramp-frame.h and trad-frame.h.
	(riscv_linux_sigframe_init): Declare.
	(RISCV_INST_LI_A7_SIGRETURN, RISCV_INT_ECALL): New.
	(riscv_linux_sigframe): New.
	(SIGFRAME_SIGINFO_SIZE, UCONTEXT_MCONTEXT_OFFSET): New.
	(riscv_linux_sigframe_init): Define.
	(riscv_linux_init_abi): Call tramp_frame_prepend_unwinder.
---
 gdb/riscv-linux-tdep.c | 80 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
index d072c0b754..ece75dba47 100644
--- a/gdb/riscv-linux-tdep.c
+++ b/gdb/riscv-linux-tdep.c
@@ -23,6 +23,8 @@
 #include "linux-tdep.h"
 #include "solib-svr4.h"
 #include "regset.h"
+#include "tramp-frame.h"
+#include "trad-frame.h"
 
 /* Define the general register mapping.  The kernel puts the PC at offset 0,
    gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
@@ -56,6 +58,82 @@ riscv_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
   /* TODO: Add FP register support.  */
 }
 
+/* Signal trampoline support.  */
+
+static void riscv_linux_sigframe_init (const struct tramp_frame *self,
+				       struct frame_info *this_frame,
+				       struct trad_frame_cache *this_cache,
+				       CORE_ADDR func);
+
+#define RISCV_INST_LI_A7_SIGRETURN	0x08b00893
+#define RISCV_INST_ECALL		0x00000073
+
+static const struct tramp_frame riscv_linux_sigframe = {
+  SIGTRAMP_FRAME,
+  4,
+  {
+    { RISCV_INST_LI_A7_SIGRETURN, ULONGEST_MAX },
+    { RISCV_INST_ECALL, ULONGEST_MAX },
+    { TRAMP_SENTINEL_INSN }
+  },
+  riscv_linux_sigframe_init,
+  NULL
+};
+
+/* Runtime signal frames look like this:
+   struct rt_sigframe {
+     struct siginfo info;
+     struct ucontext uc;
+   };
+
+   struct ucontext {
+     unsigned long __uc_flags;
+     struct ucontext *uclink;
+     stack_t uc_stack;
+     sigset_t uc_sigmask;
+     char __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
+     mcontext_t uc_mcontext;
+   }; */
+
+#define SIGFRAME_SIGINFO_SIZE		128
+#define UCONTEXT_MCONTEXT_OFFSET	176
+
+static void
+riscv_linux_sigframe_init (const struct tramp_frame *self,
+			   struct frame_info *this_frame,
+			   struct trad_frame_cache *this_cache,
+			   CORE_ADDR func)
+{
+  struct gdbarch *gdbarch = get_frame_arch (this_frame);
+  int xlen = riscv_isa_xlen (gdbarch);
+  int flen = riscv_isa_flen (gdbarch);
+  CORE_ADDR frame_sp = get_frame_sp (this_frame);
+  CORE_ADDR mcontext_base;
+  CORE_ADDR regs_base;
+
+  mcontext_base = frame_sp + SIGFRAME_SIGINFO_SIZE + UCONTEXT_MCONTEXT_OFFSET;
+
+  /* Handle the integer registers.  The first one is PC, followed by x1
+     through x31.  */
+  regs_base = mcontext_base;
+  trad_frame_set_reg_addr (this_cache, RISCV_PC_REGNUM, regs_base);
+  for (int i = 1; i < 32; i++)
+    trad_frame_set_reg_addr (this_cache, RISCV_ZERO_REGNUM + i,
+			     regs_base + (i * xlen));
+
+  /* Handle the FP registers.  First comes the 32 FP registers, followed by
+     fcsr.  */
+  regs_base += 32 * xlen;
+  for (int i = 0; i < 32; i++)
+    trad_frame_set_reg_addr (this_cache, RISCV_FIRST_FP_REGNUM + i,
+			     regs_base + (i * flen));
+  regs_base += 32 * flen;
+  trad_frame_set_reg_addr (this_cache, RISCV_CSR_FCSR_REGNUM, regs_base);
+
+  /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
+  trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));
+}
+
 /* Initialize RISC-V Linux ABI info.  */
 
 static void
@@ -82,6 +160,8 @@ riscv_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   set_gdbarch_iterate_over_regset_sections
     (gdbarch, riscv_linux_iterate_over_regset_sections);
+
+  tramp_frame_prepend_unwinder (gdbarch, &riscv_linux_sigframe);
 }
 
 /* Initialize RISC-V Linux target support.  */
-- 
2.17.1

Re: [PATCH 2/2] RISC-V: Linux signal frame support.

[Kevin Buettner]

On Thu, 25 Oct 2018 17:00:30 -0700
Jim Wilson <jimw@sifive.com> wrote:

> Add support for recognizing signal trampolines, parsing the signal frame,
> and reading register values from it.
> 
> 	gdb/
> 	* riscv-linux-tdep.c: Include tramp-frame.h and trad-frame.h.
> 	(riscv_linux_sigframe_init): Declare.
> 	(RISCV_INST_LI_A7_SIGRETURN, RISCV_INT_ECALL): New.
> 	(riscv_linux_sigframe): New.
> 	(SIGFRAME_SIGINFO_SIZE, UCONTEXT_MCONTEXT_OFFSET): New.
> 	(riscv_linux_sigframe_init): Define.
> 	(riscv_linux_init_abi): Call tramp_frame_prepend_unwinder.

Hi Jim,

Though I don't claim any knowledge of the RISC-V architecture, I
read through your patch and it looks reasonable to me.

Kevin

Re: [PATCH 2/2] RISC-V: Linux signal frame support.

[John Baldwin]

On 10/25/18 5:00 PM, Jim Wilson wrote:
> Add support for recognizing signal trampolines, parsing the signal frame,
> and reading register values from it.
> 
> 	gdb/
> 	* riscv-linux-tdep.c: Include tramp-frame.h and trad-frame.h.
> 	(riscv_linux_sigframe_init): Declare.
> 	(RISCV_INST_LI_A7_SIGRETURN, RISCV_INT_ECALL): New.
> 	(riscv_linux_sigframe): New.
> 	(SIGFRAME_SIGINFO_SIZE, UCONTEXT_MCONTEXT_OFFSET): New.
> 	(riscv_linux_sigframe_init): Define.
> 	(riscv_linux_init_abi): Call tramp_frame_prepend_unwinder.
> ---
>  gdb/riscv-linux-tdep.c | 80 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
> index d072c0b754..ece75dba47 100644
> --- a/gdb/riscv-linux-tdep.c
> +++ b/gdb/riscv-linux-tdep.c
> @@ -23,6 +23,8 @@
>  #include "linux-tdep.h"
>  #include "solib-svr4.h"
>  #include "regset.h"
> +#include "tramp-frame.h"
> +#include "trad-frame.h"
>  
>  /* Define the general register mapping.  The kernel puts the PC at offset 0,
>     gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
> @@ -56,6 +58,82 @@ riscv_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
>    /* TODO: Add FP register support.  */
>  }
>  
> +/* Signal trampoline support.  */
> +
> +static void riscv_linux_sigframe_init (const struct tramp_frame *self,
> +				       struct frame_info *this_frame,
> +				       struct trad_frame_cache *this_cache,
> +				       CORE_ADDR func);
> +
> +#define RISCV_INST_LI_A7_SIGRETURN	0x08b00893
> +#define RISCV_INST_ECALL		0x00000073
> +
> +static const struct tramp_frame riscv_linux_sigframe = {
> +  SIGTRAMP_FRAME,
> +  4,
> +  {
> +    { RISCV_INST_LI_A7_SIGRETURN, ULONGEST_MAX },
> +    { RISCV_INST_ECALL, ULONGEST_MAX },
> +    { TRAMP_SENTINEL_INSN }
> +  },
> +  riscv_linux_sigframe_init,
> +  NULL
> +};
> +
> +/* Runtime signal frames look like this:
> +   struct rt_sigframe {
> +     struct siginfo info;
> +     struct ucontext uc;
> +   };
> +
> +   struct ucontext {
> +     unsigned long __uc_flags;
> +     struct ucontext *uclink;
> +     stack_t uc_stack;
> +     sigset_t uc_sigmask;
> +     char __glibc_reserved[1024 / 8 - sizeof (sigset_t)];
> +     mcontext_t uc_mcontext;
> +   }; */
> +
> +#define SIGFRAME_SIGINFO_SIZE		128
> +#define UCONTEXT_MCONTEXT_OFFSET	176
> +
> +static void
> +riscv_linux_sigframe_init (const struct tramp_frame *self,
> +			   struct frame_info *this_frame,
> +			   struct trad_frame_cache *this_cache,
> +			   CORE_ADDR func)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (this_frame);
> +  int xlen = riscv_isa_xlen (gdbarch);
> +  int flen = riscv_isa_flen (gdbarch);
> +  CORE_ADDR frame_sp = get_frame_sp (this_frame);
> +  CORE_ADDR mcontext_base;
> +  CORE_ADDR regs_base;
> +
> +  mcontext_base = frame_sp + SIGFRAME_SIGINFO_SIZE + UCONTEXT_MCONTEXT_OFFSET;
> +
> +  /* Handle the integer registers.  The first one is PC, followed by x1
> +     through x31.  */
> +  regs_base = mcontext_base;
> +  trad_frame_set_reg_addr (this_cache, RISCV_PC_REGNUM, regs_base);
> +  for (int i = 1; i < 32; i++)
> +    trad_frame_set_reg_addr (this_cache, RISCV_ZERO_REGNUM + i,
> +			     regs_base + (i * xlen));
> +
> +  /* Handle the FP registers.  First comes the 32 FP registers, followed by
> +     fcsr.  */
> +  regs_base += 32 * xlen;
> +  for (int i = 0; i < 32; i++)
> +    trad_frame_set_reg_addr (this_cache, RISCV_FIRST_FP_REGNUM + i,
> +			     regs_base + (i * flen));
> +  regs_base += 32 * flen;
> +  trad_frame_set_reg_addr (this_cache, RISCV_CSR_FCSR_REGNUM, regs_base);
> +
> +  /* Choice of the bottom of the sigframe is somewhat arbitrary.  */
> +  trad_frame_set_id (this_cache, frame_id_build (frame_sp, func));

FYI, I recently added a new function to the trad_frame interface that lets you
re-use a register map instead of open-coding the layout of registers.  In this
case the logic isn't too complex, but you could simplify the handling for the
first register bank to just be:

    trad_frame_set_reg_regmap (this_cache, riscv_linux_gregmap, regs_base,
			       33 * riscv_isa_xlen (gdbarch));

(I added this new function when writing the signal frame unwinder for
riscv-fbsd-tdep.c because I was tired of duplicating the same logic for
various signal frame unwinders.)

You can see an example of this in the riscv_fbsd_sigframe_init() in
riscv-fbsd-tdep.c.  (For FreeBSD I also use a register map for the fp
registers since FreeBSD is dumping those in a core dump note as well.)

-- 
John Baldwin

Re: [PATCH 0/2] RISC-V: Linux signal frame support.

[Andrew Burgess]

* Jim Wilson <jimw@sifive.com> [2018-10-25 16:58:51 -0700]:

> This adds initial support for unwinding through signal trampolines.
> I've tested it by hand and can see correct register values for the
> frame where the signal occurred.  I've tested it with the gdb
> testsuite on an unleashed board with fc29 and a patched kernel, and I
> get 37 more passes and 35 fewer failures.
> 
> The result isn't as good as I hoped, as I'm getting a lot of failures
> from the fact that stepi into handler doesn't work, but that
> apparently is not expected to work with software single step which is
> all I have implemented so far.
> 
> This also fixes some duplicate comments the way that Simon Marchi
> suggested back in August, by moving them to a .h file and modifying
> the .c file to refer to the comments in the .h file.

This series looks good to me.

Thanks,
Andrew