Remove use of deprecated core functions (in NetBSD/ARM)

  This is in preparation for deleting deprecated_add_core_fns and
related code.

As a side-effect, this makes it possible to read NetBSD/ARM
core files on non-NetBSD/ARM platforms, subject to PR corefiles/25638.

I have removed this comment:
-  /* This is ok: we're running native...  */
Since we are using the gdbarch from the regcache, we should be
guaranteed to be calling the right function here, so it shouldn't
matter whether we are running native.

Tested by reading a NetBSD/ARM core file on Linux/x86-64 and NetBSD/ARM;
the "info registers" output matches the one from the system GDB.

gdb/ChangeLog:

2020-03-05  Christian Biesinger  <cbiesinger@google.com>

	* Makefile.in (HFILES_NO_SRCDIR): Add new arm-nbsd-tdep.h file.
	* arm-nbsd-nat.c (arm_supply_gregset): Moved to arm-nbsd-tdep and
	renamed to arm_nbsd_supply_gregset.
	(fetch_register): Update to call arm_nbsd_supply_gregset.
	(fetch_regs): Remove in favor of fetch_register with a -1 regno.
	(arm_netbsd_nat_target::fetch_registers): Update.
	(fetch_elfcore_registers): Removed.
	(_initialize_arm_netbsd_nat): Removed call to deprecated_add_core_fns.
	* arm-nbsd-tdep.c (struct arm_nbsd_reg): New struct.
	(arm_nbsd_supply_gregset): Moved from arm-nbsd-nat.c and updated to
	not require NetBSD system headers.
	(arm_nbsd_regset): New struct.
	(arm_nbsd_iterate_over_regset_sections): New function.
	(arm_netbsd_init_abi_common): Updated to call
	set_gdbarch_iterate_over_regset_sections.
	* arm-nbsd-tdep.h: New file.
---
 gdb/Makefile.in     |   1 +
 gdb/arm-nbsd-nat.c  | 125 ++------------------------------------------
 gdb/arm-nbsd-tdep.c |  70 ++++++++++++++++++++++++-
 gdb/arm-nbsd-tdep.h |  27 ++++++++++
 4 files changed, 101 insertions(+), 122 deletions(-)
 create mode 100644 gdb/arm-nbsd-tdep.h
  

On 2020-03-06 3:59 p.m., Christian Biesinger via gdb-patches wrote:
> This is in preparation for deleting deprecated_add_core_fns and
> related code.
> 
> As a side-effect, this makes it possible to read NetBSD/ARM
> core files on non-NetBSD/ARM platforms, subject to PR corefiles/25638.
> 
> I have removed this comment:
> -  /* This is ok: we're running native...  */
> Since we are using the gdbarch from the regcache, we should be
> guaranteed to be calling the right function here, so it shouldn't
> matter whether we are running native.
> 
> Tested by reading a NetBSD/ARM core file on Linux/x86-64 and NetBSD/ARM;
> the "info registers" output matches the one from the system GDB.
> 
> gdb/ChangeLog:
> 
> 2020-03-05  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* Makefile.in (HFILES_NO_SRCDIR): Add new arm-nbsd-tdep.h file.
> 	* arm-nbsd-nat.c (arm_supply_gregset): Moved to arm-nbsd-tdep and
> 	renamed to arm_nbsd_supply_gregset.
> 	(fetch_register): Update to call arm_nbsd_supply_gregset.
> 	(fetch_regs): Remove in favor of fetch_register with a -1 regno.
> 	(arm_netbsd_nat_target::fetch_registers): Update.
> 	(fetch_elfcore_registers): Removed.
> 	(_initialize_arm_netbsd_nat): Removed call to deprecated_add_core_fns.
> 	* arm-nbsd-tdep.c (struct arm_nbsd_reg): New struct.
> 	(arm_nbsd_supply_gregset): Moved from arm-nbsd-nat.c and updated to
> 	not require NetBSD system headers.
> 	(arm_nbsd_regset): New struct.
> 	(arm_nbsd_iterate_over_regset_sections): New function.
> 	(arm_netbsd_init_abi_common): Updated to call
> 	set_gdbarch_iterate_over_regset_sections.
> 	* arm-nbsd-tdep.h: New file.

Thanks, this looks good to me, but I would appreciate if Kamil to take a quick
look as well.

