Re: [PATCH v3 05/16] [gdb/aarch64] sme: Enable SME registers and pseudo-registers

Thiago Jung Bauermann via Gdb-patches
2023-07-28T01:19:45Z

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

> On 7/26/23 21:01, Thiago Jung Bauermann wrote:
>>
>> Hello,
>>
>> Just minor nits...
>>
>>
>> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>>
>>> @@ -890,21 +959,27 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>>> if (gdbarch_bfd_arch_info (inf->gdbarch)->bits_per_word == 32)
>>> return inf->gdbarch;
>>>
>>> - /* Only return it if the current vector length matches the one in the tdep. */
>>> + /* Only return the inferior's gdbarch if both vq and svq match the ones in
>>> + the tdep. */
>>> aarch64_gdbarch_tdep *tdep
>>> = gdbarch_tdep<aarch64_gdbarch_tdep> (inf->gdbarch);
>>> uint64_t vq = aarch64_sve_get_vq (ptid.lwp ());
>>> - if (vq == tdep->vq)
>>> + uint64_t svq = aarch64_za_get_svq (ptid.lwp ());
>>> + if (vq == tdep->vq && svq == tdep->sme_svq)
>>> return inf->gdbarch;
>>>
>>> - /* We reach here if the vector length for the thread is different from its
>>> + /* We reach here if any vector length for the thread is different from its
>>> value at process start. Lookup gdbarch via info (potentially creating a
>>> - new one) by using a target description that corresponds to the new vq value
>>> - and the current architecture features. */
>>> + new one) by using a target description that corresponds to the new vq/svq
>>> + value and the current architecture features. */
>>>
>>> const struct target_desc *tdesc = gdbarch_target_desc (inf->gdbarch);
>>> aarch64_features features = aarch64_features_from_target_desc (tdesc);
>>> features.vq = vq;
>>> + /* We cast to uint8_t here because the features struct can get large, and it
>>> + is important to store the information in as little storage as
>>> + possible. */
>>
>> Is it because features is used as a key in tdesc_aarch64_map? Mentioning
>> in the comment why it needs to be small would be useful.
>>
>
> I've changed this to:
>
> /* The svq value in the features struct is stored as uint8_t, so cast it
> properly in here. */
>
> It makes me wonder if this cast is useful though. Technically we will implicitly cast
> it to uint8_t anyway, so the value will be truncated. When we hash things into feature
> bits, it will be handled properly.

There are other places which assign an uint64_t to features.svq without
the cast, so I also think it's not really needed here.

> I'm considering removing the comment altogether and just doing the assignment with the
> implicit conversion.

I agree. But it's useful to be aware that even though GDB mostly works
with svq of 64 bits even though this field is 8 bits, so perhaps add a
comment to that effect to the definition of features.svq in aarch64.h?

>>> + features.svq = (uint8_t) svq;
>>>
>>> struct gdbarch_info info;
>>> info.bfd_arch_info = bfd_lookup_arch (bfd_arch_aarch64, bfd_mach_aarch64);

>>> +/* See nat/aarch64-scalable-linux-ptrace.h. */
>>> +
>>> +bool
>>> +write_sve_header (int tid, const struct user_sve_header &header)
>>> +{
>>> + struct iovec iovec;
>>> +
>>> + iovec.iov_len = sizeof (header);
>>> + iovec.iov_base = (void *) &header;
>>
>> Unnecessary cast.
>>
>
> Turns out this is necessary, as otherwise we'll get a complaint about trying to assign a
> (const void *)
> to (void *). Same for the other const struct cases where we write headers.

Ah, sorry about the false alarm.

--
Thiago