Re: [PATCHv2 5/9] gdb/riscv: introduce bare metal core dump support

[Luis Machado <luis.machado@linaro.org>]

Andrew,

On 1/20/21 5:23 PM, Andrew Burgess wrote:
> The commit message below includes a description of the core file
> format.  Though this description could exist in the commit message, I
> don't imagine this is really the right place to document something
> like this.

Thanks for writing this down.

> 
> For RISC-V I imagine the correct place would be here:
> 
>    https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md
> 
> And I have a commit for this repository that replicates the
> description below.  However, I wanted to post this to the binutils/gdb
> list first, and see if this description was good enough for all
> stakeholders.
> 
> If people here are happy with this commit and the documentation, then
> I would image creating a pull request against the RISC-V ABI document,
> and then if there's no negative feedback, merge this patch at that
> point.
> 
> All feedback, on docs or implementation, is welcome,
> 
> Thanks,
> Andrew
> 
> 
> 
> 
> 
> ----
> 
> This commit adds the ability for bare metal RISC-V target to generate
> core files from within GDB.
> 
> The intended use case is that a user will connect to a remote bare
> metal target, debug up to some error condition, then generate a core
> file in the normal way using:
> 
>    (gdb) generate-core-file
> 
> This core file can then be used to revisit the state of the remote
> target without having to reconnect to the remote target.
> 
> The core file creation code is split between two new files.  In
> none-tdep.c is code designed to support the architecture agnostic
> parts of a bare metal core dump.
> 
> In riscv-none-tdep.c are the RISC-V specific parts, this is where the
> regset and regcache_map_entry structures are defined that control how
> registers are laid out in the core file.
> 
> Currently for RISC-V only the x-regs and f-regs (if present) are
> written out.  In future commits I plan to add support for writing out
> the RISC-V CSRs.
> 
> The cores dump format is based around generating an ELF containing

cores -> core

> sections for the writable regions of memory that a user could be
> using.  Which regions are dumped rely on GDB's existing common core
> dumping code, GDB will attempt to figure out the stack and heap as
> well as copying out writable data sections as identified by the
> original ELF.
> 
> Register information is added to the core dump using notes, just as it
> is for Linux of FreeBSD core dumps.  The note types used consist of
> the 3 basic types you would expect in a OS based core dump,
> NT_PRPSINFO, NT_PRSTATUS, NT_FPREGSET.
> 
> The layout of these notes differs slightly (due to field sizes)
> between RV32 and RV64.  Below I describe the data layout for each
> note.  In all case, all padding fields should be set to zero.

case -> cases

> 
> Note NT_PRPSINFO is optional.  Its data layout is:
> 
>    struct prpsinfo32_t		/* For RV32.  */
>    {
>      uint8_t padding[32];
>      char fname[16];
>      char psargs[80];
>    }
> 
>    struct prpsinfo64_t		/* For RV64.  */
>    {
>      uint8_t padding[40];
>      char fname[16];
>      char psargs[80];
>    }
> 
> Field 'fname' - null terminated string consisting of the basename of
>      (up to the fist 15 characters of) the executable.  Any additional
>      space should be set to zero.  If there's no executable name then
>      this field can be set to all zero.
> 
> Field 'psargs' - a null terminated string up to 80 characters in
>      length.  Any additional space should be filled with zero.  This
>      field contains the full executable path and any arguments passed
>      to the executable.  If there's nothing sensible to write in this
>      field then fill it with zero.
> 
> Note NT_PRSTATUS is required, its data layout is:
> 
>    struct prstatus32_t		/* For RV32.  */
>    {
>      uint8_t padding_1[12];
>      uint16_t sig;
>      uint8_t padding_2[10];
>      uint32_t thread_id;
>      uint8_t padding_3[44];
>      uint32_t x_regs[32];
>      uint8_t padding_4[4];
>    }
> 
>    struct prstatus64_t		/* For RV64.  */
>    {
>      uint8_t padding_1[12];
>      uint16_t sig;
>      uint8_t padding_2[18];
>      uint32_t thread_id;
>      uint8_t padding_3[76];
>      uint64_t x_regs[32];
>      uint8_t padding_4[4];
>    }
> 
> Field 'sig' - the signal that stopped this thread.  Its implementation

Its -> It's

>      defined what this field actually means.  Within GDB this will be
>      the signal number that the remote target reports as the stop
>      reason for this thread.
> 
> Field 'thread_is' - the thread id for this thread, its implementation

its -> it's

