Re: [PATCH v3 01/16] [gdb/aarch64] Fix register fetch/store order for native AArch64 Linux

Luis Machado via Gdb-patches
2023-07-24T16:27:30Z

Hi,

On 7/24/23 16:53, Thiago Jung Bauermann wrote:
>
> Hello Luis,
>
> I'm (slowly) going through your patches. I only found a few small nits
> so far.

Thanks! That will be very helpful.

>
> Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:
>
>> @@ -521,28 +522,28 @@ aarch64_fetch_registers (struct regcache *regcache, int regno)
>> if (tdep->has_tls ())
>> fetch_tlsregs_from_thread (regcache);
>> }
>> + /* General purpose register? */
>> else if (regno < AARCH64_V0_REGNUM)
>> fetch_gregs_from_thread (regcache);
>> - else if (tdep->has_sve ())
>> + /* SVE register? */
>> + else if (tdep->has_sve () && regno <= AARCH64_SVE_VG_REGNUM)
>> fetch_sveregs_from_thread (regcache);
>> - else
>> + /* FPSIMD register? */
>> + else if (regno < AARCH64_FPCR_REGNUM)
>
> fetch_fpregs_from_thread reads AARCH64_FPCR_REGNUM, so shouldn't the if
> condition above use '<='?
>
> Ditto for aarch64_store_registers further below.
>

Yes, this does look like a mistake when reworking this code.

>> fetch_fpregs_from_thread (regcache);
>> -
>> - if (tdep->has_pauth ())
>> - {
>> - if (regno == AARCH64_PAUTH_DMASK_REGNUM (tdep->pauth_reg_base)
>> - || regno == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base))
>> - fetch_pauth_masks_from_thread (regcache);
>> - }
>> -
>> - /* Fetch individual MTE registers. */
>> - if (tdep->has_mte ()
>> - && (regno == tdep->mte_reg_base))
>> + /* PAuth register? */
>> + else if (tdep->has_pauth ()
>> + && (regno == AARCH64_PAUTH_DMASK_REGNUM (tdep->pauth_reg_base)
>> + || regno == AARCH64_PAUTH_CMASK_REGNUM (tdep->pauth_reg_base)))
>> + fetch_pauth_masks_from_thread (regcache);
>> + /* MTE register? */
>> + else if (tdep->has_mte ()
>> + && (regno == tdep->mte_reg_base))
>> fetch_mteregs_from_thread (regcache);
>> -
>> - if (tdep->has_tls ()
>> - && regno >= tdep->tls_regnum_base
>> - && regno < tdep->tls_regnum_base + tdep->tls_register_count)
>> + /* TLS register? */
>> + else if (tdep->has_tls ()
>> + && regno >= tdep->tls_regnum_base
>> + && regno < tdep->tls_regnum_base + tdep->tls_register_count)
>> fetch_tlsregs_from_thread (regcache);
>> }
>>
>> @@ -592,6 +593,7 @@ aarch64_store_registers (struct regcache *regcache, int regno)
>> aarch64_gdbarch_tdep *tdep
>> = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
>>
>> + /* Do we need to store all registers? */
>> if (regno == -1)
>> {
>> store_gregs_to_thread (regcache);
>> @@ -606,22 +608,26 @@ aarch64_store_registers (struct regcache *regcache, int regno)
>> if (tdep->has_tls ())
>> store_tlsregs_to_thread (regcache);
>> }
>> + /* General purpose register? */
>> else if (regno < AARCH64_V0_REGNUM)
>> store_gregs_to_thread (regcache);
>> - else if (tdep->has_sve ())
>> + /* SVE register? */
>> + else if (tdep->has_sve () && regno <= AARCH64_SVE_VG_REGNUM)
>> store_sveregs_to_thread (regcache);
>> - else
>> + /* FPSIMD register? */
>> + else if (regno < AARCH64_FPCR_REGNUM)
>
> Shouldn't it be "regno <= AARCH64_FPCR_REGNUM" here?
>

Yes. I'll fix those up. Thanks for spotting those.

>> store_fpregs_to_thread (regcache);
>> -
>> - /* Store MTE registers. */
>> - if (tdep->has_mte ()
>> - && (regno == tdep->mte_reg_base))
>> + /* MTE register? */
>> + else if (tdep->has_mte ()
>> + && (regno == tdep->mte_reg_base))
>> store_mteregs_to_thread (regcache);
>> -
>> - if (tdep->has_tls ()
>> - && regno >= tdep->tls_regnum_base
>> - && regno < tdep->tls_regnum_base + tdep->tls_register_count)
>> + /* TLS register? */
>> + else if (tdep->has_tls ()
>> + && regno >= tdep->tls_regnum_base
>> + && regno < tdep->tls_regnum_base + tdep->tls_register_count)
>> store_tlsregs_to_thread (regcache);
>> +
>> + /* PAuth registers are read-only. */
>> }
>>
>> /* A version of the "store_registers" target_ops method used when running
>