"Pavel Dovgalyuk" <dovga...@ispras.ru> writes: >> From: Markus Armbruster [mailto:arm...@redhat.com] >> Pavel Dovgalyuk <pavel.dovga...@ispras.ru> writes: >> >> > This patch introduces replay_break qmp and hmp commands. >> > These commands allow stopping at the specified instruction. >> > It may be useful for debugging when there are some known >> > events that should be investigated. >> > The commands have one argument - number of instructions >> > executed since the start of the replay. >> > >> > Signed-off-by: Pavel Dovgalyuk <pavel.dovga...@ispras.ru> >> > >> > -- >> > >> > v2: >> > - renamed replay_break qmp command into replay-break >> > (suggested by Eric Blake) >> > --- >> > hmp-commands.hx | 15 ++++++++++++ >> > hmp.h | 1 + >> > include/sysemu/replay.h | 3 ++ >> > qapi/misc.json | 17 ++++++++++++++ >> > replay/replay-debugging.c | 55 >> > +++++++++++++++++++++++++++++++++++++++++++++ >> > replay/replay-internal.h | 4 +++ >> > replay/replay.c | 17 ++++++++++++++ >> > 7 files changed, 112 insertions(+) >> > >> > diff --git a/hmp-commands.hx b/hmp-commands.hx >> > index db0c681..aa0794e 100644 >> > --- a/hmp-commands.hx >> > +++ b/hmp-commands.hx >> > @@ -1888,6 +1888,21 @@ Set QOM property @var{property} of object at >> > location @var{path} to >> value @var{v >> > ETEXI >> > >> > { >> > + .name = "replay_break", >> > + .args_type = "step:i", >> > + .params = "step", >> > + .help = "sets breakpoint on the specified step of the >> > replay", >> > + .cmd = hmp_replay_break, >> > + }, >> > + >> > +STEXI >> > +@item replay_break @var{step} >> > +@findex replay_break >> > +Set breakpoint on the specified step of the replay. >> > +Execution stops when the specified step is reached. >> > +ETEXI >> > + >> > + { >> > .name = "info", >> > .args_type = "item:s?", >> > .params = "[subcommand]", >> > diff --git a/hmp.h b/hmp.h >> > index d792149..ad94abe 100644 >> > --- a/hmp.h >> > +++ b/hmp.h >> > @@ -149,5 +149,6 @@ void hmp_info_vm_generation_id(Monitor *mon, const >> > QDict *qdict); >> > void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict); >> > void hmp_info_sev(Monitor *mon, const QDict *qdict); >> > void hmp_info_replay(Monitor *mon, const QDict *qdict); >> > +void hmp_replay_break(Monitor *mon, const QDict *qdict); >> > >> > #endif >> > diff --git a/include/sysemu/replay.h b/include/sysemu/replay.h >> > index a5510f2..c9ba75a 100644 >> > --- a/include/sysemu/replay.h >> > +++ b/include/sysemu/replay.h >> > @@ -73,6 +73,9 @@ void replay_finish(void); >> > void replay_add_blocker(Error *reason); >> > /*! Returns name of the replay log file */ >> > const char *replay_get_filename(void); >> > +/*! Sets breakpoint at the specified step. >> > + If step = -1LL the existing breakpoint is removed. */ >> > +void replay_break(int64_t step, QEMUTimerCB callback, void *opaque); >> > >> > /* Processing the instructions */ >> > >> > diff --git a/qapi/misc.json b/qapi/misc.json >> > index e246ce3..4fcd211 100644 >> > --- a/qapi/misc.json >> > +++ b/qapi/misc.json >> > @@ -3135,6 +3135,23 @@ >> > 'returns': 'ReplayInfo' } >> > >> > ## >> > +# @replay-break: >> > +# >> > +# Set breakpoint on the specified step of the replay. >> > +# Execution stops when the specified step is reached. >> > +# >> > +# @step: execution step to stop at >> > +# >> > +# Since: 3.1 >> > +# >> > +# Example: >> > +# >> > +# -> { "execute": "replay-break", "data": { "step": 220414 } } >> > +# >> > +## >> > +{ 'command': 'replay-break', 'data': { 'step': 'int' } } >> > + >> > +## >> > # @xen-load-devices-state: >> > # >> > # Load the state of all devices from file. The RAM and the block devices >> > diff --git a/replay/replay-debugging.c b/replay/replay-debugging.c >> > index 2d364fa..c6b80c0 100644 >> > --- a/replay/replay-debugging.c >> > +++ b/replay/replay-debugging.c >> > @@ -16,6 +16,8 @@ >> > #include "hmp.h" >> > #include "monitor/monitor.h" >> > #include "qapi/qapi-commands-misc.h" >> > +#include "qapi/qmp/qdict.h" >> > +#include "qemu/timer.h" >> > >> > void hmp_info_replay(Monitor *mon, const QDict *qdict) >> > { >> > @@ -39,3 +41,56 @@ ReplayInfo *qmp_query_replay(Error **errp) >> > retval->step = replay_get_current_step(); >> > return retval; >> > } >> > + >> > +void replay_break(int64_t step, QEMUTimerCB callback, void *opaque) >> > +{ >> > + assert(replay_mode == REPLAY_MODE_PLAY); >> > + assert(replay_mutex_locked()); >> > + >> > + replay_break_step = step; >> > + if (replay_break_timer) { >> > + timer_del(replay_break_timer); >> > + timer_free(replay_break_timer); >> > + replay_break_timer = NULL; >> > + } >> > + >> > + if (replay_break_step == -1LL) { >> > + return; >> > + } >> > + assert(replay_break_step >= replay_get_current_step()); >> > + assert(callback); >> > + >> > + replay_break_timer = timer_new_ns(QEMU_CLOCK_REALTIME, callback, >> > opaque); >> > +} >> >> This function multiplexes >> >> (a) deleting the breakpoint: @step is -1, @callback and @opaque are >> ignored >> >> (b) setting the breakpoint: step must be >= replay_get_current_step() >> (which implies it's not -1), @callback must be non-null >> >> I hate such multiplexing. I'd be more willing to tolerate it for a >> static function. Why isn't it static? I can't see uses outside this >> file... >> >> To avoid the multiplexing, you could duplicate the function, specialize >> both copies, then factor out the common code into a helper function. > > Thanks, I'll split this into two functions. > >> >> > + >> > +static void replay_stop_vm(void *opaque) >> > +{ >> > + vm_stop(RUN_STATE_PAUSED); >> > + replay_break(-1LL, NULL, NULL); >> > +} >> > + >> > +void qmp_replay_break(int64_t step, Error **errp) >> > +{ >> > + if (replay_mode == REPLAY_MODE_PLAY) { >> > + if (step >= replay_get_current_step()) { >> >> Pardon another ignorant question: what ensures replay_get_current_step() >> stays put until we completed setting the breakpoint? > > In icount mode iothread and vcpu thread use the replay mutex for > serializing their operations. Therefore such races are impossible.
Good to know. >> >> > + replay_break(step, replay_stop_vm, NULL); >> > + } else { >> > + error_setg(errp, "cannot set break at the step in the past"); >> > + } >> >> Duplicates the >= replay_get_current_step() check we just saw in >> replay_break(). Consider >> >> if (replay_mode == REPLAY_MODE_PLAY) { >> replay_break(step, replay_stop_vm, NULL, errp); >> >> and setting an error in replay_break(). Suggestion, not demand. >> >> > + } else { >> > + error_setg(errp, "setting the break is allowed only in play >> > mode"); >> >> s/the break/the breakpoint/ >> >> > + } >> > +} >> > + >> > +void hmp_replay_break(Monitor *mon, const QDict *qdict) >> > +{ >> > + int64_t step = qdict_get_try_int(qdict, "step", -1LL); >> > + Error *err = NULL; >> > + >> > + qmp_replay_break(step, &err); >> > + if (err) { >> > + monitor_printf(mon, "replay_break error: %s\n", >> > error_get_pretty(err)); >> > + error_free(err); >> >> There's no need for the "replay_break error: " prefix; the user already >> knows what command he just issued. Thus: >> >> error_report_err(err) > > Ok. > >> > + return; >> > + } >> > +} >> > diff --git a/replay/replay-internal.h b/replay/replay-internal.h >> > index 4f82676..f9cad9d 100644 >> > --- a/replay/replay-internal.h >> > +++ b/replay/replay-internal.h >> > @@ -91,6 +91,10 @@ extern ReplayState replay_state; >> > >> > /* File for replay writing */ >> > extern FILE *replay_file; >> > +/*! Step of the replay breakpoint */ >> >> Why the bang? > > This is a doxygen-style comment for automatic documentation generation. We don't generate documentation, yet. We do have many GTKDoc-style comments. Perhaps because we hope that if we only add enough of them, they'll become sentient and edit the Makefiles to generate documentation. Adding Doxygen-style to the mix can only make this harder. Please don't.