From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35759) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8oGU-0001DE-5L for qemu-devel@nongnu.org; Mon, 12 Aug 2013 05:18:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V8oGM-000269-Cs for qemu-devel@nongnu.org; Mon, 12 Aug 2013 05:18:14 -0400 Received: from e28smtp02.in.ibm.com ([122.248.162.2]:33835) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V8oGL-00025U-NT for qemu-devel@nongnu.org; Mon, 12 Aug 2013 05:18:06 -0400 Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 12 Aug 2013 14:38:13 +0530 Received: from d28relay01.in.ibm.com (d28relay01.in.ibm.com [9.184.220.58]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id CF5E5E004F for ; Mon, 12 Aug 2013 14:48:18 +0530 (IST) Received: from d28av02.in.ibm.com (d28av02.in.ibm.com [9.184.220.64]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r7C9JFkY9306168 for ; Mon, 12 Aug 2013 14:49:16 +0530 Received: from d28av02.in.ibm.com (localhost [127.0.0.1]) by d28av02.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r7C9Huox031816 for ; Mon, 12 Aug 2013 14:47:57 +0530 From: "Aneesh Kumar K.V" In-Reply-To: <5208A169.4010109@suse.de> References: <1376244860-19714-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <5208A169.4010109@suse.de> Date: Mon, 12 Aug 2013 14:47:53 +0530 Message-ID: <87bo53b73y.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] gdb: Fix gdb error List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?utf-8?Q?F=C3=A4rber?= Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org Andreas F=C3=A4rber writes: > Hi Aneesh, > > Am 11.08.2013 20:14, schrieb Aneesh Kumar K.V: >> From: "Aneesh Kumar K.V" >>=20 >> Don't update the global register count if not requested. >> Without this patch a remote gdb session gives >>=20 >> (gdb) target remote localhost:1234 >> Remote debugging using localhost:1234 >> Remote 'g' packet reply is too long: >> 0000000028000084c000000000ccba50c000000000c ... >> .... >> ... >> (gdb) >>=20 >> This is a regression introduce by a0e372f0c49ac01faeaeb73a6e8f50e8ac615f= 34 >>=20 >> Signed-off-by: Aneesh Kumar K.V > > Thanks for tracking this down. I'm willing to include a variation in > today's pull to fix 1.6.0-rc3. However, did you find an explanation > *why* it needs to be like this? IIUC our reply packet for 'g' contain more data becaue we ended up with larger cpu->gdb_num_regs. This only happens for archs that do a gdb_register_coprocessor with gpos =3D=3D 0. The older code didn't update num_g_regs in that case. Not sure why we do like that > I understand it is a revert to using the > static variable, updated to using the CPUClass field rather than the > previous preprocessor constant. > I don't really like the patch. But I also don't know enough to fix this without using the static variable. If you want me to try another version please send it across. I can easily reproduce this on PowerPC. >> --- >> gdbstub.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >>=20 >> diff --git a/gdbstub.c b/gdbstub.c >> index 1af25a6..4b58a1e 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -598,6 +598,12 @@ void gdb_register_coprocessor(CPUState *cpu, >> { >> GDBRegisterState *s; >> GDBRegisterState **p; >> + static int last_reg; >> + CPUClass *cc =3D CPU_GET_CLASS(cpu); >> + >> + if (!last_reg) { >> + last_reg =3D cc->gdb_num_core_regs; >> + } >>=20=20 >> p =3D &cpu->gdb_regs; >> while (*p) { >> @@ -608,19 +614,21 @@ void gdb_register_coprocessor(CPUState *cpu, >> } >>=20=20 >> s =3D g_new0(GDBRegisterState, 1); >> - s->base_reg =3D cpu->gdb_num_regs; >> + s->base_reg =3D last_reg; >> s->num_regs =3D num_regs; >> s->get_reg =3D get_reg; >> s->set_reg =3D set_reg; >> s->xml =3D xml; >>=20=20 >> /* Add to end of list. */ >> - cpu->gdb_num_regs +=3D num_regs; >> + last_reg +=3D num_regs; >> *p =3D s; >> if (g_pos) { >> if (g_pos !=3D s->base_reg) { >> fprintf(stderr, "Error: Bad gdb register numbering for '%s'= \n" >> "Expected %d got %d\n", xml, g_pos, s->base_reg); > >> + } else { >> + cpu->gdb_num_regs =3D last_reg; > > This bit looks wrong to me - it is updating the per-CPU count with the > global value. Could you retest without this please? > We loop with cpu->gdb_num_regs as below in gdb_handle_packet. - for (addr =3D 0; addr < num_g_regs && len > 0; addr++) { + for (addr =3D 0; addr < s->g_cpu->gdb_num_regs && len > 0; addr++)= { We updated num_g_regs if g_pos is not set before a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 @@ -2036,25 +2003,22 @@ void gdb_register_coprocessor(CPUState *cpu, } =20 s =3D g_new0(GDBRegisterState, 1); - s->base_reg =3D last_reg; + s->base_reg =3D cpu->gdb_num_regs; s->num_regs =3D num_regs; s->get_reg =3D get_reg; s->set_reg =3D set_reg; s->xml =3D xml; =20 /* Add to end of list. */ - last_reg +=3D num_regs; + cpu->gdb_num_regs +=3D num_regs; *p =3D s; if (g_pos) { if (g_pos !=3D s->base_reg) { fprintf(stderr, "Error: Bad gdb register numbering for '%s'\n" "Expected %d got %d\n", xml, g_pos, s->base_reg); - } else { - num_g_regs =3D last_reg; } } }