Re: [Qemu-devel] [PATCH v6 10/25] replay: introduce breakpoint at the specified step

[Markus Armbruster]

"Pavel Dovgalyuk" <address@hidden> writes:

>> From: Markus Armbruster [mailto:address@hidden
>> Pavel Dovgalyuk <address@hidden> 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 <address@hidden>
>> >
>> > --
>> >
>> > 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
>> > address@hidden replay_break @var{step}
>> > address@hidden 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.