| Message ID | 1376244860-19714-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
Hi Aneesh, Am 11.08.2013 20:14, schrieb Aneesh Kumar K.V: > From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> > > Don't update the global register count if not requested. > Without this patch a remote gdb session gives > > (gdb) target remote localhost:1234 > Remote debugging using localhost:1234 > Remote 'g' packet reply is too long: > 0000000028000084c000000000ccba50c000000000c ... > .... > ... > (gdb) > > This is a regression introduce by a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 > > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> 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? I understand it is a revert to using the static variable, updated to using the CPUClass field rather than the previous preprocessor constant. > --- > gdbstub.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > 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 = CPU_GET_CLASS(cpu); > + > + if (!last_reg) { > + last_reg = cc->gdb_num_core_regs; > + } > > p = &cpu->gdb_regs; > while (*p) { > @@ -608,19 +614,21 @@ void gdb_register_coprocessor(CPUState *cpu, > } > > s = g_new0(GDBRegisterState, 1); > - s->base_reg = cpu->gdb_num_regs; > + s->base_reg = last_reg; > s->num_regs = num_regs; > s->get_reg = get_reg; > s->set_reg = set_reg; > s->xml = xml; > > /* Add to end of list. */ > - cpu->gdb_num_regs += num_regs; > + last_reg += num_regs; > *p = s; > if (g_pos) { > if (g_pos != 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 = 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? Regards, Andreas > } > } > } >
Andreas Färber <afaerber@suse.de> writes: > Hi Aneesh, > > Am 11.08.2013 20:14, schrieb Aneesh Kumar K.V: >> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> >> >> Don't update the global register count if not requested. >> Without this patch a remote gdb session gives >> >> (gdb) target remote localhost:1234 >> Remote debugging using localhost:1234 >> Remote 'g' packet reply is too long: >> 0000000028000084c000000000ccba50c000000000c ... >> .... >> ... >> (gdb) >> >> This is a regression introduce by a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 >> >> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> > > 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 == 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(-) >> >> 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 = CPU_GET_CLASS(cpu); >> + >> + if (!last_reg) { >> + last_reg = cc->gdb_num_core_regs; >> + } >> >> p = &cpu->gdb_regs; >> while (*p) { >> @@ -608,19 +614,21 @@ void gdb_register_coprocessor(CPUState *cpu, >> } >> >> s = g_new0(GDBRegisterState, 1); >> - s->base_reg = cpu->gdb_num_regs; >> + s->base_reg = last_reg; >> s->num_regs = num_regs; >> s->get_reg = get_reg; >> s->set_reg = set_reg; >> s->xml = xml; >> >> /* Add to end of list. */ >> - cpu->gdb_num_regs += num_regs; >> + last_reg += num_regs; >> *p = s; >> if (g_pos) { >> if (g_pos != 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 = 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 = 0; addr < num_g_regs && len > 0; addr++) { + for (addr = 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, } s = g_new0(GDBRegisterState, 1); - s->base_reg = last_reg; + s->base_reg = cpu->gdb_num_regs; s->num_regs = num_regs; s->get_reg = get_reg; s->set_reg = set_reg; s->xml = xml; /* Add to end of list. */ - last_reg += num_regs; + cpu->gdb_num_regs += num_regs; *p = s; if (g_pos) { if (g_pos != 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 = last_reg; } } }
Am 12.08.2013 11:17, schrieb Aneesh Kumar K.V: > Andreas Färber <afaerber@suse.de> writes: >> Am 11.08.2013 20:14, schrieb Aneesh Kumar K.V: >>> From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com> >>> >>> Don't update the global register count if not requested. >>> Without this patch a remote gdb session gives >>> >>> (gdb) target remote localhost:1234 >>> Remote debugging using localhost:1234 >>> Remote 'g' packet reply is too long: >>> 0000000028000084c000000000ccba50c000000000c ... >>> .... >>> ... >>> (gdb) >>> >>> This is a regression introduce by a0e372f0c49ac01faeaeb73a6e8f50e8ac615f34 >>> >>> Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com> >> >> 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 == 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. While it's always unfortunate to have a known breakage, we decided it's too late/risky to address this for -rc3 today. I put together a different patch that hopefully fixes the breakage while avoiding to revert to static variables: http://patchwork.ozlabs.org/patch/266594/ If this fixes things for you and doesn't break other things, we can get it into qemu.git after Thursday [1] and backport it into 1.6.1. Regards, Andreas [1] http://wiki.qemu.org/Planning/1.6
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 = CPU_GET_CLASS(cpu); + + if (!last_reg) { + last_reg = cc->gdb_num_core_regs; + } p = &cpu->gdb_regs; while (*p) { @@ -608,19 +614,21 @@ void gdb_register_coprocessor(CPUState *cpu, } s = g_new0(GDBRegisterState, 1); - s->base_reg = cpu->gdb_num_regs; + s->base_reg = last_reg; s->num_regs = num_regs; s->get_reg = get_reg; s->set_reg = set_reg; s->xml = xml; /* Add to end of list. */ - cpu->gdb_num_regs += num_regs; + last_reg += num_regs; *p = s; if (g_pos) { if (g_pos != 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 = last_reg; } } }