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

[Luis <luis.machado.foss@gmail.com>]

Thanks for the patch. A few comment below.


On 05/09/2025 14:17, Ezra.Sitorus@arm.com wrote:
> 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 that's an implementation choice. Is it of any value to users 
using the debugger to have access to these fields in a clearer manner? 
If not, then it doesn´t seem very useful.

Is gdb allowed to force-change fpmr bits to switch the format being 
used? Is that useful?

> ---
>   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

s/FPMR register content/FPMR register set content

> +   thread associated with REGCACHE.  If there is no active FPMR register state,
> +   make the FPMR register contents zero.  */

The comment states not having an active FPMR register state. Is that how 
this is implemented? Or is it the usual situation where you either have 
kernel support or you don't. If you do, then you get whatever contents 
the kernel provides, and gdb doesn´t need to bother with it.

The code doesn´t seem to set anything to 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);
> +
> +    if (ptrace (PTRACE_GETREGSET, tid, NT_ARM_FPMR, &iov) < 0)
> +      perror_with_name (_("Unable to fetch FPMR"));

Maybe "Unable to fetch FPMR register set"

> +    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

New line between comment and function declaration.

> +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);
> +
> +  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"));

"Unable to store FPMR register set"

> +}
> +
>   /* 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? */

For consistency maybe use FPMR only? Is FP8 important in this context?

> +  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);
> -

Unrelated removal of blank line.

> +  /* FPMR?  */
> +  else if (tdep->has_fpmr() && regno == tdep->fpmr_regnum)

Formatting: space between function name and parenthesis.

> +    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. */

Use only FPMR?

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

Based on previous issues, I think we need to use 1ULL here due to 32-bit 
arm.

>     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;
> -
> +  features.fpmr = hwcap2 & (1UL << 48);

Same here, 1ULL.

>     /* 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);
>   }
>   
>   /* 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");
> +  }
> +
>     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;
> -

Unrelated removal of blank line.

> +  /* 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;
>     }
> +  /* Index of FPMR. This is -1 if FPMR/FP8 is not supported. */

Keeping the consistency with how the other sets were documented, I´d say:

"First FPMR register.  This is -1 if FPMR is not available."

> +  int fpmr_regnum = -1;
> +
> +  bool has_fpmr () const
> +  {
> +    return fpmr_regnum != -1;
> +  }
>   };
>   
>   const target_desc *aarch64_read_description (const aarch64_features &features);
> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
> index dff2bc16003..622138f43b5 100644
> --- a/gdb/arch/aarch64.c
> +++ b/gdb/arch/aarch64.c
> @@ -20,6 +20,7 @@
>   
>   #include "../features/aarch64-core.c"
>   #include "../features/aarch64-fpu.c"
> +#include "../features/aarch64-fpmr.c"
>   #include "../features/aarch64-sve.c"
>   #include "../features/aarch64-pauth.c"
>   #include "../features/aarch64-mte.c"
> @@ -73,6 +74,9 @@ aarch64_create_target_description (const aarch64_features &features)
>     if (features.gcs_linux)
>       regnum = create_feature_aarch64_gcs_linux (tdesc.get (), regnum);
>   
> +  if (features.fpmr)
> +    regnum = create_feature_aarch64_fpmr (tdesc.get (), regnum);
> +
>     return tdesc.release ();
>   }
>   
> diff --git a/gdb/arch/aarch64.h b/gdb/arch/aarch64.h
> index 679d845df74..731d4680248 100644
> --- a/gdb/arch/aarch64.h
> +++ b/gdb/arch/aarch64.h
> @@ -34,6 +34,7 @@ struct aarch64_features
>     uint64_t vq = 0;
>     bool pauth = false;
>     bool mte = false;
> +  bool fpmr = false;
>   
>     /* A positive TLS value indicates the number of TLS registers available.  */
>     uint8_t tls = 0;
> @@ -68,7 +69,8 @@ inline bool operator==(const aarch64_features &lhs, const aarch64_features &rhs)
>       && lhs.svq == rhs.svq
>       && lhs.sme2 == rhs.sme2
>       && lhs.gcs == rhs.gcs
> -    && lhs.gcs_linux == rhs.gcs_linux;
> +    && lhs.gcs_linux == rhs.gcs_linux
> +    && lhs.fpmr == rhs.fpmr;
>   }

Do you also need to update the operator() to account for fpmr? It 
computes the hash of the feature based on what features are present.

Unrelated, but looking at the code, it may be the case we missed adding 
gcs and gcs_linux to this operator.

