Hi Stephane, On 07/11/2018 04:52 AM, stephane duverger wrote: > 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.
Well, at least we agree the gdb stub code is not straightforward. And apparently without seeing the bigger picture about how you are using this, I am missing something. > 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. This is indeed a personal subjective suggestion, not part of QEMU Coding standard. Rational is, if in the future someone needs to modify gdb_set_stop_cpu(), it would be easier/safer for him. No worries ;) Regards, Phil.
signature.asc
Description: OpenPGP digital signature