[Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Doug Evans]

The remote protocol can't handle flipping back and forth
between 32-bit and 64-bit regs. To compensate, pretend "as if"
on 64-bit cpu when in 32-bit mode.

Signed-off-by: Doug Evans <dje@google.com>
---
  target-i386/gdbstub.c | 52  
++++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 39 insertions(+), 13 deletions(-)

diff --git a/target-i386/gdbstub.c b/target-i386/gdbstub.c
index c494535..9b94ab8 100644
--- a/target-i386/gdbstub.c
+++ b/target-i386/gdbstub.c
@@ -44,10 +44,22 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t  
*mem_buf, int n)
      X86CPU *cpu = X86_CPU(cs);
      CPUX86State *env = &cpu->env;

+    /* N.B. GDB can't deal with changes in registers or sizes in the middle
+       of a session. So if we're in 32-bit mode on a 64-bit cpu, still act
+       as if we're on a 64-bit cpu. */
+
      if (n < CPU_NB_REGS) {
-        if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
-            return gdb_get_reg64(mem_buf, env->regs[gpr_map[n]]);
-        } else if (n < CPU_NB_REGS32) {
+        if (TARGET_LONG_BITS == 64) {
+            if (env->hflags & HF_CS64_MASK) {
+                return gdb_get_reg64(mem_buf, env->regs[gpr_map[n]]);
+            } else if (n < CPU_NB_REGS32) {
+                return gdb_get_reg64(mem_buf,
+                                     env->regs[gpr_map[n]] & 0xffffffffUL);
+            } else {
+                memset(mem_buf, 0, sizeof(target_ulong));
+                return sizeof(target_ulong);
+            }
+        } else {
              return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
          }
      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
@@ -60,8 +72,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t  
*mem_buf, int n)
          return 10;
      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
          n -= IDX_XMM_REGS;
