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

  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 v4:

- Add 128-bit FP regset.

Changes from v3:

- Actually include changes from v2.

Changes from v2:

- Advance through the buffer accessed in `riscv_fill_fpregset' and 
  `riscv_store_fpregset' so as to avoid doing a variable multiplication in 
  a loop; rename the `regset' variable to `regbuf'.

- Use OPTIONAL_REGS rather than FP_REGS as the type for the floating-point 
  regsets.

- Add comments throughout.

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 |  277 ++++++++++++++++++++++++++++++++++++++++
 gdb/nat/riscv-linux-tdesc.c     |    2 
 gdb/nat/riscv-linux-tdesc.h     |    2 
 8 files changed, 293 insertions(+), 5 deletions(-)

gdb-riscv-gdbserver-linux.diff
  

On 2020-02-01 3:21 p.m., Maciej W. Rozycki wrote:
> 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.

I've taken a look at this patch (and the previous one), and it looked fine
to me.  I'll leave the RISC-V maintainers the final say though.

Simon
  

* Simon Marchi <simark@simark.ca> [2020-02-04 00:06:11 -0500]:

> On 2020-02-01 3:21 p.m., Maciej W. Rozycki wrote:
> > 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.
> 
> I've taken a look at this patch (and the previous one), and it looked fine
> to me.  I'll leave the RISC-V maintainers the final say though.

Reviewing these is high on my list of things to do.  I hope to review
the next in this series in the next day or so.

Thanks,
Andrew
  

* Maciej W. Rozycki <macro@wdc.com> [2020-02-01 20:21:18 +0000]:

> 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.

Maciej,

I took a look through, and I'm happy with the changes in
gdb/gdbserver/*.  But I wondered if we could try something slightly
different for the gdb/arch/* and gdb/nat/* changes.

I'll follow up to this mail with a patch that I'd like to apply before
we apply your patch #3, it tweaks the target description lookup API
slightly.  The result is that your patch #3 would drop all changes to
gdb/arch/ and gdb/nat/, then the riscv_arch_setup function would look
like this:

  static void
  riscv_arch_setup ()
  {
    static const char *expedite_regs[] = { "sp", "pc", NULL };

    const riscv_gdbarch_features features
      = riscv_linux_read_features (lwpid_of (current_thread));
    target_desc *tdesc = riscv_create_target_description (features);

    if (!tdesc->expedite_regs)
      init_target_desc (tdesc, expedite_regs);
    current_process ()->tdesc = tdesc;
  }

The feature lookup and target description fetch are now separate
calls.

The benefit of this is that GDB gets to retain the const in its API,
while gdbserver gets a non-const API to call.

If you're happy with this approach then feel free to push both these
patches, or let me know and I'll push them both.

Thanks,
Andrew




> ---
> Changes from v4:
> 
> - Add 128-bit FP regset.
> 
> Changes from v3:
> 
> - Actually include changes from v2.
> 
> Changes from v2:
> 
> - Advance through the buffer accessed in `riscv_fill_fpregset' and 
>   `riscv_store_fpregset' so as to avoid doing a variable multiplication in 
>   a loop; rename the `regset' variable to `regbuf'.
> 
> - Use OPTIONAL_REGS rather than FP_REGS as the type for the floating-point 
>   regsets.
> 
> - Add comments throughout.
> 
> 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 |  277 ++++++++++++++++++++++++++++++++++++++++
>  gdb/nat/riscv-linux-tdesc.c     |    2 
>  gdb/nat/riscv-linux-tdesc.h     |    2 
>  8 files changed, 293 insertions(+), 5 deletions(-)
> 
> gdb-riscv-gdbserver-linux.diff
> 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,277 @@
> +/* 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;
> +}
> +
> +/* Collect GPRs from REGCACHE into BUF.  */
> +
> +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);
> +}
> +
> +/* Supply GPRs from BUF into REGCACHE.  */
> +
> +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);
> +}
> +
> +/* Collect FPRs from REGCACHE into BUF.  */
> +
> +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 *regbuf = (gdb_byte *) buf;
> +  int i;
> +
> +  for (i = 0; i < ELF_NFPREG - 1; i++, regbuf += flen)
> +    collect_register (regcache, regno + i, regbuf);
> +  collect_register_by_name (regcache, "fcsr", regbuf);
> +}
> +
> +/* Supply FPRs from BUF into REGCACHE.  */
> +
> +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 *regbuf = (const gdb_byte *) buf;
> +  int i;
> +
> +  for (i = 0; i < ELF_NFPREG - 1; i++, regbuf += flen)
> +    supply_register (regcache, regno + i, regbuf);
> +  supply_register_by_name (regcache, "fcsr", regbuf);
> +}
> +
> +/* RISC-V/Linux regsets.  FPRs are optional and come in different sizes,
> +   so define multiple regsets for them marking them all as OPTIONAL_REGS
> +   rather than FP_REGS, so that "regsets_fetch_inferior_registers" picks
> +   the right one according to size.  */
> +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_q_ext_state), OPTIONAL_REGS,
> +    riscv_fill_fpregset, riscv_store_fpregset },
> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
> +    sizeof (struct __riscv_mc_d_ext_state), OPTIONAL_REGS,
> +    riscv_fill_fpregset, riscv_store_fpregset },
> +  { PTRACE_GETREGSET, PTRACE_SETREGSET, NT_FPREGSET,
> +    sizeof (struct __riscv_mc_f_ext_state), OPTIONAL_REGS,
> +    riscv_fill_fpregset, riscv_store_fpregset },
> +  NULL_REGSET
> +};
> +
> +/* RISC-V/Linux regset information.  */
> +static struct regsets_info riscv_regsets_info =
> +  {
> +    riscv_regsets, /* regsets */
> +    0, /* num_regsets */
> +    NULL, /* disabled_regsets */
> +  };
> +
> +/* Definition of linux_target_ops data member "regs_info".  */
> +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;
> +}
> +
> +/* RISC-V/Linux target operations.  */
> +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,
> +};
> +
> +/* Initialize the RISC-V/Linux target.  */
> +
> +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 */
  