I noted some nits below (no need to send another version just for them).

> @@ -35,6 +37,70 @@ static const gdb_byte arm_nbsd_arm_be_breakpoint[] = {0xe6, 0x00, 0x00, 0x11};
>  static const gdb_byte arm_nbsd_thumb_le_breakpoint[] = {0xfe, 0xde};
>  static const gdb_byte arm_nbsd_thumb_be_breakpoint[] = {0xde, 0xfe};
>  
> +/* This matches struct reg from NetBSD's sys/arch/arm/include/reg.h */

I think it would be nice to provide the link in the comment (a permalink,
in case the file ever moves):

https://github.com/NetBSD/src/blob/7c13e6e6773bb171f4ed3ed53013e9d24b3c1eac/sys/arch/arm/include/reg.h#L39

> +struct arm_nbsd_reg {

Curly bracket on next line.

> +  uint32_t reg[13];
> +  uint32_t sp;
> +  uint32_t lr;
> +  uint32_t pc;
> +  uint32_t cpsr;
> +};
> +
> +void
> +arm_nbsd_supply_gregset (const struct regset *regset, struct regcache *regcache,
> +			 int regnum, const void *gregs, size_t len)
> +{
> +  const arm_nbsd_reg *gregset = static_cast<const arm_nbsd_reg *>(gregs);
> +  gdb_assert (len >= sizeof (arm_nbsd_reg));
> +
> +  /* Integer registers.  */
> +  for (int i = ARM_A1_REGNUM; i < ARM_SP_REGNUM; i++)
> +    if (regnum == -1 || regnum == i)
> +      regcache->raw_supply (i, (char *) &gregset->reg[i]);
> +
> +  if (regnum == -1 || regnum == ARM_SP_REGNUM)
> +    regcache->raw_supply (ARM_SP_REGNUM, (char *) &gregset->sp);
> +  if (regnum == -1 || regnum == ARM_LR_REGNUM)
> +    regcache->raw_supply (ARM_LR_REGNUM, (char *) &gregset->lr);
> +  if (regnum == -1 || regnum == ARM_PC_REGNUM)
> +    {
> +      CORE_ADDR r_pc = gdbarch_addr_bits_remove (regcache->arch (), gregset->pc);
> +      regcache->raw_supply (ARM_PC_REGNUM, (char *) &r_pc);
> +    }

Can you add some empty lines between each separate condition?  I find this
looks too much like if/else if.

Simon
  

On 3/6/20 12:59 PM, Christian Biesinger via gdb-patches wrote:
> This is in preparation for deleting deprecated_add_core_fns and
> related code.
> 
> As a side-effect, this makes it possible to read NetBSD/ARM
> core files on non-NetBSD/ARM platforms, subject to PR corefiles/25638.
> 
> I have removed this comment:
> -  /* This is ok: we're running native...  */
> Since we are using the gdbarch from the regcache, we should be
> guaranteed to be calling the right function here, so it shouldn't
> matter whether we are running native.
> 
> Tested by reading a NetBSD/ARM core file on Linux/x86-64 and NetBSD/ARM;
> the "info registers" output matches the one from the system GDB.
> 
> diff --git a/gdb/arm-nbsd-tdep.c b/gdb/arm-nbsd-tdep.c
> index 2642024022..689c4b6219 100644
> --- a/gdb/arm-nbsd-tdep.c
> +++ b/gdb/arm-nbsd-tdep.c
> @@ -35,6 +37,70 @@ static const gdb_byte arm_nbsd_arm_be_breakpoint[] = {0xe6, 0x00, 0x00, 0x11};
>  static const gdb_byte arm_nbsd_thumb_le_breakpoint[] = {0xfe, 0xde};
>  static const gdb_byte arm_nbsd_thumb_be_breakpoint[] = {0xde, 0xfe};
>  
> +/* This matches struct reg from NetBSD's sys/arch/arm/include/reg.h */
> +struct arm_nbsd_reg {
> +  uint32_t reg[13];
> +  uint32_t sp;
> +  uint32_t lr;
> +  uint32_t pc;
> +  uint32_t cpsr;
> +};

I would encourage you to use a regcache_map instead.  It's a bit simpler to
work with, and if NetBSD copies the same structure out as part of the
sigcontext on the signal stack, you can reuse the regcache_map in your
signal frame unwinder.  I've updated the arm, aarch64, and risc-v FreeBSD
architectures to use this if you want to see a reference on how to use a
map, e.g. from arm-fbsd-tdep.c:

/* Register maps.  */

static const struct regcache_map_entry arm_fbsd_gregmap[] =
  {
    { 13, ARM_A1_REGNUM, 4 }, /* r0 ... r12 */
    { 1, ARM_SP_REGNUM, 4 },
    { 1, ARM_LR_REGNUM, 4 },
    { 1, ARM_PC_REGNUM, 4 },
    { 1, ARM_PS_REGNUM, 4 },
    { 0 }
  };

...

/* Register set definitions.  */

const struct regset arm_fbsd_gregset =
  {
    arm_fbsd_gregmap,
    regcache_supply_regset, regcache_collect_regset
  };

...

/* Implement the "iterate_over_regset_sections" gdbarch method.  */

static void
arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
				       iterate_over_regset_sections_cb *cb,
				       void *cb_data,
				       const struct regcache *regcache)
{
  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);

  cb (".reg", ARM_FBSD_SIZEOF_GREGSET, ARM_FBSD_SIZEOF_GREGSET,
      &arm_fbsd_gregset, NULL, cb_data);


