This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH 3/4] Add FreeBSD/arm architecture.
- From: Pedro Alves <palves at redhat dot com>
- To: John Baldwin <jhb at FreeBSD dot org>, gdb-patches at sourceware dot org, binutils at sourceware dot org
- Date: Wed, 11 Oct 2017 12:05:35 +0100
- Subject: Re: [PATCH 3/4] Add FreeBSD/arm architecture.
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=palves at redhat dot com
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AFB17356E2
- References: <20170914151831.65362-1-jhb@FreeBSD.org> <20170914151831.65362-4-jhb@FreeBSD.org>
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.
> +
> +/* 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);
> + 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++)
> + {
> + 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).
Thanks,
Pedro Alves