>   
>   namespace std
> diff --git a/gdb/features/Makefile b/gdb/features/Makefile
> index d17c349b6cf..ed1b8bf119c 100644
> --- a/gdb/features/Makefile
> +++ b/gdb/features/Makefile
> @@ -201,6 +201,7 @@ $(outdir)/%.dat: %.xml number-regs.xsl sort-regs.xsl gdbserver-regs.xsl
>   # For targets with feature based target descriptions,
>   # the set of xml files we'll generate .c files for GDB from.
>   FEATURE_XMLFILES = aarch64-core.xml \
> +	aarch64-fpmr.xml \
>   	aarch64-fpu.xml \
>   	aarch64-pauth.xml \
>   	aarch64-mte.xml \
> diff --git a/gdb/features/aarch64-fpmr.c b/gdb/features/aarch64-fpmr.c
> new file mode 100644
> index 00000000000..a372b12530b
> --- /dev/null
> +++ b/gdb/features/aarch64-fpmr.c
> @@ -0,0 +1,44 @@
> +/* THIS FILE IS GENERATED.  -*- buffer-read-only: t -*- vi:set ro:
> +  Original: aarch64-fpmr.xml */
> +
> +#include "gdbsupport/tdesc.h"
> +
> +static int
> +create_feature_aarch64_fpmr (struct target_desc *result, long regnum)
> +{
> +  struct tdesc_feature *feature;
> +
> +  feature = tdesc_create_feature (result, "org.gnu.gdb.aarch64.fpmr");
> +  tdesc_type_with_fields *type_with_fields;
> +  type_with_fields = tdesc_create_enum (feature, "fp8_fmt", 3);
> +  tdesc_add_enum_value (type_with_fields, 0, "E5M2");
> +  tdesc_add_enum_value (type_with_fields, 1, "E4M3");
> +
> +  type_with_fields = tdesc_create_enum (feature, "osc", 1);
> +  tdesc_add_enum_value (type_with_fields, 0, "Inf/NaN");
> +  tdesc_add_enum_value (type_with_fields, 1, "MaxNormal");
> +
> +  type_with_fields = tdesc_create_enum (feature, "osm", 1);
> +  tdesc_add_enum_value (type_with_fields, 0, "Inf");
> +  tdesc_add_enum_value (type_with_fields, 1, "MaxNormal");
> +
> +  type_with_fields = tdesc_create_flags (feature, "fpmr_flags", 8);
> +  tdesc_type *field_type;
> +  field_type = tdesc_named_type (feature, "fp8_fmt");
> +  tdesc_add_typed_bitfield (type_with_fields, "F8S1", 0, 2, field_type);
> +  field_type = tdesc_named_type (feature, "fp8_fmt");
> +  tdesc_add_typed_bitfield (type_with_fields, "F8S2", 3, 5, field_type);
> +  field_type = tdesc_named_type (feature, "fp8_fmt");
> +  tdesc_add_typed_bitfield (type_with_fields, "F8D", 6, 8, field_type);
> +  field_type = tdesc_named_type (feature, "osm");
> +  tdesc_add_typed_bitfield (type_with_fields, "OSM", 14, 14, field_type);
> +  field_type = tdesc_named_type (feature, "osc");
> +  tdesc_add_typed_bitfield (type_with_fields, "OSC", 15, 15, field_type);
> +  tdesc_add_bitfield (type_with_fields, "LSCALE", 16, 22);
> +  field_type = tdesc_named_type (feature, "int8");
> +  tdesc_add_typed_bitfield (type_with_fields, "NSCALE", 24, 31, field_type);
> +  tdesc_add_bitfield (type_with_fields, "LSCALE2", 32, 37);
> +
> +  tdesc_create_reg (feature, "fpmr", regnum++, 1, NULL, 64, "fpmr_flags");
> +  return regnum;
> +}
> diff --git a/gdb/features/aarch64-fpmr.xml b/gdb/features/aarch64-fpmr.xml
> new file mode 100644
> index 00000000000..1f2f9c0d4cf
> --- /dev/null
> +++ b/gdb/features/aarch64-fpmr.xml
> @@ -0,0 +1,55 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2009-2025 Free Software Foundation, Inc.

If the file wasn´t derived from another, this should be 2025 only.

> +     Contributed by ARM Ltd.

I don´t think we're adding these "Contributed by" entries anymore. From 
the commit log, it should be clear where the contribution came from.

> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-target.dtd">
> +<feature name="org.gnu.gdb.aarch64.fpmr">
> +  <enum id="fp8_fmt" size="3">
> +    <!-- E5M2 -->

I´d expand the comment a bit. As it is, it isn´t useful. If there isn´t 
anything useful to say about the register, leave it without a comment. 
Same for other cases below where the comment is just the register name.

> +    <evalue name="E5M2" value="0"/>
> +    <!-- E4M3 -->
> +    <evalue name="E4M3" value="1"/>
> +  </enum>
> +
> +  <!-- Overflow saturation for FP8 convert instructions. Specifies the result
> +  when a floating-point Overflow exception is detected. -->
> +  <enum id="osc" size="1">
> +    <!-- Infinity or NaN is generated. -->
> +    <evalue name="Inf/NaN" value="0"/>
> +    <!-- Maximum normal number is generated. -->
> +    <evalue name="MaxNormal" value="1"/>
> +  </enum>
> +
> +  <!-- Overflow saturation for FP8 multiplication instructions. Specifies the
> +  result when a floating-point Overflow exception is detected. -->
> +  <enum id="osm" size="1">
> +    <!-- Infinity generated. -->
> +    <evalue name="Inf" value="0"/>
> +    <!-- Maximum normal number is generated. -->
> +    <evalue name="MaxNormal" value="1"/>
> +  </enum>
> +
> +  <flags id="fpmr_flags" size="8">
> +    <!-- SRC1 Format.  -->
> +    <field name="F8S1" start="0" end="2" type="fp8_fmt"/>
> +    <!-- SRC2 Format.  -->
> +    <field name="F8S2" start="3" end="5" type="fp8_fmt"/>
> +    <!-- F8D Format.  -->
> +    <field name="F8D" start="6" end="8" type="fp8_fmt"/>
> +    <!-- OSM. -->
> +    <field name="OSM" start="14" end="14" type="osm"/>
> +    <!-- OSC. -->
> +    <field name="OSC" start="15" end="15" type="osc"/>
> +    <!-- LSCALE.  -->
> +    <field name="LSCALE" start="16" end="22"/>
> +    <!-- NSCALE.  -->
> +    <field name="NSCALE" start="24" end="31" type="int8"/>
> +    <!-- LSCALE2.  -->
> +    <field name="LSCALE2" start="32" end="37"/>
> +  </flags>
> +  <reg name="fpmr" bitsize="64" type="fpmr_flags"/>
> +</feature>

A spotted some misc formatting issues, like space between function and 
parenthesis, 2 spaces after period, blank line between function 
declaration and comment. When working through the comment, please make 
sure those get addressed.