The native target is also able to use the regcache_map, e.g. from
arm-fbsd-nat.c:

/* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
   for all registers.  */

void
arm_fbsd_nat_target::fetch_registers (struct regcache *regcache, int regnum)
{
  pid_t pid = get_ptrace_pid (regcache->ptid ());

  if (regnum == -1 || getregs_supplies (regnum))
    {
      struct reg regs;

      if (ptrace (PT_GETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
	perror_with_name (_("Couldn't get registers"));

      regcache->supply_regset (&arm_fbsd_gregset, regnum, &regs,
			       sizeof (regs));
    }

...

And for the signal frame unwinder in arm-fbsd-tdep.c, the single
call to trad_frame_set_reg_regmap() lets you pull in all the register
from the sigcontext in one go:

/* Implement the "init" method of struct tramp_frame.  */

static void
arm_fbsd_sigframe_init (const struct tramp_frame *self,
			struct frame_info *this_frame,
			struct trad_frame_cache *this_cache,
			CORE_ADDR func)
{
  struct gdbarch *gdbarch = get_frame_arch (this_frame);
  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
  CORE_ADDR sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
  CORE_ADDR mcontext_addr = (sp
			     + ARM_SIGFRAME_UCONTEXT_OFFSET
			     + ARM_UCONTEXT_MCONTEXT_OFFSET);
  ULONGEST mcontext_vfp_addr;

  trad_frame_set_reg_regmap (this_cache, arm_fbsd_gregmap, mcontext_addr,
			     regcache_map_entry_size (arm_fbsd_gregmap));
  

On Fri, Mar 6, 2020 at 4:41 PM John Baldwin <jhb@freebsd.org> wrote:
>
> On 3/6/20 12:59 PM, Christian Biesinger via gdb-patches wrote:
> > This is in preparation for deleting deprecated_add_core_fns and
> > related code.
> >
> > As a side-effect, this makes it possible to read NetBSD/ARM
> > core files on non-NetBSD/ARM platforms, subject to PR corefiles/25638.
> >
> > I have removed this comment:
> > -  /* This is ok: we're running native...  */
> > Since we are using the gdbarch from the regcache, we should be
> > guaranteed to be calling the right function here, so it shouldn't
> > matter whether we are running native.
> >
> > Tested by reading a NetBSD/ARM core file on Linux/x86-64 and NetBSD/ARM;
> > the "info registers" output matches the one from the system GDB.
> >
> > diff --git a/gdb/arm-nbsd-tdep.c b/gdb/arm-nbsd-tdep.c
> > index 2642024022..689c4b6219 100644
> > --- a/gdb/arm-nbsd-tdep.c
> > +++ b/gdb/arm-nbsd-tdep.c
> > @@ -35,6 +37,70 @@ static const gdb_byte arm_nbsd_arm_be_breakpoint[] = {0xe6, 0x00, 0x00, 0x11};
> >  static const gdb_byte arm_nbsd_thumb_le_breakpoint[] = {0xfe, 0xde};
> >  static const gdb_byte arm_nbsd_thumb_be_breakpoint[] = {0xde, 0xfe};
> >
> > +/* This matches struct reg from NetBSD's sys/arch/arm/include/reg.h */
> > +struct arm_nbsd_reg {
> > +  uint32_t reg[13];
> > +  uint32_t sp;
> > +  uint32_t lr;
> > +  uint32_t pc;
> > +  uint32_t cpsr;
> > +};
>
> I would encourage you to use a regcache_map instead.  It's a bit simpler to
> work with, and if NetBSD copies the same structure out as part of the
> sigcontext on the signal stack, you can reuse the regcache_map in your
> signal frame unwinder.  I've updated the arm, aarch64, and risc-v FreeBSD
> architectures to use this if you want to see a reference on how to use agdbarch_addr_bits_remove
> map, e.g. from arm-fbsd-tdep.c:

Thanks for the feedback; I thought about that but there are two places
in this function that make this harder -- there is an arm_apcs_32
check that affects what gets supplied for ARM_PS_REGNUM and there's a
call to gdbarch_addr_bits_remove on the PC, perhaps for the thumb bit.
So I think I cannot convert it while keeping the same semantics?

The fetch_registers implementation does share this code with my patch.

Any thoughts?

Christian

>
> /* Register maps.  */
>
> static const struct regcache_map_entry arm_fbsd_gregmap[] =
>   {
>     { 13, ARM_A1_REGNUM, 4 }, /* r0 ... r12 */
>     { 1, ARM_SP_REGNUM, 4 },
>     { 1, ARM_LR_REGNUM, 4 },
>     { 1, ARM_PC_REGNUM, 4 },
>     { 1, ARM_PS_REGNUM, 4 },
>     { 0 }
>   };
>
> ...
>
> /* Register set definitions.  */
>
> const struct regset arm_fbsd_gregset =
>   {
>     arm_fbsd_gregmap,
>     regcache_supply_regset, regcache_collect_regset
>   };
>
> ...
>
> /* Implement the "iterate_over_regset_sections" gdbarch method.  */
>
> static void
> arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
>                                        iterate_over_regset_sections_cb *cb,
>                                        void *cb_data,
>                                        const struct regcache *regcache)
> {
>   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
>
>   cb (".reg", ARM_FBSD_SIZEOF_GREGSET, ARM_FBSD_SIZEOF_GREGSET,
>       &arm_fbsd_gregset, NULL, cb_data);
>
>
> The native target is also able to use the regcache_map, e.g. from
> arm-fbsd-nat.c:
>
> /* Fetch register REGNUM from the inferior.  If REGNUM is -1, do this
>    for all registers.  */
>
> void
> arm_fbsd_nat_target::fetch_registers (struct regcache *regcache, int regnum)
> {
>   pid_t pid = get_ptrace_pid (regcache->ptid ());
>
>   if (regnum == -1 || getregs_supplies (regnum))
>     {
>       struct reg regs;
>
>       if (ptrace (PT_GETREGS, pid, (PTRACE_TYPE_ARG3) &regs, 0) == -1)
>         perror_with_name (_("Couldn't get registers"));
>
>       regcache->supply_regset (&arm_fbsd_gregset, regnum, &regs,
>                                sizeof (regs));
>     }
>
> ...
>
> And for the signal frame unwinder in arm-fbsd-tdep.c, the single
> call to trad_frame_set_reg_regmap() lets you pull in all the register
> from the sigcontext in one go:
>
> /* Implement the "init" method of struct tramp_frame.  */
>
> static void
> arm_fbsd_sigframe_init (const struct tramp_frame *self,
>                         struct frame_info *this_frame,
>                         struct trad_frame_cache *this_cache,
>                         CORE_ADDR func)
> {
>   struct gdbarch *gdbarch = get_frame_arch (this_frame);
>   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
>   CORE_ADDR sp = get_frame_register_unsigned (this_frame, ARM_SP_REGNUM);
>   CORE_ADDR mcontext_addr = (sp
>                              + ARM_SIGFRAME_UCONTEXT_OFFSET
>                              + ARM_UCONTEXT_MCONTEXT_OFFSET);
>   ULONGEST mcontext_vfp_addr;
>
>   trad_frame_set_reg_regmap (this_cache, arm_fbsd_gregmap, mcontext_addr,
>                              regcache_map_entry_size (arm_fbsd_gregmap));
>
>
> --
> John Baldwin