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

["Christian Biesinger via gdb-patches" <gdb-patches at sourceware dot org>]

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