This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 3/4] Add FreeBSD/arm architecture.
- From: John Baldwin <jhb at freebsd dot org>
- To: Pedro Alves <palves at redhat dot com>
- Cc: gdb-patches at sourceware dot org, binutils at sourceware dot org
- Date: Wed, 11 Oct 2017 09:53:15 -0700
- Subject: Re: [PATCH 3/4] Add FreeBSD/arm architecture.
- Authentication-results: sourceware.org; auth=none
- References: <20170914151831.65362-1-jhb@FreeBSD.org> <20170914151831.65362-4-jhb@FreeBSD.org> <2ffe1b55-809c-a0d8-c5c8-92ffba36998b@redhat.com>
On Wednesday, October 11, 2017 12:05:35 PM Pedro Alves wrote:
> On 09/14/2017 04:18 PM, John Baldwin wrote:
> > Support for collecting and supplying general purpose and floating
> > point registers is provided along with signal frame unwinding. While
> > FreeBSD/arm kernels do populate NT_FPREGSET notes, they are always
> > zero-filled, so this implementation ignores them. Recent FreeBSD/arm
> > kernels generate NT_ARM_VFP notes which are used to supply
> > floating-point registers. As with Linux, the AT_HWCAP feature flags
> > are used to determine the correct target description.
> >
>
> Hi John. FWIW, this looks good to me. I'm comfortable with
> you self-approving this as FreeBSD maintainer after a
> period, BTW. A few minor nits below.
>
> > diff --git a/gdb/NEWS b/gdb/NEWS
> > index 2e6d48c016..f33b7ac49f 100644
> > --- a/gdb/NEWS
> > +++ b/gdb/NEWS
> > @@ -105,6 +105,7 @@ FreeBSD/aarch64 aarch64*-*-freebsd*
> > * New targets
> >
> > FreeBSD/aarch64 aarch64*-*-freebsd*
> > +FreeBSD/arm arm*-*-freebsd*
>
> It'd be nice to update the hosts table at:
> https://sourceware.org/gdb/wiki/Systems
>
> (I've added a notes column now, thinking that we'd start
> saying something like "since GDB 8.1". We could rename
> the column too.)
>
> > +#define ARM_MCONTEXT_REG_SIZE 4
> > +#define ARM_MCONTEXT_VFP_REG_SIZE 8
> > +#define ARM_SIGFRAME_UCONTEXT_OFFSET 64
> > +#define ARM_UCONTEXT_MCONTEXT_OFFSET 16
> > +#define ARM_MCONTEXT_VFP_PTR_OFFSET 72
>
> Space vs tab after #define in the last line above.
Fixed.
> > +
> > +/* 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;
>
> = goes on next line. Then per GNU standards (because that's
> what Emacs likes), to line up the multiple lines, wrap in parens:
>
> CORE_ADDR mcontext_addr
> = (sp
> + ARM_SIGFRAME_UCONTEXT_OFFSET
> + ARM_UCONTEXT_MCONTEXT_OFFSET);
>
> Thought this would fit too, and is shorter:
>
> CORE_ADDR mcontext_addr = (sp
> + ARM_SIGFRAME_UCONTEXT_OFFSET
> + ARM_UCONTEXT_MCONTEXT_OFFSET);
Ok, the second looks better to me.
> > + CORE_ADDR mcontext_vfp_addr;
> > + gdb_byte buf[4];
> > + int i;
> > +
> > + for (i = 0; i < 16; i++)
>
> Suggest writing:
>
> for (int i = 0; i < 16; i++)
Done.
> > + {
> > + trad_frame_set_reg_addr (this_cache,
> > + ARM_A1_REGNUM + i,
> > + mcontext_addr + i * ARM_MCONTEXT_REG_SIZE);
> > + }
> > + trad_frame_set_reg_addr (this_cache, ARM_PS_REGNUM,
> > + mcontext_addr + 16 * ARM_MCONTEXT_REG_SIZE);
> > +
> > + mcontext_vfp_addr = 0;
> > + if (target_read_memory (mcontext_addr + ARM_MCONTEXT_VFP_PTR_OFFSET, buf,
> > + 4) == 0)
> > + mcontext_vfp_addr = extract_unsigned_integer (buf, 4, byte_order);
>
> I mildly wonder whether this be:
>
> if (safe_read_memory_unsigned_integer (mcontext_addr + ARM_MCONTEXT_VFP_PTR_OFFSET, 4,
> byte_order, &mcontext_vfp_addr)
> {
> for (i = 0; i < 32; i++)
> ....
>
> I'd convey intention and avoid the "= 0" initialization + "!= 0" check
> (unless you need it anyway).
I do need the != 0 check, but safe_read_memory_unsigned_integer does indeed
look nicer.
BTW, while working on the arm and aarch64 FreeBSD backends I've really liked
having the arrays of 'struct regcache_map_entry' objects. I think I'd like
to add a similar type of 'map' array for work with trad_frame used for
signal frames as then one can just define a structure to describe the layout
and perhaps give a starting CORE_ADDR and the 'map' array as args to a
trad_frame function that would handle calling trad_frame_set_reg_addr().
--
John Baldwin