diff mbox series

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

Message ID 20230630134616.1238105-2-luis.machado@arm.com
State New
Headers
Series SME support for AArch64 gdb/gdbserver on Linux |

Commit Message

Luis Machado June 30, 2023, 1:46 p.m. UTC 
  I noticed we don't handle register reads/writes in the best way for native
AArch64 Linux.  Some registers are fetched/stored even if upper level code
told us to fetch a particular register number.

Fix this by being more strict about which registers we touch when
reading/writing them in the native AArch64 Linux layer.

There should be no user-visible changes due to this patch.

Regression-tested on aarch64-linux Ubuntu 22.04/20.04.
---
 gdb/aarch64-linux-nat.c | 60 ++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 27 deletions(-)

Comments

Thiago Jung Bauermann July 24, 2023, 3:53 p.m. UTC | #1 
Hello Luis,

I'm (slowly) going through your patches. I only found a few small nits
so far.

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.

>      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?

>      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
Luis Machado July 24, 2023, 4:26 p.m. UTC | #2 
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
>
diff mbox series

Patch

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index ecb2eeb9540..62656a7347b 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -504,6 +504,7 @@  aarch64_fetch_registers (struct regcache *regcache, int regno)
   aarch64_gdbarch_tdep *tdep
     = gdbarch_tdep<aarch64_gdbarch_tdep> (regcache->arch ());
 
+  /* Do we need to fetch all registers?  */
   if (regno == -1)
     {
       fetch_gregs_from_thread (regcache);
@@ -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 (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)
     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