Andrew,

> I took a look through, and I'm happy with the changes in
> gdb/gdbserver/*.  But I wondered if we could try something slightly
> different for the gdb/arch/* and gdb/nat/* changes.
> 
> I'll follow up to this mail with a patch that I'd like to apply before
> we apply your patch #3, it tweaks the target description lookup API
> slightly.  The result is that your patch #3 would drop all changes to
> gdb/arch/ and gdb/nat/, then the riscv_arch_setup function would look
> like this:
> 
>   static void
>   riscv_arch_setup ()
>   {
>     static const char *expedite_regs[] = { "sp", "pc", NULL };
> 
>     const riscv_gdbarch_features features
>       = riscv_linux_read_features (lwpid_of (current_thread));
>     target_desc *tdesc = riscv_create_target_description (features);
> 
>     if (!tdesc->expedite_regs)
>       init_target_desc (tdesc, expedite_regs);
>     current_process ()->tdesc = tdesc;
>   }
> 
> The feature lookup and target description fetch are now separate
> calls.
> 
> The benefit of this is that GDB gets to retain the const in its API,
> while gdbserver gets a non-const API to call.

 Fine with me.

> If you're happy with this approach then feel free to push both these
> patches, or let me know and I'll push them both.

 Thank you for your review and rework.

 We've got a mid-air collision with Tom's `gdbserver' directory structure 
rearrangement however, so I had to adjust v6 2/2 accordingly and I'm 
currently rerunning verification as I had to update the sources.

 I'll post v7 once that has completed, which may take several hours yet.  
Changes are mechanical, applying to configury/Makefile structure only, so 
I'll assume your approval for this updated version with a reasonable 
amount of time allowed to chime in.

 NB what's the `Change-Id' included with the change descriptions of your 
patches; is that needed/useful for anything or just internal bookkeeping?

  Maciej
  

* Maciej W. Rozycki <macro@wdc.com> [2020-02-10 09:01:53 +0000]:

> Andrew,
> 
> > I took a look through, and I'm happy with the changes in
> > gdb/gdbserver/*.  But I wondered if we could try something slightly
> > different for the gdb/arch/* and gdb/nat/* changes.
> > 
> > I'll follow up to this mail with a patch that I'd like to apply before
> > we apply your patch #3, it tweaks the target description lookup API
> > slightly.  The result is that your patch #3 would drop all changes to
> > gdb/arch/ and gdb/nat/, then the riscv_arch_setup function would look
> > like this:
> > 
> >   static void
> >   riscv_arch_setup ()
> >   {
> >     static const char *expedite_regs[] = { "sp", "pc", NULL };
> > 
> >     const riscv_gdbarch_features features
> >       = riscv_linux_read_features (lwpid_of (current_thread));
> >     target_desc *tdesc = riscv_create_target_description (features);
> > 
> >     if (!tdesc->expedite_regs)
> >       init_target_desc (tdesc, expedite_regs);
> >     current_process ()->tdesc = tdesc;
> >   }
> > 
> > The feature lookup and target description fetch are now separate
> > calls.
> > 
> > The benefit of this is that GDB gets to retain the const in its API,
> > while gdbserver gets a non-const API to call.
> 
>  Fine with me.
> 
> > If you're happy with this approach then feel free to push both these
> > patches, or let me know and I'll push them both.
> 
>  Thank you for your review and rework.
> 
>  We've got a mid-air collision with Tom's `gdbserver' directory structure 
> rearrangement however, so I had to adjust v6 2/2 accordingly and I'm 
> currently rerunning verification as I had to update the sources.
> 
>  I'll post v7 once that has completed, which may take several hours yet.  
> Changes are mechanical, applying to configury/Makefile structure only, so 
> I'll assume your approval for this updated version with a reasonable 
> amount of time allowed to chime in.

That sounds like a good plan.

>
>  NB what's the `Change-Id' included with the change descriptions of your 
> patches; is that needed/useful for anything or just internal bookkeeping?

The Change-Id is nothing important, I should get rid of it.  It's left
over from when we tried using gerrit, and is added automatically by my
git setup.

Thanks,
Andrew