Re: [Qemu-devel] [PATCH v1 2/2] gdbstub: Fix vCont behaviour

[Paolo Bonzini]

On 10/10/2016 13:50, Claudio Imbrenda wrote:
> +        /*
> +         * XXX vm_start also calls qemu_vmstop_requested(&requested); here, 
> is
> +         * it actually important? it's static in vl.c
> +         */

Yes, it is, :) and so is qapi_event_send_resume (which is automatically
generated in qapi-event.c).

You can make qemu_vmstop_requested non-static, but the right thing to do is:

1) move vm_start to cpus.c

2) move qemu_clock_enable from resume_all_cpus to vm_start

3) extract vm_start_noresume out of vm_start and call it here.

Another suggestion is to extract the whole handling of vCont into a
separate function; not just gdb_continue_partial.  And because the logic
for actions is quite complex:

> +        if (def == 0) {
> +            for (cx = 0; scpus && scpus[cx]; cx++) {
> +                cpu_single_step(scpus[cx], sstep_flags);
> +                cpu_resume(scpus[cx]);
> +            }
> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
> +                cpu_resume(ccpus[cx]);
> +            }
> +        } else if (def == 'c' || def == 'C') {
> +            for (cx = 0; scpus && scpus[cx]; cx++) {
> +                cpu_single_step(scpus[cx], sstep_flags);
> +            }
> +            CPU_FOREACH(cpu) {
> +                cpu_resume(cpu);
> +            }
> +        } else if (def == 's' || def == 'S') {
> +            CPU_FOREACH(cpu) {
> +                cpu_single_step(cpu, sstep_flags);
> +            }
> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
> +                cpu_single_step(cpu, 0);
> +            }
> +            CPU_FOREACH(cpu) {
> +                cpu_resume(cpu);
> +            }

it looks better with a couple helper functions:

            if (def == 's' || def == 'S') {
                /* single-step all CPUs but ccpus */
                gdb_single_step_cpus(NULL, sstep_flags);
                gdb_single_step_cpus(ccpus, 0);
                resume_all_cpus();
            } else {
                gdb_single_step_cpus(scpus, sstep_flags);
                if (def == 0) {
                    gdb_resume_cpus(scpus);
                    gdb_resume_cpus(ccpus);
                } else {
                    resume_all_cpus();
                }
            }

Also:

> +                    if (*p == ':') {
> +                        if (broadcast_action) {
> +                            res = -22;
> +                            break;
> +                        }
> +                        p++;
> +                        if ((p[0] == '-') && (p[1] == '1')) {
> +                            if (broadcast_action || scnt || ccnt) {

You can't get here with broadcast_action != 0, it's checked above.

You're also not implementing this on user-mode emulation, but you're not
documenting this anywhere.  Perhaps user-mode emulation should not
support vCont at all unless gdbstub is moved to a separate thread
altogether?

Finally, some more stylistic choices:

1) use g_new instead of g_malloc.

2) the handling of res is very messy.  What do -1 and -22 mean?

3) Please do error checking on qemu_strtoul

Thanks,

Paolo

> +        cpu_enable_ticks();
> +        runstate_set(RUN_STATE_RUNNING);
> +        vm_state_notify(1, RUN_STATE_RUNNING);
> +        qemu_clock_enable(QEMU_CLOCK_VIRTUAL, true);
> +        if (def == 0) {
> +            for (cx = 0; scpus && scpus[cx]; cx++) {
> +                cpu_single_step(scpus[cx], sstep_flags);
> +                cpu_resume(scpus[cx]);
> +            }
> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
> +                cpu_resume(ccpus[cx]);
> +            }
> +        } else if (def == 'c' || def == 'C') {
> +            for (cx = 0; scpus && scpus[cx]; cx++) {
> +                cpu_single_step(scpus[cx], sstep_flags);
> +            }
> +            CPU_FOREACH(cpu) {
> +                cpu_resume(cpu);
> +            }
> +        } else if (def == 's' || def == 'S') {
> +            CPU_FOREACH(cpu) {
> +                cpu_single_step(cpu, sstep_flags);
> +            }
> +            for (cx = 0; ccpus && ccpus[cx]; cx++) {
> +                cpu_single_step(cpu, 0);
> +            }
> +            CPU_FOREACH(cpu) {
> +                cpu_resume(cpu);
> +            }
> +        } else {
> +            res = -1;
> +        }
> +        /*
> +         * XXX vm_start also calls qapi_event_send_resume(&error_abort); 
> here,
> +         * is it actually important? moreover I can't find where it's 
> defined,
> +         * and calling it here yields a compiler error.
> +         */
> +        /* qapi_event_send_resume(&error_abort); */