Re: [PATCH 1/2] RISC-V GDB Port

[Yao Qi <qiyaoltc at gmail dot com>]

Palmer Dabbelt <palmer@dabbelt.com> writes:

Hi Palmer,
you need a ChangeLog entry, and a news entry for this new gdb port.
There are some code style issues, please look at
https://sourceware.org/gdb/wiki/ContributionChecklist

> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 2c88434..e4e497b 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -691,7 +691,7 @@ ALL_TARGET_OBS = \
>  	nios2-tdep.o nios2-linux-tdep.o \
>  	nto-tdep.o \
>  	ppc-linux-tdep.o ppcfbsd-tdep.o ppcnbsd-tdep.o ppcobsd-tdep.o  \
> -	ppc-sysv-tdep.o ppc64-tdep.o rl78-tdep.o \
> +	ppc-sysv-tdep.o ppc64-tdep.o riscv-tdep.o rl78-tdep.o \

Miss riscv-linux-tdep.o?

>  	rs6000-aix-tdep.o rs6000-tdep.o solib-aix.o ppc-ravenscar-thread.o \
>  	rs6000-lynx178-tdep.o \
>  	rx-tdep.o \
> @@ -946,7 +946,7 @@ sparc64-tdep.h ppcobsd-tdep.h \
>  coff-pe-read.h parser-defs.h gdb_ptrace.h mips-linux-tdep.h \
>  m68k-tdep.h spu-tdep.h environ.h amd64-tdep.h \
>  doublest.h regset.h hppa-tdep.h ppc-linux-tdep.h ppc64-tdep.h \
> -rs6000-tdep.h rs6000-aix-tdep.h \
> +riscv-tdep.h rs6000-tdep.h rs6000-aix-tdep.h \
>  common/gdb_locale.h arch-utils.h trad-frame.h gnu-nat.h \
>  language.h nbsd-tdep.h solib-svr4.h \
>  macroexp.h ui-file.h regcache.h tracepoint.h tracefile.h i386-tdep.h \
> @@ -1734,6 +1734,7 @@ ALLDEPFILES = \
>  	ravenscar-thread.c \
>  	remote-sim.c \
>  	dcache.c \
> +	riscv-tdep.c \

Miss riscv-linux-tdep.c?

>  	rl78-tdep.c \
>  	rs6000-nat.c rs6000-tdep.c solib-aix.c ppc-ravenscar-thread.c \
>  	rs6000-lynx178-tdep.c \
> diff --git a/gdb/config/riscv/linux.mh b/gdb/config/riscv/linux.mh
> new file mode 100644
> index 0000000..f68f371
> --- /dev/null
> +++ b/gdb/config/riscv/linux.mh
> @@ -0,0 +1,30 @@
> +#  Host: RISC-V based machine running GNU/Linux
> +#
> +#  Copyright (C) 2015 Free Software Foundation, Inc.

2016

> +#  Contributed by Albert Ou <albert@sifive.com>.
> +#

Albert Ou doesn't have FSF copyright assignment.

> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -515,6 +515,9 @@ esac
>  # We might need to link with -lm; most simulators need it.
>  AC_CHECK_LIB(m, main)
>  
> +# The RISC-V simulator needs clock_gettime()
> +AC_SEARCH_LIBS([clock_gettime], [rt])
> +

I don't know much in sim configure, but this is about sim, so I think
this should be moved to sim/riscv/configure.ac.

> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index 8e34b7e..4457304 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -3447,8 +3447,9 @@ otherwise all the threads in the program are stopped.  To \n\
>  interrupt all running threads in non-stop mode, use the -a option."));
>  
>    c = add_info ("registers", nofp_registers_info, _("\
> -List of integer registers and their contents, for selected stack frame.\n\
> -Register name as argument means describe only that register."));
> +Display registers and their contents, for the selected stack frame.\n\
> +Usage: info registers [all|register|register group] ...\n\
> +By default, the general register group will be shown."));

Please split this change out this patch.  Put it into a separated patch,
and give the reason why do you change that.


