| Message ID | 20231011103329.670525-20-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/25] tests/avocado: update firmware to enable OpenBSD test on sbsa-ref | expand |
Alex Bennée <alex.bennee@linaro.org> writes: > From: Akihiko Odaki <akihiko.odaki@daynix.com> > > An array is a more appropriate data structure than a list for gdb_regs > since it is initialized only with append operation and read-only after > initialization. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com> > Message-Id: <20230912224107.29669-13-akihiko.odaki@daynix.com> > [AJB: fixed a checkpatch violation] > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Message-Id: <20231009164104.369749-20-alex.bennee@linaro.org> > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 7b8347ed5a..3968369554 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -502,7 +502,7 @@ struct CPUState { > > CPUJumpCache *tb_jmp_cache; > > - struct GDBRegisterState *gdb_regs; > + GArray *gdb_regs; > int gdb_num_regs; > int gdb_num_g_regs; > QTAILQ_ENTRY(CPUState) node; > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c > index 62608a5389..b1532118d1 100644 > --- a/gdbstub/gdbstub.c > +++ b/gdbstub/gdbstub.c > @@ -51,7 +51,6 @@ typedef struct GDBRegisterState { > gdb_get_reg_cb get_reg; > gdb_set_reg_cb set_reg; > const char *xml; > - struct GDBRegisterState *next; > } GDBRegisterState; > > GDBState gdbserver_state; > @@ -386,7 +385,8 @@ static const char *get_feature_xml(const char *p, const char **newp, > xml, > g_markup_printf_escaped("<xi:include href=\"%s\"/>", > cc->gdb_core_xml_file)); > - for (r = cpu->gdb_regs; r; r = r->next) { > + for (guint i = 0; i < cpu->gdb_regs->len; i++) { It seems we can reach here before having initialized gdb_regs at gdb_register_coprocessor(): Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault. 0x0000555555e5310b in get_feature_xml (p=0x555556a99118 <gdbserver_state+56> "target.xml:0,ffb", newp=0x7fffffffc6b0, process=0x555557a21dd0) at ../gdbstub/gdbstub.c:388 (gdb) p/x cpu->gdb_regs $1 = 0x0 Using: qemu-system-x86 ... -s -s just connect GDB and it crashes.
On 2023/10/17 23:05, Fabiano Rosas wrote: > Alex Bennée <alex.bennee@linaro.org> writes: > >> From: Akihiko Odaki <akihiko.odaki@daynix.com> >> >> An array is a more appropriate data structure than a list for gdb_regs >> since it is initialized only with append operation and read-only after >> initialization. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> Reviewed-by: Alistair Francis <alistair.francis@wdc.com> >> Message-Id: <20230912224107.29669-13-akihiko.odaki@daynix.com> >> [AJB: fixed a checkpatch violation] >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Message-Id: <20231009164104.369749-20-alex.bennee@linaro.org> >> >> diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h >> index 7b8347ed5a..3968369554 100644 >> --- a/include/hw/core/cpu.h >> +++ b/include/hw/core/cpu.h >> @@ -502,7 +502,7 @@ struct CPUState { >> >> CPUJumpCache *tb_jmp_cache; >> >> - struct GDBRegisterState *gdb_regs; >> + GArray *gdb_regs; >> int gdb_num_regs; >> int gdb_num_g_regs; >> QTAILQ_ENTRY(CPUState) node; >> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c >> index 62608a5389..b1532118d1 100644 >> --- a/gdbstub/gdbstub.c >> +++ b/gdbstub/gdbstub.c >> @@ -51,7 +51,6 @@ typedef struct GDBRegisterState { >> gdb_get_reg_cb get_reg; >> gdb_set_reg_cb set_reg; >> const char *xml; >> - struct GDBRegisterState *next; >> } GDBRegisterState; >> >> GDBState gdbserver_state; >> @@ -386,7 +385,8 @@ static const char *get_feature_xml(const char *p, const char **newp, >> xml, >> g_markup_printf_escaped("<xi:include href=\"%s\"/>", >> cc->gdb_core_xml_file)); >> - for (r = cpu->gdb_regs; r; r = r->next) { >> + for (guint i = 0; i < cpu->gdb_regs->len; i++) { > > It seems we can reach here before having initialized gdb_regs at > gdb_register_coprocessor(): > > Thread 1 "qemu-system-x86" received signal SIGSEGV, Segmentation fault. > 0x0000555555e5310b in get_feature_xml (p=0x555556a99118 > <gdbserver_state+56> "target.xml:0,ffb", newp=0x7fffffffc6b0, > process=0x555557a21dd0) at ../gdbstub/gdbstub.c:388 > > (gdb) p/x cpu->gdb_regs > $1 = 0x0 > > > Using: > qemu-system-x86 ... -s -s > > just connect GDB and it crashes. Hi, Sorry for trouble and thank you for reporting. I have just posted a fix "[PATCH v4 1/5] gdbstub: Check if gdb_regs is NULL" as part of series "[PATCH v4 0/5] gdbstub and TCG plugin improvements". Please test it if possible. Alex, you may pick the patch early since the bug is quite a serious. Please add "Reported-by: Fabiano Rosas <farosas@suse.de>" when you do so since I forgot it. You may skip applying target/riscv patches though since the maintainers may still have something to comment. Regards, Akihiko Odaki
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 7b8347ed5a..3968369554 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -502,7 +502,7 @@ struct CPUState { CPUJumpCache *tb_jmp_cache; - struct GDBRegisterState *gdb_regs; + GArray *gdb_regs; int gdb_num_regs; int gdb_num_g_regs; QTAILQ_ENTRY(CPUState) node; diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index 62608a5389..b1532118d1 100644 --- a/gdbstub/gdbstub.c +++ b/gdbstub/gdbstub.c @@ -51,7 +51,6 @@ typedef struct GDBRegisterState { gdb_get_reg_cb get_reg; gdb_set_reg_cb set_reg; const char *xml; - struct GDBRegisterState *next; } GDBRegisterState; GDBState gdbserver_state; @@ -386,7 +385,8 @@ static const char *get_feature_xml(const char *p, const char **newp, xml, g_markup_printf_escaped("<xi:include href=\"%s\"/>", cc->gdb_core_xml_file)); - for (r = cpu->gdb_regs; r; r = r->next) { + for (guint i = 0; i < cpu->gdb_regs->len; i++) { + r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i); g_ptr_array_add( xml, g_markup_printf_escaped("<xi:include href=\"%s\"/>", @@ -430,7 +430,8 @@ static int gdb_read_register(CPUState *cpu, GByteArray *buf, int reg) return cc->gdb_read_register(cpu, buf, reg); } - for (r = cpu->gdb_regs; r; r = r->next) { + for (guint i = 0; i < cpu->gdb_regs->len; i++) { + r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i); if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) { return r->get_reg(env, buf, reg - r->base_reg); } @@ -448,7 +449,8 @@ static int gdb_write_register(CPUState *cpu, uint8_t *mem_buf, int reg) return cc->gdb_write_register(cpu, mem_buf, reg); } - for (r = cpu->gdb_regs; r; r = r->next) { + for (guint i = 0; i < cpu->gdb_regs->len; i++) { + r = &g_array_index(cpu->gdb_regs, GDBRegisterState, i); if (r->base_reg <= reg && reg < r->base_reg + r->num_regs) { return r->set_reg(env, mem_buf, reg - r->base_reg); } @@ -461,17 +463,23 @@ void gdb_register_coprocessor(CPUState *cpu, int num_regs, const char *xml, int g_pos) { GDBRegisterState *s; - GDBRegisterState **p; - - p = &cpu->gdb_regs; - while (*p) { - /* Check for duplicates. */ - if (strcmp((*p)->xml, xml) == 0) - return; - p = &(*p)->next; + guint i; + + if (cpu->gdb_regs) { + for (i = 0; i < cpu->gdb_regs->len; i++) { + /* Check for duplicates. */ + s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i); + if (strcmp(s->xml, xml) == 0) { + return; + } + } + } else { + cpu->gdb_regs = g_array_new(false, false, sizeof(GDBRegisterState)); + i = 0; } - s = g_new0(GDBRegisterState, 1); + g_array_set_size(cpu->gdb_regs, i + 1); + s = &g_array_index(cpu->gdb_regs, GDBRegisterState, i); s->base_reg = cpu->gdb_num_regs; s->num_regs = num_regs; s->get_reg = get_reg; @@ -480,7 +488,6 @@ void gdb_register_coprocessor(CPUState *cpu, /* Add to end of list. */ cpu->gdb_num_regs += num_regs; - *p = s; if (g_pos) { if (g_pos != s->base_reg) { error_report("Error: Bad gdb register numbering for '%s', "