Re: [PATCH v3 14/16] [gdb/aarch64] sme: Core file support for Linux

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

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> This patch enables dumping SME state via gdb's gcore command and also
> enables gdb to read SME state from a core file generated by the Linux
> Kernel.
>
> Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
> ---
>  gdb/aarch64-linux-tdep.c          | 532 ++++++++++++++++++++++++++++--
>  gdb/arch/aarch64-scalable-linux.c |  34 ++
>  gdb/arch/aarch64-scalable-linux.h |  15 +
>  3 files changed, 548 insertions(+), 33 deletions(-)
>
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 7ce34ee6846..0bd75daa994 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -57,6 +57,10 @@
>  
>  #include "elf/common.h"
>  #include "elf/aarch64.h"
> +#include "arch/aarch64-insn.h"
> +
> +/* For std::sqrt */

s/sqrt/pow/

> +#include <cmath>

> @@ -853,14 +901,89 @@ aarch64_linux_supply_sve_regset (const struct regset *regset,
>      }
>  }
>  
> +/* Collect an inactive SVE register set state.  This is equivalent to a
> +   fpsimd layout.
> +
> +   Collect the data from REGCACHE to BUF, using the register
> +   map in REGSET.  */
> +
> +static void
> +collect_inactive_sve_regset (const struct regcache *regcache,
> +			     void *buf, size_t size, int vg_regnum)
> +{
> +  gdb_byte *header = (gdb_byte *) buf;
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> +  gdb_assert (buf != nullptr);
> +  gdb_assert (size > SVE_HEADER_SIZE);

This assert should check for SVE_CORE_DUMMY_SIZE.

> +
> +  /* Zero out everything first.  */
> +  memset ((gdb_byte *) buf, 0, SVE_CORE_DUMMY_SIZE);
> +
> +  /* BUF starts with a SVE header prior to the register dump.  */
> +
> +  /* Dump the default size of an empty SVE payload.  */
> +  uint32_t real_size = SVE_CORE_DUMMY_SIZE;
> +  store_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET,
> +			  SVE_HEADER_SIZE_LENGTH, byte_order, real_size);
> +
> +  /* Dump a dummy max size.  */
> +  uint32_t max_size = SVE_CORE_DUMMY_MAX_SIZE;
> +  store_unsigned_integer (header + SVE_HEADER_MAX_SIZE_OFFSET,
> +			  SVE_HEADER_MAX_SIZE_LENGTH, byte_order, max_size);
> +
> +  /* Dump the vector length.  */
> +  ULONGEST vg = 0;
> +  regcache->raw_collect (vg_regnum, &vg);
> +  uint16_t vl = sve_vl_from_vg (vg);
> +  store_unsigned_integer (header + SVE_HEADER_VL_OFFSET, SVE_HEADER_VL_LENGTH,
> +			  byte_order, vl);
> +
> +  /* Dump the standard maximum vector length.  */
> +  uint16_t max_vl = SVE_CORE_DUMMY_MAX_VL;
> +  store_unsigned_integer (header + SVE_HEADER_MAX_VL_OFFSET,
> +			  SVE_HEADER_MAX_VL_LENGTH, byte_order,
> +			  max_vl);
> +
> +  /* The rest of the fields is zero.  */

s/is/are/

> +  uint16_t flags = SVE_CORE_DUMMY_FLAGS;
> +  store_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET,
> +			  SVE_HEADER_FLAGS_LENGTH, byte_order,
> +			  flags);
> +  uint16_t reserved = SVE_CORE_DUMMY_RESERVED;
> +  store_unsigned_integer (header + SVE_HEADER_RESERVED_OFFSET,
> +			  SVE_HEADER_RESERVED_LENGTH, byte_order, reserved);
> +
> +  /* We are done with the header part of it.  Now dump the register state
> +     in the FPSIMD format.  */
> +
> +  /* Dump the first 128 bits of each of the Z registers.  */
> +  header += AARCH64_SVE_CONTEXT_REGS_OFFSET;
> +  for (int i = 0; i < AARCH64_SVE_Z_REGS_NUM; i++)
> +    regcache->raw_collect_part (AARCH64_SVE_Z0_REGNUM + i, 0, V_REGISTER_SIZE,
> +				header + V_REGISTER_SIZE * i);
> +
> +  /* Dump FPSR and FPCR.  */
> +  header += 32 * V_REGISTER_SIZE;
> +  regcache->raw_collect (AARCH64_FPSR_REGNUM, header);
> +  regcache->raw_collect (AARCH64_FPCR_REGNUM, header);

