On 3/22/19 2:29 PM, Jan Kiszka wrote: > On 07.12.18 10:01, Luc Michel wrote: >> Add the gdb_first_attached_cpu() and gdb_next_attached_cpu() to iterate >> over all the CPUs in currently attached processes. >> >> Add the gdb_first_cpu_in_process() and gdb_next_cpu_in_process() to >> iterate over CPUs of a given process. >> >> Use them to add multiprocess extension support to vCont packets. >> >> Signed-off-by: Luc Michel <luc.mic...@greensocs.com> >> Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> >> Reviewed-by: Edgar E. Iglesias <edgar.igles...@xilinx.com> >> Acked-by: Alistair Francis <alistair.fran...@wdc.com> >> --- >> gdbstub.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 102 insertions(+), 15 deletions(-) >> >> diff --git a/gdbstub.c b/gdbstub.c >> index 911faa225a..77b3dbb2c8 100644 >> --- a/gdbstub.c >> +++ b/gdbstub.c >> @@ -721,10 +721,40 @@ static CPUState *find_cpu(uint32_t thread_id) >> } >> >> return NULL; >> } >> >> +static CPUState *get_first_cpu_in_process(const GDBState *s, >> + GDBProcess *process) >> +{ >> + CPUState *cpu; >> + >> + CPU_FOREACH(cpu) { >> + if (gdb_get_cpu_pid(s, cpu) == process->pid) { >> + return cpu; >> + } >> + } >> + >> + return NULL; >> +} >> + >> +static CPUState *gdb_next_cpu_in_process(const GDBState *s, CPUState *cpu) >> +{ >> + uint32_t pid = gdb_get_cpu_pid(s, cpu); >> + cpu = CPU_NEXT(cpu); >> + >> + while (cpu) { >> + if (gdb_get_cpu_pid(s, cpu) == pid) { >> + break; >> + } >> + >> + cpu = CPU_NEXT(cpu); >> + } >> + >> + return cpu; >> +} >> + >> static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) >> { >> GDBProcess *process; >> CPUState *cpu; >> >> @@ -750,10 +780,41 @@ static CPUState *gdb_get_cpu(const GDBState *s, >> uint32_t pid, uint32_t tid) >> } >> >> return cpu; >> } >> >> +/* Return the cpu following @cpu, while ignoring >> + * unattached processes. >> + */ >> +static CPUState *gdb_next_attached_cpu(const GDBState *s, CPUState *cpu) >> +{ >> + cpu = CPU_NEXT(cpu); >> + >> + while (cpu) { >> + if (gdb_get_cpu_process(s, cpu)->attached) { >> + break; >> + } >> + >> + cpu = CPU_NEXT(cpu); >> + } >> + >> + return cpu; >> +} >> + >> +/* Return the first attached cpu */ >> +static CPUState *gdb_first_attached_cpu(const GDBState *s) >> +{ >> + CPUState *cpu = first_cpu; >> + GDBProcess *process = gdb_get_cpu_process(s, cpu); >> + >> + if (!process->attached) { >> + return gdb_next_attached_cpu(s, cpu); >> + } >> + >> + return cpu; >> +} >> + >> static const char *get_feature_xml(const char *p, const char **newp, >> CPUClass *cc) >> { >> size_t len; >> int i; >> @@ -1088,14 +1149,16 @@ static int is_query_packet(const char *p, const char >> *query, char separator) >> * returns -ENOTSUP if a command is unsupported, -EINVAL or -ERANGE if >> there is >> * a format error, 0 on success. >> */ >> static int gdb_handle_vcont(GDBState *s, const char *p) >> { >> - int res, idx, signal = 0; >> + int res, signal = 0; >> char cur_action; >> char *newstates; >> unsigned long tmp; >> + uint32_t pid, tid; >> + GDBProcess *process; >> CPUState *cpu; >> #ifdef CONFIG_USER_ONLY >> int max_cpus = 1; /* global variable max_cpus exists only in system >> mode */ >> >> CPU_FOREACH(cpu) { >> @@ -1134,29 +1197,52 @@ static int gdb_handle_vcont(GDBState *s, const char >> *p) >> } else if (cur_action != 'c' && cur_action != 's') { >> /* unknown/invalid/unsupported command */ >> res = -ENOTSUP; >> goto out; >> } >> - /* thread specification. special values: (none), -1 = all; 0 = any >> */ >> - if ((p[0] == ':' && p[1] == '-' && p[2] == '1') || (p[0] != ':')) { >> - if (*p == ':') { >> - p += 3; >> - } >> - for (idx = 0; idx < max_cpus; idx++) { >> - if (newstates[idx] == 1) { >> - newstates[idx] = cur_action; >> + >> + if (*p++ != ':') { >> + res = -ENOTSUP; >> + goto out; >> + } >> + >> + switch (read_thread_id(p, &p, &pid, &tid)) { >> + case GDB_READ_THREAD_ERR: >> + res = -EINVAL; >> + goto out; >> + >> + case GDB_ALL_PROCESSES: >> + cpu = gdb_first_attached_cpu(s); >> + while (cpu) { >> + if (newstates[cpu->cpu_index] == 1) { >> + newstates[cpu->cpu_index] = cur_action; >> } >> + >> + cpu = gdb_next_attached_cpu(s, cpu); >> } >> - } else if (*p == ':') { >> - p++; >> - res = qemu_strtoul(p, &p, 16, &tmp); >> - if (res) { >> + break; >> + >> + case GDB_ALL_THREADS: >> + process = gdb_get_process(s, pid); >> + >> + if (!process->attached) { >> + res = -EINVAL; >> goto out; >> } >> >> - /* 0 means any thread, so we pick the first valid CPU */ >> - cpu = tmp ? find_cpu(tmp) : first_cpu; >> + cpu = get_first_cpu_in_process(s, process); >> + while (cpu) { >> + if (newstates[cpu->cpu_index] == 1) { >> + newstates[cpu->cpu_index] = cur_action; >> + } >> + >> + cpu = gdb_next_cpu_in_process(s, cpu); >> + } >> + break; >> + >> + case GDB_ONE_THREAD: >> + cpu = gdb_get_cpu(s, pid, tid); >> >> /* invalid CPU/thread specified */ >> if (!cpu) { >> res = -EINVAL; >> goto out; >> @@ -1164,10 +1250,11 @@ static int gdb_handle_vcont(GDBState *s, const char >> *p) >> >> /* only use if no previous match occourred */ >> if (newstates[cpu->cpu_index] == 1) { >> newstates[cpu->cpu_index] = cur_action; >> } >> + break; >> } >> } >> s->signal = signal; >> gdb_continue_partial(s, newstates); >> >> > > This breaks system-level debugging with gdb, at least with version > 7.7.50 and for x86 targets.
I think it's related to https://www.mail-archive.com/qemu-devel@nongnu.org/msg601065.html [snip] > Sending packet: $vCont;s:1;c#c1...Ack > Packet received: E22 > warning: Remote failure reply: E22 [snip] The vCont packet does not specifies a thread-id for the last action. This is a valid syntax, incorrectly treated as an error by the stub. Lucien: do you plan to send a re-roll? Otherwise I'll do it on next Monday (25/03) because I would like this bug to be fixed before it hits 4.0. Thanks. -- Luc