This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 1/2] RISC-V GDB Port
- From: Palmer Dabbelt <palmer at dabbelt dot com>
- To: qiyaoltc at gmail dot com
- Cc: Andrew Waterman <andrew at sifive dot com>
- Cc: gdb-patches at sourceware dot org
- Cc: amodra at gmail dot com
- Date: Mon, 14 Nov 2016 14:39:33 -0800 (PST)
- Subject: Re: [PATCH 1/2] RISC-V GDB Port
- Authentication-results: sourceware.org; auth=none
Sorry for taking a while to respond to this, we got a bit distracted by other
parts of the binutils port upstreaming process.
On Thu, 03 Nov 2016 05:38:53 PDT (-0700), qiyaoltc@gmail.com wrote:
> 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?
https://github.com/riscv/riscv-binutils-gdb/commit/76e33b6883db0e2a21b5fa460e7d2952f5760c08
>
>> 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?
https://github.com/riscv/riscv-binutils-gdb/commit/76e33b6883db0e2a21b5fa460e7d2952f5760c08
>> 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
https://github.com/riscv/riscv-binutils-gdb/commit/7f008660df2b4859b80f1e7f8a29257ba831423d
>
>> +# Contributed by Albert Ou <albert@sifive.com>.
>> +#
>
> Albert Ou doesn't have FSF copyright assignment.
Sorry about that, we got his paperwork all sorted out.
>> --- 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.
I agree, but I couldn't make that work when I initially tried to implement this
(something about SIM_EXTRA_LIBS not getting passed far enough around).
Luckily, we've actually removed the dependency of our simulatior on
clock_gettime, so this can just be removed entirely.
https://github.com/riscv/riscv-binutils-gdb/commit/f64950f9d68068acf1236f1ee9dd36eebc7b807e
>> 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.
Sorry, I can't figure out why that's there. I'm going to assume I screwed up
merging something at some point and drop the diff.
https://github.com/riscv/riscv-binutils-gdb/commit/4ee773ddb3334393e0912c21db660a5d4a6f0ba4
Thanks for noticing!
>> 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.
OK, thanks! At some point there was a merge that dropped the old way of doing
it and I couldn't figure out how to do it right. Thanks for noticing!
https://github.com/riscv/riscv-binutils-gdb/commit/0a9a29028cd26da628883bb49c0d5d06abaa30f4
>> --- /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.
OK, we'll try to sort this out. One of the other bluespec guys has this filled
out, so I don't think it'll be a big headache. I must have just missed a
contributer. Thanks for finding him!
>> +
>> + 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.
I'll rebase every patchset I submit on top of the latest master, but this
specific issue has already been fixed. Thanks for the heads up!
>> +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.
I went through and fixed those issues. There was also a large style-fix commit
that went through, hopefully it's all OK now.
https://github.com/riscv/riscv-binutils-gdb/commit/ad8325bd484c1b2d0eb27ab403b1e87d69dd594e
>> +{
>> + 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.
I'm not sure what a "target description" is. Are you talking about this
https://sourceware.org/gdb/onlinedocs/gdb/Target-Description-Format.html
? If so then I don't think we have one, but I also can't figure out where
anyone else's is.
riscv-binutils-gdb $ find -iname "*.dtd"
./gdb/features/threads.dtd
./gdb/features/btrace.dtd
./gdb/features/library-list.dtd
./gdb/features/osdata.dtd
./gdb/features/traceframe-info.dtd
./gdb/features/xinclude.dtd
./gdb/features/gdb-target.dtd
./gdb/features/library-list-svr4.dtd
./gdb/features/btrace-conf.dtd
./gdb/features/library-list-aix.dtd
./gdb/syscalls/gdb-syscalls.dtd
riscv-binutils-gdb $ find -iname "*.dtd" | xargs grep 86
I feel like I must be missing something here. Sorry!
>> +
>> +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.
Both fixed: https://github.com/riscv/riscv-binutils-gdb/commit/f75a58ffd7450915ee776594baceb95a0e10af99
>
>> + len -= regsize;
>> + valbuf += regsize;
>> + }
>> +}
>> +
>
> /* Implement the return_value gdbarch method. */
I'm not sure what you mean by this.
>> +
>> +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.
I think it was only one
https://github.com/riscv/riscv-binutils-gdb/commit/875e1bbf5b783d4b6b23786e73a89623fd4dadaa
>
>> + /* 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. */
I'm afraid I'm not sure what you're trying to say here, sorry!
>> +{
>> + 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?
I think this is actually correct as of right now -- we landed upstream a bit
quickly into binutils and one of the things that was still broken was that
"long double" was 64-bit on RISC-V. Patches to fix this are part of the next
set I'm going to send to binutils, I'll fix this as well when they get
submitted.
>
>> + 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.
https://github.com/riscv/riscv-binutils-gdb/commit/b8cfc4293684c3dcaa39fbcd68c75cb486452c00
>
>> + }
>> +}
>> +
>> +/* 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.
MIPS (where this code came from) still uses the deprecated function. I can't
figure out why this was deprecated or why MIPS is still using the old version,
and since this code does dangerous looking things (silently copying half of a
double-precision float) I'm not really sure how to change it.
>> + 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
Ah, thanks -- this would have been a pain to fix :)
https://github.com/riscv/riscv-binutils-gdb/commit/f0ded1266c58a364481c8de57a884f8ac2d173a0
>> +
>> +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.
It looks like this is another one we got from MIPS. I'm OK changing it, but
I'm not sure what the implications are. I think 0 is the sanest value to put
in there?
https://github.com/riscv/riscv-binutils-gdb/commit/73656f235a8b7cedaab10ee49bbc55dbf02e86ce
If that looks OK to you then I can go try to figure out if it's the right thing
to do.
>> +}
>> +
>> +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?
We don't currently have any RISC-V instructions that are longer than 4 bytes.
The standard allows for this, but it doesn't look like there'll be any in the
near future. I fixed it anyway, because the old version was uglier
https://github.com/riscv/riscv-binutils-gdb/commit/34a6a4b4f553171d55de955ce75346ee781b6b2a
>> + 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.
I agree. Whoever wrote this code must have either not known about riscv-opc.h,
or not liked those macros. I added a FIXME for now
https://github.com/riscv/riscv-binutils-gdb/commit/1b58570b0310850290ed5355776e78192e893d71
>> +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);
>
I think there were two formatting issues (one line was 80 characters)
https://github.com/riscv/riscv-binutils-gdb/commit/c24e88a3e4464c922a669f84c1c67a1ae84444a1
>> +
>> +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.
Sounds good to me.
https://github.com/riscv/riscv-binutils-gdb/commit/09a286c89cdeea2c5ee21d239250a06bb3d92bc8
>> +
>> + /* 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);
OK, thanks for the heads up. I'll be sure to handle this when I rebase on top
of those, I don't think they've made it in yet.
>> +
>> + /* Check any target description for validity. */
>> + if (tdesc_has_registers (info.target_desc))
>> + {
>
> We don't have riscv target description yet.
Yes. I'm amenable to adding one, but I'm not sure how.
>> + 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.
Sounds good to me.
https://github.com/riscv/riscv-binutils-gdb/commit/69e45cdcc667371ce60db88ae613709e6f37dcce
>> +
>> +#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.
Yes, I must have added it by mistake.
Thanks for all the feedback. I'll incorporate all this into a v2, which will
be rebased onto the latest binutils-gdb master.