Header needs to be incremented before writing FPCR, otherwise
raw_collect() will just overwrite FPSR.

> +
> +  /* Dump two reserved empty fields of 4 bytes.  */
> +  header += 8;
> +  memset (header, 0, 8);
> +
> +  /* We should have a FPSIMD-formatted register dump now.  */
> +}

> +/* Supply register REGNUM from BUF to REGCACHE, using the register map
> +   in REGSET.  If REGNUM is -1, do this for all registers in REGSET.
> +   If BUF is NULL, set the registers to "unavailable" status.  */
> +
> +static void
> +aarch64_linux_supply_sve_regset (const struct regset *regset,
> +				 struct regcache *regcache,
> +				 int regnum, const void *buf, size_t size)
> +{
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +
> +  if (tdep->has_sme ())
> +    {
> +      ULONGEST svcr = 0;
> +      regcache->raw_collect (tdep->sme_svcr_regnum, &svcr);

Instead of relying on parsing the SSVE section before the SVE section to
ensure that the SVCR register is valid by the time we arrive here, isn't
it more robust to read the flags field from the SVE header in buf, as is
done in suply_sve_regset()? Also, it's what the SSE version does.

> +
> +      /* Is streaming mode enabled?  */
> +      if (svcr & SVCR_SM_BIT)
> +	/* If so, don't load SVE data from the SVE section.  The data to be
> +	   used is in the SSVE section.  */
> +	return;
> +    }
> +  /* If streaming mode is not enabled, load the SVE regcache data from the SVE
> +     section.  */
> +  supply_sve_regset (regset, regcache, regnum, buf, size);
> +}
> +
> +/* Collect register REGNUM from REGCACHE to BUF, using the register
> +   map in REGSET.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +static void
> +aarch64_linux_collect_sve_regset (const struct regset *regset,
> +				  const struct regcache *regcache,
> +				  int regnum, void *buf, size_t size)
> +{
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +  bool streaming_mode = false;
> +
> +  if (tdep->has_sme ())
> +    {
> +      ULONGEST svcr = 0;
> +      regcache->raw_collect (tdep->sme_svcr_regnum, &svcr);
> +
> +      /* Is streaming mode enabled?  */
> +      if (svcr & SVCR_SM_BIT)
> +	{
> +	  /* If so, don't dump SVE regcache data to the SVE section.  The SVE
> +	     data should be dumped to the SSVE section.  Dump an empty SVE
> +	     block instead.  */
> +	  streaming_mode = true;
> +	}
> +    }
> +
> +  /* If streaming mode is not enabled or there is no SME support, dump the
> +     SVE regcache data to the SVE section.  */
> +
> +  /* Check if we have an active SVE state (non-zero Z/P/FFR registers).
> +     If so, then we need to dump registers in the SVE format.
> +
> +     Otherwise we should dump the registers in the FPSIMD format.  */
> +  if (sve_state_is_empty (regcache) || streaming_mode)
> +    collect_inactive_sve_regset (regcache, buf, size, AARCH64_SVE_VG_REGNUM);

If regnum != -1, collect_inactive_sve_regset () will still dump the
entirety of the fpsimd regset. Shouldn't it get the regnum as an
argument and dump just the requested register?

Same comment in aarch64_linux_collect_ssve_regset ().

> +  else
> +    collect_sve_regset (regset, regcache, regnum, buf, size);
> +}

