| Message ID | 20180912081847.3228.15101.stgit@pasha-VirtualBox (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixing record/replay and adding reverse debugging | expand |
Pavel Dovgalyuk <Pavel.Dovgaluk@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.Dovgaluk@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. > + > +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? > + 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) > + 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? > +extern int64_t replay_break_step; > +/*! Timer for the replay breakpoint callback */ Likewise. > +extern QEMUTimer *replay_break_timer; > > void replay_put_byte(uint8_t byte); > void replay_put_event(uint8_t event); [...]
> From: Markus Armbruster [mailto:armbru@redhat.com] > Pavel Dovgalyuk <Pavel.Dovgaluk@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.Dovgaluk@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. > > > + 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. Pavel Dovgalyuk
"Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: >> From: Markus Armbruster [mailto:armbru@redhat.com] >> Pavel Dovgalyuk <Pavel.Dovgaluk@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.Dovgaluk@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.
On 24 September 2018 at 14:02, Markus Armbruster <armbru@redhat.com> wrote: > "Pavel Dovgalyuk" <dovgaluk@ispras.ru> writes: >> 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. This is on my todo list. (It got bumped by more important things and also because somebody else said they were going to look at it, and then it got bumped off *their* todo list by more important things :-)) > Adding Doxygen-style to the mix can only make this harder. Please don't. Agreed on this part. The doc comment format we want to converge on is Linux-kernel-style doc-comments. (The plan is to use Sphinx and borrow the kernel's Sphinx plugin for doc-comment handling.) thanks -- PMM
On 24/09/2018 15:12, Peter Maydell wrote: > It got bumped by more important things > and also because somebody else said they were going to look at it, > and then it got bumped off *their* todo list by more important > things :-)) I sense the force calling me... Well, my plans were did not actually involve GTKDoc and the developer documentation, but rather the rest of the manual. What I wanted to do, but didn't manage to do, was to work on a new table of contents for the manual (not so much for the developer documentation), so that it could be converted to rST and generated with Sphinx. Converting Texinfo to rST is not trivial, but it seems doable via makeinfo's Docbook backend and pandoc. (I still don't like rST, but I guess that ship has sailed, also because Linux's scripts/kernel-doc has since dropped all backends but rST and man, so my other experiment of a Texinfo/Docbook backend for Sphinx is dead). Paolo
On 24 September 2018 at 17:50, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 24/09/2018 15:12, Peter Maydell wrote: >> It got bumped by more important things >> and also because somebody else said they were going to look at it, >> and then it got bumped off *their* todo list by more important >> things :-)) > > I sense the force calling me... Well, my plans were did not actually > involve GTKDoc and the developer documentation, but rather the rest of > the manual. What I wanted to do, but didn't manage to do, was to work > on a new table of contents for the manual (not so much for the developer > documentation), so that it could be converted to rST and generated with > Sphinx. Converting Texinfo to rST is not trivial, but it seems doable > via makeinfo's Docbook backend and pandoc. I think Kashyap was the other person who'd expressed interest in doing something around Sphinx ? thanks -- PMM
On 24/09/2018 19:14, Peter Maydell wrote: > On 24 September 2018 at 17:50, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 24/09/2018 15:12, Peter Maydell wrote: >>> It got bumped by more important things >>> and also because somebody else said they were going to look at it, >>> and then it got bumped off *their* todo list by more important >>> things :-)) >> >> I sense the force calling me... Well, my plans were did not actually >> involve GTKDoc and the developer documentation, but rather the rest of >> the manual. What I wanted to do, but didn't manage to do, was to work >> on a new table of contents for the manual (not so much for the developer >> documentation), so that it could be converted to rST and generated with >> Sphinx. Converting Texinfo to rST is not trivial, but it seems doable >> via makeinfo's Docbook backend and pandoc. > > I think Kashyap was the other person who'd expressed interest > in doing something around Sphinx ? Yes, but I think his interest was also more in user (he works on OpenStack) documentation than QEMU developer docuementation. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 24/09/2018 15:12, Peter Maydell wrote: >> It got bumped by more important things >> and also because somebody else said they were going to look at it, >> and then it got bumped off *their* todo list by more important >> things :-)) > > I sense the force calling me... Well, my plans were did not actually > involve GTKDoc and the developer documentation, but rather the rest of > the manual. What I wanted to do, but didn't manage to do, was to work > on a new table of contents for the manual (not so much for the developer > documentation), so that it could be converted to rST and generated with > Sphinx. Converting Texinfo to rST is not trivial, but it seems doable > via makeinfo's Docbook backend and pandoc. Remind me, why would rST be an improvement? > (I still don't like rST, but I guess that ship has sailed, also because > Linux's scripts/kernel-doc has since dropped all backends but rST and > man, so my other experiment of a Texinfo/Docbook backend for Sphinx is > dead). > > Paolo
On Mon, Sep 24, 2018 at 08:44:07PM +0200, Markus Armbruster wrote:
[...]
> Remind me, why would rST be an improvement?
Relatively easy on the eye, more maintainable, active community around
the Sphinx tooling / extensions, not extremely arcane syntax (besides
some weird quirks), etc.
Take a look at its output (from a random page out of kernel
documentation):
https://www.kernel.org/doc/html/v4.11/driver-api/80211/mac80211-advanced.html
LWN did some analysis[1][2] on how the Linux kernel arrived at rST (with
Sphinx).
An example rendering of a document from the QEMU Git tree:
https://kashyapc.fedorapeople.org/QEMU-Docs/_build/html/docs/interop/live-block-operations.html
Not sure if that's improvement in your judgement, though. :-)
[...]
[1] https://lwn.net/Articles/692704/
[2] https://lwn.net/Articles/692705/
On Mon, Sep 24, 2018 at 07:38:28PM +0200, Paolo Bonzini wrote: > On 24/09/2018 19:14, Peter Maydell wrote: > > On 24 September 2018 at 17:50, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 24/09/2018 15:12, Peter Maydell wrote: > >>> It got bumped by more important things > >>> and also because somebody else said they were going to look at it, > >>> and then it got bumped off *their* todo list by more important > >>> things :-)) > >> > >> I sense the force calling me... Well, my plans were did not actually > >> involve GTKDoc and the developer documentation, but rather the rest of > >> the manual. What I wanted to do, but didn't manage to do, was to work > >> on a new table of contents for the manual (not so much for the developer > >> documentation), so that it could be converted to rST and generated with > >> Sphinx. Converting Texinfo to rST is not trivial, but it seems doable > >> via makeinfo's Docbook backend and pandoc. > > > > I think Kashyap was the other person who'd expressed interest > > in doing something around Sphinx ? Afraid, I couldn't manage my time to concentrate on the Sphinx stuff :-( And I'm occupied with existing things on my plate until November; I'll see if I can find some personal free time to get to this. Currently what I have is the 10 documents from the docs/ directory that are convereted to rST (some of them are already in QEMU Git); the below rendering is built from QEMU 3.0): https://kashyapc.fedorapeople.org/QEMU-Docs/_build/html/ TODO: Modify the Makefile in QEMU source to integrate Sphinx. > Yes, but I think his interest was also more in user (he works on > OpenStack) documentation than QEMU developer docuementation. Yeah, FWIW, I'm fine with QMP interfaces, functional usage of various QEMU APIs (I spent most time playing with the block layer), command-line, and "things that libvirt consumes". But for documenting the internals (e.g. of emulated devices) such as structs, functions, their parameters, return types, etc -- I don't have the relevant in-depth expertise, as I don't spend a lot of time looking at QEMU code, nor the time it takes to dig into, I'm afraid. A decent chunk of the time I spend on QEMU is my own. And given the nature of my role (across the layers - OpenStack Nova, libvirt, and QEMU), I am forever catching up with the relevant bits that I'm working on, and struggle to effectively juggle it all; still learning. - - - Given the complexity and breadth of QEMU, it's easy to imagine more than a full-time role for high-quality user and developer documentation. As you need to spend time soaking into the code, actively follow the upstream mailing list(s), try patch sets while they're in flux, write test programs, and _then_ there is the "small matter of detailed documentation". Reminds me of the inimitable Michael Kerrisk of Linux 'man-pages'.
On 24/09/2018 20:44, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 24/09/2018 15:12, Peter Maydell wrote: >>> It got bumped by more important things >>> and also because somebody else said they were going to look at it, >>> and then it got bumped off *their* todo list by more important >>> things :-)) >> >> I sense the force calling me... Well, my plans were did not actually >> involve GTKDoc and the developer documentation, but rather the rest of >> the manual. What I wanted to do, but didn't manage to do, was to work >> on a new table of contents for the manual (not so much for the developer >> documentation), so that it could be converted to rST and generated with >> Sphinx. Converting Texinfo to rST is not trivial, but it seems doable >> via makeinfo's Docbook backend and pandoc. > > Remind me, why would rST be an improvement? Personally I don't think it is an improvement over Texinfo, I find it to be the Perl of ASCII-based markups. :) In fact in my infamous poll (the one where I asked "which one would be hard to read/write before and after showing an example), Texinfo was the only one that got a better score after showing an example, and rST was the only one that got a worse score after the example. Still, rST got a higher score than Texinfo so others disagree with me and you. However, we currently have the worst of both worlds. We have a manual in Texinfo that almost nobody updates (except for the small parts that are generated from .hx files in the code) and sparse documentation written in a mix of rST, Markdown-but-not-quite, and not-even-trying-to-be-Markdown. The main disadvantage of Texinfo is that it's insanely hard to parse; it's pretty much the only format for which pandoc has a producer but not a reader. The silver lining is that makeinfo can produce both some strange XML and Docbook, but still connecting Texinfo to Sphinx rrequired a combination of Makeinfo to convert it to Docbook, XSLT to split the resulting file by chapter, and then a custom Sphinx parser for Docbook. The result was pretty good (http://people.redhat.com/pbonzini/qemu-test-doc/_build/html/) but I'm not sure I want to inflict that on the community, I've already used all my karma on block comment style. :) Paolo >> (I still don't like rST, but I guess that ship has sailed, also because >> Linux's scripts/kernel-doc has since dropped all backends but rST and >> man, so my other experiment of a Texinfo/Docbook backend for Sphinx is >> dead). >> >> Paolo >
On 25 September 2018 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote: > However, we currently have the worst of both worlds. We have a manual > in Texinfo that almost nobody updates (except for the small parts that > are generated from .hx files in the code) and sparse documentation > written in a mix of rST, Markdown-but-not-quite, and > not-even-trying-to-be-Markdown. One improvement I've thought about here would be having board documentation (eg "what is this board, what devices does it implement or not yet implement") be in a doc comment in the relevant source file, and then we could stitch those together into the user-facing docs. Right now the texinfo (as you say) has a really outdated list of boards and info on them. thanks -- PMM
On 25 September 2018 at 09:58, Kashyap Chamarthy <kchamart@redhat.com> wrote: > Currently what I have is the 10 documents from the docs/ directory that > are convereted to rST (some of them are already in QEMU Git); the below > rendering is built from QEMU 3.0): > https://kashyapc.fedorapeople.org/QEMU-Docs/_build/html/ Did you need to make changes to the qemu git tree (sphinx config files, etc) to do that, or is it simply "hand run sphinx on the rst files, publish them" ? > Given the complexity and breadth of QEMU, it's easy to imagine more than > a full-time role for high-quality user and developer documentation. As > you need to spend time soaking into the code, actively follow the > upstream mailing list(s), try patch sets while they're in flux, write > test programs, and _then_ there is the "small matter of detailed > documentation". Reminds me of the inimitable Michael Kerrisk of Linux > 'man-pages'. Definitely. I think what we should aim for to start with is to have the basic framework in place (so that for instance we do build the rst files in docs/ into html which we ship/publish). I think it's easier for people to make contributions to improving the docs if they're just putting another brick into a pre-existing framework, rather than having to tackle the structural issues first. thanks -- PMM
On Tue, Sep 25, 2018 at 11:15:05AM +0100, Peter Maydell wrote: > On 25 September 2018 at 09:58, Kashyap Chamarthy <kchamart@redhat.com> wrote: > > > Currently what I have is the 10 documents from the docs/ directory that > > are convereted to rST (some of them are already in QEMU Git); the below > > rendering is built from QEMU 3.0): > > https://kashyapc.fedorapeople.org/QEMU-Docs/_build/html/ > > Did you need to make changes to the qemu git tree (sphinx > config files, etc) to do that, or is it simply "hand run > sphinx on the rst files, publish them" ? It's the latter -- hand-run Sphinx on the rST files. You can see the slightly modified 'conf.py' (you have to download it; can't view in the browser) and 'Makefile' (IIRC, unmodified) if you traverse two directories backwards: https://kashyapc.fedorapeople.org/QEMU-Docs/. To produce that, I just ran the `sphinx-quickstart` (the documentation template generator) from a shell, which asks a bunch of questions (it can be automated, of course); then it creates all the necessary files: conf.py, index.rst, Makefile, et al. And once you have the docs/ directory populated, update the 'toctree', then it's a trivial `make html`. > > Given the complexity and breadth of QEMU, it's easy to imagine more than > > a full-time role for high-quality user and developer documentation. As > > you need to spend time soaking into the code, actively follow the > > upstream mailing list(s), try patch sets while they're in flux, write > > test programs, and _then_ there is the "small matter of detailed > > documentation". Reminds me of the inimitable Michael Kerrisk of Linux > > 'man-pages'. > > Definitely. I think what we should aim for to start with is to > have the basic framework in place (so that for instance we do > build the rst files in docs/ into html which we ship/publish). > I think it's easier for people to make contributions to improving > the docs if they're just putting another brick into a pre-existing > framework, rather than having to tackle the structural issues first. Yeah, very true; you articulate the issue more clearly.
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); +} + +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()) { + replay_break(step, replay_stop_vm, NULL); + } else { + error_setg(errp, "cannot set break at the step in the past"); + } + } else { + error_setg(errp, "setting the break is allowed only in play mode"); + } +} + +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); + 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 */ +extern int64_t replay_break_step; +/*! Timer for the replay breakpoint callback */ +extern QEMUTimer *replay_break_timer; void replay_put_byte(uint8_t byte); void replay_put_event(uint8_t event); diff --git a/replay/replay.c b/replay/replay.c index 80afbd5..a106211 100644 --- a/replay/replay.c +++ b/replay/replay.c @@ -34,6 +34,10 @@ static char *replay_filename; ReplayState replay_state; static GSList *replay_blockers; +/* Replay breakpoints */ +int64_t replay_break_step = -1LL; +QEMUTimer *replay_break_timer; + bool replay_next_event_is(int event) { bool res = false; @@ -73,6 +77,13 @@ int replay_get_instructions(void) replay_mutex_lock(); if (replay_next_event_is(EVENT_INSTRUCTION)) { res = replay_state.instructions_count; + if (replay_break_step != -1LL) { + uint64_t current = replay_get_current_step(); + assert(replay_break_step >= current); + if (current + res > replay_break_step) { + res = replay_break_step - current; + } + } } replay_mutex_unlock(); return res; @@ -99,6 +110,12 @@ void replay_account_executed_instructions(void) will be read from the log. */ qemu_notify_event(); } + /* Execution reached the break step */ + if (replay_break_step == replay_state.current_step) { + /* Cannot make callback directly from the vCPU thread */ + timer_mod_ns(replay_break_timer, + qemu_clock_get_ns(QEMU_CLOCK_REALTIME)); + } } } }
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.Dovgaluk@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(+)