| Message ID | 20190107163117.16269-7-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7d8c87da79c23b2caa8f68a0a9cd0b15cedf8b41 |
| Headers | show |
| Series | target-arm queue | expand |
On Mon, 7 Jan 2019 at 16:31, Peter Maydell <peter.maydell@linaro.org> wrote: > > From: Luc Michel <luc.michel@greensocs.com> > > Add a couple of helper functions to cope with GDB threads and processes. > > The gdb_get_process() function looks for a process given a pid. > > The gdb_get_cpu() function returns the CPU corresponding to the (pid, > tid) pair given as parameters. > > The read_thread_id() function parses the thread-id sent by the peer. > This function supports the multiprocess extension thread-id syntax. The > return value specifies if the parsing failed, or if a special case was > encountered (all processes or all threads). > > Use them in 'H' and 'T' packets handling to support the multiprocess > extension. > +static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) > +{ > + GDBProcess *process; > + CPUState *cpu; > + > + if (!tid) { > + /* 0 means any thread, we take the first one */ > + tid = 1; > + } > + > + cpu = find_cpu(tid); > + > + if (cpu == NULL) { > + return NULL; > + } > + > + process = gdb_get_cpu_process(s, cpu); > + > + if (process->pid != pid) { > + return NULL; > + } > + > + if (!process->attached) { > + return NULL; > + } > + > + return cpu; > +} I'm finding that (with my out-of-tree board model) gdb asks for "process 0 thread 0", which should mean "anything you like", so we end up calling gdb_get_cpu(s, 0, 0). Then we find a CPU via its TID, and of course it has some specific PID in process->pid which is not zero, so we fail the "process->pid != pid" test and return NULL here incorrectly. Should that check be --- a/gdbstub.c +++ b/gdbstub.c @@ -736,7 +736,8 @@ static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) process = gdb_get_cpu_process(s, cpu); - if (process->pid != pid) { + /* pid == 0 means any process, so this one is fine */ + if (pid != 0 && process->pid != pid) { return NULL; } ? thanks -- PMM
On Thu, 17 Jan 2019 at 18:13, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 7 Jan 2019 at 16:31, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > From: Luc Michel <luc.michel@greensocs.com> > > > > Add a couple of helper functions to cope with GDB threads and processes. > > > > The gdb_get_process() function looks for a process given a pid. > > > > The gdb_get_cpu() function returns the CPU corresponding to the (pid, > > tid) pair given as parameters. > > > > The read_thread_id() function parses the thread-id sent by the peer. > > This function supports the multiprocess extension thread-id syntax. The > > return value specifies if the parsing failed, or if a special case was > > encountered (all processes or all threads). > > > > Use them in 'H' and 'T' packets handling to support the multiprocess > > extension. > > > +static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) > > +{ > > + GDBProcess *process; > > + CPUState *cpu; > > + > > + if (!tid) { > > + /* 0 means any thread, we take the first one */ > > + tid = 1; > > + } > > + > > + cpu = find_cpu(tid); > > + > > + if (cpu == NULL) { > > + return NULL; > > + } > > + > > + process = gdb_get_cpu_process(s, cpu); > > + > > + if (process->pid != pid) { > > + return NULL; > > + } > > + > > + if (!process->attached) { > > + return NULL; > > + } > > + > > + return cpu; > > +} > > I'm finding that (with my out-of-tree board model) gdb > asks for "process 0 thread 0", which should mean "anything you > like", so we end up calling gdb_get_cpu(s, 0, 0). Then we > find a CPU via its TID, and of course it has some specific > PID in process->pid which is not zero, so we fail the > "process->pid != pid" test and return NULL here incorrectly. Also, the "use TID 1 if asked for any-task" logic assumes that TID 1 is actually part of the requested process and that that task is attached. Shouldn't we instead find the requested process (or any random attached process, if tid == 0) and then pick one of the TIDs it has? thanks -- PMM
diff --git a/gdbstub.c b/gdbstub.c index 02beb44d973..644377db9f5 100644 --- a/gdbstub.c +++ b/gdbstub.c @@ -688,6 +688,71 @@ out: #endif } +static GDBProcess *gdb_get_process(const GDBState *s, uint32_t pid) +{ + int i; + + if (!pid) { + /* 0 means any process, we take the first one */ + return &s->processes[0]; + } + + for (i = 0; i < s->process_num; i++) { + if (s->processes[i].pid == pid) { + return &s->processes[i]; + } + } + + return NULL; +} + +static GDBProcess *gdb_get_cpu_process(const GDBState *s, CPUState *cpu) +{ + return gdb_get_process(s, gdb_get_cpu_pid(s, cpu)); +} + +static CPUState *find_cpu(uint32_t thread_id) +{ + CPUState *cpu; + + CPU_FOREACH(cpu) { + if (cpu_gdb_index(cpu) == thread_id) { + return cpu; + } + } + + return NULL; +} + +static CPUState *gdb_get_cpu(const GDBState *s, uint32_t pid, uint32_t tid) +{ + GDBProcess *process; + CPUState *cpu; + + if (!tid) { + /* 0 means any thread, we take the first one */ + tid = 1; + } + + cpu = find_cpu(tid); + + if (cpu == NULL) { + return NULL; + } + + process = gdb_get_cpu_process(s, cpu); + + if (process->pid != pid) { + return NULL; + } + + if (!process->attached) { + return NULL; + } + + return cpu; +} + static const char *get_feature_xml(const char *p, const char **newp, CPUClass *cc) { @@ -944,19 +1009,6 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc) cpu_set_pc(cpu, pc); } -static CPUState *find_cpu(uint32_t thread_id) -{ - CPUState *cpu; - - CPU_FOREACH(cpu) { - if (cpu_gdb_index(cpu) == thread_id) { - return cpu; - } - } - - return NULL; -} - static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu, char *buf, size_t buf_size) { @@ -970,6 +1022,60 @@ static char *gdb_fmt_thread_id(const GDBState *s, CPUState *cpu, return buf; } +typedef enum GDBThreadIdKind { + GDB_ONE_THREAD = 0, + GDB_ALL_THREADS, /* One process, all threads */ + GDB_ALL_PROCESSES, + GDB_READ_THREAD_ERR +} GDBThreadIdKind; + +static GDBThreadIdKind read_thread_id(const char *buf, const char **end_buf, + uint32_t *pid, uint32_t *tid) +{ + unsigned long p, t; + int ret; + + if (*buf == 'p') { + buf++; + ret = qemu_strtoul(buf, &buf, 16, &p); + + if (ret) { + return GDB_READ_THREAD_ERR; + } + + /* Skip '.' */ + buf++; + } else { + p = 1; + } + + ret = qemu_strtoul(buf, &buf, 16, &t); + + if (ret) { + return GDB_READ_THREAD_ERR; + } + + *end_buf = buf; + + if (p == -1) { + return GDB_ALL_PROCESSES; + } + + if (pid) { + *pid = p; + } + + if (t == -1) { + return GDB_ALL_THREADS; + } + + if (tid) { + *tid = t; + } + + return GDB_ONE_THREAD; +} + static int is_query_packet(const char *p, const char *query, char separator) { unsigned int query_len = strlen(query); @@ -1078,12 +1184,14 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) 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]; uint8_t *registers; target_ulong addr, len; + GDBThreadIdKind thread_kind; trace_gdbstub_io_command(line_buf); @@ -1291,12 +1399,18 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) break; case 'H': type = *p++; - thread = strtoull(p, (char **)&p, 16); - if (thread == -1 || thread == 0) { + + thread_kind = read_thread_id(p, &p, &pid, &tid); + if (thread_kind == GDB_READ_THREAD_ERR) { + put_packet(s, "E22"); + break; + } + + if (thread_kind != GDB_ONE_THREAD) { put_packet(s, "OK"); break; } - cpu = find_cpu(thread); + cpu = gdb_get_cpu(s, pid, tid); if (cpu == NULL) { put_packet(s, "E22"); break; @@ -1316,8 +1430,12 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf) } break; case 'T': - thread = strtoull(p, (char **)&p, 16); - cpu = find_cpu(thread); + thread_kind = read_thread_id(p, &p, &pid, &tid); + if (thread_kind == GDB_READ_THREAD_ERR) { + put_packet(s, "E22"); + break; + } + cpu = gdb_get_cpu(s, pid, tid); if (cpu != NULL) { put_packet(s, "OK");