> +/* Collect register REGNUM from REGCACHE to BUF, using the register
> +   map in REGSET.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +static void
> +aarch64_linux_collect_ssve_regset (const struct regset *regset,
> +				   const struct regcache *regcache,
> +				   int regnum, void *buf, size_t size)
> +{
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +  ULONGEST svcr = 0;
> +  regcache->raw_collect (tdep->sme_svcr_regnum, &svcr);
> +
> +  /* Is streaming mode enabled?  */
> +  if (svcr & SVCR_SM_BIT)
> +    {
> +      /* If so, dump SVE regcache data to the SSVE section.  */
> +      collect_sve_regset (regset, regcache, regnum, buf, size);
> +    }
> +  else
> +    {
> +      /* Otherwise dump an empty SVE block to the SSVE section with the
> +	 streaming vector length.  */
> +      collect_inactive_sve_regset (regcache, buf, size, tdep->sme_svg_regnum);

Same argument here about regnum != -1 as in
aarch64_linux_collect_sve_regset().

> +    }
> +}
> +
> +/* Supply register REGNUM from BUF to REGCACHE, using the register map
> +   in REGSET.  If REGNUM is -1, do this for all registers in REGSET.
> +   If BUF is NULL, set the registers to "unavailable" status.  */
> +
> +static void
> +aarch64_linux_supply_za_regset (const struct regset *regset,
> +				struct regcache *regcache,
> +				int regnum, const void *buf, size_t size)
> +{
> +  gdb_byte *header = (gdb_byte *) buf;
> +  struct gdbarch *gdbarch = regcache->arch ();
> +  enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
> +
> +  /* Handle an empty buffer.  */
> +  if (buf == nullptr)
> +    return regcache->supply_regset (regset, regnum, nullptr, size);
> +
> +  if (size < SVE_HEADER_SIZE)
> +    warning (_("ZA state header size (%s) invalid.  Should be at least %s."),
> +	     pulongest (size), pulongest (SVE_HEADER_SIZE));

If the header is truncated, I don't think the rest of this function
works correctly. I'd suggest either changing the warning to an error, or
treating this case in the same way as the buf == nullptr case above.

> +
> +  /* The ZA register note in a core file can have a couple of states:
> +
> +     1 - Just the header without the payload.  This means that there is no
> +	 ZA data, and we should populate only SVCR and SVG registers on GDB's
> +	 side.  The ZA data should be marked as unavailable.
> +
> +     2 - The header with an additional data payload.  This means there is
> +	 actual ZA data, and we should populate ZA, SVCR and SVG.  */
> +
> +  aarch64_gdbarch_tdep *tdep = gdbarch_tdep<aarch64_gdbarch_tdep> (gdbarch);
> +
> +  /* Populate SVG.  */
> +  ULONGEST svg
> +    = sve_vg_from_vl (extract_unsigned_integer (header + SVE_HEADER_VL_OFFSET,
> +						SVE_HEADER_VL_LENGTH,
> +						byte_order));
> +  regcache->raw_supply (tdep->sme_svg_regnum, &svg);
> +
> +  size_t data_size
> +    = extract_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET,
> +				SVE_HEADER_SIZE_LENGTH, byte_order)
> +      - SVE_HEADER_SIZE;
> +
> +  /* Populate SVCR.  */
> +  bool has_za_payload = (data_size > 0);
> +  ULONGEST svcr;
> +  regcache->raw_collect (tdep->sme_svcr_regnum, &svcr);
> +
> +  /* If we have a ZA payload, enable bit 2 of SVCR, otherwise clear it.  This
> +     register gets updated by the SVE/SSVE-handling functions as well, as they
> +     report the SM bit 1.  */
> +  if (has_za_payload)
> +    svcr |= SVCR_ZA_BIT;
> +  else
> +    svcr &= ~SVCR_ZA_BIT;
> +
> +  /* Update SVCR in the register buffer.  */
> +  regcache->raw_supply (tdep->sme_svcr_regnum, &svcr);
> +
> +  /* Populate the register cache with ZA register contents, if we have any.  */
> +  buf = has_za_payload ? (gdb_byte *) buf + SVE_HEADER_SIZE : nullptr;
> +
> +  /* Update ZA in the register buffer.  */
> +  if (has_za_payload)
> +    regcache->raw_supply (tdep->sme_za_regnum, buf);

Do we need to make sure that buf is big enough to supply the whole
contents of the ZA register? Otherwise, the memcpy() in raw_supply() will
copy garbage.

