From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29510 invoked by alias); 14 Nov 2016 22:39:47 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 29497 invoked by uid 89); 14 Nov 2016 22:39:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=1.5 required=5.0 tests=AWL,BAYES_50,KAM_LOTSOFHASH,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=Miss, luckily, Port, palmer X-HELO: mail-pg0-f66.google.com Received: from mail-pg0-f66.google.com (HELO mail-pg0-f66.google.com) (74.125.83.66) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Nov 2016 22:39:36 +0000 Received: by mail-pg0-f66.google.com with SMTP id p66so10004125pga.2 for ; Mon, 14 Nov 2016 14:39:36 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:cc:cc:subject:in-reply-to :message-id:mime-version; bh=o+STUNoitf3zCk52oJkX/3YxxpiCd4m6LH6FO5IPTYA=; b=JICb6uOM5rZPxvKrP4GcdI7xZiemlY61qeQfouqvNh0sKd0pweOADumgR/EQWbQqk/ r4k7TmERwd5iMSpyT24fm4WOleX7GyKVsR1Eic3qpC6dCHrm41aR9miIFsFQ1PYaewwt zfTw00pcgwjz5lHtlUtvVQJeXiELtTl8J/rlRaT+Pl2CA3VZjbQwBjm4j9958Jartsqs QeTfW/x891ieu9mq8nufPzDLQ0Ny1t9ylJAI0SGr0WftdlO2ZXod4NSmZV7mL//V9WK6 RWUxUPCqJ4zAKbFVTLeZ9BSt141NePZyERaWdISpUwJSmwnuBO/cMLmMhHnNrBUdFweQ gmuw== X-Gm-Message-State: ABUngvfI2hNz5XB16cE8ypXu1CJJBAWi3nS8Q2WM9IbUdxk7alagDPn7iqnuhthVuvQj7g== X-Received: by 10.99.232.17 with SMTP id s17mr72683572pgh.127.1479163174029; Mon, 14 Nov 2016 14:39:34 -0800 (PST) Received: from localhost (dhcp-39-103.EECS.Berkeley.EDU. [128.32.39.103]) by smtp.gmail.com with ESMTPSA id p25sm37543699pfk.20.2016.11.14.14.39.33 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Nov 2016 14:39:33 -0800 (PST) Date: Mon, 14 Nov 2016 22:39:00 -0000 X-Google-Original-Date: Mon, 14 Nov 2016 14:39:10 PST (-0800) From: Palmer Dabbelt To: qiyaoltc@gmail.com CC: Andrew Waterman CC: gdb-patches@sourceware.org CC: amodra@gmail.com Subject: Re: [PATCH 1/2] RISC-V GDB Port In-Reply-To: <86y4107n6q.fsf@gmail.com> Message-ID: Mime-Version: 1.0 (MHng) X-IsSubscribed: yes X-SW-Source: 2016-11/txt/msg00363.txt.bz2 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 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 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 . >> + >> + 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 . */ >> + >> +#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 >> + and by Mike Frysinger . > > 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 . */ >> + >> +#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 >> + > > > >> + >> +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"); >> +} >> + > > > > >> + >> +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 >> + and by Mike Frysinger . >> + >> + 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 . */ >> + >> +#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 . */ >> + >> +/* 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.