>      defined what this field actually means.  Within GDB this will be
>      thread thread-id that is assigned to each remote thread.
> 
> Field 'x_regs' - at index 0 we store the program counter, and at
>      indices 1 to 31 we store x-registers 1 to 31.  x-register 0 is not
>      stored, its value is always zero anyway.
> 
> Note NT_FPREGSET is optional, its data layout is:
> 
>    fpregset32_t			/* For targets with 'F' extension.  */
>    {
>      uint32_t f_regs[32];
>      uint32_t fcsr;
>    }
> 
>    fpregset64_t			/* For targets with 'D' extension .  */
>    {
>      uint64_t f_regs[32];
>      uint32_t fcsr;
>    }
> 
> Field 'f_regs' - stores f-registers 0 to 31.
> 
> Field 'fcsr' - stores the fcsr CSR register, and is always 4-bytes.
> 
> The rules for ordering the notes is the same as for Linux.  The
> NT_PRSTATUS note must come before any other notes about additional
> register sets.  And for multi-threaded targets all registers for a
> single thread should be grouped together.  This is because only
> NT_PRSTATUS includes a thread-id, all additional register notes after
> a NT_PRSTATUS are assumed to belong to the same thread until a
> different NT_PRSTATUS is seen.

I think the above documentation is a good start, and we can expand it 
with more information.

I'm wondering if we should document this in the GDB manual. Then again, 
it may be a bit too technical for the user manual.

Another idea is to document arch-specific bits in arch-specific files 
and the generic parts in the none-tdep.c file?

