Re: [PATCHv2] gdb: Initial baremetal riscv support

[Simon Marchi <simark@simark.ca>]

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 <http://www.gnu.org/licenses/>.
> +
> +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