Andreas Färber <afaer...@suse.de> writes: > Hi Aneesh, > > Am 11.08.2013 20:14, schrieb Aneesh Kumar K.V: >> From: "Aneesh Kumar K.V" <aneesh.ku...@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.ku...@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; } } }