1. Patchew
  2. QEMU
  3. View series

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

Mikael Szreder posted 1 patch 1 year ago
Failed in applying to current master (apply log)
There is a newer version of this series
target/sparc/gdbstub.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
[PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62
Posted by Mikael Szreder 1 year ago 
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
Posted by Richard Henderson 11 months, 4 weeks ago 
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
Posted by Mikael Szreder 11 months, 4 weeks ago 
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
Posted by Richard Henderson 11 months, 4 weeks ago 
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~
Re: [PATCH] target/sparc: Fix gdbstub incorrectly handling registers f32-f62
Posted by Mikael Szreder 11 months, 4 weeks ago 
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:

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.