> diff --git a/gdb/riscv-linux-tdep.c b/gdb/riscv-linux-tdep.c
> new file mode 100644
> index 0000000..b66860c
> --- /dev/null
> +++ b/gdb/riscv-linux-tdep.c
> @@ -0,0 +1,83 @@
> +/* Target-dependent code for GNU/Linux RISC-V.
> +
> +   Copyright (C) 2015 Free Software Foundation, Inc.
> +   Contributed by Albert Ou <albert@sifive.com>.
> +
> +   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 "defs.h"
> +
> +#include "gdbarch.h"
> +#include "osabi.h"
> +#include "linux-tdep.h"
> +#include "solib-svr4.h"
> +#include "glibc-tdep.h"
> +
> +#include "riscv-tdep.h"
> +#include "riscv-linux-tdep.h"
> +
> +#include "regcache.h"
> +#include "regset.h"
> +
> +static const struct regcache_map_entry riscv_linux_gregmap[] =
> +{
> +  { 1,  RISCV_PC_REGNUM, 0 },
> +  { 31, RISCV_RA_REGNUM, 0 }, /* x1 to x31 */
> +  { 0 }
> +};
> +
> +const struct regset riscv_linux_gregset =
> +{
> +  riscv_linux_gregmap, regcache_supply_regset, regcache_collect_regset
> +};
> +

All gdbarch methods are should be documented like this,

/* Implement the iterate_over_regset_sections gdbarch method.  */

> +static void
> +riscv_linux_iterate_over_regset_sections (struct gdbarch *gdbarch,
> +					  iterate_over_regset_sections_cb *cb,
> +					  void *cb_data,
> +					  const struct regcache *regcache)
> +{
> +  cb (".reg", (RISCV_NGREG * riscv_isa_regsize (gdbarch)),
> +      &riscv_linux_gregset, NULL, cb_data);
> +}
> +
> +static void
> +riscv_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  linux_init_abi (info, gdbarch);
> +
> +  /* GNU/Linux uses SVR4-style shared libraries.  */
> +  /* FIXME: This '0' should actually be a check to see if we're on
> +     rv32, but I can't figure out how to hook that up (it's in
> +     gdbarch_tdep, which we don't have here). */
> +  set_solib_svr4_fetch_link_map_offsets
> +    (gdbarch, (0) ?
> +      svr4_ilp32_fetch_link_map_offsets :
> +      svr4_lp64_fetch_link_map_offsets);

gdbarch_tdep (gdbarch)->riscv_abi can tell you rv32 or rv64 is used.

> --- /dev/null
> +++ b/gdb/riscv-tdep.c
> @@ -0,0 +1,1292 @@
> +/* Target-dependent code for the RISC-V architecture, for GDB.
> +
> +   Copyright (C) 1988-2015 Free Software Foundation, Inc.
> +
> +   Contributed by Alessandro Forin(af@cs.cmu.edu) at CMU
> +   and by Per Bothner(bothner@cs.wisc.edu) at U.Wisconsin
> +   and by Todd Snyder <todd@bluespec.com>
> +   and by Mike Frysinger <vapier@gentoo.org>.

I don't find Todd Snyder's FSF copyright assignment.

> +
> +   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 "defs.h"
> +#include "frame.h"
> +#include "inferior.h"
> +#include "symtab.h"
> +#include "value.h"
> +#include "gdbcmd.h"
> +#include "language.h"
> +#include "gdbcore.h"
> +#include "symfile.h"
> +#include "objfiles.h"
> +#include "gdbtypes.h"
> +#include "target.h"
> +#include "arch-utils.h"
> +#include "regcache.h"
> +#include "osabi.h"
> +#include "riscv-tdep.h"
> +#include "block.h"
> +#include "reggroups.h"
> +#include "opcode/riscv.h"
> +#include "elf/riscv.h"
> +#include "elf-bfd.h"
> +#include "symcat.h"
> +#include "sim-regno.h"
> +#include "gdb/sim-riscv.h"
> +#include "dis-asm.h"
> +#include "frame-unwind.h"
> +#include "frame-base.h"
> +#include "trad-frame.h"
> +#include "infcall.h"
> +#include "floatformat.h"
> +#include "remote.h"
> +#include "target-descriptions.h"
> +#include "dwarf2-frame.h"
> +#include "user-regs.h"
> +#include "valprint.h"
> +#include "opcode/riscv-opc.h"
> +#include <algorithm>
> +



> +
> +static void
> +riscv_remote_breakpoint_from_pc (struct gdbarch *gdbarch, CORE_ADDR *pcptr, int *kindptr)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  riscv_breakpoint_from_pc (gdbarch, pcptr, kindptr);
> +}
> +

remote_breakpoint_from_pc is removed in my patches
https://sourceware.org/ml/gdb-patches/2016-11/msg00031.html
They are not committed yet.  It would be good if you can rebase your
patches on top of mine when my patches are committed.

> +static struct value *
> +value_of_riscv_user_reg (struct frame_info *frame, const void *baton)
> +{
> +  const int *reg_p = (const int *)baton;
> +
> +  return value_of_register (*reg_p, frame);
> +}
> +
> +static const char *
> +register_name (struct gdbarch *gdbarch,
> +	       int             regnum,
> +               int             prefer_alias)

Code style issue, only one space is needed between "int" and "regnum".
Many instances of such problem around your patch.

> +{
> +  int i;
> +  static char buf[20];
> +
> +  if (tdesc_has_registers (gdbarch_target_desc (gdbarch)))
> +    return tdesc_register_name (gdbarch, regnum);

I don't think riscv port has target description yet.



> +
> +static void
> +riscv_extract_return_value (struct type *type,
> +			    struct regcache *regs,
> +			    gdb_byte *dst,
> +			    int regnum)

You need some comments on this function.

> +{
> +  struct gdbarch *gdbarch = get_regcache_arch (regs);
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  int regsize = riscv_isa_regsize (gdbarch);
> +  bfd_byte *valbuf = dst;
> +  int len = TYPE_LENGTH (type);
> +  ULONGEST tmp;
> +
> +  gdb_assert (len <= 2 * regsize);
> +
> +  while (len > 0)
> +    {
> +      regcache_cooked_read_unsigned (regs, regnum++, &tmp);
> +      store_unsigned_integer (valbuf, std::min (regsize, len), byte_order, tmp);

This line is too long.


> +      len -= regsize;
> +      valbuf += regsize;
> +    }
> +}
> +

/* Implement the return_value gdbarch method.  */

