Re: [RFC PATCH 0/5] Deprecate/rename singlestep command line option

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > The command line option '-singlestep' and its HMP equivalent
> > the 'singlestep' command are very confusingly named, because
> > they have nothing to do with single-stepping the guest (either
> > via the gdb stub or by emulation of guest CPU architectural
> > debug facilities). What they actually do is put TCG into a
> > mode where it puts only one guest instruction into each
> > translation block. This is useful for some circumstances
> > such as when you want the -d debug logging to be easier to
> > interpret, or if you have a finicky guest binary that wants
> > to see interrupts delivered at something other than the end
> > of a basic block.
> >
> > The confusing name is made worse by the fact that our
> > documentation for these is so minimal as to be useless
> > for telling users what they really do.
> >
> > This series:
> >  * renames the 'singlestep' global variable to 'one_insn_per_tb'
> >  * Adds new '-one-insn-per-tb' command line options and a
> >    HMP 'one-insn-per-tb' command
> >  * Documents the '-singlestep' options and 'singlestep'
> >    HMP command as deprecated synonyms for the new ones
> >
> > It does not do anything about the other place where we surface
> > 'singlestep', which is in the QMP StatusInfo object returned by the
> > 'query-status' command.  This is incorrectly documented as "true if
> > VCPUs are in single-step mode" and "singlestep is enabled through
> > the GDB stub", because what it's actually returning is the
> > one-insn-per-tb state.
> >
> > Things I didn't bother with because this is only an RFC but
> > will do if it turns into a non-RFC patchset:
> >  * test the bsd-user changes :-)
> >  * add text to deprecated.rst
> >
> > So, questions:
> >
> > (1) is this worth bothering with at all? We could just
> >     name our global variable etc better, and document what
> >     -singlestep actually does, and not bother with the new
> >     names for the options/commands.
> 
> The feature is kind of esoteric.  Rather weak excuse for not fixing bad
> UI, in my opinion.  Weaker still since you already did a good part of
> the actual work.
> 
> > (2) if we do do it, do we retain the old name indefinitely,
> >     or actively put it on the deprecate-and-drop list?
> 
> By "the old name", you mean the CLI option name, right?
> 
> I'd prefer deprecate and drop.
> 
> > (3) what should we do about the HMP StatusInfo object?
> >     I'm not sure how we handle compatibility for HMP.
> 
> Uh, you mean *QMP*, don't you?
> 
> As you wrote above, StatusInfo is returned by query-status, which is a
> stable interface.  Changes to members therefore require the usual
> deprecation grace period.  We'd add a new member with a sane name, and
> deprecate the old one.
> 
> The matching HMP command is "info status".  It shows member @singlestep
> as " (single step mode)".  Changing that is fine; HMP is not a stable
> interface.

Right, and similarly you don't need to keep the old 'singlestep' command
around; you can just rename.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK