From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 69868 invoked by alias); 19 Feb 2018 20:01:58 -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 69848 invoked by uid 89); 19 Feb 2018 20:01:56 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.1 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=simulator, unusable X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Feb 2018 20:01:53 +0000 Received: from [10.0.0.11] (192-222-251-162.qc.cable.ebox.net [192.222.251.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by simark.ca (Postfix) with ESMTPSA id EC71D1E093; Mon, 19 Feb 2018 15:01:51 -0500 (EST) Subject: Re: [PATCH] gdb: Initial baremetal riscv support To: Andrew Burgess , gdb-patches@sourceware.org Cc: palmer@sifive.com References: <20180213191455.2405-1-andrew.burgess@embecosm.com> From: Simon Marchi Message-ID: <29d81d1c-4f11-740b-2e53-e01de517a1ef@simark.ca> Date: Mon, 19 Feb 2018 20:01:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180213191455.2405-1-andrew.burgess@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-02/txt/msg00253.txt.bz2 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 '. */ > + > + 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 > + 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 > + > +/* RiscV register numbers. */ > +enum { Curly brace on the next line. Thanks, Simon