> +
> +static enum return_value_convention
> +riscv_return_value (struct gdbarch  *gdbarch,
> +		    struct value *function,
> +		    struct type     *type,
> +		    struct regcache *regcache,
> +		    gdb_byte        *readbuf,
> +		    const gdb_byte  *writebuf)
> +{
> +  enum type_code rv_type = TYPE_CODE (type);
> +  unsigned int rv_size = TYPE_LENGTH (type);
> +  int fp, regnum;
> +  ULONGEST tmp;
> +
> +  /* Paragraph on return values taken from RISC-V specification (post v2.0):
> +
> +     Values are returned from functions in integer registers a0 and a1 and
> +     floating-point registers fa0 and fa1.  Floating-point values are returned
> +     in floating-point registers only if they are primitives or members of a
> +     struct consisting of only one or two floating-point values.  Other return
> +     values that fit into two pointer-words are returned in a0 and a1.  Larger
> +     return values are passed entirely in memory; the caller allocates this
> +     memory region and passes a pointer to it as an implicit first parameter to
> +     the callee.  */
> +

Nit: some lines are too long.

> +  /* Deal with struct/unions first that are passed via memory.  */
> +  if (rv_size > 2 * riscv_isa_regsize (gdbarch))
> +    {
> +      if (readbuf || writebuf)
> +	regcache_cooked_read_unsigned (regcache, RISCV_A0_REGNUM, &tmp);
> +      if (readbuf)
> +	read_memory (tmp, readbuf, rv_size);
> +      if (writebuf)
> +	write_memory (tmp, writebuf, rv_size);
> +      return RETURN_VALUE_ABI_RETURNS_ADDRESS;
> +    }
> +
> +  /* Are we dealing with a floating point value?  */
> +  fp = 0;
> +  if (rv_type == TYPE_CODE_FLT)
> +    fp = 1;
> +  else if (rv_type == TYPE_CODE_STRUCT || rv_type == TYPE_CODE_UNION)
> +    {
> +      unsigned int rv_fields = TYPE_NFIELDS (type);
> +
> +      if (rv_fields == 1)
> +	{
> +	  struct type *fieldtype = TYPE_FIELD_TYPE (type, 0);
> +	  if (TYPE_CODE (check_typedef (fieldtype)) == TYPE_CODE_FLT)
> +	    fp = 1;
> +	}
> +      else if (rv_fields == 2)
> +	{
> +	  struct type *fieldtype0 = TYPE_FIELD_TYPE (type, 0);
> +	  struct type *fieldtype1 = TYPE_FIELD_TYPE (type, 1);
> +
> +	  if (TYPE_CODE (check_typedef (fieldtype0)) == TYPE_CODE_FLT
> +	      && TYPE_CODE (check_typedef (fieldtype1)) == TYPE_CODE_FLT)
> +	    fp = 1;
> +	}
> +    }
> +
> +  /* Handle return value in a register.  */
> +  regnum = fp ? RISCV_FA0_REGNUM : RISCV_A0_REGNUM;
> +
> +  if (readbuf)
> +    riscv_extract_return_value (type, regcache, readbuf, regnum);
> +
> +  if (writebuf)
> +    riscv_store_return_value (type, regcache, writebuf, regnum);
> +
> +  return RETURN_VALUE_REGISTER_CONVENTION;
> +}
> +

