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

[Christian Biesinger]

Ping -- Kamil, any thoughts on the below patch?

On Fri, Mar 6, 2020 at 3:12 PM Simon Marchi <simark@simark.ca> wrote:
>
> 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
>