Re: [PATCH v2] Fix arm-netbsd build error: convert from FPA to VFP

[Alan Hayward <Alan.Hayward@arm.com>]

> On 28 Feb 2020, at 13:18, Christian Biesinger via gdb-patches <gdb-patches@sourceware.org> wrote:
> 
> [Fixes the issue pointed out by Alan based on code by Kamil.
> This took way longer than it should have due to an unrelated regression,
> https://sourceware.org/ml/gdb-patches/2020-02/msg01038.html]
> 
> The floating point register interface has changed to this:
> https://github.com/NetBSD/src/blob/trunk/sys/arch/arm/include/reg.h
> 
> It now uses VFP instead of FPA registers. This patch updates
> arm-nbsd-nat.c accordingly.
> 
> Also implements read_description so that these registers are correctly
> printed by "info registers" et al.
> 
> Tested by compiling & running on arm-netbsd on qemu.
> 
> gdb/ChangeLog:
> 
> 2020-02-11  Christian Biesinger  <cbiesinger@google.com>
> 
> 	* arm-nbsd-nat.c (arm_supply_fparegset): Rename to...
> 	(arm_supply_vfpregset): ...this, and update to use VFP registers.
> 	(fetch_fp_register): Update.
> 	(fetch_fp_regs): Update.
> 	(store_fp_register): Update.
> 	(store_fp_regs): Update.
> 	(arm_netbsd_nat_target::read_description): New function.
> 	(fetch_elfcore_registers): Update.
> ---
> gdb/arm-nbsd-nat.c | 112 +++++++++++++++++++++++----------------------
> 1 file changed, 58 insertions(+), 54 deletions(-)
> 
> diff --git a/gdb/arm-nbsd-nat.c b/gdb/arm-nbsd-nat.c
> index 11afc289c3..2e7d840adf 100644
> --- a/gdb/arm-nbsd-nat.c
> +++ b/gdb/arm-nbsd-nat.c
> @@ -26,10 +26,12 @@
> #include "target.h"
> #include <sys/types.h>
> #include <sys/ptrace.h>
> +#include <sys/sysctl.h>
> #include <machine/reg.h>
> #include <machine/frame.h>
> 
> #include "arm-tdep.h"
> +#include "aarch32-tdep.h"
> #include "inf-ptrace.h"
> 
> class arm_netbsd_nat_target final : public inf_ptrace_target
> @@ -38,6 +40,7 @@ public:
>   /* Add our register access methods.  */
>   void fetch_registers (struct regcache *, int) override;
>   void store_registers (struct regcache *, int) override;
> +  const struct target_desc *read_description () override;
> };
> 
> static arm_netbsd_nat_target the_arm_netbsd_nat_target;
> @@ -65,15 +68,13 @@ arm_supply_gregset (struct regcache *regcache, struct reg *gregset)
> }
> 
> static void
> -arm_supply_fparegset (struct regcache *regcache, struct fpreg *fparegset)
> +arm_supply_vfpregset (struct regcache *regcache, struct fpreg *fpregset)
> {
> -  int regno;
> -
> -  for (regno = ARM_F0_REGNUM; regno <= ARM_F7_REGNUM; regno++)
> -    regcache->raw_supply (regno,
> -			  (char *) &fparegset->fpr[regno - ARM_F0_REGNUM]);
> +  struct vfpreg &vfp = fpregset->fpr_vfp;
> +  for (int regno = ARM_D0_REGNUM; regno <= ARM_D31_REGNUM; regno++)
> +    regcache->raw_supply (regno, (char *) &vfp.vfp_regs[regno - ARM_D0_REGNUM]);
> 
> -  regcache->raw_supply (ARM_FPS_REGNUM, (char *) &fparegset->fpr_fpsr);
> +  regcache->raw_supply (ARM_FPSCR_REGNUM, (char *) &vfp.vfp_fpscr);
> }
> 
> static void
> @@ -147,10 +148,10 @@ static void
> fetch_fp_register (struct regcache *regcache, int regno)
> {
>   struct fpreg inferior_fp_registers;
> -  int ret;
> +  int ret = ptrace (PT_GETFPREGS, regcache->ptid ().pid (),
> +		    (PTRACE_TYPE_ARG3) &inferior_fp_registers, 0);
> 
> -  ret = ptrace (PT_GETFPREGS, regcache->ptid ().pid (),
> -		(PTRACE_TYPE_ARG3) &inferior_fp_registers, 0);
> +  struct vfpreg &vfp = inferior_fp_registers.fpr_vfp;
> 
>   if (ret < 0)
>     {
> @@ -158,18 +159,15 @@ fetch_fp_register (struct regcache *regcache, int regno)
>       return;
>     }
> 
> -  switch (regno)
> +  if (regno == ARM_FPSCR_REGNUM)
> +    regcache->raw_supply (ARM_FPSCR_REGNUM, (char *) &vfp.vfp_fpscr);
> +  else if (regno >= ARM_D0_REGNUM && regno <= ARM_D31_REGNUM)
>     {
> -    case ARM_FPS_REGNUM:
> -      regcache->raw_supply (ARM_FPS_REGNUM,
> -			    (char *) &inferior_fp_registers.fpr_fpsr);
> -      break;
> -
> -    default:
> -      regcache->raw_supply
> -	(regno, (char *) &inferior_fp_registers.fpr[regno - ARM_F0_REGNUM]);
> -      break;
> +      regcache->raw_supply (regno,
> +			    (char *) &vfp.vfp_regs[regno - ARM_D0_REGNUM]);
>     }
> +  else
> +    warning (_("Invalid register number."));
> }
> 
> static void
> @@ -188,7 +186,7 @@ fetch_fp_regs (struct regcache *regcache)
>       return;
>     }
> 
> -  arm_supply_fparegset (regcache, &inferior_fp_registers);
> +  arm_supply_vfpregset (regcache, &inferior_fp_registers);
> }
> 
> void
> @@ -327,10 +325,9 @@ static void
> store_fp_register (const struct regcache *regcache, int regno)
> {
>   struct fpreg inferior_fp_registers;
> -  int ret;
> -
> -  ret = ptrace (PT_GETFPREGS, regcache->ptid ().pid (),
> -		(PTRACE_TYPE_ARG3) &inferior_fp_registers, 0);
> +  int ret = ptrace (PT_GETFPREGS, regcache->ptid ().pid (),
> +		    (PTRACE_TYPE_ARG3) &inferior_fp_registers, 0);
> +  struct vfpreg &vfp = inferior_fp_registers.fpr_vfp;
> 
>   if (ret < 0)
>     {
> @@ -338,18 +335,15 @@ store_fp_register (const struct regcache *regcache, int regno)
>       return;
>     }
> 
> -  switch (regno)
> +  if (regno == ARM_FPSCR_REGNUM)
> +    regcache->raw_collect (ARM_FPSCR_REGNUM, (char *) &vfp.vfp_fpscr);
> +  else if (regno >= ARM_D0_REGNUM && regno <= ARM_D31_REGNUM)
>     {
> -    case ARM_FPS_REGNUM:
> -      regcache->raw_collect (ARM_FPS_REGNUM,
> -			     (char *) &inferior_fp_registers.fpr_fpsr);
> -      break;
> -
> -    default:
> -      regcache->raw_collect
> -	(regno, (char *) &inferior_fp_registers.fpr[regno - ARM_F0_REGNUM]);
> -      break;
> +      regcache->raw_collect (regno,
> +			     (char *) &vfp.vfp_regs[regno - ARM_D0_REGNUM]);
>     }
> +  else
> +    warning (_("Invalid register number."));
> 
>   ret = ptrace (PT_SETFPREGS, regcache->ptid ().pid (),
> 		(PTRACE_TYPE_ARG3) &inferior_fp_registers, 0);
> @@ -361,20 +355,17 @@ store_fp_register (const struct regcache *regcache, int regno)
> static void
> store_fp_regs (const struct regcache *regcache)
> {
> -  struct fpreg inferior_fp_registers;
> -  int ret;
> -  int regno;
> +  struct fpreg fpregs;
> 
> -
> -  for (regno = ARM_F0_REGNUM; regno <= ARM_F7_REGNUM; regno++)
> +  for (int regno = ARM_D0_REGNUM; regno <= ARM_D31_REGNUM; regno++)
>     regcache->raw_collect
> -      (regno, (char *) &inferior_fp_registers.fpr[regno - ARM_F0_REGNUM]);
> +      (regno, (char *) &fpregs.fpr_vfp.vfp_regs[regno - ARM_D0_REGNUM]);
> 
> -  regcache->raw_collect (ARM_FPS_REGNUM,
> -			 (char *) &inferior_fp_registers.fpr_fpsr);
> +  regcache->raw_collect (ARM_FPSCR_REGNUM,
> +			 (char *) &fpregs.fpr_vfp.vfp_fpscr);
> 
> -  ret = ptrace (PT_SETFPREGS, regcache->ptid ().pid (),
> -		(PTRACE_TYPE_ARG3) &inferior_fp_registers, 0);
> +  int ret = ptrace (PT_SETFPREGS, regcache->ptid ().pid (),
> +		    (PTRACE_TYPE_ARG3) &fpregs, 0);
> 

This looks fine if you have a tdesc of type ARM_FP_TYPE_VFPV3.
What happens when you create a tdesc of type ARM_FP_TYPE_NONE or AArch32?

The fetch/supply functions need to check what type the description is,
otherwise you’ll overflow the regcache.

Check for AArch32 with:
if (gdbarch_bfd_arch_info (gdbarch)->bits_per_word == 32)

And you can probably count the number or size of registers to confirm if
it’s ARM_FP_TYPE_VFPV3 or ARM_FP_TYPE_NONE.

Alternatively, I think you could stash the arm_fp_type in the tdep structure
and then grab hold of it.

(Everything else looks ok)


Alan.

>   if (ret < 0)
>     warning (_("unable to store floating-point registers"));
> @@ -397,6 +388,23 @@ arm_netbsd_nat_target::store_registers (struct regcache *regcache, int regno)
>     }
> }
> 
> +const struct target_desc *
> +arm_netbsd_nat_target::read_description ()
> +{
> +  int flag;
> +  size_t len = sizeof (flag);
> +
> +  if (sysctlbyname("machdep.fpu_present", &flag, &len, NULL, 0) != 0
> +      || !flag)
> +    return arm_read_description (ARM_FP_TYPE_NONE);
> +
> +  len = sizeof(flag);
> +  if (sysctlbyname("machdep.neon_present", &flag, &len, NULL, 0) == 0 && flag)
> +    return aarch32_read_description ();
> +
> +  return arm_read_description (ARM_FP_TYPE_VFPV3);
> +}
> +
> static void
> fetch_elfcore_registers (struct regcache *regcache,
> 			 gdb_byte *core_reg_sect, unsigned core_reg_size,
> @@ -420,15 +428,11 @@ fetch_elfcore_registers (struct regcache *regcache,
>       break;
> 
>     case 2:
> -      if (core_reg_size != sizeof (struct fpreg))
> -	warning (_("wrong size of FPA register set in core file"));
> -      else
> -	{
> -	  /* The memcpy may be unnecessary, but we can't really be sure
> -	     of the alignment of the data in the core file.  */
> -	  memcpy (&fparegset, core_reg_sect, sizeof (fparegset));
> -	  arm_supply_fparegset (regcache, &fparegset);
> -	}
> +      /* cbiesinger/2020-02-12 -- as far as I can tell, ARM/NetBSD does
> +         not write any floating point registers into the core file (tested
> +	 with NetBSD 9.1_RC1).  When it does, this block will need to read them,
> +	 and the arm-netbsd gdbarch will need a core_read_description function
> +	 to return the right description for them.  */
>       break;
> 
>     default:
> -- 
> 2.25.1.481.gfbce0eb801-goog
>