> 
> gdb/ChangeLog:
> 
> 	* Makefile.in (ALL_TARGET_OBS): Add riscv-none-tdep.o.
> 	(ALLDEPFILES): Add riscv-none-tdep.c.
> 	* configure.tgt (riscv*-*-*): Include riscv-none-tdep.c.
> 	* none-tdep.c: New file.
> 	* none-tdep.h: New file.
> 	* riscv-none-tdep.c: New file.
> ---
>   gdb/ChangeLog         |  10 ++++
>   gdb/Makefile.in       |   4 ++
>   gdb/configure.tgt     |   2 +-
>   gdb/none-tdep.c       | 119 ++++++++++++++++++++++++++++++++++++++++++
>   gdb/none-tdep.h       |  30 +++++++++++
>   gdb/riscv-none-tdep.c | 108 ++++++++++++++++++++++++++++++++++++++
>   6 files changed, 272 insertions(+), 1 deletion(-)
>   create mode 100644 gdb/none-tdep.c
>   create mode 100644 gdb/none-tdep.h
>   create mode 100644 gdb/riscv-none-tdep.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 9267bea7beb..5f88b6a78cf 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -807,6 +807,7 @@ ALL_TARGET_OBS = \
>   	ravenscar-thread.o \
>   	riscv-fbsd-tdep.o \
>   	riscv-linux-tdep.o \
> +	riscv-none-tdep.o \
>   	riscv-ravenscar-thread.o \
>   	riscv-tdep.o \
>   	rl78-tdep.o \
> @@ -1100,6 +1101,7 @@ COMMON_SFILES = \
>   	minsyms.c \
>   	mipsread.c \
>   	namespace.c \
> +	none-tdep.c \
>   	objc-lang.c \
>   	objfiles.c \
>   	observable.c \
> @@ -1360,6 +1362,7 @@ HFILES_NO_SRCDIR = \
>   	netbsd-tdep.h \
>   	nds32-tdep.h \
>   	nios2-tdep.h \
> +	none-tdep.h \
>   	nto-tdep.h \
>   	objc-lang.h \
>   	objfiles.h \
> @@ -2271,6 +2274,7 @@ ALLDEPFILES = \
>   	riscv-fbsd-tdep.c \
>   	riscv-linux-nat.c \
>   	riscv-linux-tdep.c \
> +	riscv-none-tdep.c \
>   	riscv-ravenscar-thread.c \
>   	riscv-tdep.c \
>   	rl78-tdep.c \
> diff --git a/gdb/configure.tgt b/gdb/configure.tgt
> index 6e039838748..ad88ddd9302 100644
> --- a/gdb/configure.tgt
> +++ b/gdb/configure.tgt
> @@ -85,7 +85,7 @@ ia64*-*-*)
>   	;;
>   
>   riscv*-*-*)
> -	cpu_obs="riscv-tdep.o arch/riscv.o \
> +	cpu_obs="riscv-tdep.o riscv-none-tdep.o arch/riscv.o \
>   	         ravenscar-thread.o riscv-ravenscar-thread.o";;
>   
>   x86_64-*-*)
> diff --git a/gdb/none-tdep.c b/gdb/none-tdep.c
> new file mode 100644
> index 00000000000..8ec2407ad45
> --- /dev/null
> +++ b/gdb/none-tdep.c
> @@ -0,0 +1,119 @@
> +/* Target-dependent code for none, architecture independent.
> +
> +   Copyright (C) 2020 Free Software Foundation, Inc.

2020~2021 now I think.

> +
> +   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 "none-tdep.h"
> +#include "regset.h"
> +#include "elf-bfd.h"            /* for elfcore_write_* */
> +#include "inferior.h"
> +#include "regcache.h"
> +#include "gdbarch.h"
> +#include "gcore.h"
> +
> +/* Build the note section for a corefile, and return it in a malloc
> +   buffer.  Currently this just  dumps all available registers for each

Spurious whitespace in "just  dumps"

> +   thread.  */
> +
> +static gdb::unique_xmalloc_ptr<char>
> +none_make_corefile_notes (struct gdbarch *gdbarch, bfd *obfd, int *note_size)
> +{
> +  gdb::unique_xmalloc_ptr<char> note_data;
> +
> +  /* Add note information about the executable and its arguments.  */
> +  std::string fname;
> +  std::string psargs;
> +  static const size_t fname_len = 16;
> +  static const size_t psargs_len = 80;
> +  if (get_exec_file (0))
> +    {
> +      const char *exe = get_exec_file (0);
> +      fname = lbasename (exe);
> +      psargs = std::string (exe);
> +
> +      const char *infargs = get_inferior_args ();
> +      if (infargs != nullptr)
> +	psargs += " " + std::string (infargs);
> +
> +      /* All existing targets that handling writing out prpsinfo expect the

handling -> handle

> +	 fname and psargs strings to be at least 16 and 80 characters long
> +	 respectively, including a null terminator at the end.  Resize to
> +	 the expected length minus one to ensure there is a null within the
> +	 required length.  */
> +      fname.resize (fname_len - 1);
> +      psargs.resize (psargs_len - 1);
> +    }
> +
> +  /* Resize the buffers up to their required lengths.  This will fill any
> +     remaining space with the null character.  */
> +  fname.resize (fname_len);
> +  psargs.resize (psargs_len);
> +
> +  /* Now write out the prpsinfo structure.  */
> +  note_data.reset (elfcore_write_prpsinfo (obfd, note_data.release (),
> +					   note_size, fname.c_str (),
> +					   psargs.c_str ()));
> +  if (note_data == nullptr)
> +    return nullptr;
> +
> +  /* Thread register information.  */
> +  try
> +    {
> +      update_thread_list ();
> +    }
> +  catch (const gdb_exception_error &e)
> +    {
> +      exception_print (gdb_stderr, e);
> +    }
> +
> +  /* Like the Linux kernel, prefer dumping the signalled thread first.
> +     "First thread" is what tools use to infer the signalled thread.  */
> +  thread_info *signalled_thr = gcore_find_signalled_thread ();
> +
> +  /* All threads are reported as having been stopped by the same signal
> +     that stopped SIGNALLED_THR.  */
> +  gdb_signal stop_signal;
> +  if (signalled_thr != nullptr)
> +    stop_signal = signalled_thr->suspend.stop_signal;
> +  else
> +    stop_signal = GDB_SIGNAL_0;
> +
> +  if (signalled_thr != nullptr)
> +    gcore_build_thread_register_notes (gdbarch, signalled_thr,
> +				       stop_signal, obfd, &note_data,
> +				       note_size);
> +  for (thread_info *thr : current_inferior ()->non_exited_threads ())
> +    {
> +      if (thr == signalled_thr)
> +	continue;
> +
> +      gcore_build_thread_register_notes (gdbarch, thr, stop_signal, obfd,
> +					 &note_data, note_size);
> +    }
> +
> +  return note_data;
> +}
> +
> +/* See none-tdep.h.  */
> +
> +void
> +none_init_abi (struct gdbarch *gdbarch)
> +{
> +  /* Default core file support.  */
> +  set_gdbarch_make_corefile_notes (gdbarch, none_make_corefile_notes);
> +}
> diff --git a/gdb/none-tdep.h b/gdb/none-tdep.h
> new file mode 100644
> index 00000000000..46dcfe219ef
> --- /dev/null
> +++ b/gdb/none-tdep.h
> @@ -0,0 +1,30 @@
> +/* Architecture independent code for ABI 'none' (bare-metal).
> +
> +   Copyright (C) 2021 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/>.  */
> +
> +#ifndef NONE_TDEP_H
> +#define NONE_TDEP_H
> +
> +struct gdbarch;
> +
> +/* Initialize support for cross-architecture features applicable for the
> +   GDB_OSABI_NONE ABI, that is bare-metal targets.  */
> +
> +void none_init_abi (struct gdbarch *gdbarch);
> +
> +#endif /* NONE_TDEP_H */
> diff --git a/gdb/riscv-none-tdep.c b/gdb/riscv-none-tdep.c
> new file mode 100644
> index 00000000000..e06ee0981fe
> --- /dev/null
> +++ b/gdb/riscv-none-tdep.c
> @@ -0,0 +1,108 @@
> +/* Copyright (C) 2020 Free Software Foundation, Inc.

2020~2021 now.

> +
> +   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/>.  */
> +
> +/* This file contain code that is specific for bare-metal RISC-V targets.  */
> +
> +#include "defs.h"
> +#include "arch-utils.h"
> +#include "regcache.h"
> +#include "riscv-tdep.h"
> +#include "elf-bfd.h"
> +#include "regset.h"
> +#include "none-tdep.h"
> +
> +/* Define the general register mapping.  This follows the same format as
> +   the RISC-V linux corefile.  The linux kernel puts the PC at offset 0,
> +   gdb puts it at offset 32.  Register x0 is always 0 and can be ignored.
> +   Registers x1 to x31 are in the same place.  */
> +
> +static const struct regcache_map_entry riscv_gregmap[] =
> +{
> +  { 1,  RISCV_PC_REGNUM, 0 },
> +  { 31, RISCV_RA_REGNUM, 0 }, /* x1 to x31 */
> +  { 0 }
> +};
> +
> +/* Define the FP register mapping.  This follows the same format as the
> +   RISC-V linux corefile.  The kernel puts the 32 FP regs first, and then
> +   FCSR.  */
> +
> +static const struct regcache_map_entry riscv_fregmap[] =
> +{
> +  { 32, RISCV_FIRST_FP_REGNUM, 0 },
> +  { 1, RISCV_CSR_FCSR_REGNUM, 4 },	/* Always stored as 4-bytes.  */
> +  { 0 }
> +};
> +
> +/* Define the general register regset.  */
> +
> +static const struct regset riscv_gregset =
> +{
> +  riscv_gregmap, riscv_supply_regset, regcache_collect_regset
> +};
> +
> +/* Define the FP register regset.  */
> +
> +static const struct regset riscv_fregset =
> +{
> +  riscv_fregmap, riscv_supply_regset, regcache_collect_regset
> +};
> +
> +/* Implement the "iterate_over_regset_sections" gdbarch method.  */
> +
> +static void
> +riscv_iterate_over_regset_sections (struct gdbarch *gdbarch,
> +				    iterate_over_regset_sections_cb *cb,
> +				    void *cb_data,
> +				    const struct regcache *regcache)
> +{
> +  /* Write out the GPRs.  */
> +  int sz = 32 * riscv_isa_xlen (gdbarch);
> +  cb (".reg", sz, sz, &riscv_gregset, NULL, cb_data);
> +
> +  /* Write out the FPRs, but only if present.  */
> +  if (riscv_isa_flen (gdbarch) > 0)
> +    {
> +      sz = (32 * riscv_isa_flen (gdbarch)
> +	    + register_size (gdbarch, RISCV_CSR_FCSR_REGNUM));
> +      cb (".reg2", sz, sz, &riscv_fregset, NULL, cb_data);
> +    }
> +}
> +
> +/* Initialize RISC-V bare-metal ABI info.  */
> +
> +static void
> +riscv_none_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
> +{
> +  none_init_abi (gdbarch);
> +
> +  /* Iterate over registers for reading and writing bare metal RISC-V core
> +     files.  */
> +  set_gdbarch_iterate_over_regset_sections
> +    (gdbarch, riscv_iterate_over_regset_sections);
> +
> +}
> +
> +/* Initialize RISC-V bare-metal target support.  */
> +
> +void _initialize_riscv_none_tdep ();
> +void
> +_initialize_riscv_none_tdep ()
> +{
> +  gdbarch_register_osabi (bfd_arch_riscv, 0, GDB_OSABI_NONE,
> +			  riscv_none_init_abi);
> +}
> 

Otherwise this looks good to me.