/* Implement the XXX gdbarch method.  */

> +static enum register_status
> +riscv_pseudo_register_read (struct gdbarch  *gdbarch,
> +			    struct regcache *regcache,
> +			    int              regnum,
> +			    gdb_byte        *buf)

Code style issue.

> +{
> +  return regcache_raw_read (regcache, regnum, buf);
> +}
> +

> +
> +static struct type *
> +riscv_register_type (struct gdbarch  *gdbarch,
> +		     int              regnum)
> +{
> +  int regsize = riscv_isa_regsize (gdbarch);
> +
> +  if (regnum < RISCV_FIRST_FP_REGNUM)
> +    {
> + int_regsizes:
> +      switch (regsize)
> +	{
> +	case 4:
> +	  return builtin_type (gdbarch)->builtin_int32;
> +	case 8:
> +	  return builtin_type (gdbarch)->builtin_int64;
> +	case 16:
> +	  return builtin_type (gdbarch)->builtin_int128;
> +	default:
> +	  internal_error (__FILE__, __LINE__,
> +			  _("unknown isa regsize %i"), regsize);
> +	}
> +    }
> +  else if (regnum <= RISCV_LAST_FP_REGNUM)
> +    {
> +      switch (regsize)
> +	{
> +	case 4:
> +	  return builtin_type (gdbarch)->builtin_float;
> +	case 8:
> +	case 16:
> +	  return builtin_type (gdbarch)->builtin_double;

return builtin_long_double in case regsize is 16?

> +	default:
> +	  internal_error (__FILE__, __LINE__,
> +			  _("unknown isa regsize %i"), regsize);
> +	}
> +    }
> +  else if (regnum == RISCV_PRIV_REGNUM)
> +    {
> +      return builtin_type (gdbarch)->builtin_int8;
> +    }
> +  else
> +    {
> +      if (regnum == RISCV_CSR_FFLAGS_REGNUM
> +	  || regnum == RISCV_CSR_FRM_REGNUM
> +	  || regnum == RISCV_CSR_FCSR_REGNUM)
> +	return builtin_type (gdbarch)->builtin_int32;
> +
> +      goto int_regsizes;

goto is not necessarily needed.  Please remove it.

> +    }
> +}
> +
> +/* TODO: Replace all this with tdesc XML files.  */
> +static void
> +riscv_read_fp_register_single (struct frame_info *frame, int regno,
> +			       gdb_byte *rare_buffer)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  int raw_size = register_size (gdbarch, regno);
> +  gdb_byte *raw_buffer = (gdb_byte *) alloca (raw_size);
> +
> +  if (!deprecated_frame_register_read (frame, regno, raw_buffer))

Use get_frame_register_value, your code can be simpler.

