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

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

Luis Machado <luis.machado@arm.com> writes:

> On 8/3/23 01:18, Thiago Jung Bauermann wrote:
>> 
>> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>> 
>>> +  /* 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.
>>
>
> Did you mean s/SSE/SME?
>
> I'm not sure I understand. We need to process the SSVE entry first to make sure we have
> the right values for SVCR, and
> the SVE entry may be inactive. Could you please expand on your idea?

I am assuming that if SVE is inactive, the buffer will at least have a
header with sane values such as the dummy one written by
collect_inactive_sve_regset. In this case, we could check whether the
flags field has the SVE bit set:

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 0bd75daa994c..ce9cc4e32000 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -1034,11 +1034,14 @@ aarch64_linux_supply_sve_regset (const struct regset *regset,
 
   if (tdep->has_sme ())
     {
-      ULONGEST svcr = 0;
-      regcache->raw_collect (tdep->sme_svcr_regnum, &svcr);
+      gdb_byte *header = (gdb_byte *) buf;
+      enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
+      uint16_t flags
+	  = extract_unsigned_integer (header + SVE_HEADER_FLAGS_OFFSET,
+				      SVE_HEADER_FLAGS_LENGTH, byte_order);
 
       /* Is streaming mode enabled?  */
-      if (svcr & SVCR_SM_BIT)
+      if (flags & SVE_HEADER_FLAG_SVE)
 	/* If so, don't load SVE data from the SVE section.  The data to be
 	   used is in the SSVE section.  */
 	return;

But if we can't assume dummy values for the SVE header, then my idea
won't work.

>>> +
>>> +      /* 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 ().
>> 
>
> That is a bit strange, but I don't see this function being called with a specified regnum
> other than -1.
>
> For instance, in gcore-elf.c:gcore_elf_build_thread_register_notes, we fetch all
> registers.
>
> Also, in corelow.c:core_target::get_core_register_section, supply is also explicitly
> called with -1.
>
> Maybe this was updated at some point, and we don't fetch single registers from core files
> anymore.
>
> Do you see any paths calling this with regnum != -1?

Indeed you're right, I don't. I made this comment based on the
documentation of regset and regcache functions, and existing regcache
code that deals with regnum != -1.

I see a few targets that seems to expect calling regset->collect_regset
with regnum != -1 (e.g., fbsd_nat_target::store_register_set and
ppc-linux-nat.c's store_regset) but nothing in generic code or
aarch64-linux nat or tdep.

>>> +
>>> +  /* 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?
>> 
>
> Yes. It will be invalidated and memset-ed to 0. Though that behavior might not be a strong
> guarantee.

Ok, thanks for the explanation.

-- 
Thiago