Re: [RFC PATCH 1/5] gdb/aarch64: Enable FPMR for AArch64 in gdb on Linux

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

<Ezra.Sitorus@arm.com> writes:

> From: Ezra Sitorus <ezra.sitorus@arm.com>
>
> The Floating Point Mode Register controls the behaviours of FP8
> instructions. This patch add FPMR to GDB if it is enabled on the
> target.
>
> Manually validated with Shrinkwrap (Arm FVP).
>
> For this patch:
> * Are the FPMR fields useful? What to do if the format is not valid
>   (0x2)? What about the OSC/OSM fields with Inf/Nan, are these helpful
>   too?

I think they are useful. It's always good to get a "decoded" value from
this kind of register.

For invalid values, I think (though not sure) that GDB will just display
the "raw" value of the field since no enum value will match...

> ---
>  gdb/aarch64-linux-nat.c       | 58 ++++++++++++++++++++++++++++++++++-
>  gdb/aarch64-linux-tdep.c      |  2 +-
>  gdb/aarch64-tdep.c            | 18 +++++++++--
>  gdb/aarch64-tdep.h            |  7 +++++
>  gdb/arch/aarch64.c            |  4 +++
>  gdb/arch/aarch64.h            |  4 ++-
>  gdb/features/Makefile         |  1 +
>  gdb/features/aarch64-fpmr.c   | 44 ++++++++++++++++++++++++++
>  gdb/features/aarch64-fpmr.xml | 55 +++++++++++++++++++++++++++++++++
>  9 files changed, 188 insertions(+), 5 deletions(-)
>  create mode 100644 gdb/features/aarch64-fpmr.c
>  create mode 100644 gdb/features/aarch64-fpmr.xml
>
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index 89ecedda57d..b524aa352be 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -604,6 +604,48 @@ store_gcsregs_to_thread (regcache *regcache)
>      perror_with_name (_("Unable to store GCS registers"));
>  }
>  
> +/* Fill GDB's REGCACHE with the FPMR register content from the
> +   thread associated with REGCACHE.  If there is no active FPMR register state,
> +   make the FPMR register contents zero.  */
> +
> +static void
> +fetch_fpmr_from_thread (struct regcache *regcache)
> +{
> +  aarch64_gdbarch_tdep *tdep
> +    = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
> +
> +    int tid = regcache->ptid ().lwp ();
> +
> +    struct iovec iov;
> +    uint64_t val;
> +    iov.iov_base = &val;
> +    iov.iov_len = sizeof(val);

Missing space between function name and parenthesis.

> +
> +    if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_FPMR, &iov) < 0)
> +      perror_with_name (_("Unable to fetch FPMR"));
> +    regcache->raw_supply (tdep->fpmr_regnum, &val);
> +}
> +
> +/* Store the NT_ARM_FPMR register set contents from GDB's REGCACHE to the
> +    thread associated with REGCACHE.  */
> +static void
> +store_fpmr_to_thread (struct regcache *regcache)
> +{
> +  aarch64_gdbarch_tdep *tdep
> +    = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
> +
> +  int tid = regcache->ptid ().lwp ();
> +
> +  struct iovec iov;
> +  uint64_t val;
> +  iov.iov_base = &val;
> +  iov.iov_len = sizeof(val);

Missing space between function name and parenthesis.

> +
> +  regcache->raw_collect (tdep->fpmr_regnum, (char *) &val);
> +  if (ptrace (PTRACE_SETREGSET, tid, NT_ARM_FPMR, &iov) < 0)
> +    perror_with_name (_("Unable to store FPMR"));
> +}
> +
>  /* The AArch64 version of the "fetch_registers" target_ops method.  Fetch
>     REGNO from the target and place the result into REGCACHE.  */
>  
> @@ -642,6 +684,9 @@ aarch64_fetch_registers (struct regcache *regcache, int regno)
>  
>        if (tdep->has_gcs_linux ())
>  	fetch_gcsregs_from_thread (regcache);
> +
> +      if (tdep->has_fpmr ())
> +	fetch_fpmr_from_thread (regcache);
>      }
>    /* General purpose register?  */
>    else if (regno < AARCH64_V0_REGNUM)
> @@ -679,6 +724,9 @@ aarch64_fetch_registers (struct regcache *regcache, int regno)
>  	   && (regno == tdep->gcs_reg_base || regno == tdep->gcs_linux_reg_base
>  	       || regno == tdep->gcs_linux_reg_base + 1))
>      fetch_gcsregs_from_thread (regcache);
> +  /* FP8/FPMR? */
> +  else if (tdep->has_fpmr () && (regno == tdep->fpmr_regnum))
> +    fetch_fpmr_from_thread (regcache);
>  }
>  
>  /* A version of the "fetch_registers" target_ops method used when running
> @@ -753,6 +801,9 @@ aarch64_store_registers (struct regcache *regcache, int regno)
>  
>        if (tdep->has_gcs_linux ())
>  	store_gcsregs_to_thread (regcache);
> +
> +      if (tdep->has_fpmr ())
> +	store_fpmr_to_thread (regcache);
>      }
>    /* General purpose register?  */
>    else if (regno < AARCH64_V0_REGNUM)
> @@ -784,7 +835,9 @@ aarch64_store_registers (struct regcache *regcache, int regno)
>  	   && (regno == tdep->gcs_reg_base || regno == tdep->gcs_linux_reg_base
>  	       || regno == tdep->gcs_linux_reg_base + 1))
>      store_gcsregs_to_thread (regcache);
> -
> +  /* FPMR?  */
> +  else if (tdep->has_fpmr() && regno == tdep->fpmr_regnum)
> +    store_fpmr_to_thread (regcache);
>    /* PAuth registers are read-only.  */
>  }
>  
> @@ -969,6 +1022,9 @@ aarch64_linux_nat_target::read_description ()
>    if ((hwcap2 & HWCAP2_SME2) || (hwcap2 & HWCAP2_SME2P1))
>      features.sme2 = supports_zt_registers (tid);
>  
> +  /* Check for FP8/FPMR. */

