[v2,3/3] gdbserver: Add RISC-V/Linux support

Message ID alpine.LFD.2.21.2001291412560.14118@redsun52.ssa.fujisawa.hgst.com
State Superseded
Headers

Commit Message

Maciej W. Rozycki Jan. 29, 2020, 6:14 p.m. UTC
  Implement RISC-V/Linux support for both RV64 and RV32 systems, including 
XML target description handling based on features determined, GPR and 
FPR regset support including dynamic sizing of the latter, and software 
breakpoint handling.  Define two NT_FPREGSET regsets of a different size 
matching the FPR sizes supported for generic `gdbserver' code to pick 
from according to what the OS supplies.

Also handle a glibc bug where ELF_NFPREG is defined in terms of NFPREG,
however NFPREG is nowhere defined.

	gdb/
	* arch/riscv.h (riscv_create_target_description): Remove `const' 
	qualifier from the return type.
	* arch/riscv.c (riscv_create_target_description): Likewise.
	* nat/riscv-linux-tdesc.h (riscv_linux_read_description): 
	Likewise.
	* nat/riscv-linux-tdesc.c (riscv_linux_read_description): 
	Likewise.
	* configure.tgt <riscv*-*-linux*>: Set build_gdbserver=yes.

	gdb/gdbserver/
	* linux-riscv-low.c: New file.
	* Makefile.in (SFILES): Add linux-riscv-low.c, arch/riscv.c, and 
	nat/riscv-linux-tdesc.c.
	* configure.srv <riscv*-*-linux*> (srv_tgtobj)
	(srv_linux_regsets, srv_linux_usrregs, srv_linux_thread_db): 
	Define.
---
Changes from v1:

- Make `gdbserver' selected for automatic build in a RISC-V/Linux/native
  GDB configuration (thanks, Jim, for pointing this out!).

- Remove most of `riscv_arch_setup' and use `riscv_linux_read_description' 
  from 2/3 instead.

- Stop using `elf_fpregset_t*' in favour to just a raw `gdb_byte *' buffer 
  and size the regset according to the FPR size in `riscv_fill_fpregset' 
  and `riscv_store_fpregset'.

- Define 2 NT_FPREGSET regsets of a different size for generic `gdbserver' 
  code to pick from according to what the OS supplies.
---
 gdb/arch/riscv.c                |    2 
 gdb/arch/riscv.h                |    4 
 gdb/configure.tgt               |    1 
 gdb/gdbserver/Makefile.in       |    3 
 gdb/gdbserver/configure.srv     |    7 +
 gdb/gdbserver/linux-riscv-low.c |  257 ++++++++++++++++++++++++++++++++++++++++
 gdb/nat/riscv-linux-tdesc.c     |    2 
 gdb/nat/riscv-linux-tdesc.h     |    2 
 8 files changed, 273 insertions(+), 5 deletions(-)

gdb-riscv-gdbserver-linux.diff
  

Comments

Jim Wilson Jan. 29, 2020, 11:39 p.m. UTC | #1
On Wed, Jan 29, 2020 at 10:14 AM Maciej W. Rozycki <macro@wdc.com> wrote:
>         gdb/
>         * arch/riscv.h (riscv_create_target_description): Remove `const'
>         qualifier from the return type.
>         * arch/riscv.c (riscv_create_target_description): Likewise.
>         * nat/riscv-linux-tdesc.h (riscv_linux_read_description):
>         Likewise.
>         * nat/riscv-linux-tdesc.c (riscv_linux_read_description):
>         Likewise.
>         * configure.tgt <riscv*-*-linux*>: Set build_gdbserver=yes.
>
>         gdb/gdbserver/
>         * linux-riscv-low.c: New file.
>         * Makefile.in (SFILES): Add linux-riscv-low.c, arch/riscv.c, and
>         nat/riscv-linux-tdesc.c.
>         * configure.srv <riscv*-*-linux*> (srv_tgtobj)
>         (srv_linux_regsets, srv_linux_usrregs, srv_linux_thread_db):
>         Define.

