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

Thiago Jung Bauermann via Gdb-patches
2023-08-04T20:46:07Z

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