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

[Fabien Chouteau]

On 07/09/2011 21:02, Blue Swirl wrote:
> On Tue, Sep 6, 2011 at 10:38 AM, Fabien Chouteau <address@hidden> wrote:
>> On 05/09/2011 21:22, Blue Swirl wrote:
>>> On Mon, Sep 5, 2011 at 9:33 AM, Fabien Chouteau <address@hidden> wrote:
>>>> 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.
>>>
>>> That can be done with inline functions too.
>>
>> OK, how do you do that?
> 
> #ifndef TARGET_CPU_MEMORY_RW_DEBUG
> int target_memory_rw_debug(CPUState *env, target_ulong addr,
>                               uint8_t *buf, int len, int is_write)
> {
>     return cpu_memory_rw_debug(env, addr, buf, len, is_write);
> }
> #else
> /* target_memory_rw_debug() defined in cpu.h */
> #endif
> 

OK, understood.


>>>>>> +#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.
>>>
>>> cpu_gdb_{read,write}_register needs to force calculation of lazy
>>> condition codes. On Sparc this is handled by cpu_get_psr(), so it is
>>> not explicit.
>>
>> I still don't understand you point. Do you suggest a cpu_gdb_sync_memory() 
>> that
>> will flush register windows?
>
> Not really but nevermind.
>
>>>>>> +
>>>>>> +/* 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.
>>>
>>> But if the ABI assumption happens to be wrong (for example registers
>>> contain random values), memory may be corrupted because this would
>>> happily use whatever the registers contain.
>>
>> This cannot corrupt memory, the point is to read/write in registers instead 
>> of
>> memory.
>
> Sorry, I misread a part of the patch, guest memory is not written
> unlike I mistakenly assumed (simple register to memory flush).
> However, wrong ABI assumption may instead corrupt the registers.
>
>>> Another way to fix this would be that GDB would tell QEMU what ABI to
>>> use for flushing. But how would one tell GDB about a non-standard ABI?
>>>
>>> For user emulators we can make ABI assumptions, there similar patch
>>> could make sense. But system emulators can't assume anything about the
>>> guest OS, it could be Linux, *BSD, a commercial OS or even a toy OS.
>>
>> I think all of these kernels follow the SPARC32 ABI, and if they don't Gdb
>> cannot handle them anyway.
>>
>> This solution covers 99% of the problem.
>
> As is, it's not 100% correct and the failure case is destructive. But
> would it make sense if the registers were not touched on write? Then
> to GDB the windows would appear as if flushed to memory, but like real
> hardware the registers would not automatically get updated from memory
> if it's changed by GDB. I don't think corruption would be possible in
> that case, though GDB (or the user) could get temporarily confused if
> a read from memory location would not return its true value.
>

I think this might be the best compromise. So I'll just handle reads in
register windows.

> BTW, cpu_cwp_inc() is called but there is no effort to restore CWP afterward.
>

The CWP in CPUState is never modified by cpu_cpw_inc().

Version 2 is on its way...

Regards,

-- 
Fabien Chouteau