Looks good to me, though I'm not an official review.

I noticed on the gdbserver console I'm getting a lot of ptrace warnings.
    ptrace(regsets_fetch_inferior_registers) PID=1678103: Invalid argument
    Warning: ptrace(regsets_store_inferior_registers): Invalid argument
This looks like a side effect of having two FP regsets defined, it
tries the first one, fails, and then tries the second one which is
correct.  If you mark them as OPTIONAL_REGS we would only get the
warning once which would be OK, except that they can't be both FP_REGS
and OPTIONAL_REGS at the same time.  I don't know what if anything
would break if they aren't marked as FP_REGS.   I only see explicit
checks for GENERAL_REGS and OPTIONAL_REGS; I don't see any checks for
FP_REGS.  Anyways, I would suggest as a future improvement that the
linux gdbserver regset support be extended so that a regset can be
marked as both FP_REGS and OPTIONAL_REGS.

There are some new functions and structures that don't have
explanatory comments before them, but this is a minor issue.

Jim
  
Maciej W. Rozycki Jan. 30, 2020, 12:13 a.m. UTC | #2
On Wed, 29 Jan 2020, Jim Wilson wrote:

> I noticed on the gdbserver console I'm getting a lot of ptrace warnings.
>     ptrace(regsets_fetch_inferior_registers) PID=1678103: Invalid argument
>     Warning: ptrace(regsets_store_inferior_registers): Invalid argument
> This looks like a side effect of having two FP regsets defined, it
> tries the first one, fails, and then tries the second one which is
> correct.  If you mark them as OPTIONAL_REGS we would only get the
> warning once which would be OK, except that they can't be both FP_REGS
> and OPTIONAL_REGS at the same time.  I don't know what if anything
> would break if they aren't marked as FP_REGS.   I only see explicit
> checks for GENERAL_REGS and OPTIONAL_REGS; I don't see any checks for
> FP_REGS.  Anyways, I would suggest as a future improvement that the
> linux gdbserver regset support be extended so that a regset can be
> marked as both FP_REGS and OPTIONAL_REGS.

 Hmm, good point.  I think OPTIONAL_REGS might become a flag, however as 
