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.
Signed-off-by: Luc Michel <luc.michel@greensocs.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
gdbstub.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 134 insertions(+), 18 deletions(-)
diff --git a/gdbstub.c b/gdbstub.c
index 4fbc05dfe3..fa2b7077b2 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -680,10 +680,73 @@ out:
#else
return s->processes[0].pid;
#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 = find_cpu(tid);
+
+ if (!tid) {
+ /* 0 means any thread, we take the first one */
+ tid = 1;
+ }
+
+ 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)
{
size_t len;
int i;
@@ -936,23 +999,10 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
cpu_synchronize_state(cpu);
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)
{
if (s->multiprocess) {
snprintf(buf, buf_size, "p%02x.%02x",
@@ -962,10 +1012,64 @@ 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);
return strncmp(p, query, query_len) == 0 &&
@@ -1070,16 +1174,18 @@ 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];
uint8_t *registers;
target_ulong addr, len;
+ GDBThreadIdKind thread_kind;
trace_gdbstub_io_command(line_buf);
p = line_buf;
ch = *p++;
@@ -1283,16 +1389,22 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
else
put_packet(s, "E22");
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;
}
switch (type) {
@@ -1308,12 +1420,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
put_packet(s, "E22");
break;
}
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");
} else {
put_packet(s, "E22");
--
2.19.1
On Thu, Nov 15, 2018 at 10:41:55AM +0100, Luc Michel wrote:
> 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.
>
> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Hi Luc,
> ---
> gdbstub.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 134 insertions(+), 18 deletions(-)
>
> diff --git a/gdbstub.c b/gdbstub.c
> index 4fbc05dfe3..fa2b7077b2 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -680,10 +680,73 @@ out:
> #else
> return s->processes[0].pid;
> #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 = find_cpu(tid);
> +
> + if (!tid) {
> + /* 0 means any thread, we take the first one */
> + tid = 1;
> + }
> +
> + if (cpu == NULL) {
> + return NULL;
> + }
Not sure about this. If tid is zero, you fix up the wildcard by setting tid to one.
Shouldn't you also retry find_cpu(tid) in that case?
Otherwise, tid doesn't seem to be used after the wildcard fixup.
Other than that, I think this looks good:
Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
> +
> + 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)
> {
> size_t len;
> int i;
> @@ -936,23 +999,10 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
>
> cpu_synchronize_state(cpu);
> 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)
> {
> if (s->multiprocess) {
> snprintf(buf, buf_size, "p%02x.%02x",
> @@ -962,10 +1012,64 @@ 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);
>
> return strncmp(p, query, query_len) == 0 &&
> @@ -1070,16 +1174,18 @@ 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];
> uint8_t *registers;
> target_ulong addr, len;
> + GDBThreadIdKind thread_kind;
>
> trace_gdbstub_io_command(line_buf);
>
> p = line_buf;
> ch = *p++;
> @@ -1283,16 +1389,22 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> else
> put_packet(s, "E22");
> 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;
> }
> switch (type) {
> @@ -1308,12 +1420,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
> put_packet(s, "E22");
> break;
> }
> 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");
> } else {
> put_packet(s, "E22");
> --
> 2.19.1
>
On 11/16/18 10:51 AM, Edgar E. Iglesias wrote:
> On Thu, Nov 15, 2018 at 10:41:55AM +0100, Luc Michel wrote:
>> 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.
>>
>> Signed-off-by: Luc Michel <luc.michel@greensocs.com>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>
> Hi Luc,
>
>
>
>> ---
>> gdbstub.c | 152 +++++++++++++++++++++++++++++++++++++++++++++++-------
>> 1 file changed, 134 insertions(+), 18 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 4fbc05dfe3..fa2b7077b2 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -680,10 +680,73 @@ out:
>> #else
>> return s->processes[0].pid;
>> #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 = find_cpu(tid);
>> +
>> + if (!tid) {
>> + /* 0 means any thread, we take the first one */
>> + tid = 1;
>> + }
>> +
>> + if (cpu == NULL) {
>> + return NULL;
>> + }
>
> Not sure about this. If tid is zero, you fix up the wildcard by setting tid to one.
> Shouldn't you also retry find_cpu(tid) in that case?
>
> Otherwise, tid doesn't seem to be used after the wildcard fixup.
Thanks for catching this. It's a refactoring mistake. It will be fixed
in the next re-roll.
--
Luc
>
> Other than that, I think this looks good:
> Reviewed-by: Edgar E. Iglesias <edgar.iglesias@xilinx.com>
>
>
>
>> +
>> + 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)
>> {
>> size_t len;
>> int i;
>> @@ -936,23 +999,10 @@ static void gdb_set_cpu_pc(GDBState *s, target_ulong pc)
>>
>> cpu_synchronize_state(cpu);
>> 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)
>> {
>> if (s->multiprocess) {
>> snprintf(buf, buf_size, "p%02x.%02x",
>> @@ -962,10 +1012,64 @@ 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);
>>
>> return strncmp(p, query, query_len) == 0 &&
>> @@ -1070,16 +1174,18 @@ 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];
>> uint8_t *registers;
>> target_ulong addr, len;
>> + GDBThreadIdKind thread_kind;
>>
>> trace_gdbstub_io_command(line_buf);
>>
>> p = line_buf;
>> ch = *p++;
>> @@ -1283,16 +1389,22 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> else
>> put_packet(s, "E22");
>> 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;
>> }
>> switch (type) {
>> @@ -1308,12 +1420,16 @@ static int gdb_handle_packet(GDBState *s, const char *line_buf)
>> put_packet(s, "E22");
>> break;
>> }
>> 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");
>> } else {
>> put_packet(s, "E22");
>> --
>> 2.19.1
>>
© 2016 - 2025 Red Hat, Inc.