[PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62

[PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62

[Mikael Szreder]

The gdbstub implementation for the Sparc architecture would incorectly
 calculate the the floating point register offset.
This would cause register pairs(eg f32,f33) to point to the same value.

Fixes: 30038fd81808 ("target-sparc: Change fpr representation to doubles.")
Signed-off-by: Mikael Szreder <git@miszr.win>
---
 target/sparc/gdbstub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
index ec0036e9ef..5578fa9813 100644
--- a/target/sparc/gdbstub.c
+++ b/target/sparc/gdbstub.c
@@ -80,7 +80,7 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     }
     if (n < 80) {
         /* f32-f62 (double width, even numbers only) */
-        return gdb_get_reg64(mem_buf, env->fpr[(n - 32) / 2].ll);
+        return gdb_get_reg64(mem_buf, env->fpr[(n - 64) + 16].ll);
     }
     switch (n) {
     case 80:
@@ -174,7 +174,7 @@ int sparc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         return 4;
     } else if (n < 80) {
         /* f32-f62 (double width, even numbers only) */
-        env->fpr[(n - 32) / 2].ll = tmp;
+        env->fpr[(n - 64) + 16].ll = tmp;
     } else {
         switch (n) {
         case 80:
-- 
2.48.1

Re: [PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62

[Mikael Szreder]

Ping

On February 3, 2025 3:50:56 PM GMT+01:00, Mikael Szreder <git@miszr.win> wrote:
>The gdbstub implementation for the Sparc architecture would incorectly
> calculate the the floating point register offset.
>This would cause register pairs(eg f32,f33) to point to the same value.
>
>Fixes: 30038fd81808 ("target-sparc: Change fpr representation to doubles.")
>Signed-off-by: Mikael Szreder <git@miszr.win>
>---
> target/sparc/gdbstub.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
>index ec0036e9ef..5578fa9813 100644
>--- a/target/sparc/gdbstub.c
>+++ b/target/sparc/gdbstub.c
>@@ -80,7 +80,7 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>     }
>     if (n < 80) {
>         /* f32-f62 (double width, even numbers only) */
>-        return gdb_get_reg64(mem_buf, env->fpr[(n - 32) / 2].ll);
>+        return gdb_get_reg64(mem_buf, env->fpr[(n - 64) + 16].ll);
>     }
>     switch (n) {
>     case 80:
>@@ -174,7 +174,7 @@ int sparc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>         return 4;
>     } else if (n < 80) {
>         /* f32-f62 (double width, even numbers only) */
>-        env->fpr[(n - 32) / 2].ll = tmp;
>+        env->fpr[(n - 64) + 16].ll = tmp;
>     } else {
>         switch (n) {
>         case 80:

Re: [PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62

[Richard Henderson]

On 2/3/25 06:50, Mikael Szreder wrote:
> The gdbstub implementation for the Sparc architecture would incorectly
>   calculate the the floating point register offset.
> This would cause register pairs(eg f32,f33) to point to the same value.
> 
> Fixes: 30038fd81808 ("target-sparc: Change fpr representation to doubles.")
> Signed-off-by: Mikael Szreder <git@miszr.win>
> ---
>   target/sparc/gdbstub.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
> index ec0036e9ef..5578fa9813 100644
> --- a/target/sparc/gdbstub.c
> +++ b/target/sparc/gdbstub.c
> @@ -80,7 +80,7 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>       }
>       if (n < 80) {
>           /* f32-f62 (double width, even numbers only) */
> -        return gdb_get_reg64(mem_buf, env->fpr[(n - 32) / 2].ll);
> +        return gdb_get_reg64(mem_buf, env->fpr[(n - 64) + 16].ll);

I don't understand your issue with f33, since there is no such thing.  Sparc v9 has f0, 
f1, ... f31, then f32, f34, ... f62.  The odd numbers above 32 do not exist.

Certainly the indexing is incorrect afterward.

Originally,

   f32 = 64 -> (64 - 32) / 2 = 16
   f34 = 66 -> (66 - 32) / 2 = 17

whereas your replacement gives

   f34 = 66 -> (66 - 64) + 16 = 18.

One could rewrite this as

   (n - 64) / 2 + 16

but that's algebraically identical to what we have now, so I don't see the point.

One plausible adjustment in this area would be to reject odd numbers from this block and 
let them fall through to 'return 0' at the end.


r~

Re: [PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62

[Mikael Szreder]

Sorry. As you correctly pointed out there is no f33 register. I wrote f33, when I really meant f34.

I believe the index of f32 is 64 and f34 is 65. This is confirmed when looking at the XML description of the registers in GDB: <https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/features/sparc/sparc64-fpu.xml;h=8710585774d821c660af5e76f92707637633daf7;hb=HEAD>


Thanks in advance 
Mikael Szreder

On February 13, 2025 5:57:59 PM GMT+01:00, Richard Henderson <richard.henderson@linaro.org> wrote:
>On 2/3/25 06:50, Mikael Szreder wrote:
>> The gdbstub implementation for the Sparc architecture would incorectly
>>   calculate the the floating point register offset.
>> This would cause register pairs(eg f32,f33) to point to the same value.
>> 
>> Fixes: 30038fd81808 ("target-sparc: Change fpr representation to doubles.")
>> Signed-off-by: Mikael Szreder <git@miszr.win>
>> ---
>>   target/sparc/gdbstub.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/target/sparc/gdbstub.c b/target/sparc/gdbstub.c
>> index ec0036e9ef..5578fa9813 100644
>> --- a/target/sparc/gdbstub.c
>> +++ b/target/sparc/gdbstub.c
>> @@ -80,7 +80,7 @@ int sparc_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>>       }
>>       if (n < 80) {
>>           /* f32-f62 (double width, even numbers only) */
>> -        return gdb_get_reg64(mem_buf, env->fpr[(n - 32) / 2].ll);
>> +        return gdb_get_reg64(mem_buf, env->fpr[(n - 64) + 16].ll);
>
>I don't understand your issue with f33, since there is no such thing.  Sparc v9 has f0, f1, ... f31, then f32, f34, ... f62.  The odd numbers above 32 do not exist.
>
>Certainly the indexing is incorrect afterward.
>
>Originally,
>
>  f32 = 64 -> (64 - 32) / 2 = 16
>  f34 = 66 -> (66 - 32) / 2 = 17
>
>whereas your replacement gives
>
>  f34 = 66 -> (66 - 64) + 16 = 18.
>
>One could rewrite this as
>
>  (n - 64) / 2 + 16
>
>but that's algebraically identical to what we have now, so I don't see the point.
>
>One plausible adjustment in this area would be to reject odd numbers from this block and let them fall through to 'return 0' at the end.
>
>
>r~

Re: [PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62

[Richard Henderson]

On 2/13/25 11:03, Mikael Szreder wrote:
> Sorry. As you correctly pointed out there is no f33 register. I wrote f33, when I really meant f34.
> 
> I believe the index of f32 is 64 and f34 is 65. This is confirmed when looking at the XML description of the registers in GDB: <https://sourceware.org/git/?p=binutils-gdb.git;a=blob;f=gdb/features/sparc/sparc64-fpu.xml;h=8710585774d821c660af5e76f92707637633daf7;hb=HEAD>

You're correct.  As should also be obvious because 80 - 64 == 16.

So, we could use some updates to the patch description and to the comments within the code 
itself to match.


r~