Re: [Qemu-devel] [PATCH v8 05/16] gdbstub: add multiprocess support to vCont packets

[Luc Michel]

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 <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> Reviewed-by: Edgar E. Iglesias <address@hidden>
>> Acked-by: Alistair Francis <address@hidden>
>> ---
>>   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/address@hidden/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