| Message ID | 20181115094207.22846-8-luc.michel@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | gdbstub: support for the multiprocess extension | expand |
On Thu, Nov 15, 2018 at 10:41:58AM +0100, Luc Michel wrote: > Change the thread info related packets handling to support multiprocess > extension. > > Add the CPUs class name in the extra info to help differentiate > them in multiprocess mode. > > Signed-off-by: Luc Michel <luc.michel@greensocs.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > gdbstub.c | 35 +++++++++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 10 deletions(-) > > diff --git a/gdbstub.c b/gdbstub.c > index d19b0137e8..292dee8914 100644 > --- a/gdbstub.c > +++ b/gdbstub.c > @@ -1260,11 +1260,10 @@ out: > static int gdb_handle_packet(GDBState *s, const char *line_buf) > { > CPUState *cpu; > CPUClass *cc; > const char *p; > - uint32_t thread; > uint32_t pid, tid; > int ch, reg_size, type, res; > uint8_t mem_buf[MAX_PACKET_LENGTH]; > char buf[sizeof(mem_buf) + 1 /* trailing NUL */]; > char thread_id[16]; > @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > snprintf(buf, sizeof(buf), "QC%s", > gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id))); > put_packet(s, buf); > break; > } else if (strcmp(p,"fThreadInfo") == 0) { > - s->query_cpu = first_cpu; > + s->query_cpu = gdb_first_cpu(s); > goto report_cpuinfo; > } else if (strcmp(p,"sThreadInfo") == 0) { > report_cpuinfo: > if (s->query_cpu) { > - snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu)); > + snprintf(buf, sizeof(buf), "m%s", > + gdb_fmt_thread_id(s, s->query_cpu, > + thread_id, sizeof(thread_id))); > put_packet(s, buf); > - s->query_cpu = CPU_NEXT(s->query_cpu); > + s->query_cpu = gdb_next_cpu(s, s->query_cpu); > } else > put_packet(s, "l"); > break; > } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) { > - thread = strtoull(p+16, (char **)&p, 16); > - cpu = find_cpu(thread); > + if (read_thread_id(p + 16, &p, &pid, &tid) == GDB_READ_THREAD_ERR) { > + put_packet(s, "E22"); > + break; > + } > + cpu = gdb_get_cpu(s, pid, tid); > if (cpu != NULL) { > cpu_synchronize_state(cpu); > - /* memtohex() doubles the required space */ > - len = snprintf((char *)mem_buf, sizeof(buf) / 2, > - "CPU#%d [%s]", cpu->cpu_index, > - cpu->halted ? "halted " : "running"); > + > + if (s->multiprocess && (s->process_num > 1)) { > + /* Print the CPU model in multiprocess mode */ > + ObjectClass *oc = object_get_class(OBJECT(cpu)); > + const char *cpu_model = object_class_get_name(oc); > + len = snprintf((char *)mem_buf, sizeof(buf) / 2, > + "CPU#%d %s [%s]", cpu->cpu_index, > + cpu_model, > + cpu->halted ? "halted " : "running"); I wonder if we could also print a friendly name here deducted from QOM? In some of our use-cases we have an array of MicroBlazes that all live in different HW subsystems and are named differently (e.g CSU, PMU, PMC, PSM etc). Instead of just seeing a list of MicroBlaze cores it may be more useful to see the actual core name of some sort, e.g: Instead of: CPU#0 MicroBlaze [running] CPU#1 MicroBlaze [running] CPU#2 MicroBlaze [running] CPU#3 MicroBlaze [running] Perhaps something like: CPU#0 MicroBlaze PMU [running] CPU#1 MicroBlaze PMC-PPU0 [running] CPU#2 MicroBlaze PMC-PPU1 [running] CPU#3 MicroBlaze PSM [running] Any thoughts on that? Thanks, Edgar > + } else { > + /* memtohex() doubles the required space */ > + len = snprintf((char *)mem_buf, sizeof(buf) / 2, > + "CPU#%d [%s]", cpu->cpu_index, > + cpu->halted ? "halted " : "running"); > + } > trace_gdbstub_op_extra_info((char *)mem_buf); > memtohex(buf, mem_buf, len); > put_packet(s, buf); > } > break; > -- > 2.19.1 >
On 11/16/18 11:04 AM, Edgar E. Iglesias wrote: > On Thu, Nov 15, 2018 at 10:41:58AM +0100, Luc Michel wrote: >> Change the thread info related packets handling to support multiprocess >> extension. >> >> Add the CPUs class name in the extra info to help differentiate >> them in multiprocess mode. >> >> Signed-off-by: Luc Michel <luc.michel@greensocs.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> gdbstub.c | 35 +++++++++++++++++++++++++---------- >> 1 file changed, 25 insertions(+), 10 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index d19b0137e8..292dee8914 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -1260,11 +1260,10 @@ out: >> static int gdb_handle_packet(GDBState *s, const char *line_buf) >> { >> CPUState *cpu; >> CPUClass *cc; >> const char *p; >> - uint32_t thread; >> uint32_t pid, tid; >> int ch, reg_size, type, res; >> uint8_t mem_buf[MAX_PACKET_LENGTH]; >> char buf[sizeof(mem_buf) + 1 /* trailing NUL */]; >> char thread_id[16]; >> @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) >> snprintf(buf, sizeof(buf), "QC%s", >> gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id))); >> put_packet(s, buf); >> break; >> } else if (strcmp(p,"fThreadInfo") == 0) { >> - s->query_cpu = first_cpu; >> + s->query_cpu = gdb_first_cpu(s); >> goto report_cpuinfo; >> } else if (strcmp(p,"sThreadInfo") == 0) { >> report_cpuinfo: >> if (s->query_cpu) { >> - snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu)); >> + snprintf(buf, sizeof(buf), "m%s", >> + gdb_fmt_thread_id(s, s->query_cpu, >> + thread_id, sizeof(thread_id))); >> put_packet(s, buf); >> - s->query_cpu = CPU_NEXT(s->query_cpu); >> + s->query_cpu = gdb_next_cpu(s, s->query_cpu); >> } else >> put_packet(s, "l"); >> break; >> } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) { >> - thread = strtoull(p+16, (char **)&p, 16); >> - cpu = find_cpu(thread); >> + if (read_thread_id(p + 16, &p, &pid, &tid) == GDB_READ_THREAD_ERR) { >> + put_packet(s, "E22"); >> + break; >> + } >> + cpu = gdb_get_cpu(s, pid, tid); >> if (cpu != NULL) { >> cpu_synchronize_state(cpu); >> - /* memtohex() doubles the required space */ >> - len = snprintf((char *)mem_buf, sizeof(buf) / 2, >> - "CPU#%d [%s]", cpu->cpu_index, >> - cpu->halted ? "halted " : "running"); >> + >> + if (s->multiprocess && (s->process_num > 1)) { >> + /* Print the CPU model in multiprocess mode */ >> + ObjectClass *oc = object_get_class(OBJECT(cpu)); >> + const char *cpu_model = object_class_get_name(oc); >> + len = snprintf((char *)mem_buf, sizeof(buf) / 2, >> + "CPU#%d %s [%s]", cpu->cpu_index, >> + cpu_model, >> + cpu->halted ? "halted " : "running"); > > > > I wonder if we could also print a friendly name here deducted from QOM? > In some of our use-cases we have an array of MicroBlazes that all live > in different HW subsystems and are named differently (e.g CSU, PMU, PMC, > PSM etc). > > Instead of just seeing a list of MicroBlaze cores it may be more useful > to see the actual core name of some sort, e.g: > > Instead of: > CPU#0 MicroBlaze [running] > CPU#1 MicroBlaze [running] > CPU#2 MicroBlaze [running] > CPU#3 MicroBlaze [running] > > Perhaps something like: > CPU#0 MicroBlaze PMU [running] > CPU#1 MicroBlaze PMC-PPU0 [running] > CPU#2 MicroBlaze PMC-PPU1 [running] > CPU#3 MicroBlaze PSM [running] > > Any thoughts on that? I wanted to avoid the ThreadExtraInfo packet to become too much cluttered. Here are some tests adding the component part of the CPU canonical name: (gdb) info threads Id Target Id Frame 1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu apu-cpu[0] [running]) 0x0000000000000000 in ?? () 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu apu-cpu[1] [halted ]) 0x0000000000000000 in ?? () 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu apu-cpu[2] [halted ]) 0x0000000000000000 in ?? () 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu apu-cpu[3] [halted ]) 0x0000000000000000 in ?? () * 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu rpu-cpu[0] [halted ]) 0xffff0000 in ?? () 2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu rpu-cpu[1] [halted ]) 0xffff0000 in ?? () The model name takes quite some room. The interesting info are `arm` and `cortex-xxx`, but AFAIK there is no way of extracting that for a CPU generically. In this case, having the component part of the canonical name is ok because self-explanatory. However we could encounter cases where the parent name would be necessary to discriminate the CPUs, something like: cluster[0]/cpu[0] /cpu[1] cluster[1]/cpu[0] /cpu[1] ... The "safest" way would be to have the whole path: (gdb) info threads Id Target Id Frame 1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[0] [running]) 0x0000000000000000 in ?? () 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[1] [halted ]) 0x0000000000000000 in ?? () 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[2] [halted ]) 0x0000000000000000 in ?? () 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu /machine/soc/apu-cluster/apu-cpu[3] [halted ]) 0x0000000000000000 in ?? () * 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu /machine/soc/rpu-cluster/rpu-cpu[0] [halted ]) 0xffff0000 in ?? () 2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu /machine/soc/rpu-cluster/rpu-cpu[1] [halted ]) 0xffff0000 in ?? () But that becomes really cluttered... We could also remove the CPU model completely. What are your thoughts? Thanks, Luc > > Thanks, > Edgar > >> + } else { >> + /* memtohex() doubles the required space */ >> + len = snprintf((char *)mem_buf, sizeof(buf) / 2, >> + "CPU#%d [%s]", cpu->cpu_index, >> + cpu->halted ? "halted " : "running"); >> + } >> trace_gdbstub_op_extra_info((char *)mem_buf); >> memtohex(buf, mem_buf, len); >> put_packet(s, buf); >> } >> break; >> -- >> 2.19.1 >>
On Mon, Nov 19, 2018 at 11:12:45AM +0100, Luc Michel wrote: > > > On 11/16/18 11:04 AM, Edgar E. Iglesias wrote: > > On Thu, Nov 15, 2018 at 10:41:58AM +0100, Luc Michel wrote: > >> Change the thread info related packets handling to support multiprocess > >> extension. > >> > >> Add the CPUs class name in the extra info to help differentiate > >> them in multiprocess mode. > >> > >> Signed-off-by: Luc Michel <luc.michel@greensocs.com> > >> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com> > >> --- > >> gdbstub.c | 35 +++++++++++++++++++++++++---------- > >> 1 file changed, 25 insertions(+), 10 deletions(-) > >> > >> diff --git a/gdbstub.c b/gdbstub.c > >> index d19b0137e8..292dee8914 100644 > >> --- a/gdbstub.c > >> +++ b/gdbstub.c > >> @@ -1260,11 +1260,10 @@ out: > >> static int gdb_handle_packet(GDBState *s, const char *line_buf) > >> { > >> CPUState *cpu; > >> CPUClass *cc; > >> const char *p; > >> - uint32_t thread; > >> uint32_t pid, tid; > >> int ch, reg_size, type, res; > >> uint8_t mem_buf[MAX_PACKET_LENGTH]; > >> char buf[sizeof(mem_buf) + 1 /* trailing NUL */]; > >> char thread_id[16]; > >> @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) > >> snprintf(buf, sizeof(buf), "QC%s", > >> gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id))); > >> put_packet(s, buf); > >> break; > >> } else if (strcmp(p,"fThreadInfo") == 0) { > >> - s->query_cpu = first_cpu; > >> + s->query_cpu = gdb_first_cpu(s); > >> goto report_cpuinfo; > >> } else if (strcmp(p,"sThreadInfo") == 0) { > >> report_cpuinfo: > >> if (s->query_cpu) { > >> - snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu)); > >> + snprintf(buf, sizeof(buf), "m%s", > >> + gdb_fmt_thread_id(s, s->query_cpu, > >> + thread_id, sizeof(thread_id))); > >> put_packet(s, buf); > >> - s->query_cpu = CPU_NEXT(s->query_cpu); > >> + s->query_cpu = gdb_next_cpu(s, s->query_cpu); > >> } else > >> put_packet(s, "l"); > >> break; > >> } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) { > >> - thread = strtoull(p+16, (char **)&p, 16); > >> - cpu = find_cpu(thread); > >> + if (read_thread_id(p + 16, &p, &pid, &tid) == GDB_READ_THREAD_ERR) { > >> + put_packet(s, "E22"); > >> + break; > >> + } > >> + cpu = gdb_get_cpu(s, pid, tid); > >> if (cpu != NULL) { > >> cpu_synchronize_state(cpu); > >> - /* memtohex() doubles the required space */ > >> - len = snprintf((char *)mem_buf, sizeof(buf) / 2, > >> - "CPU#%d [%s]", cpu->cpu_index, > >> - cpu->halted ? "halted " : "running"); > >> + > >> + if (s->multiprocess && (s->process_num > 1)) { > >> + /* Print the CPU model in multiprocess mode */ > >> + ObjectClass *oc = object_get_class(OBJECT(cpu)); > >> + const char *cpu_model = object_class_get_name(oc); > >> + len = snprintf((char *)mem_buf, sizeof(buf) / 2, > >> + "CPU#%d %s [%s]", cpu->cpu_index, > >> + cpu_model, > >> + cpu->halted ? "halted " : "running"); > > > > > > > > I wonder if we could also print a friendly name here deducted from QOM? > > In some of our use-cases we have an array of MicroBlazes that all live > > in different HW subsystems and are named differently (e.g CSU, PMU, PMC, > > PSM etc). > > > > Instead of just seeing a list of MicroBlaze cores it may be more useful > > to see the actual core name of some sort, e.g: > > > > Instead of: > > CPU#0 MicroBlaze [running] > > CPU#1 MicroBlaze [running] > > CPU#2 MicroBlaze [running] > > CPU#3 MicroBlaze [running] > > > > Perhaps something like: > > CPU#0 MicroBlaze PMU [running] > > CPU#1 MicroBlaze PMC-PPU0 [running] > > CPU#2 MicroBlaze PMC-PPU1 [running] > > CPU#3 MicroBlaze PSM [running] > > > > Any thoughts on that? > I wanted to avoid the ThreadExtraInfo packet to become too much cluttered. > > Here are some tests adding the component part of the CPU canonical name: > > (gdb) info threads > Id Target Id Frame > 1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu apu-cpu[0] [running]) > 0x0000000000000000 in ?? () > 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu apu-cpu[1] [halted ]) > 0x0000000000000000 in ?? () > 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu apu-cpu[2] [halted ]) > 0x0000000000000000 in ?? () > 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu apu-cpu[3] [halted ]) > 0x0000000000000000 in ?? () > * 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu rpu-cpu[0] [halted ]) > 0xffff0000 in ?? () > 2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu rpu-cpu[1] [halted ]) > 0xffff0000 in ?? () > > The model name takes quite some room. The interesting info are `arm` and > `cortex-xxx`, but AFAIK there is no way of extracting that for a CPU > generically. > > In this case, having the component part of the canonical name is ok > because self-explanatory. However we could encounter cases where the > parent name would be necessary to discriminate the CPUs, something like: > cluster[0]/cpu[0] > /cpu[1] > cluster[1]/cpu[0] > /cpu[1] > ... > > The "safest" way would be to have the whole path: > > (gdb) info threads > Id Target Id Frame > 1.1 Thread 1.1 (CPU#0 cortex-a53-arm-cpu > /machine/soc/apu-cluster/apu-cpu[0] [running]) 0x0000000000000000 in ?? () > 1.2 Thread 1.2 (CPU#1 cortex-a53-arm-cpu > /machine/soc/apu-cluster/apu-cpu[1] [halted ]) 0x0000000000000000 in ?? () > 1.3 Thread 1.3 (CPU#2 cortex-a53-arm-cpu > /machine/soc/apu-cluster/apu-cpu[2] [halted ]) 0x0000000000000000 in ?? () > 1.4 Thread 1.4 (CPU#3 cortex-a53-arm-cpu > /machine/soc/apu-cluster/apu-cpu[3] [halted ]) 0x0000000000000000 in ?? () > * 2.1 Thread 2.5 (CPU#4 cortex-r5f-arm-cpu > /machine/soc/rpu-cluster/rpu-cpu[0] [halted ]) 0xffff0000 in ?? () > 2.2 Thread 2.6 (CPU#5 cortex-r5f-arm-cpu > /machine/soc/rpu-cluster/rpu-cpu[1] [halted ]) 0xffff0000 in ?? () > > But that becomes really cluttered... We could also remove the CPU model > completely. > > What are your thoughts? Thanks Luc, Not sure... (CPU#0 cortex-a53-arm-cpu apu-cpu[0] [running]) Looks a little long but I think still the better option here. Would be interesting to hear others opinion. Also, would it make sense to remove the CPU#X alltogether here? It's not of much use in GDB since we controll things by process and thread anyway... Cheers, Edgar > > Thanks, > Luc > > > > > Thanks, > > Edgar > > > >> + } else { > >> + /* memtohex() doubles the required space */ > >> + len = snprintf((char *)mem_buf, sizeof(buf) / 2, > >> + "CPU#%d [%s]", cpu->cpu_index, > >> + cpu->halted ? "halted " : "running"); > >> + } > >> trace_gdbstub_op_extra_info((char *)mem_buf); > >> memtohex(buf, mem_buf, len); > >> put_packet(s, buf); > >> } > >> break; > >> -- > >> 2.19.1 > >>
diff --git a/gdbstub.c b/gdbstub.c index d19b0137e8..292dee8914 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -1260,11 +1260,10 @@ out: static int gdb_handle_packet(GDBState *s, const char *line_buf) { CPUState *cpu; CPUClass *cc; const char *p; - uint32_t thread; uint32_t pid, tid; int ch, reg_size, type, res; uint8_t mem_buf[MAX_PACKET_LENGTH]; char buf[sizeof(mem_buf) + 1 /* trailing NUL */]; char thread_id[16]; @@ -1556,30 +1555,46 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) snprintf(buf, sizeof(buf), "QC%s", gdb_fmt_thread_id(s, cpu, thread_id, sizeof(thread_id))); put_packet(s, buf); break; } else if (strcmp(p,"fThreadInfo") == 0) { - s->query_cpu = first_cpu; + s->query_cpu = gdb_first_cpu(s); goto report_cpuinfo; } else if (strcmp(p,"sThreadInfo") == 0) { report_cpuinfo: if (s->query_cpu) { - snprintf(buf, sizeof(buf), "m%x", cpu_gdb_index(s->query_cpu)); + snprintf(buf, sizeof(buf), "m%s", + gdb_fmt_thread_id(s, s->query_cpu, + thread_id, sizeof(thread_id))); put_packet(s, buf); - s->query_cpu = CPU_NEXT(s->query_cpu); + s->query_cpu = gdb_next_cpu(s, s->query_cpu); } else put_packet(s, "l"); break; } else if (strncmp(p,"ThreadExtraInfo,", 16) == 0) { - thread = strtoull(p+16, (char **)&p, 16); - cpu = find_cpu(thread); + if (read_thread_id(p + 16, &p, &pid, &tid) == GDB_READ_THREAD_ERR) { + put_packet(s, "E22"); + break; + } + cpu = gdb_get_cpu(s, pid, tid); if (cpu != NULL) { cpu_synchronize_state(cpu); - /* memtohex() doubles the required space */ - len = snprintf((char *)mem_buf, sizeof(buf) / 2, - "CPU#%d [%s]", cpu->cpu_index, - cpu->halted ? "halted " : "running"); + + if (s->multiprocess && (s->process_num > 1)) { + /* Print the CPU model in multiprocess mode */ + ObjectClass *oc = object_get_class(OBJECT(cpu)); + const char *cpu_model = object_class_get_name(oc); + len = snprintf((char *)mem_buf, sizeof(buf) / 2, + "CPU#%d %s [%s]", cpu->cpu_index, + cpu_model, + cpu->halted ? "halted " : "running"); + } else { + /* memtohex() doubles the required space */ + len = snprintf((char *)mem_buf, sizeof(buf) / 2, + "CPU#%d [%s]", cpu->cpu_index, + cpu->halted ? "halted " : "running"); + } trace_gdbstub_op_extra_info((char *)mem_buf); memtohex(buf, mem_buf, len); put_packet(s, buf); } break;