Re: [PATCH] gdb: Initial baremetal riscv support

[Simon Marchi <simark at simark dot ca>]

Hi Andrew,

I wnt through the patch and put some comments to the best of my knowledge.
I don't really have the expertise to go through all the details about unwinding,
infcall, etc, so I only read those at a high level.  But in general, I find the
code very well organized and commented.

On 2018-02-13 02:14 PM, Andrew Burgess wrote:
> This patch adds initial RiscV support to GDB.
> 
> The work is based on the work that was previously submitted here:
> 
>    https://sourceware.org/ml/gdb-patches/2017-03/msg00048.html
> 
> However, this new patch only adds bare-metal support right now.  I've
> made quite a few changes from the original patch, any mistakes should
> be considered my own.
> 
> I've been testing the new riscv target using a simulator that is based
> on this patch:
> 
>    https://sourceware.org/ml/gdb-patches/2017-03/msg00049.html
> 
> but I'm not submitting the simulator again at this point, as I've not
> had time to fully review the simulator code, and though it does seem
> to be working well enough for testing, I'm reluctant to post it just
> yet.  Hopefully I'll be able to review and test it soon, and then I'll
> post it for review.
> 
> I've been testing against 8 different riscv variants, the table below
> lists the latest results, the failure rate is currently under 1%:
> 
>   |             | rv32im | rv32imc | rv32imf | rv32imfc | rv64im | rv64imc | rv64imfd | rv64imfdc |
>   -------------------------------------------------------------------------------------------------
>   |        PASS |  34789 |   34791 |   34910 |    34904 |  34818 |   34811 |    34928 |     34921 |
>   |        FAIL |    334 |     328 |     216 |      214 |    328 |     327 |      218 |       217 |
>   |       XPASS |      1 |       1 |       1 |        1 |      1 |       1 |        1 |         1 |
>   |       XFAIL |     30 |      30 |      30 |       30 |     32 |      32 |       32 |        32 |
>   |       KPASS |      3 |       3 |       3 |        3 |      3 |       3 |        3 |         3 |
>   |       KFAIL |     53 |      53 |      53 |       53 |     51 |      51 |       51 |        51 |
>   |  UNRESOLVED |     24 |      21 |      21 |       21 |     21 |      21 |       21 |        21 |
>   |    UNTESTED |    230 |     230 |     230 |      230 |    228 |     228 |      228 |       228 |
>   | UNSUPPORTED |    326 |     326 |     326 |      326 |    326 |     326 |      326 |       326 |
>   -------------------------------------------------------------------------------------------------
> 
> At this point I feel like the port is good enough that I should get
> this reviewed and hopefully merged.
> 
> To reproduce the results above the easiest way will be to clone the
> toolchain build script I've been using, and use that to fetch and
> build the tools:
> 
>    mkdir riscv
>    cd riscv
>    git clone -b gdb-upstream-riscv https://github.com/embecosm/riscv-toolchain.git toolchain
>    cd toolchain
>    ./clone-all.sh
>    RV_XLEN=32
>    RV_ARCH=imc
>    RV_TARGET=rv${RV_XLEN}${RV_ARCH}
>    ./build-tools.sh --build-dir $PWD/../build.${RV_TARGET} \
>                     --install-dir $PWD/../install.${RV_TARGET} \
>                     --with-xlen ${RV_XLEN} \
>                     --with-arch ${RV_ARCH}
>    cd ../build.${RV_TARGET}/gdb
>    PATH=$(cd ../../install.${RV_TARGET} && pwd):$PATH
>    make check-gdb RUNTESTFLAGS="--target_board=riscv-sim"
> 
> You can adjust the values of RV_XLEN and RV_ARCH based on the target
> you want to test.
> 
> The commands above will clone the gdb-riscv-sim branch from:
> 
>   https://github.com/embecosm/riscv-gdb.git
> 
> which starts with the riscv patch I am posting here, and then adds the
> riscv simulator on top.
> 
> The next things I'd like to look at after this patch, in no particular
> order, are:
> 
>   1. Running and testing against a gdbserver target.
> 
>   2. Trying to squash some of the remaining test failures.
> 
>   3. Review and post the simulator code.
> 
> I've taken the liberty of proposing that myself and Palmer Dabbelt
> (the original patch submitter) as maintainers for riscv, and added our
> names to the MAINTAINERS file in the relevant place.  If this is not
> appropriate, just let me know and I can drop that change.

Since you are submitting the initial support, I think it's the natural
thing to do.

> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> new file mode 100644
> index 00000000000..76f768ac6fa
> --- /dev/null
> +++ b/gdb/riscv-tdep.c
> @@ -0,0 +1,2689 @@
> +/* Target-dependent code for the RISC-V architecture, for GDB.
> +
> +   Copyright (C) 1988-2015 Free Software Foundation, Inc.

Are the copyright years the right ones?  It's ok to put an earlier year
even for new code, if you copied some existing code, but the end of
the range should probably be 2018 if you've worked on it this year.

> +static uint32_t
> +read_misa_reg (bool *read_p)
> +{
> +  static bool read = false;
> +  static uint32_t value = 0;

Does this handle correctly having multiple inferiors with different misa values?
Also, does it handle debugging an inferior with a certain misa value, disconnecting
and debugging an inferior with a different misa value?

> +
> +  if (!read && target_has_registers)
> +    {
> +      struct frame_info *frame = get_current_frame ();
> +      TRY
> +	{
> +	  value = get_frame_register_unsigned (frame, RISCV_CSR_MISA_REGNUM);
> +	}
> +      CATCH (ex, RETURN_MASK_ERROR)
> +	{
> +	  /* Old cores might have MISA located at a different offset.  */
> +	  value = get_frame_register_unsigned (frame,
> +					       RISCV_CSR_LEGACY_MISA_REGNUM);
> +	}
> +      END_CATCH
> +      read = true;
> +    }
> +
> +  if (read_p != nullptr)
> +    *read_p = read;
> +
> +  return value;
> +}
> +/* 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.  */
> +
> +static int
> +riscv_isa_xlen (struct gdbarch *gdbarch)
> +{
> +  switch (gdbarch_tdep (gdbarch)->abi.fields.base_len)
> +    {
> +    default:
> +      /* Unknown width, assume 32-bit.  */

Should we print a warning (e.g. "unknown width, assuming 32-bits")?  If this is a
sign that something is probably wrong and should be looked into, a warning would help
being aware that there is a problem.

> +    case 1:
> +      return 4;
> +    case 2:
> +      return 8;
> +    case 3:
> +      return 16;
> +    }
> +
> +  return 0; /* Never reached.  */

Is this needed?  I think all compiler used in practice are smart to understand
that execution won't go past the switch and won't warn about this.

> +/* Implement the sw_breakpoint_from_kind gdbarch method.  */
> +
> +static const gdb_byte *
> +riscv_sw_breakpoint_from_kind (struct gdbarch *gdbarch, int kind, int *size)
> +{
> +  static const gdb_byte ebreak[] = { 0x73, 0x00, 0x10, 0x00, };
> +  static const gdb_byte c_ebreak[] = { 0x02, 0x90 };
> +
> +  *size = kind;
> +  switch (kind)
> +    {
> +    case 2:
> +      return c_ebreak;
> +    case 4:
> +      return ebreak;
> +    default:
> +      gdb_assert(0);

Space before parenthesis. You can use gdb_assert_not_reached with a meaningful
message also.

> +/* Implement the register_type gdbarch method.  */
> +
> +static struct type *
> +riscv_register_type (struct gdbarch *gdbarch, int regnum)
> +{
> +  int regsize;
> +
> +  if (regnum < RISCV_FIRST_FP_REGNUM)
> +    {
> +      if (regnum == gdbarch_pc_regnum (gdbarch)
> +	  || regnum == RISCV_RA_REGNUM)
> +	return builtin_type (gdbarch)->builtin_func_ptr;
> +
> +      if (regnum == RISCV_FP_REGNUM
> +	  || regnum == RISCV_SP_REGNUM
> +	  || regnum == RISCV_GP_REGNUM
> +	  || regnum == RISCV_TP_REGNUM)
> +	return builtin_type (gdbarch)->builtin_data_ptr;
> +
> +      /* Remaining GPRs vary in size based on the current ISA.  */
> +      regsize = riscv_isa_xlen (gdbarch);
> +      switch (regsize)
> +	{
> +	case 4:
> +	  return builtin_type (gdbarch)->builtin_uint32;
> +	case 8:
> +	  return builtin_type (gdbarch)->builtin_uint64;
> +	case 16:
> +	  return builtin_type (gdbarch)->builtin_uint128;
> +	default:
> +	  internal_error (__FILE__, __LINE__,
> +			  _("unknown isa regsize %i"), regsize);
> +	}
> +    }
> +  else if (regnum <= RISCV_LAST_FP_REGNUM)
> +    {
> +      regsize = riscv_isa_xlen (gdbarch);
> +      switch (regsize)
> +	{
> +	case 4:
> +	  return builtin_type (gdbarch)->builtin_float;
> +	case 8:
> +	case 16:
> +	  return builtin_type (gdbarch)->builtin_double;

Just curious why this doesn't return builtin_long_double when the size is 16.

> +/* Helper for riscv_print_registers_info, prints info for a single register
> +   REGNUM.  */
> +
> +static void
> +riscv_print_one_register_info (struct gdbarch *gdbarch,
> +			       struct ui_file *file,
> +			       struct frame_info *frame,
> +			       int regnum)
> +{
> +  const char *name = gdbarch_register_name (gdbarch, regnum);
> +  struct value *val = value_of_register (regnum, frame);
> +  struct type *regtype = value_type (val);
> +  int print_raw_format;
> +
> +  fputs_filtered (name, file);
> +  print_spaces_filtered (15 - strlen (name), file);

This magic 15 is the same as tab_stops::value_column_1 in infcmd.c, isn't it?  If so,
it might be a good idea to place that enum in a header file that you could include.
If I understand correctly, this function is used when printing some registers, so
if somebody changed the alignment in default_print_one_register_info, the output
of this function would end up misaligned with the rest.

You could also use the other tab_stop value, if necessary.

> +/* Implement the register_reggroup_p gdbarch method.  */
> +
> +static int
> +riscv_register_reggroup_p (struct gdbarch  *gdbarch, int regnum,
> +			   struct reggroup *reggroup)
> +{
> +  int float_p;
> +  int raw_p;
> +  unsigned int i;
> +
> +  /* Used by 'info registers' and 'info registers <groupname>'.  */
> +
> +  if (gdbarch_register_name (gdbarch, regnum) == NULL
> +      || gdbarch_register_name (gdbarch, regnum)[0] == '\0')
> +    return 0;
> +
> +  if (reggroup == all_reggroup)
> +    {
> +      if (regnum < RISCV_FIRST_CSR_REGNUM || regnum == RISCV_PRIV_REGNUM)
> +	return 1;
> +      /* Only include CSRs that have aliases.  */
> +      for (i = 0; i < ARRAY_SIZE (riscv_register_aliases); ++i)
> +	{
> +	  if (regnum == riscv_register_aliases[i].regnum)
> +	    return 1;
> +	}
> +      return 0;
> +    }
> +  else if (reggroup == float_reggroup)
> +    return ((regnum >= RISCV_FIRST_FP_REGNUM && regnum <= RISCV_LAST_FP_REGNUM)
> +	    || (regnum == RISCV_CSR_FCSR_REGNUM
> +		|| regnum == RISCV_CSR_FFLAGS_REGNUM
> +	        || regnum == RISCV_CSR_FRM_REGNUM));
> +  else if (reggroup == general_reggroup)
> +    return regnum < RISCV_FIRST_FP_REGNUM;
> +  else if (reggroup == restore_reggroup || reggroup == save_reggroup)
> +    {
> +      if (riscv_has_fp_regs (gdbarch))
> +	return regnum <= RISCV_LAST_FP_REGNUM;
> +      else
> +	return regnum < RISCV_FIRST_FP_REGNUM;
> +    }
> +  else if (reggroup == system_reggroup)
> +    {
> +      if (regnum == RISCV_PRIV_REGNUM)
> +	return 1;
> +      if (regnum < RISCV_FIRST_CSR_REGNUM || regnum > RISCV_LAST_CSR_REGNUM)
> +	return 0;
> +      /* Only include CSRs that have aliases.  */
> +      for (i = 0; i < ARRAY_SIZE (riscv_register_aliases); ++i)
> +	{
> +	  if (regnum == riscv_register_aliases[i].regnum)
> +	    return 1;
> +	}
> +      return 0;
> +    }
> +  else if (reggroup == vector_reggroup)
> +    return 0;
> +  else
> +    internal_error (__FILE__, __LINE__, _("unhandled reggroup"));

I find it a bit extreme to call internal_error here.  If somebody were to add
a builtin reggroup to GDB, it could easily make the port unusable.  Also, there
are user-defined groups.  I don't know if we can end up in this function with
reggroup being a user-defined group, but it would be good to check.  In any
case, I think a default of "return 0" is reasonnable.

> +static void
> +riscv_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 = 0;

I am not sure what "reset" means in this function, but the reset value of addr
is -1, so don't you want to assign this value?  What about realreg, does it
need resetting too?

> +/* Fetch from target memory an instruction at PC and decode it into the
> +   INSN structure.  */
> +
> +static void
> +riscv_decode_instruction (struct gdbarch *gdbarch,
> +			  CORE_ADDR pc,
> +			  struct riscv_insn *insn)
> +{
> +  ULONGEST ival;
> +  int len;
> +
> +  /* Silence compiler warnings by clearing INSN.  */
> +  memset (insn, 0, sizeof (*insn));

I'm curious, what was the compiler warning?

In the long run, I think it would be more maintainable to make it a non-POD
type and initialize the fields directly in the class declaration.  This way
you can't have an uninitialized object.

> +/* Initialise SINFO, and then then call the worker function to perform an
> +   analysis of TYPE.  */
> +
> +static void
> +riscv_struct_analysis_for_call (struct type *type,
> +				struct riscv_struct_info *sinfo)
> +{
> +  sinfo->number_of_fields = 0;
> +  sinfo->types[0] = nullptr;
> +  sinfo->types[1] = nullptr;

I would suggest initializing these fields in the class declaration, it reduces
the risk of mis-using it.  This comment could apply to other structures as well,
as you see fit.

> diff --git a/gdb/riscv-tdep.h b/gdb/riscv-tdep.h
> new file mode 100644
> index 00000000000..b2609672c4f
> --- /dev/null
> +++ b/gdb/riscv-tdep.h
> @@ -0,0 +1,83 @@
> +/* 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
> +
> +/* RiscV register numbers.  */
> +enum {

Curly brace on the next line.

Thanks,

Simon