Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32

[Fabien Chouteau]

On 03/09/2011 11:25, Blue Swirl wrote:
> On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau <address@hidden> wrote:
>> Gdb expects all registers windows to be flushed in ram, which is not the case
>> in Qemu. Therefore the back-trace generation doesn't work. This patch adds a
>> function to handle reads/writes in stack frames as if windows were flushed.
>>
>> Signed-off-by: Fabien Chouteau <address@hidden>
>> ---
>>  gdbstub.c             |   10 ++++--
>>  target-sparc/cpu.h    |    7 ++++
>>  target-sparc/helper.c |   85 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 99 insertions(+), 3 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 3b87c27..85d5ad7 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -41,6 +41,9 @@
>>  #include "qemu_socket.h"
>>  #include "kvm.h"
>>
>> +#ifndef TARGET_CPU_MEMORY_RW_DEBUG
>> +#define TARGET_CPU_MEMORY_RW_DEBUG cpu_memory_rw_debug
>
> These days, inline functions are preferred over macros.
>

This is to allow target-specific implementation of the function.

>> +#endif
>>
>>  enum {
>>     GDB_SIGNAL_0 = 0,
>> @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const char 
>> *line_buf)
>>         if (*p == ',')
>>             p++;
>>         len = strtoull(p, NULL, 16);
>> -        if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 0) != 0) {
>> +        if (TARGET_CPU_MEMORY_RW_DEBUG(s->g_cpu, addr, mem_buf, len, 0) != 
>> 0) {
>
> cpu_memory_rw_debug() could remain unwrapped with a generic function
> like cpu_gdb_sync_memory() which gdbstub should explicitly call.
>
> Maybe the lazy condition codes etc. could be handled in similar way,
> cpu_gdb_sync_registers().
>

Excuse me, I don't understand here.

>>             put_packet (s, "E14");
>>         } else {
>>             memtohex(buf, mem_buf, len);
>> @@ -2028,10 +2031,11 @@ static int gdb_handle_packet(GDBState *s, const char 
>> *line_buf)
>>         if (*p == ':')
>>             p++;
>>         hextomem(mem_buf, p, len);
>> -        if (cpu_memory_rw_debug(s->g_cpu, addr, mem_buf, len, 1) != 0)
>> +        if (TARGET_CPU_MEMORY_RW_DEBUG(s->g_cpu, addr, mem_buf, len, 1) != 
>> 0) {
>>             put_packet(s, "E14");
>> -        else
>> +        } else {
>>             put_packet(s, "OK");
>> +        }
>>         break;
>>     case 'p':
>>         /* Older gdb are really dumb, and don't use 'g' if 'p' is avaialable.
>> diff --git a/target-sparc/cpu.h b/target-sparc/cpu.h
>> index 8654f26..3f76eaf 100644
>> --- a/target-sparc/cpu.h
>> +++ b/target-sparc/cpu.h
>> @@ -495,6 +495,13 @@ int cpu_sparc_handle_mmu_fault(CPUSPARCState *env1, 
>> target_ulong address, int rw
>>  target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, int 
>> mmulev);
>>  void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *env);
>>
>> +#if !defined(TARGET_SPARC64)
>> +int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr,
>> +                              uint8_t *buf, int len, int is_write);
>> +#define TARGET_CPU_MEMORY_RW_DEBUG sparc_cpu_memory_rw_debug
>> +#endif
>> +
>> +
>>  /* translate.c */
>>  void gen_intermediate_code_init(CPUSPARCState *env);
>>
>> diff --git a/target-sparc/helper.c b/target-sparc/helper.c
>> index 1fe1f07..2cf4e8b 100644
>> --- a/target-sparc/helper.c
>> +++ b/target-sparc/helper.c
>> @@ -358,6 +358,91 @@ void dump_mmu(FILE *f, fprintf_function cpu_fprintf, 
>> CPUState *env)
>>     }
>>  }
>>
>> +
>> +/* Gdb expects all registers windows to be flushed in ram. This function 
>> handles
>> + * reads/writes in stack frames as if windows were flushed. We assume that 
>> the
>> + * sparc ABI is followed.
>> + */
>
> We can't assume that, it depends on what we are executing (BIOS, OS,
> even application).

Well, maybe the statement is too strong. The ABI is required to get a valid
result. Gdb cannot build back-traces if the ABI is not followed anyway.

> On Sparc64 there are two ABIs (32 bit and 64 bit
> with offset of -2047), though calling flushw instruction could handle
> that.

This solution is for SPARC32 only.

> If the flush happens to trigger a fault, we're in big trouble.
>

That's why it's safer/easier to use this "hackish" read/write in the registers.

> Overall, I think this is too hackish. Maybe this is a bug in GDB
> instead, information from backtrace is not reliable if ABI is not
> known.
>

It's not a bug in Gdb. To build back-traces you have to read stack frames. To
read stack frames, register windows must be flushed. In Qemu we can avoid
flushing with this little trick.

Regards,

-- 
Fabien Chouteau