Re: [Qemu-arm] [PATCH v6 04/16] gdbstub: add multiprocess support to 'H' and 'T' packets

[Luc Michel]

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 <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> 
> 
> 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 <address@hidden>
> 
> 
> 
>> +
>> +    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
>>