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

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

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