-        if (n < CPU_NB_REGS32 ||
-            (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK)) {
+        if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
              stq_p(mem_buf, env->xmm_regs[n].ZMM_Q(0));
              stq_p(mem_buf + 8, env->xmm_regs[n].ZMM_Q(1));
              return 16;
@@ -69,8 +80,12 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t  
*mem_buf, int n)
      } else {
          switch (n) {
          case IDX_IP_REG:
-            if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
-                return gdb_get_reg64(mem_buf, env->eip);
+            if (TARGET_LONG_BITS == 64) {
+                if (env->hflags & HF_CS64_MASK) {
+                    return gdb_get_reg64(mem_buf, env->eip);
+                } else {
+                    return gdb_get_reg64(mem_buf, env->eip & 0xffffffffUL);
+                }
              } else {
                  return gdb_get_reg32(mem_buf, env->eip);
              }
@@ -151,9 +166,17 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t  
*mem_buf, int n)
      CPUX86State *env = &cpu->env;
      uint32_t tmp;

+    /* N.B. GDB can't deal with changes in registers or sizes in the middle
+       of a session. So if we're in 32-bit mode on a 64-bit cpu, still act
+       as if we're on a 64-bit cpu. */
+
      if (n < CPU_NB_REGS) {
-        if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
-            env->regs[gpr_map[n]] = ldtul_p(mem_buf);
+        if (TARGET_LONG_BITS == 64) {
+            if (env->hflags & HF_CS64_MASK) {
+                env->regs[gpr_map[n]] = ldtul_p(mem_buf);
+            } else if (n < CPU_NB_REGS32) {
+                env->regs[gpr_map[n]] = ldtul_p(mem_buf) & 0xffffffffUL;
+            }
              return sizeof(target_ulong);
          } else if (n < CPU_NB_REGS32) {
              n = gpr_map32[n];
@@ -169,8 +192,7 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t  
*mem_buf, int n)
          return 10;
      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
          n -= IDX_XMM_REGS;
-        if (n < CPU_NB_REGS32 ||
-            (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK)) {
+        if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
              env->xmm_regs[n].ZMM_Q(0) = ldq_p(mem_buf);
              env->xmm_regs[n].ZMM_Q(1) = ldq_p(mem_buf + 8);
              return 16;
@@ -178,8 +200,12 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t  
*mem_buf, int n)
      } else {
          switch (n) {
          case IDX_IP_REG:
-            if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
-                env->eip = ldq_p(mem_buf);
+            if (TARGET_LONG_BITS == 64) {
+                if (env->hflags & HF_CS64_MASK) {
+                    env->eip = ldq_p(mem_buf);
+                } else {
+                    env->eip = ldq_p(mem_buf) & 0xffffffffUL;
+                }
                  return 8;
              } else {
                  env->eip &= ~0xffffffffUL;
-- 

Re: [Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Richard Henderson]

On 11/03/2016 05:35 PM, Doug Evans wrote:
> The remote protocol can't handle flipping back and forth
> between 32-bit and 64-bit regs. To compensate, pretend "as if"
> on 64-bit cpu when in 32-bit mode.
>
> Signed-off-by: Doug Evans <dje@google.com>
> ---
>  target-i386/gdbstub.c | 52 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Peter Maydell]

On 4 November 2016 at 16:29, Richard Henderson <rth@twiddle.net> wrote:
> On 11/03/2016 05:35 PM, Doug Evans wrote:
>>
>> The remote protocol can't handle flipping back and forth
>> between 32-bit and 64-bit regs. To compensate, pretend "as if"
>> on 64-bit cpu when in 32-bit mode.
>>
>> Signed-off-by: Doug Evans <dje@google.com>
>> ---
>>  target-i386/gdbstub.c | 52
>> ++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 39 insertions(+), 13 deletions(-)
>
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Has anybody ever looked at fixing this on the gdb end?
(There are some similar cases for ARM CPUs, and actually
being able to debug across 32/64 boundaries, big/little
endian boundaries etc would be quite handy in some
situations.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Pedro Alves]

On 11/04/2016 04:34 PM, Peter Maydell wrote:
> On 4 November 2016 at 16:29, Richard Henderson <rth@twiddle.net> wrote:
>> On 11/03/2016 05:35 PM, Doug Evans wrote:
>>>
>>> The remote protocol can't handle flipping back and forth
>>> between 32-bit and 64-bit regs. To compensate, pretend "as if"
>>> on 64-bit cpu when in 32-bit mode.
>>>
>>> Signed-off-by: Doug Evans <dje@google.com>
>>> ---
>>>  target-i386/gdbstub.c | 52
>>> ++++++++++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 39 insertions(+), 13 deletions(-)
>>
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
> Has anybody ever looked at fixing this on the gdb end?
> (There are some similar cases for ARM CPUs, and actually
> being able to debug across 32/64 boundaries, big/little
> endian boundaries etc would be quite handy in some
> situations.)
> 

A while ago I was chatting with Andy Lutomirski about this,
and he was saying that at least for x86_64, he'd prefer if gdb
always thought of the register file as 64-bit, even if
in 16-bit/32-bit mode.

 http://lists-archives.com/linux-kernel/28605147-x86-signal-rewire-the-restart_block-syscall-to-have-a-constant-nr.html
 http://lists-archives.com/linux-kernel/28605329-x86-signal-rewire-the-restart_block-syscall-to-have-a-constant-nr.html

I never followed up with that, which is totally lame of me.

Even if we did that, when debugging 32-bit mode code,
I think you'd want gdb to present a 32-bit view of the program,
so that printing expressions uses the correct types,
calling 32-bit/64-bit functions in the inferior builds
a call frame using the right ABI, etc. (and that exposes
other challenges like maybe needing to switch modes
while the infcall is running if you call a function of
the "wrong" mode, etc.)  I believe the MIPS 64-bit gdb port actually
works that way - it always transfers 64-bit registers across the
wire, and then gdb presents 32-bit registers to debugging an elf
of a 32-bit ABI.  I think it's the only port that does that.
Whatever approach is taken, I suspect that there's a good amount
of work needed to make things work completely seamlessly, though.

Thanks,
Pedro Alves

Re: [Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Doug Evans]

On Fri, Nov 4, 2016 at 12:01 PM, Pedro Alves <palves@redhat.com> wrote:
> On 11/04/2016 04:34 PM, Peter Maydell wrote:
>> On 4 November 2016 at 16:29, Richard Henderson <rth@twiddle.net> wrote:
>>> On 11/03/2016 05:35 PM, Doug Evans wrote:
>>>>
>>>> The remote protocol can't handle flipping back and forth
>>>> between 32-bit and 64-bit regs. To compensate, pretend "as if"
>>>> on 64-bit cpu when in 32-bit mode.
>>>>
>>>> Signed-off-by: Doug Evans <dje@google.com>
>>>> ---
>>>>  target-i386/gdbstub.c | 52
>>>> ++++++++++++++++++++++++++++++++++++++-------------
>>>>  1 file changed, 39 insertions(+), 13 deletions(-)
>>>
>>>
>>> Reviewed-by: Richard Henderson <rth@twiddle.net>
>>
>> Has anybody ever looked at fixing this on the gdb end?
>> (There are some similar cases for ARM CPUs, and actually
>> being able to debug across 32/64 boundaries, big/little
>> endian boundaries etc would be quite handy in some
>> situations.)
>>
>
> A while ago I was chatting with Andy Lutomirski about this,
> and he was saying that at least for x86_64, he'd prefer if gdb
> always thought of the register file as 64-bit, even if
> in 16-bit/32-bit mode.
>
>  http://lists-archives.com/linux-kernel/28605147-x86-signal-rewire-the-restart_block-syscall-to-have-a-constant-nr.html
>  http://lists-archives.com/linux-kernel/28605329-x86-signal-rewire-the-restart_block-syscall-to-have-a-constant-nr.html
>
> I never followed up with that, which is totally lame of me.
>
> Even if we did that, when debugging 32-bit mode code,
> I think you'd want gdb to present a 32-bit view of the program,
> so that printing expressions uses the correct types,
> calling 32-bit/64-bit functions in the inferior builds
> a call frame using the right ABI, etc. (and that exposes
> other challenges like maybe needing to switch modes
> while the infcall is running if you call a function of
> the "wrong" mode, etc.)  I believe the MIPS 64-bit gdb port actually
> works that way - it always transfers 64-bit registers across the
> wire, and then gdb presents 32-bit registers to debugging an elf
> of a 32-bit ABI.  I think it's the only port that does that.
> Whatever approach is taken, I suspect that there's a good amount
> of work needed to make things work completely seamlessly, though.

I agree that in 32-bit mode gdb should present a 32-bit view of the
world to the user.
This needs to work in the absence of debug info, thus there needs to
be some protocol extension to provide this information (attaching it
to the stop notification is one way, though that'd be insufficient I
think).

In the general case (i.e., to handle all arches) it'd be insufficient
to rely on being able to just use the 64-bit wire format, but until
gdb can support arch-switching as needed here, it allows things to
work that previously caused things like "'g' packet too long" and
totally broke the user's debugging session.

I also agree this is a fair bit of work.

Re: [Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Paolo Bonzini]

On 04/11/2016 00:35, Doug Evans wrote:
> The remote protocol can't handle flipping back and forth
> between 32-bit and 64-bit regs. To compensate, pretend "as if"
> on 64-bit cpu when in 32-bit mode.
> 
> Signed-off-by: Doug Evans <dje@google.com>
> ---
>  target-i386/gdbstub.c | 52
> ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/target-i386/gdbstub.c b/target-i386/gdbstub.c
> index c494535..9b94ab8 100644
> --- a/target-i386/gdbstub.c
> +++ b/target-i386/gdbstub.c
> @@ -44,10 +44,22 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t
> *mem_buf, int n)
>      X86CPU *cpu = X86_CPU(cs);
>      CPUX86State *env = &cpu->env;
> 
> +    /* N.B. GDB can't deal with changes in registers or sizes in the
> middle
> +       of a session. So if we're in 32-bit mode on a 64-bit cpu, still act
> +       as if we're on a 64-bit cpu. */
> +
>      if (n < CPU_NB_REGS) {
> -        if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
> -            return gdb_get_reg64(mem_buf, env->regs[gpr_map[n]]);
> -        } else if (n < CPU_NB_REGS32) {
> +        if (TARGET_LONG_BITS == 64) {
> +            if (env->hflags & HF_CS64_MASK) {
> +                return gdb_get_reg64(mem_buf, env->regs[gpr_map[n]]);
> +            } else if (n < CPU_NB_REGS32) {
> +                return gdb_get_reg64(mem_buf,
> +                                     env->regs[gpr_map[n]] &
> 0xffffffffUL);
> +            } else {
> +                memset(mem_buf, 0, sizeof(target_ulong));
> +                return sizeof(target_ulong);
> +            }
> +        } else {
>              return gdb_get_reg32(mem_buf, env->regs[gpr_map32[n]]);
>          }
>      } else if (n >= IDX_FP_REGS && n < IDX_FP_REGS + 8) {
> @@ -60,8 +72,7 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t
> *mem_buf, int n)
>          return 10;
>      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>          n -= IDX_XMM_REGS;
> -        if (n < CPU_NB_REGS32 ||
> -            (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK)) {
> +        if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
>              stq_p(mem_buf, env->xmm_regs[n].ZMM_Q(0));
>              stq_p(mem_buf + 8, env->xmm_regs[n].ZMM_Q(1));
>              return 16;
> @@ -69,8 +80,12 @@ int x86_cpu_gdb_read_register(CPUState *cs, uint8_t
> *mem_buf, int n)
>      } else {
>          switch (n) {
>          case IDX_IP_REG:
> -            if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
> -                return gdb_get_reg64(mem_buf, env->eip);
> +            if (TARGET_LONG_BITS == 64) {
> +                if (env->hflags & HF_CS64_MASK) {
> +                    return gdb_get_reg64(mem_buf, env->eip);
> +                } else {
> +                    return gdb_get_reg64(mem_buf, env->eip &
> 0xffffffffUL);
> +                }
>              } else {
>                  return gdb_get_reg32(mem_buf, env->eip);
>              }
> @@ -151,9 +166,17 @@ int x86_cpu_gdb_write_register(CPUState *cs,
> uint8_t *mem_buf, int n)
>      CPUX86State *env = &cpu->env;
>      uint32_t tmp;
> 
> +    /* N.B. GDB can't deal with changes in registers or sizes in the
> middle
> +       of a session. So if we're in 32-bit mode on a 64-bit cpu, still act
> +       as if we're on a 64-bit cpu. */
> +
>      if (n < CPU_NB_REGS) {
> -        if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
> -            env->regs[gpr_map[n]] = ldtul_p(mem_buf);
> +        if (TARGET_LONG_BITS == 64) {
> +            if (env->hflags & HF_CS64_MASK) {
> +                env->regs[gpr_map[n]] = ldtul_p(mem_buf);
> +            } else if (n < CPU_NB_REGS32) {
> +                env->regs[gpr_map[n]] = ldtul_p(mem_buf) & 0xffffffffUL;
> +            }
>              return sizeof(target_ulong);
>          } else if (n < CPU_NB_REGS32) {
>              n = gpr_map32[n];
> @@ -169,8 +192,7 @@ int x86_cpu_gdb_write_register(CPUState *cs, uint8_t
> *mem_buf, int n)
>          return 10;
>      } else if (n >= IDX_XMM_REGS && n < IDX_XMM_REGS + CPU_NB_REGS) {
>          n -= IDX_XMM_REGS;
> -        if (n < CPU_NB_REGS32 ||
> -            (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK)) {
> +        if (n < CPU_NB_REGS32 || TARGET_LONG_BITS == 64) {
>              env->xmm_regs[n].ZMM_Q(0) = ldq_p(mem_buf);
>              env->xmm_regs[n].ZMM_Q(1) = ldq_p(mem_buf + 8);
>              return 16;
> @@ -178,8 +200,12 @@ int x86_cpu_gdb_write_register(CPUState *cs,
> uint8_t *mem_buf, int n)
>      } else {
>          switch (n) {
>          case IDX_IP_REG:
> -            if (TARGET_LONG_BITS == 64 && env->hflags & HF_CS64_MASK) {
> -                env->eip = ldq_p(mem_buf);
> +            if (TARGET_LONG_BITS == 64) {
> +                if (env->hflags & HF_CS64_MASK) {
> +                    env->eip = ldq_p(mem_buf);
> +                } else {
> +                    env->eip = ldq_p(mem_buf) & 0xffffffffUL;
> +                }
>                  return 8;
>              } else {
>                  env->eip &= ~0xffffffffUL;

Queued for 2.9, thanks.

Paolo

Re: [Qemu-devel] [PATCH] x86: Fix x86_64 'g' packet response to gdb from 32-bit mode.

[Doug Evans]

On Wed, Dec 14, 2016 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 04/11/2016 00:35, Doug Evans wrote:
>> The remote protocol can't handle flipping back and forth
>> between 32-bit and 64-bit regs. To compensate, pretend "as if"
>> on 64-bit cpu when in 32-bit mode.
>>
>> Signed-off-by: Doug Evans <dje@google.com>
>> ---
>>  target-i386/gdbstub.c | 52
>> ++++++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 39 insertions(+), 13 deletions(-)
>> ...
>
> Queued for 2.9, thanks.

Awesome! Thanks very much!