> +    error (_("can't read register %d (%s)"), regno,
> +	   gdbarch_register_name (gdbarch, regno));
> +
> +  if (raw_size == 8)
> +    {
> +      int offset;
> +
> +      if (gdbarch_byte_order (gdbarch) == BFD_ENDIAN_BIG)
> +	offset = 4;
> +      else
> +	offset = 0;
> +
> +      memcpy (rare_buffer, raw_buffer + offset, 4);
> +    }
> +  else
> +    memcpy (rare_buffer, raw_buffer, 4);
> +}
> +
> +static void
> +riscv_read_fp_register_double (struct frame_info *frame, int regno,
> +			       gdb_byte *rare_buffer)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  int raw_size = register_size (gdbarch, regno);
> +
> +  if (raw_size == 8)
> +    {
> +      if (!deprecated_frame_register_read (frame, regno, rare_buffer))
> +	error (_("can't read register %d (%s)"), regno,
> +	       gdbarch_register_name (gdbarch, regno));
> +    }
> +  else
> +    internal_error (__FILE__, __LINE__,
> +		    _("%s: size says 32-bits, read is 64-bits."), __func__);
> +}
> +
> +static void
> +riscv_print_fp_register (struct ui_file *file, struct frame_info *frame,
> +			 int regnum)
> +{
> +  struct gdbarch *gdbarch = get_frame_arch (frame);
> +  gdb_byte *raw_buffer;
> +  struct value_print_options opts;
> +  double val;
> +  int inv;
> +  const char *regname;
> +
> +  raw_buffer = (gdb_byte *) alloca (2 * register_size (gdbarch, RISCV_FIRST_FP_REGNUM));
> +

This line is too long.

> +  fprintf_filtered (file, "%-15s", gdbarch_register_name (gdbarch, regnum));
> +
> +  if (register_size (gdbarch, regnum) == 4)
> +    {
> +      riscv_read_fp_register_single (frame, regnum, raw_buffer);
> +      val = unpack_double (builtin_type (gdbarch)->builtin_float, raw_buffer,
> +			   &inv);
> +
> +      get_formatted_print_options (&opts, 'x');
> +      print_scalar_formatted (raw_buffer,
> +			      builtin_type (gdbarch)->builtin_float,
> +			      &opts, 'w', file);
> +
> +      if (!inv)
> +	fprintf_filtered (file, "\t%-17.9g", val);
> +    }
> +  else
> +    {
> +      riscv_read_fp_register_double (frame, regnum, raw_buffer);
> +      val = unpack_double (builtin_type (gdbarch)->builtin_double, raw_buffer,
> +			   &inv);
> +
> +      get_formatted_print_options (&opts, 'x');
> +      print_scalar_formatted (raw_buffer,
> +			      builtin_type (gdbarch)->builtin_double,
> +			      &opts, 'g', file);
> +
> +      if (!inv)
> +	fprintf_filtered (file, "\t%-24.17g", val);
> +    }
> +
> +  if (inv)
> +    fprintf_filtered (file, "\t<invalid>");
> +}
> +




> +
> +static ULONGEST
> +riscv_fetch_instruction (struct gdbarch *gdbarch, CORE_ADDR addr)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);

gdbarch_byte_order_for_code


