| Message ID | 6414ff4730fb53bd210cce947c201ca011135831.1681993775.git.quic_mathbern@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hexagon: add lldb support | expand |
Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes: > From: Brian Cain <bcain@quicinc.com> > > Signed-off-by: Brian Cain <bcain@quicinc.com> > Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > --- > include/hw/core/cpu.h | 4 ++++ > gdbstub/gdbstub.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 397fd3ac68..cfdf5514d9 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -124,6 +124,8 @@ struct SysemuCPUOps; > * its Harvard architecture split code and data. > * @gdb_num_core_regs: Number of core registers accessible to GDB. > * @gdb_core_xml_file: File name for core registers GDB XML description. > + * @gdb_qreg_info_lines: Array of lines of registers qRegisterInfo description. > + * @gdb_qreg_info_line_count: Count of lines for @gdb_qreg_info_lines. > * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop > * before the insn which triggers a watchpoint rather than after it. > * @gdb_arch_name: Optional callback that returns the architecture name known > @@ -159,6 +161,8 @@ struct CPUClass { > vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr); > > const char *gdb_core_xml_file; > + const char **gdb_qreg_info_lines; > + int gdb_qreg_info_line_count; > gchar * (*gdb_arch_name)(CPUState *cpu); > const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname); > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c > index be18568d0a..f19f8c58c3 100644 > --- a/gdbstub/gdbstub.c > +++ b/gdbstub/gdbstub.c > @@ -1409,6 +1409,27 @@ static void handle_query_curr_tid(GArray *params, void *user_ctx) > gdb_put_strbuf(); > } > > +static void handle_query_regs(GArray *params, void *user_ctx) > +{ > + if (!params->len) { > + return; > + } > + > + CPUClass *cc = CPU_GET_CLASS(gdbserver_state.g_cpu); > + if (!cc->gdb_qreg_info_lines) { > + gdb_put_packet(""); > + return; > + } > + > + int reg_num = get_param(params, 0)->val_ul; > + if (reg_num >= cc->gdb_qreg_info_line_count) { > + gdb_put_packet(""); > + return; > + } > + > + gdb_put_packet(cc->gdb_qreg_info_lines[reg_num]); > +} > + > static void handle_query_threads(GArray *params, void *user_ctx) > { > if (!gdbserver_state.query_cpu) { > @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = { > .handler = handle_query_curr_tid, > .cmd = "C", > }, > + { > + .handler = handle_query_regs, > + .cmd = "RegisterInfo", > + .cmd_startswith = 1, > + .schema = "l0" > + }, Where is this defined in the protocol spec, I can't see it in: https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets and it seems to be information that is handled by the xml register description. Is there a reason that isn't used for Hexagon? > { > .handler = handle_query_threads, > .cmd = "sThreadInfo",
On 20/4/23 14:31, Matheus Tavares Bernardino wrote: > From: Brian Cain <bcain@quicinc.com> > > Signed-off-by: Brian Cain <bcain@quicinc.com> > Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> > --- > include/hw/core/cpu.h | 4 ++++ > gdbstub/gdbstub.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 31 insertions(+) > @@ -159,6 +161,8 @@ struct CPUClass { > vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr); > > const char *gdb_core_xml_file; > + const char **gdb_qreg_info_lines; Shouldn't this be 'const char *const *gdb_qreg_info_lines;'? > + int gdb_qreg_info_line_count; > gchar * (*gdb_arch_name)(CPUState *cpu); > const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname);
On 20/4/23 16:04, Philippe Mathieu-Daudé wrote: > On 20/4/23 14:31, Matheus Tavares Bernardino wrote: >> From: Brian Cain <bcain@quicinc.com> >> >> Signed-off-by: Brian Cain <bcain@quicinc.com> >> Signed-off-by: Matheus Tavares Bernardino <quic_mathbern@quicinc.com> >> --- >> include/hw/core/cpu.h | 4 ++++ >> gdbstub/gdbstub.c | 27 +++++++++++++++++++++++++++ >> 2 files changed, 31 insertions(+) > > >> @@ -159,6 +161,8 @@ struct CPUClass { >> vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr); >> const char *gdb_core_xml_file; >> + const char **gdb_qreg_info_lines; > > Shouldn't this be 'const char *const *gdb_qreg_info_lines;'? > >> + int gdb_qreg_info_line_count; Also, unsigned or size_t? >> gchar * (*gdb_arch_name)(CPUState *cpu); >> const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char >> *xmlname);
Alex Bennée <alex.bennee@linaro.org> wrote: > > > Matheus Tavares <quic_mathbern@quicinc.com> wrote: > > > > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c > > index be18568d0a..f19f8c58c3 100644 > > --- a/gdbstub/gdbstub.c > > +++ b/gdbstub/gdbstub.c > > @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = { > > .handler = handle_query_curr_tid, > > .cmd = "C", > > }, > > + { > > + .handler = handle_query_regs, > > + .cmd = "RegisterInfo", > > + .cmd_startswith = 1, > > + .schema = "l0" > > + }, > > Where is this defined in the protocol spec, I can't see it in: > > https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets > > and it seems to be information that is handled by the xml register > description. Is there a reason that isn't used for Hexagon? Good point. It's actually an lldb extension to the protocol: https://github.com/llvm/llvm-project/blob/main/lldb/docs/lldb-gdb-remote.txt#L573 But indeed, lldb should be able to use the xml register description as well. I'll take a look and try to do that instead. Thanks!
Matheus Tavares Bernardino <quic_mathbern@quicinc.com> writes: > Alex Bennée <alex.bennee@linaro.org> wrote: >> >> > Matheus Tavares <quic_mathbern@quicinc.com> wrote: >> > >> > diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c >> > index be18568d0a..f19f8c58c3 100644 >> > --- a/gdbstub/gdbstub.c >> > +++ b/gdbstub/gdbstub.c >> > @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = { >> > .handler = handle_query_curr_tid, >> > .cmd = "C", >> > }, >> > + { >> > + .handler = handle_query_regs, >> > + .cmd = "RegisterInfo", >> > + .cmd_startswith = 1, >> > + .schema = "l0" >> > + }, >> >> Where is this defined in the protocol spec, I can't see it in: >> >> https://sourceware.org/gdb/onlinedocs/gdb/General-Query-Packets.html#General-Query-Packets >> >> and it seems to be information that is handled by the xml register >> description. Is there a reason that isn't used for Hexagon? > > Good point. It's actually an lldb extension to the protocol: > https://github.com/llvm/llvm-project/blob/main/lldb/docs/lldb-gdb-remote.txt#L573 > > But indeed, lldb should be able to use the xml register description as > well. I'll take a look and try to do that instead. There may be an argument for supporting both but only if the details of the xml/RegisterInfo would be sorted out by gdbstub or some other common code rather than each front-end growing special support. For now see how the XML does for you. > > Thanks!
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 397fd3ac68..cfdf5514d9 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -124,6 +124,8 @@ struct SysemuCPUOps; * its Harvard architecture split code and data. * @gdb_num_core_regs: Number of core registers accessible to GDB. * @gdb_core_xml_file: File name for core registers GDB XML description. + * @gdb_qreg_info_lines: Array of lines of registers qRegisterInfo description. + * @gdb_qreg_info_line_count: Count of lines for @gdb_qreg_info_lines. * @gdb_stop_before_watchpoint: Indicates whether GDB expects the CPU to stop * before the insn which triggers a watchpoint rather than after it. * @gdb_arch_name: Optional callback that returns the architecture name known @@ -159,6 +161,8 @@ struct CPUClass { vaddr (*gdb_adjust_breakpoint)(CPUState *cpu, vaddr addr); const char *gdb_core_xml_file; + const char **gdb_qreg_info_lines; + int gdb_qreg_info_line_count; gchar * (*gdb_arch_name)(CPUState *cpu); const char * (*gdb_get_dynamic_xml)(CPUState *cpu, const char *xmlname); diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c index be18568d0a..f19f8c58c3 100644 --- a/gdbstub/gdbstub.c +++ b/gdbstub/gdbstub.c @@ -1409,6 +1409,27 @@ static void handle_query_curr_tid(GArray *params, void *user_ctx) gdb_put_strbuf(); } +static void handle_query_regs(GArray *params, void *user_ctx) +{ + if (!params->len) { + return; + } + + CPUClass *cc = CPU_GET_CLASS(gdbserver_state.g_cpu); + if (!cc->gdb_qreg_info_lines) { + gdb_put_packet(""); + return; + } + + int reg_num = get_param(params, 0)->val_ul; + if (reg_num >= cc->gdb_qreg_info_line_count) { + gdb_put_packet(""); + return; + } + + gdb_put_packet(cc->gdb_qreg_info_lines[reg_num]); +} + static void handle_query_threads(GArray *params, void *user_ctx) { if (!gdbserver_state.query_cpu) { @@ -1578,6 +1599,12 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = { .handler = handle_query_curr_tid, .cmd = "C", }, + { + .handler = handle_query_regs, + .cmd = "RegisterInfo", + .cmd_startswith = 1, + .schema = "l0" + }, { .handler = handle_query_threads, .cmd = "sThreadInfo",