From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35912) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0ekm-00016p-Bh for qemu-devel@nongnu.org; Mon, 05 Sep 2011 15:22:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R0ekl-0002Sg-0G for qemu-devel@nongnu.org; Mon, 05 Sep 2011 15:22:44 -0400 Received: from mail-qy0-f173.google.com ([209.85.216.173]:56443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0ekk-0002Sa-SF for qemu-devel@nongnu.org; Mon, 05 Sep 2011 15:22:42 -0400 Received: by qyk31 with SMTP id 31so89668qyk.4 for ; Mon, 05 Sep 2011 12:22:42 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <4E64976D.2050405@adacore.com> References: <1314886646-10714-1-git-send-email-chouteau@adacore.com> <4E64976D.2050405@adacore.com> From: Blue Swirl Date: Mon, 5 Sep 2011 19:22:22 +0000 Message-ID: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] [SPARC] Gdbstub: Fix back-trace on SPARC32 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Fabien Chouteau Cc: qemu-devel@nongnu.org On Mon, Sep 5, 2011 at 9:33 AM, Fabien Chouteau wrot= e: > On 03/09/2011 11:25, Blue Swirl wrote: >> On Thu, Sep 1, 2011 at 2:17 PM, Fabien Chouteau w= rote: >>> Gdb expects all registers windows to be flushed in ram, which is not th= e case >>> in Qemu. Therefore the back-trace generation doesn't work. This patch a= dds a >>> function to handle reads/writes in stack frames as if windows were flus= hed. >>> >>> Signed-off-by: Fabien Chouteau >>> --- >>> =C2=A0gdbstub.c =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | =C2=A0 10 += +++-- >>> =C2=A0target-sparc/cpu.h =C2=A0 =C2=A0| =C2=A0 =C2=A07 ++++ >>> =C2=A0target-sparc/helper.c | =C2=A0 85 +++++++++++++++++++++++++++++++= ++++++++++++++++++ >>> =C2=A03 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 @@ >>> =C2=A0#include "qemu_socket.h" >>> =C2=A0#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. >>> +#endif >>> >>> =C2=A0enum { >>> =C2=A0 =C2=A0 GDB_SIGNAL_0 =3D 0, >>> @@ -2013,7 +2016,7 @@ static int gdb_handle_packet(GDBState *s, const c= har *line_buf) >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*p =3D=3D ',') >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 p++; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 len =3D strtoull(p, NULL, 16); >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (cpu_memory_rw_debug(s->g_cpu, addr, me= m_buf, len, 0) !=3D 0) { >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (TARGET_CPU_MEMORY_RW_DEBUG(s->g_cpu, a= ddr, mem_buf, len, 0) !=3D 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. >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 put_packet (s, "E14"); >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 } else { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 memtohex(buf, mem_buf, len); >>> @@ -2028,10 +2031,11 @@ static int gdb_handle_packet(GDBState *s, const= char *line_buf) >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*p =3D=3D ':') >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 p++; >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 hextomem(mem_buf, p, len); >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0if (cpu_memory_rw_debug(s->g_cpu, addr, me= m_buf, len, 1) !=3D 0) >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0if (TARGET_CPU_MEMORY_RW_DEBUG(s->g_cpu, a= ddr, mem_buf, len, 1) !=3D 0) { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 put_packet(s, "E14"); >>> - =C2=A0 =C2=A0 =C2=A0 =C2=A0else >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} else { >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 put_packet(s, "OK"); >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0} >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 break; >>> =C2=A0 =C2=A0 case 'p': >>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* 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 >>> =C2=A0target_ulong mmu_probe(CPUSPARCState *env, target_ulong address, = int mmulev); >>> =C2=A0void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUState *en= v); >>> >>> +#if !defined(TARGET_SPARC64) >>> +int sparc_cpu_memory_rw_debug(CPUState *env, target_ulong addr, >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0uint8_t *buf, int len, int is_write); >>> +#define TARGET_CPU_MEMORY_RW_DEBUG sparc_cpu_memory_rw_debug >>> +#endif >>> + >>> + >>> =C2=A0/* translate.c */ >>> =C2=A0void 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_fprint= f, CPUState *env) >>> =C2=A0 =C2=A0 } >>> =C2=A0} >>> >>> + >>> +/* Gdb expects all registers windows to be flushed in ram. This functi= on 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 val= id > 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. 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. >> 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 regi= sters. 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. >> 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. > In Qemu we can avoid > flushing with this little trick. This doesn't avoid flushing but performs it magically during GDB memory acc= ess.