diff mbox series

target/i386/gdbstub: Fix a bug about order of FPU stack in 'g' packets.

Message ID TY0PR0101MB4285923FBE9AD97CE832D95BA4E59@TY0PR0101MB4285.apcprd01.prod.exchangelabs.com (mailing list archive)
State New, archived
Headers show
Series target/i386/gdbstub: Fix a bug about order of FPU stack in 'g' packets. | expand

Commit Message

伊藤 太清 Dec. 19, 2022, 4:04 a.m. UTC
Before this commit, when GDB attached an OS working on QEMU, order of FPU
stack registers printed by GDB command 'info float' was wrong. There was a
bug causing the problem in 'g' packets sent by QEMU to GDB. The packets have
values of registers of machine emulated by QEMU containing FPU stack
registers. There are 2 ways to specify a x87 FPU stack register. The first
is specifying by absolute indexed register names (R0, ..., R7). The second
is specifying by stack top relative indexed register names (ST0, ..., ST7).
Values of the FPU stack registers should be located in 'g' packet and be
ordered by the relative index. But QEMU had located these registers ordered
by the absolute index. After this commit, when QEMU reads registers to make
a 'g' packet, QEMU specifies FPU stack registers by the relative index.
Then, the registers are ordered correctly in the packet. As a result, GDB,
the packet receiver, can print FPU stack registers in the correct order.

Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
---
 target/i386/gdbstub.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Alex Bennée Dec. 19, 2022, 3:40 p.m. UTC | #1
TaiseiIto <taisei1212@outlook.jp> writes:

> Before this commit, when GDB attached an OS working on QEMU, order of FPU
> stack registers printed by GDB command 'info float' was wrong. There was a
> bug causing the problem in 'g' packets sent by QEMU to GDB. The packets have
> values of registers of machine emulated by QEMU containing FPU stack
> registers. There are 2 ways to specify a x87 FPU stack register. The first
> is specifying by absolute indexed register names (R0, ..., R7). The second
> is specifying by stack top relative indexed register names (ST0, ..., ST7).
> Values of the FPU stack registers should be located in 'g' packet and be
> ordered by the relative index. But QEMU had located these registers ordered
> by the absolute index. After this commit, when QEMU reads registers to make
> a 'g' packet, QEMU specifies FPU stack registers by the relative index.
> Then, the registers are ordered correctly in the packet. As a result, GDB,
> the packet receiver, can print FPU stack registers in the correct order.
>
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>  target/i386/gdbstub.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index c3a2cf6f28..6109ad166d 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -121,7 +121,9 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>              return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
>          }
>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> -        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> +        int st_index = n - IDX_FP_REGS;
> +        int r_index = (st_index + env->fpstt) % 8;
> +        floatx80 *fp = &env->fpregs[r_index].d;
>          int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
>          len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>          return len;

Shouldn't this have Richard's reviewed by tag? It's also useful if you
add a revision number to the subject (e.g. git send-email -v2) as well
as noting the differences under a --- marker so reviewers can see what
changed.

  https://qemu.readthedocs.io/en/latest/devel/submitting-a-patch.html#when-resending-patches-add-a-version-tag

Thanks,
Richard Henderson Dec. 19, 2022, 6:11 p.m. UTC | #2
On 12/18/22 20:04, TaiseiIto wrote:
> Before this commit, when GDB attached an OS working on QEMU, order of FPU
> stack registers printed by GDB command 'info float' was wrong. There was a
> bug causing the problem in 'g' packets sent by QEMU to GDB. The packets have
> values of registers of machine emulated by QEMU containing FPU stack
> registers. There are 2 ways to specify a x87 FPU stack register. The first
> is specifying by absolute indexed register names (R0, ..., R7). The second
> is specifying by stack top relative indexed register names (ST0, ..., ST7).
> Values of the FPU stack registers should be located in 'g' packet and be
> ordered by the relative index. But QEMU had located these registers ordered
> by the absolute index. After this commit, when QEMU reads registers to make
> a 'g' packet, QEMU specifies FPU stack registers by the relative index.
> Then, the registers are ordered correctly in the packet. As a result, GDB,
> the packet receiver, can print FPU stack registers in the correct order.
> 
> Signed-off-by: TaiseiIto <taisei1212@outlook.jp>
> ---
>   target/i386/gdbstub.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

> 
> diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
> index c3a2cf6f28..6109ad166d 100644
> --- a/target/i386/gdbstub.c
> +++ b/target/i386/gdbstub.c
> @@ -121,7 +121,9 @@ int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
>               return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
>           }
>       } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> -        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
> +        int st_index = n - IDX_FP_REGS;
> +        int r_index = (st_index + env->fpstt) % 8;
> +        floatx80 *fp = &env->fpregs[r_index].d;
>           int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
>           len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
>           return len;
Paolo Bonzini Feb. 15, 2023, 12:36 p.m. UTC | #3
Queued, thanks.

Paolo
diff mbox series

Patch

diff --git a/target/i386/gdbstub.c b/target/i386/gdbstub.c
index c3a2cf6f28..6109ad166d 100644
--- a/target/i386/gdbstub.c
+++ b/target/i386/gdbstub.c
@@ -121,7 +121,9 @@  int x86_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
             return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
         }
     } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
-        floatx80 *fp = (floatx80 *) &env->fpregs[n - IDX_FP_REGS];
+        int st_index = n - IDX_FP_REGS;
+        int r_index = (st_index + env->fpstt) % 8;
+        floatx80 *fp = &env->fpregs[r_index].d;
         int len = gdb_get_reg64(mem_buf, cpu_to_le64(fp->low));
         len += gdb_get_reg16(mem_buf, cpu_to_le16(fp->high));
         return len;