Two spaces after the period.

> +  features.fpmr = hwcap2 & (1UL << 48);
> +

Both Linux and glibc define HWCAP2_FPMR so it's better to use it here
and in the two other places were you use this value (one of them is in
another patch).

You can provide a definition in a header file for the case where the
system headers aren't new enough to have it, as is done for HWCAP_GCS
and HWCAP_MTE. With the difference that in both of those cases there's a
separate header specific for that hardware feature. For FPMR, I'd put
the definition in gdb/arch/aarch64.h.

>    return aarch64_read_description (features);
>  }
>  
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 76bde85188b..acb650db514 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -1713,7 +1713,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>    features.pauth = hwcap & AARCH64_HWCAP_PACA;
>    features.gcs = features.gcs_linux = hwcap & HWCAP_GCS;
>    features.mte = hwcap2 & HWCAP2_MTE;
> -

It's subjective, but I think the code looks better with this blank line
separating this setup of the features struct and the "Handle the TLS
section" part below.

> +  features.fpmr = hwcap2 & (1UL << 48);

Here too, you can use HWCAP2_FPMR.

>    /* Handle the TLS section.  */
>    asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>    if (tls != nullptr)
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index 500ac77d75a..33bef91c44e 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -4141,6 +4141,10 @@ aarch64_features_from_target_desc (const struct target_desc *tdesc)
>  			!= nullptr);
>  
>    return features;
> +
> +  /* Check for FPMR feature.  */
> +  features.fpmr = (tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpmr")
> +		   != nullptr);

This needs to be above the return.

>  }
>  
>  /* Implement the "cannot_store_register" gdbarch method.  */
> @@ -4448,11 +4452,12 @@ static struct gdbarch *
>  aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  {
>    const struct tdesc_feature *feature_core, *feature_fpu, *feature_sve;
> -  const struct tdesc_feature *feature_pauth;
> +  const struct tdesc_feature *feature_pauth, *feature_fpmr;
>    bool valid_p = true;
>    int i, num_regs = 0, num_pseudo_regs = 0;
>    int first_pauth_regnum = -1, ra_sign_state_offset = -1;
>    int first_mte_regnum = -1, first_tls_regnum = -1;
> +  int fpmr_regnum = -1;
>    uint64_t vq = aarch64_get_tdesc_vq (info.target_desc);
>    uint64_t svq = aarch64_get_tdesc_svq (info.target_desc);
>  
> @@ -4550,6 +4555,14 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>        num_pseudo_regs += 32;	/* add the Bn scalar register pseudos */
>      }
>  
> +  feature_fpmr = tdesc_find_feature (tdesc, "org.gnu.gdb.aarch64.fpmr");
> +  if (feature_fpmr != nullptr)
> +  {
> +    fpmr_regnum = num_regs++;
> +    valid_p &= tdesc_numbered_register (feature_fpmr, tdesc_data.get (),
> +					fpmr_regnum, "fpmr");
> +  }
> +

The curly braces need to be indented 2 spaces forward.

>    int first_sme_regnum = -1;
>    int first_sme2_regnum = -1;
>    int first_sme_pseudo_regnum = -1;
> @@ -4760,7 +4773,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>  
>    /* Set the SME2 register set details.  */
>    tdep->sme2_zt0_regnum = first_sme2_regnum;
> -
> +  /* Set the FPMR regnum.  */
> +  tdep->fpmr_regnum = fpmr_regnum;
>    set_gdbarch_push_dummy_call (gdbarch, aarch64_push_dummy_call);
>    set_gdbarch_frame_align (gdbarch, aarch64_frame_align);
>  
> diff --git a/gdb/aarch64-tdep.h b/gdb/aarch64-tdep.h
> index 99e7d26ce4a..403743516df 100644
> --- a/gdb/aarch64-tdep.h
> +++ b/gdb/aarch64-tdep.h
> @@ -207,6 +207,13 @@ struct aarch64_gdbarch_tdep : gdbarch_tdep_base
>    {
>      return gcs_linux_reg_base != -1;
>    }

Missing blank line here.

> +  /* Index of FPMR. This is -1 if FPMR/FP8 is not supported. */

Missing two spaces after the two periods.

> +  int fpmr_regnum = -1;
> +
> +  bool has_fpmr () const

bool needs to be on its own line.

> +  {
> +    return fpmr_regnum != -1;
> +  }
>  };

-- 
Thiago