To reach gdb_set_stop_cpu() with gdbserver_state == NULL, you previously
> entered gdb_vm_state_change() with and use CPUState *cpu =
> gdbserver_state->c_cpu = NULL deref, which shouldn't happen.
> Also in gdb_set_stop_cpu() you finally call cpu_single_step(cpu=crap)
> which then deref crap->singlestep_enabled.
>
> So I think this is not the correct fix.
>
I think you are wrong. You can enter gdb_vm_state_change() only if it has
been registered through qemu_add_vm_change_state_handler(). And this
happens in gdbserver_start() which is called only when you start the
gdbserver stub.
This is exactly what we don't want to do here: use qemu breakpoints and
debug events forwarding without the need to enable a gdb stub.
There might be an historical reason that vCPU debug events are always
forwarded to the gdbserver (from cpu_handle_guest_debug()) but this
should not be mandatory.
One could consider a check to the gdbserver state right before:
if (gdbserver_enabled())
gdb_set_stop_cpu(cpu);
As found in other part of qemu code: kvm_enabled, hax_enabled, ...
> Since this shouldn't happen, I'd add an assert(gdbserver_state) in
> gdb_set_stop_cpu(), and clean gdb_vm_state_change()'s code flow to not
> reach this.
>
Correct if you previously add the gdbserver_enabled() check. Else this
would abort the qemu instance each time a debug event is triggered
and forwarded to a vm_change_state handler.
>>> void gdb_set_stop_cpu(CPUState *cpu)
> >>> {
> >>> + if (!gdbserver_state) {
> >>> + return;
> >>> + }
> >>> gdbserver_state->c_cpu = cpu;
> >>> gdbserver_state->g_cpu = cpu;
>
> I find it safer the opposite way, if (s) { s-> ... }
>
Sincerely, this argument is subjective. If it's part of Qemu coding
standard,
i would agree. Again there is a lot of situations in the Qemu code where
exit conditions are checked first and then lead to a return, preventing an
unneeded level of indentation. Seriously, we are talking about a 2 lines
function.
stephane
