From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 41999 invoked by alias); 27 Feb 2018 03:33:56 -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 41986 invoked by uid 89); 27 Feb 2018 03:33:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=dat, cleans 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; Tue, 27 Feb 2018 03:33: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 4B53F1E584; Mon, 26 Feb 2018 22:33:51 -0500 (EST) Subject: Re: [PATCHv2] gdb: Initial baremetal riscv support To: Andrew Burgess , gdb-patches@sourceware.org Cc: Eli Zaretskii , palmer@sifive.com References: <20180213191455.2405-1-andrew.burgess@embecosm.com> <20180227010906.GW2700@embecosm.com> From: Simon Marchi Message-ID: <1b9ec42a-a0fd-667b-59a5-780e84cce451@simark.ca> Date: Tue, 27 Feb 2018 03:33: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: <20180227010906.GW2700@embecosm.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-02/txt/msg00410.txt.bz2 Hi Andrew, Thanks for the update. I have some more small comments, but overall it looks pretty good. On 2018-02-26 08:09 PM, Andrew Burgess wrote: > Thanks for the reviews. I've revised the patch inline with the > feedback, and include a second revision below. Changes since v1, > include: > > - Rewritten the NEWS file entry > - Fixed copyright years to 2018 > - Reworked reading of the MISA register from the target to cache the > value read on a per-inferior basis. > - A set of small code cleans up suggested by Simon, whitespace, > warnings, etc > - Dropped the unused code that was an initial attempt at creating a > prologue scanner that could track saved registers. We currently > rely entirely on the DWARF scanner. I plan to revisit this later, > but I don't want to wrap fixing that into this initial patch. > - Made more use of classes rather than just POD types, which has > allowed better grouping of code, and removal of some wrapper > functions that were only used for initialisation. > > I've regression tested this revision against the same 8 targets that > the original patch was tested against, and I see no regressions. > > Further feedback always welcome, > > Thanks, > Andrew > > --- > > This commit introduces basic support for baremetal RiscV as a GDB > target. This target is currently only tested against the RiscV software > simulator, which is not included as part of this commit. The target has > been tested against the following RiscV variants: rv32im, rv32imc, > rv32imf, rv32imfc, rv64im, rv64imc, rv64imfd, rv64imfdc. > > Across these variants we pass on average 34858 tests, and fail 272 > tests, which is ~0.8%. > > gdb/ChangeLog: > > * Makefile.in (ALL_TARGET_OBS): Add riscv-tdep.o > (HFILES_NO_SRCDIR): Add riscv-tdep.h. > (ALLDEPFILES): Add riscv-tdep.c > * configure.tgt: Add riscv support. > * riscv-tdep.c: New file. > * riscv-tdep.h: New file. > * NEWS: Mention new target. > * MAINTAINERS: Add entry for riscv. > > gdb/testsuite/ChangeLog: > > * gdb.arch/riscv-callfuncs.c: New file. > * gdb.arch/riscv-callfuncs.exp: New file. > * gdb.base/float.exp: Add riscv support. ... > +/* Read the MISA register from the target. The register will only be read > + once, and the value read will be cached. If the register can't be read > + from the target then a default value (0) will be returned. If the > + pointer READ_P is not null, then the bool pointed to is updated to > + indicate if the value returned was read from the target (true) or is the > + default (false). */ > + > +static uint32_t > +riscv_read_misa_reg (bool *read_p) > +{ > + struct riscv_inferior_data *inf_data > + = riscv_inferior_data (current_inferior ()); > + > + if (!inf_data->misa_read && target_has_registers) > + { > + uint32_t value = 0; > + 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 > + > + inf_data->misa_read = true; > + inf_data->misa_value = value; > + } > + > + if (read_p != nullptr) > + *read_p = inf_data->misa_read; > + > + return inf_data->misa_value; > +} Did you verify what happens when reusing the same inferior to run programs of two different architectures? Something like: 1. file bin-arch1 2. target sim ... read misa 3. kill 4. file bin-arch2 5. target sim ... does it read misa again? Since there's only one inferior the whole time, there's only one riscv_inferior_data instance, so it probably needs to be reset at some point. > +/* Fetch from target memory an instruction at PC and decode it. */ > + > +void > +riscv_insn::decode (struct gdbarch *gdbarch, CORE_ADDR pc) > +{ > + ULONGEST ival; > + int len; > + > + /* Fetch the instruction, and the instructions length. Compute the > + next pc we decode. We don't support instructions longer than 4 What does "Compute the next pc we decode" mean? > + bytes yet. */ > + ival = fetch_instruction (gdbarch, pc, &len); Nit: you could remove the "len" variable and pass &m_length directly. > +/* Structure defining the RiscV normal frame unwind functions. Since we > + are the fallback unwinder (DWARF unwinder is used first), we use the > + default frame sniffer, which always accepts the frame. */ > + > +static const struct frame_unwind riscv_frame_unwind = > +{ > + /*.type =*/ NORMAL_FRAME, > + /*.stop_reason =*/ default_frame_unwind_stop_reason, > + /*.this_id =*/ riscv_frame_this_id, > + /*.prev_register =*/ riscv_frame_prev_register, > + /*.unwind_data =*/ NULL, > + /*.sniffer =*/ default_frame_sniffer, > + /*.dealloc_cache =*/ NULL, > + /*.prev_arch =*/ NULL, > +}; > + > + > + > + > + > + Unneeded empty lines? > +/* Allocate new riscv_inferior_data object. */ > + > +static struct riscv_inferior_data * > +riscv_new_inferior_data (void) > +{ > + struct riscv_inferior_data *const inf_data > + = XCNEW (struct riscv_inferior_data); > + inf_data->misa_read = false; > + return inf_data;> +} > + > +/* Free inferior data. */ > + > +static void > +riscv_inferior_data_cleanup (struct inferior *const inf, void *const dat) > +{ > + xfree (dat); > +} You can use new/delete here too if you want. > diff --git a/gdb/testsuite/gdb.arch/riscv-callfuncs.exp b/gdb/testsuite/gdb.arch/riscv-callfuncs.exp > new file mode 100644 > index 00000000000..f987dd0d541 > --- /dev/null > +++ b/gdb/testsuite/gdb.arch/riscv-callfuncs.exp > @@ -0,0 +1,52 @@ > +# Copyright 2018 Free Software Foundation, Inc. > + > +# 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 . > + > +standard_testfile > + > +set compile_flags {debug} > +if [support_complex_tests] { > + lappend compile_flags "additional_flags=-DTEST_COMPLEX" > +} > + > +# Some targets can't do function calls, so don't even bother with this > +# test. > +if [target_info exists gdb,cannot_call_functions] { > + unsupported "this target can not call functions" > + continue > +} > + > +set skip_float_test [gdb_skip_float_test] > + > +if { [prepare_for_testing "failed to prepare" $testfile $srcfile "$compile_flags"] } { > + continue > +} > + > +if { ![runto_main] } { > + perror "couldn't run to main" > + continue > +} > + > +gdb_breakpoint breakpt > +gdb_continue_to_breakpoint "breakpt" > + > +gdb_test "p handle_single_f_f (f_f_val1)" " = 1" > +gdb_test "p handle_single_d_d (d_d_val1)" " = 1" > +gdb_test "p handle_single_ld_ld (ld_ld_val1)" " = 1" > + > +if [support_complex_tests] { > + gdb_test "p handle_single_fc (fc_val1)" " = 1" > + gdb_test "p handle_single_dc (dc_val1)" " = 1" > + gdb_test "p handle_single_ldc (ldc_val1)" " = 1" > +} A question about this test: if it's really riscv-specific, there should be a check at the top to skip the file if the current target arch is not riscv (see other files in gdb.arch). Otherwise, it will be executed on other arches and fail. If the test is not really riscv-specific, then maybe it would be better to enhance an existing test case in gdb.base, if you have found that some cases were not well tested. It would then benefit all arches. Thanks! Simon