> +
> +static void
> +set_reg_offset (struct gdbarch *gdbarch, struct riscv_frame_cache *this_cache,
> +		int regnum, CORE_ADDR offset)
> +{
> +  if (this_cache != NULL && this_cache->saved_regs[regnum].addr == -1)
> +    this_cache->saved_regs[regnum].addr = offset;
> +}
> +
> +static void
> +reset_saved_regs (struct gdbarch *gdbarch, struct riscv_frame_cache *this_cache)
> +{
> +  const int num_regs = gdbarch_num_regs (gdbarch);
> +  int i;
> +
> +  if (this_cache == NULL || this_cache->saved_regs == NULL)
> +    return;
> +
> +  for (i = 0; i < num_regs; ++i)
> +    this_cache->saved_regs[i].addr = -1;

IIRC, .addr's type is CORE_ADDR, which is unsigned.  Don't assign -1 to
a unsigned type.

> +}
> +
> +static CORE_ADDR
> +riscv_scan_prologue (struct gdbarch *gdbarch,
> +		     CORE_ADDR start_pc, CORE_ADDR limit_pc,
> +		     struct frame_info *this_frame,
> +		     struct riscv_frame_cache *this_cache)
> +{
> +  CORE_ADDR cur_pc;
> +  CORE_ADDR frame_addr = 0;
> +  CORE_ADDR sp;
> +  long frame_offset;
> +  int frame_reg = RISCV_SP_REGNUM;
> +
> +  CORE_ADDR end_prologue_addr = 0;
> +  int seen_sp_adjust = 0;
> +  int load_immediate_bytes = 0;
> +
> +  /* Can be called when there's no process, and hence when there's no THIS_FRAME.  */
> +  if (this_frame != NULL)
> +    sp = get_frame_register_signed (this_frame, RISCV_SP_REGNUM);
> +  else
> +    sp = 0;
> +
> +  if (limit_pc > start_pc + 200)
> +    limit_pc = start_pc + 200;
> +
> + restart:
> +
> +  frame_offset = 0;
> +  /* TODO: Handle compressed extensions.  */
> +  for (cur_pc = start_pc; cur_pc < limit_pc; cur_pc += 4)
> +    {
> +      unsigned long inst, opcode;
> +      int reg, rs1, imm12, rs2, offset12, funct3;
> +
> +      /* Fetch the instruction.  */
> +      inst = (unsigned long) riscv_fetch_instruction (gdbarch, cur_pc);

What if (unsigned long) is 4-bytes long, but riscv instruction is
8-bytes long?

> +      opcode = inst & 0x7F;
> +      reg = (inst >> 7) & 0x1F;
> +      rs1 = (inst >> 15) & 0x1F;
> +      imm12 = (inst >> 20) & 0xFFF;
> +      rs2 = (inst >> 20) & 0x1F;
> +      offset12 = (((inst >> 25) & 0x7F) << 5) + ((inst >> 7) & 0x1F);
> +      funct3 = (inst >> 12) & 0x7;
> +
> +      /* Look for common stack adjustment insns.  */
> +      if ((opcode == 0x13 || opcode == 0x1B) && reg == RISCV_SP_REGNUM
> +	  && rs1 == RISCV_SP_REGNUM)

Please use macros defined in include/opcode/riscv-opc.h, then the code
is much more readable.

> +static CORE_ADDR
> +riscv_push_dummy_call (struct gdbarch *gdbarch,
> +		       struct value *function,
> +		       struct regcache *regcache,
> +		       CORE_ADDR bp_addr,
> +		       int nargs,
> +		       struct value **args,
> +		       CORE_ADDR sp,
> +		       int struct_return,
> +		       CORE_ADDR struct_addr)
> +{
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +  gdb_byte buf[4];
> +  int i;
> +  CORE_ADDR func_addr = find_function_addr (function, NULL);
> +
> +  /* Push excess arguments in reverse order.  */
> +
> +  for (i = nargs; i >= 8; --i)
> +    {
> +      struct type *value_type = value_enclosing_type (args[i]);
> +      int container_len = align_up (TYPE_LENGTH (value_type), 3);
> +
> +      sp -= container_len;
> +      write_memory (sp, value_contents_writeable (args[i]), container_len);
> +    }
> +
> +  /* Initialize argument registers.  */
> +
> +  for (i = 0; i < nargs && i < 8; ++i)
> +    {
> +      struct type *value_type = value_enclosing_type (args[i]);
> +      const gdb_byte *arg_bits = value_contents_all (args[i]);
> +      int regnum = TYPE_CODE (value_type) == TYPE_CODE_FLT ?
> +	RISCV_FA0_REGNUM : RISCV_A0_REGNUM;
> +

code style issue,
https://www.gnu.org/prep/standards/standards.html#Formatting

int regnum = (TYPE_CODE (value_type) == TYPE_CODE_FLT
               ? RISCV_FA0_REGNUM : RISCV_A0_REGNUM);


> +
> +static struct gdbarch *
> +riscv_gdbarch_init (struct gdbarch_info  info,
> +		    struct gdbarch_list *arches)
> +{
> +  struct gdbarch *gdbarch;
> +  struct gdbarch_tdep *tdep;
> +  const struct bfd_arch_info *binfo = info.bfd_arch_info;
> +
> +  int abi, i;
> +
> +  /* For now, base the abi on the elf class.  */
> +  /* Allow the ELF class to override the register size. Ideally the target
> +   * (OpenOCD/spike/...) would communicate the register size to gdb instead. */
> +  abi = RISCV_ABI_FLAG_RV32I;
> +  if (info.abfd && bfd_get_flavour (info.abfd) == bfd_target_elf_flavour)
> +    {
> +      unsigned char eclass = elf_elfheader (info.abfd)->e_ident[EI_CLASS];
> +
> +      if (eclass == ELFCLASS32)
> +	abi = RISCV_ABI_FLAG_RV32I;
> +      else if (eclass == ELFCLASS64)
> +	abi = RISCV_ABI_FLAG_RV64I;
> +      else
> +        internal_error (__FILE__, __LINE__, _("unknown ELF header class %d"), eclass);
> +    }
> +  else
> +    {
> +      if (binfo->bits_per_word == 32)
> +        abi = RISCV_ABI_FLAG_RV32I;
> +      else if (binfo->bits_per_word == 64)
> +        abi = RISCV_ABI_FLAG_RV64I;
> +      else
> +        internal_error (__FILE__, __LINE__, _("unknown bits_per_word %d"),
> +            binfo->bits_per_word);
> +    }
> +
> +  /* Find a candidate among the list of pre-declared architectures.  */
> +  for (arches = gdbarch_list_lookup_by_info (arches, &info);
> +       arches != NULL;
> +       arches = gdbarch_list_lookup_by_info (arches->next, &info))
> +    if (gdbarch_tdep (arches->gdbarch)->riscv_abi == abi)
> +      return arches->gdbarch;
> +
> +  /* None found, so create a new architecture from the information provided.
> +     Can't initialize all the target dependencies until we actually know which
> +     target we are talking to, but put in some defaults for now.  */
> +
> +  tdep = (struct gdbarch_tdep *) xmalloc (sizeof *tdep);
> +  gdbarch = gdbarch_alloc (&info, tdep);
> +
> +  tdep->riscv_abi = abi;
> +  switch (abi)
> +    {
> +    case RISCV_ABI_FLAG_RV32I:
> +      tdep->register_size = 4;
> +      break;
> +    case RISCV_ABI_FLAG_RV64I:
> +      tdep->register_size = 8;

Since you've already had riscv_abi field in tdep, you can determine the
registers size on the fly.  Then, field register_size is not necessary
to me.

> +
> +  /* Information about the target architecture.  */
> +  set_gdbarch_return_value (gdbarch, riscv_return_value);
> +  set_gdbarch_breakpoint_from_pc (gdbarch, riscv_breakpoint_from_pc);

breakpoint_from_pc will be replaced by two new gdbarch methods, added in
my patches https://sourceware.org/ml/gdb-patches/2016-11/msg00031.html

> +  set_gdbarch_remote_breakpoint_from_pc (gdbarch, riscv_remote_breakpoint_from_pc);


> +
> +  /* Check any target description for validity.  */
> +  if (tdesc_has_registers (info.target_desc))
> +    {

We don't have riscv target description yet.

> +      const struct tdesc_feature *feature;
> +      struct tdesc_arch_data *tdesc_data;
> +      int valid_p;
> +
> +      feature = tdesc_find_feature (info.target_desc, "org.gnu.gdb.riscv.cpu");
> +      if (feature == NULL)
> +	goto no_tdata;
> +
> +      tdesc_data = tdesc_data_alloc ();
> +
> +      valid_p = 1;
> +      for (i = RISCV_ZERO_REGNUM; i <= RISCV_LAST_FP_REGNUM; ++i)
> +        valid_p &= tdesc_numbered_register (feature, tdesc_data, i,
> +                                            riscv_gdb_reg_names[i]);
> +      for (i = RISCV_FIRST_CSR_REGNUM; i <= RISCV_LAST_CSR_REGNUM; ++i)
> +        {
> +          char buf[20];
> +          sprintf (buf, "csr%d", i - RISCV_FIRST_CSR_REGNUM);
> +          valid_p &= tdesc_numbered_register (feature, tdesc_data, i, buf);
> +        }
> +
> +      valid_p &= tdesc_numbered_register (feature, tdesc_data, i++, "priv");
> +
> +      if (!valid_p)
> +	tdesc_data_cleanup (tdesc_data);
> +      else
> +	tdesc_use_registers (gdbarch, info.target_desc, tdesc_data);
> +    }
> + no_tdata:
> +
> +  for (i = 0; i < ARRAY_SIZE (riscv_register_aliases); ++i)
> +    user_reg_add (gdbarch, riscv_register_aliases[i].name,
> +		  value_of_riscv_user_reg, &riscv_register_aliases[i].regnum);
> +
> +  return gdbarch;
> +}
> +

> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> new file mode 100644
> index 0000000..ef10209
> --- /dev/null
> +++ b/gdb/riscv-tdep.h
> @@ -0,0 +1,101 @@
> +/* Target-dependent header for the RISC-V architecture, for GDB, the GNU Debugger.
> +
> +   Copyright (C) 2002-2015 Free Software Foundation, Inc.
> +
> +   Contributed by Alessandro Forin(af@cs.cmu.edu) at CMU
> +   and by Per Bothner(bothner@cs.wisc.edu) at U.Wisconsin
> +   and by Todd Snyder <todd@bluespec.com>
> +   and by Mike Frysinger <vapier@gentoo.org>.
> +
> +   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/>.  */
> +
> +#ifndef RISCV_TDEP_H
> +#define RISCV_TDEP_H
> +
> +struct gdbarch;
> +
> +/* All the official RISC-V ABIs.  These mostly line up with mcpuid purely for
> +   convenience.  */
> +#define RISCV_ABI_FLAG_RV32I	(0x00000000)	/* 32-bit Integer GPRs.  */
> +#define RISCV_ABI_FLAG_RV32E	(0x10000000)	/* 32-bit Embedded Integer GPRs.  */
> +#define RISCV_ABI_FLAG_RV64I	(0x40000000)	/* 64-bit Integer GPRs.  */
> +#define RISCV_ABI_FLAG_RV128I	(0x80000000)	/* 128-bit Integer GPRs.  */
> +#define RISCV_ABI_FLAG_A	(1 << 0)	/* Atomics.  */
> +#define RISCV_ABI_FLAG_B	(1 << 1)	/* Bit Manipulation.  */
> +#define RISCV_ABI_FLAG_C	(1 << 2)	/* 16-bit Compressed Instructions.  */
> +#define RISCV_ABI_FLAG_D	(1 << 3)	/* Double-Precision Floating-Point.  */
> +#define RISCV_ABI_FLAG_E	(1 << 4)	/* Embedded base.  */
> +#define RISCV_ABI_FLAG_F	(1 << 5)	/* Single-Precision Floating-Point.  */
> +#define RISCV_ABI_FLAG_H	(1 << 7)	/* Hypervisor mode.  */
> +#define RISCV_ABI_FLAG_I	(1 << 8)	/* Base integer.  */
> +#define RISCV_ABI_FLAG_L	(1 << 11)	/* Decimal Floating-Point.  */
> +#define RISCV_ABI_FLAG_M	(1 << 12)	/* Integer Multiply and Division.  */
> +#define RISCV_ABI_FLAG_P	(1 << 15)	/* Packed-SIMD Extensions.  */
> +#define RISCV_ABI_FLAG_Q	(1 << 16)	/* Quad-Precision Floating-Point.  */
> +#define RISCV_ABI_FLAG_S	(1 << 18)	/* Supervisor mode.  */
> +#define RISCV_ABI_FLAG_T	(1 << 19)	/* Transactional Memory.  */
> +#define RISCV_ABI_FLAG_U	(1 << 20)	/* User mode.  */

I don't find where are they used.  Let us add them when they are really
used somewhere in the code.

> +
> +#define IS_RV32I(x)	(((x) & 0xF0000000) == RISCV_ABI_FLAG_RV32I)
> +#define IS_RV64I(x)	(((x) & 0xF0000000) == RISCV_ABI_FLAG_RV64I)
> +#define IS_RV128I(x)	(((x) & 0xF0000000) == RISCV_ABI_FLAG_RV128I)
> +
> +#define HAS_FPU(x)	((x) & (RISCV_ABI_FLAG_D | RISCV_ABI_FLAG_F))
> +

> +#endif /* RISCV_TDEP_H */
> diff --git a/include/gdb/sim-riscv.h b/include/gdb/sim-riscv.h
> new file mode 100644
> index 0000000..932cf49
> --- /dev/null
> +++ b/include/gdb/sim-riscv.h
> @@ -0,0 +1,98 @@
> +/* This file defines the interface between the RISC-V simulator and GDB.
> +
> +   Copyright (C) 2005-2015 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/>.  */
> +
> +/* Order has to match gdb riscv-tdep list.  */
> +enum sim_riscv_regnum {

I don't see how is this header file related to this patch?  Probably
this should be moved to sim patch.

-- 
Yao (齐尧)