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

[Fabien Chouteau]

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?

>>>> +#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?


>>>> +
>>>> +/* 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.


> 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.

>
>>> 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.
>
> No, if the fault happens here, handling it may be tricky. See for
> example what paranoia Linux has to do for user window flushing, it
> involves the no-fault mode in MMU.

There's no possible fault, as we do not read or write in memory, that's the
point of this implementation.

>
>>> 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.
>
> Yes, but the flusher should be GDB, assuming that flushing is even a
> good idea which I doubt.
>
> Back traces are not reliable in any case. The code could be compiled
> to omit the frame pointer.

This is a corner case. And again, something not supported by Gdb.


>> In Qemu we can avoid
>> flushing with this little trick.
>
> This doesn't avoid flushing but performs it magically during GDB memory 
> access.
>

...instead of writing and reading all register windows each time Gdb stops Qemu.
That's what I call "avoid flushing".

Regards,

-- 
Fabien Chouteau