Re: [PATCH 3/4] Add FreeBSD/arm architecture.

[Pedro Alves <palves at redhat dot com>]

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