you have observed there seems to be no special meaning indeed to FP_REGS 
and actually only a couple of `gdbserver' hosts use this type, so using 
OPTIONAL_REGS should be fine.  I'll send an update.

> There are some new functions and structures that don't have
> explanatory comments before them, but this is a minor issue.

 Right, though I think they are self-explanatory.

  Maciej
  

Patch

Index: binutils-gdb/gdb/arch/riscv.c
===================================================================
--- binutils-gdb.orig/gdb/arch/riscv.c
+++ binutils-gdb/gdb/arch/riscv.c
@@ -43,7 +43,7 @@  static std::unordered_map<riscv_gdbarch_
 
 /* See arch/riscv.h.  */
 
-const target_desc *
+target_desc *
 riscv_create_target_description (struct riscv_gdbarch_features features)
 {
   /* Have we seen this feature set before?  If we have return the same
Index: binutils-gdb/gdb/arch/riscv.h
===================================================================
--- binutils-gdb.orig/gdb/arch/riscv.h
+++ binutils-gdb/gdb/arch/riscv.h
@@ -69,7 +69,7 @@  struct riscv_gdbarch_features
 /* Create and return a target description that is compatible with
    FEATURES.  */
 
-const target_desc *riscv_create_target_description
-	(struct riscv_gdbarch_features features);
+target_desc *riscv_create_target_description
+  (struct riscv_gdbarch_features features);
 
 #endif /* ARCH_RISCV_H */
Index: binutils-gdb/gdb/configure.tgt
===================================================================
--- binutils-gdb.orig/gdb/configure.tgt
+++ binutils-gdb/gdb/configure.tgt
@@ -553,6 +553,7 @@  riscv*-*-linux*)
 	# Target: Linux/RISC-V
 	gdb_target_obs="riscv-linux-tdep.o glibc-tdep.o \
  			linux-tdep.o solib-svr4.o symfile-mem.o linux-record.o"
+	build_gdbserver=yes
 	;;
 
 riscv*-*-*)
Index: binutils-gdb/gdb/gdbserver/Makefile.in
===================================================================
--- binutils-gdb.orig/gdb/gdbserver/Makefile.in
+++ binutils-gdb/gdb/gdbserver/Makefile.in
@@ -177,6 +177,7 @@  SFILES = \
 	$(srcdir)/linux-mips-low.c \
 	$(srcdir)/linux-nios2-low.c \
 	$(srcdir)/linux-ppc-low.c \
+	$(srcdir)/linux-riscv-low.c \
 	$(srcdir)/linux-s390-low.c \
 	$(srcdir)/linux-sh-low.c \
 	$(srcdir)/linux-sparc-low.c \
@@ -203,6 +204,7 @@  SFILES = \
 	$(srcdir)/../arch/arm-get-next-pcs.c \
 	$(srcdir)/../arch/arm-linux.c \
 	$(srcdir)/../arch/ppc-linux-common.c \
+	$(srcdir)/../arch/riscv.c \
 	$(srcdir)/../../gdbsupport/btrace-common.c \
 	$(srcdir)/../../gdbsupport/buffer.c \
 	$(srcdir)/../../gdbsupport/cleanups.c \
@@ -236,6 +238,7 @@  SFILES = \
 	$(srcdir)/../nat/linux-personality.c \
 	$(srcdir)/../nat/mips-linux-watch.c \
 	$(srcdir)/../nat/ppc-linux.c \
+	$(srcdir)/../nat/riscv-linux-tdesc.c \
 	$(srcdir)/../nat/fork-inferior.c \
 	$(srcdir)/../target/waitstatus.c
 
Index: binutils-gdb/gdb/gdbserver/configure.srv
===================================================================
--- binutils-gdb.orig/gdb/gdbserver/configure.srv
+++ binutils-gdb/gdb/gdbserver/configure.srv
@@ -267,6 +267,13 @@  case "${target}" in
 			srv_xmlfiles="${srv_xmlfiles} rs6000/power-fpu.xml"
 			srv_lynxos=yes
 			;;
+  riscv*-*-linux*)	srv_tgtobj="arch/riscv.o nat/riscv-linux-tdesc.o"
+			srv_tgtobj="${srv_tgtobj} linux-riscv-low.o"
+			srv_tgtobj="${srv_tgtobj} ${srv_linux_obj}"
+			srv_linux_regsets=yes
+			srv_linux_usrregs=yes
+			srv_linux_thread_db=yes
+			;;
   s390*-*-linux*)	srv_regobj="s390-linux32.o"
 			srv_regobj="${srv_regobj} s390-linux32v1.o"
 			srv_regobj="${srv_regobj} s390-linux32v2.o"
Index: binutils-gdb/gdb/gdbserver/linux-riscv-low.c
===================================================================
--- /dev/null
+++ binutils-gdb/gdb/gdbserver/linux-riscv-low.c
@@ -0,0 +1,257 @@ 
+/* GNU/Linux/RISC-V specific low level interface, for the remote server
+   for GDB.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "server.h"
+
+#include "linux-low.h"
+#include "tdesc.h"
+#include "elf/common.h"
+#include "nat/riscv-linux-tdesc.h"
+#include "opcode/riscv.h"
+
+/* Work around glibc header breakage causing ELF_NFPREG not to be usable.  */
+#ifndef NFPREG
+# define NFPREG 33
+#endif
+
+/* Implementation of linux_target_ops method "arch_setup".  */
+
+static void
+riscv_arch_setup ()
+{
+  static const char *expedite_regs[] = { "sp", "pc", NULL };
+  target_desc *tdesc;
+
+  tdesc = riscv_linux_read_description (lwpid_of (current_thread));
+  if (!tdesc->expedite_regs)
+    init_target_desc (tdesc, expedite_regs);
+  current_process ()->tdesc = tdesc;
+}
+
+static void
+riscv_fill_gregset (struct regcache *regcache, void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  elf_gregset_t *regset = (elf_gregset_t *) buf;
+  int regno = find_regno (tdesc, "zero");
+  int i;
+
+  collect_register_by_name (regcache, "pc", *regset);
+  for (i = 1; i < ARRAY_SIZE (*regset); i++)
+    collect_register (regcache, regno + i, *regset + i);
+}
+
+static void
+riscv_store_gregset (struct regcache *regcache, const void *buf)
+{
+  const elf_gregset_t *regset = (const elf_gregset_t *) buf;
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "zero");
+  int i;
+
+  supply_register_by_name (regcache, "pc", *regset);
+  supply_register_zeroed (regcache, regno);
+  for (i = 1; i < ARRAY_SIZE (*regset); i++)
+    supply_register (regcache, regno + i, *regset + i);
+}
+
+static void
+riscv_fill_fpregset (struct regcache *regcache, void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "ft0");
+  int flen = register_size (regcache->tdesc, regno);
+  gdb_byte *regset = (gdb_byte *) buf;
+  int i;
+
+  for (i = 0; i < ELF_NFPREG - 1; i++)
+    collect_register (regcache, regno + i, regset + i * flen);
+  collect_register_by_name (regcache, "fcsr", regset + i * flen);
+}
+
+static void
+riscv_store_fpregset (struct regcache *regcache, const void *buf)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+  int regno = find_regno (tdesc, "ft0");
+  int flen = register_size (regcache->tdesc, regno);
+  const gdb_byte *regset = (const gdb_byte *) buf;
+  int i;
+
+  for (i = 0; i < ELF_NFPREG - 1; i++)
+    supply_register (regcache, regno + i, regset + i * flen);
+  supply_register_by_name (regcache, "fcsr", regset + i * flen);
+}
+
+static struct regset_info riscv_regsets[] = {
+  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_PRSTATUS,
+    sizeof (elf_gregset_t), GENERAL_REGS,
+    riscv_fill_gregset, riscv_store_gregset },
+  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
+    sizeof (struct __riscv_mc_d_ext_state), FP_REGS,
+    riscv_fill_fpregset, riscv_store_fpregset },
+  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
+    sizeof (struct __riscv_mc_f_ext_state), FP_REGS,
+    riscv_fill_fpregset, riscv_store_fpregset },
+  NULL_REGSET
+};
+
+static struct regsets_info riscv_regsets_info =
+  {
+    riscv_regsets, /* regsets */
+    0, /* num_regsets */
+    NULL, /* disabled_regsets */
+  };
+
+static struct regs_info riscv_regs =
+  {
+    NULL, /* regset_bitmap */
+    NULL, /* usrregs */
+    &riscv_regsets_info,
+  };
+
+/* Implementation of linux_target_ops method "regs_info".  */
+
+static const struct regs_info *
+riscv_regs_info ()
+{
+  return &riscv_regs;
+}
+
+/* Implementation of linux_target_ops method "fetch_register".  */
+
+static int
+riscv_fetch_register (struct regcache *regcache, int regno)
+{
+  const struct target_desc *tdesc = regcache->tdesc;
+
+  if (regno != find_regno (tdesc, "zero"))
+    return 0;
+  supply_register_zeroed (regcache, regno);
+  return 1;
+}
+
+/* Implementation of linux_target_ops method "get_pc".  */
+
+static CORE_ADDR
+riscv_get_pc (struct regcache *regcache)
+{
+  elf_gregset_t regset;
+
+  if (sizeof (regset[0]) == 8)
+    return linux_get_pc_64bit (regcache);
+  else
+    return linux_get_pc_32bit (regcache);
+}
+
+/* Implementation of linux_target_ops method "set_pc".  */
+
+static void
+riscv_set_pc (struct regcache *regcache, CORE_ADDR newpc)
+{
+  elf_gregset_t regset;
+
+  if (sizeof (regset[0]) == 8)
+    linux_set_pc_64bit (regcache, newpc);
+  else
+    linux_set_pc_32bit (regcache, newpc);
+}
+
+/* Correct in either endianness.  */
+static const uint16_t riscv_ibreakpoint[] = { 0x0073, 0x0010 };
+static const uint16_t riscv_cbreakpoint = 0x9002;
+
+/* Implementation of linux_target_ops method "breakpoint_kind_from_pc".  */
+
+static int
+riscv_breakpoint_kind_from_pc (CORE_ADDR *pcptr)
+{
+  union
+    {
+      gdb_byte bytes[2];
+      uint16_t insn;
+    }
+  buf;
+
+  if (target_read_memory (*pcptr, buf.bytes, sizeof (buf.insn)) == 0
+      && riscv_insn_length (buf.insn == sizeof (riscv_ibreakpoint)))
+    return sizeof (riscv_ibreakpoint);
+  else
+    return sizeof (riscv_cbreakpoint);
+}
+
+/* Implementation of linux_target_ops method "sw_breakpoint_from_kind".  */
+
+static const gdb_byte *
+riscv_sw_breakpoint_from_kind (int kind, int *size)
+{
+  *size = kind;
+  switch (kind)
+    {
+      case sizeof (riscv_ibreakpoint):
+	return (const gdb_byte *) &riscv_ibreakpoint;
+      default:
+	return (const gdb_byte *) &riscv_cbreakpoint;
+    }
+}
+
+/* Implementation of linux_target_ops method "breakpoint_at".  */
+
+static int
+riscv_breakpoint_at (CORE_ADDR pc)
+{
+  union
+    {
+      gdb_byte bytes[2];
+      uint16_t insn;
+    }
+  buf;
+
+  if (target_read_memory (pc, buf.bytes, sizeof (buf.insn)) == 0
+      && (buf.insn == riscv_cbreakpoint
+	  || (buf.insn == riscv_ibreakpoint[0]
+	      && target_read_memory (pc + sizeof (buf.insn), buf.bytes,
+				     sizeof (buf.insn)) == 0
+	      && buf.insn == riscv_ibreakpoint[1])))
+    return 1;
+  else
+    return 0;
+}
+
+struct linux_target_ops the_low_target =
+{
+  riscv_arch_setup,
+  riscv_regs_info,
+  NULL, /* cannot_fetch_register */
+  NULL, /* cannot_store_register */
+  riscv_fetch_register,
+  riscv_get_pc,
+  riscv_set_pc,
+  riscv_breakpoint_kind_from_pc,
+  riscv_sw_breakpoint_from_kind,
+  NULL, /* get_next_pcs */
+  0,    /* decr_pc_after_break */
+  riscv_breakpoint_at,
+};
+
+void
+initialize_low_arch ()
+{
+  initialize_regsets_info (&riscv_regsets_info);
+}
Index: binutils-gdb/gdb/nat/riscv-linux-tdesc.c
===================================================================
--- binutils-gdb.orig/gdb/nat/riscv-linux-tdesc.c
+++ binutils-gdb/gdb/nat/riscv-linux-tdesc.c
@@ -33,7 +33,7 @@ 
 
 /* Determine XLEN and FLEN and return a corresponding target description.  */
 
-const struct target_desc *
+struct target_desc *
 riscv_linux_read_description (int tid)
 {
   struct riscv_gdbarch_features features;
Index: binutils-gdb/gdb/nat/riscv-linux-tdesc.h
===================================================================
--- binutils-gdb.orig/gdb/nat/riscv-linux-tdesc.h
+++ binutils-gdb/gdb/nat/riscv-linux-tdesc.h
@@ -22,6 +22,6 @@ 
 struct target_desc;
 
 /* Return a target description for the LWP identified by TID.  */
-const struct target_desc *riscv_linux_read_description (int tid);
+struct target_desc *riscv_linux_read_description (int tid);
 
 #endif /* NAT_RISCV_LINUX_TDESC_H */