> +  else
> +    {
> +      size_t za_bytes = std::pow (sve_vl_from_vg (svg), 2);
> +      gdb_byte za_zeroed[za_bytes];
> +      memset (za_zeroed, 0, za_bytes);
> +      regcache->raw_supply (tdep->sme_za_regnum, za_zeroed);
> +    }
> +}
> +
> +/* Collect register REGNUM from REGCACHE to BUF, using the register
> +   map in REGSET.  If REGNUM is -1, do this for all registers in
> +   REGSET.  */
> +
> +static void
> +aarch64_linux_collect_za_regset (const struct regset *regset,
> +				 const struct regcache *regcache,
> +				 int regnum, void *buf, size_t size)
> +{
> +  gdb_assert (buf != nullptr);
> +
> +  if (size < SVE_HEADER_SIZE)
> +    warning (_("ZA state header size (%s) invalid.  Should be at least %s."),
> +	     pulongest (size), pulongest (SVE_HEADER_SIZE));

Here too, if size < SVE_HEADER_SIZE I don't think it makes sense to try
to continue. Perhaps put this condition in a gdb_assert()?

> +
> +  /* The ZA register note in a core file can have a couple of states:
> +
> +     1 - Just the header without the payload.  This means that there is no
> +	 ZA data, and we should dump just the header.
> +
> +     2 - The header with an additional data payload.  This means there is
> +	 actual ZA data, and we should dump both the header and the ZA data
> +	 payload.  */
> +
> +  aarch64_gdbarch_tdep *tdep
> +    = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
> +
> +  /* Determine if we have ZA state from the SVCR register ZA bit.  */
> +  ULONGEST svcr;
> +  regcache->raw_collect (tdep->sme_svcr_regnum, &svcr);
> +
> +  /* Check the ZA payload.  */
> +  bool has_za_payload = (svcr & SVCR_ZA_BIT) != 0;
> +  size = has_za_payload ? size : SVE_HEADER_SIZE;
> +
> +  /* Write the size and max_size fields.  */
> +  gdb_byte *header = (gdb_byte *) buf;
> +  enum bfd_endian byte_order = gdbarch_byte_order (regcache->arch ());
> +  store_unsigned_integer (header + SVE_HEADER_SIZE_OFFSET,
> +			  SVE_HEADER_SIZE_LENGTH, byte_order, size);
> +
> +  uint32_t max_size
> +    = SVE_HEADER_SIZE + std::pow (sve_vl_from_vq (tdep->sme_svq), 2);
> +  store_unsigned_integer (header + SVE_HEADER_MAX_SIZE_OFFSET,
> +			  SVE_HEADER_MAX_SIZE_LENGTH, byte_order, max_size);
> +
> +  /* Output the other fields of the ZA header (vl, max_vl, flags and
> +     reserved).  */
> +  uint64_t svq = tdep->sme_svq;
> +  store_unsigned_integer (header + SVE_HEADER_VL_OFFSET, SVE_HEADER_VL_LENGTH,
> +			  byte_order, sve_vl_from_vq (svq));
> +
> +  uint16_t max_vl = SVE_CORE_DUMMY_MAX_VL;
> +  store_unsigned_integer (header + SVE_HEADER_MAX_VL_OFFSET,
> +			  SVE_HEADER_MAX_VL_LENGTH, byte_order,
> +			  max_vl);
> +
> +  uint16_t flags = SVE_CORE_DUMMY_FLAGS;
> +  store_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET,
> +			  SVE_HEADER_FLAGS_LENGTH, byte_order, flags);
> +
> +  uint16_t reserved = SVE_CORE_DUMMY_RESERVED;
> +  store_unsigned_integer (header + SVE_HEADER_RESERVED_OFFSET,
> +			  SVE_HEADER_RESERVED_LENGTH, byte_order, reserved);
> +
> +  buf = has_za_payload ? (gdb_byte *) buf + SVE_HEADER_SIZE : nullptr;
> +
> +  /* Dump the register cache contents for the ZA register to the buffer.  */
> +  regcache->collect_regset (regset, regnum, (gdb_byte *) buf,
> +			    size - SVE_HEADER_SIZE);

Calling collect with buf == nullptr will invalidate the regset. Is that
the intention?

> +}

-- 
Thiago