Re: [PATCH] Fix MI output for multi-location breakpoints

[Simon Marchi <simon.marchi@ericsson.com>]

>>>> -@samp{--interpreter=mi} (or @samp{--interpreter=mi2}) causes
>>>> +@samp{--interpreter=mi} (or @samp{--interpreter=mi3}) causes
>>>>  @value{GDBN} to use the @dfn{@sc{gdb/mi} interface} (@pxref{GDB/MI, ,
>>>> -The @sc{gdb/mi} Interface}) included since @value{GDBN} version 6.0.  The
>>>> -previous @sc{gdb/mi} interface, included in @value{GDBN} version 5.3 and
>>>> -selected with @samp{--interpreter=mi1}, is deprecated.  Earlier
>>>> -@sc{gdb/mi} interfaces are no longer supported.
>>>> +The @sc{gdb/mi} Interface}).  The @sc{gdb/mi} interfaces 1 and 2 are
>>>> +available, but deprecated.  Earlier @sc{gdb/mi} interfaces are no
>>>> +longer available.
>>>
>>> Please don't remove the version information from this text.  At the
>>> very least, we should tell what GDB version introduced the latest mi3
>>> syntax.  We should tell this here, and not only in the "Interpreters"
>>> node, because this section is a concise list of invocation options,
>>> and should include the important information without sending the
>>> reader to read the more detailed parts.
>>
>> The reason I thought this was not really appropriate here is that this manual
>> applies to a particular version of GDB (e.g. it's the manual shipped with
>> GDB 9.1).  So if you are using that version of GDB, it doesn't really matter
>> which version of GDB introduced mi3, all that matters is that it exists now.
> 
> People read newer manuals even if they have older versions of GDB
> available.  E.g., I have on my development system all the versions of
> GDB since v6.3, but only one version of the manual, the latest one.
> Since Texinfo doesn't provide a way of installing several versions of
> the same manual side by side, we should assume that my situation is
> quite typical.
> 
> So let's not assume that when one reads a manual for GDB X.Y they are
> interested only in that GDB version.

But we could say that for every single feature.  We introduced (or rather, will
introduce) "set index-cache" in GDB 8.3, and we don't mention in the doc that it
exists since 8.3.

>> As you said, this section is a concise list of invocation options.  But I don't
>> consider this very important information to have at the fingertips.
> 
> I respectfully disagree.  A manual is more often than not read as a
> reference, where the reader wants to quickly find the information they
> are after, and stop reading right after that.  We should cater to such
> usage of the manual, since it's IME the most important use.

I agree that the general use case of the manual is not to read it from cover to cover.

>> As Andrew suggested, I think we should have a list or table of all mi versions,
>> when they were introduced, and what the breaking changes are.  This would help
>> people writing or upgrading their MI frontend.   But I don't think that the
>> casual user looking for the possible command line flags will need to know the
>> history of MI.
> 
> I'm okay with adding a detailed table elsewhere, but I don't think
> it's a good idea to remove the above information from the description
> of the -mi command-line switch.

Would a cross-reference to the table be good enough?  If we add a detailed table
of MI versions (which I think is required), we will have the information about which
GDB version introduced each MI versions at three separate places in the manual...
I think this just adds unnecessary overhead for when we want to add a new MI version
(I know this doesn't happen often, but still).

Actually, there is already a cross reference just above.  I would consider rewording
things a bit to be clear that "to know more about the available interpreters, see
Command Interpreters".

>> Finally, the goal is to reduce the duplication of information, so there is less
>> things to update when releasing a new MI version (therefore less chance of having
>> outdated information).
> 
> That is not the main goal, though.  The main goal is to produce a
> useful manual that allows finding the information one needs quickly
> and efficiently.

Right, I meant "my goal", not the goal of the GDB manual.  I believe a cross-reference
would allow people find that information quickly and efficiently.

>>>>  @item mi1
>>>>  @cindex mi1 interpreter
>>>> -The @sc{gdb/mi} interface included in @value{GDBN} 5.1, 5.2, and 5.3.
>>>> +The @sc{gdb/mi} interface introduced in @value{GDBN} 5.1.
>>>
>>> I think the old text is better.
>>
>> The problem I see with the current wording is that it implies that mi1 is included
>> in version 5.1, 5.2 and 5.3 only.
> 
> That's not my interpretation of "included in".
>>> In reality, it's included in all versions from 5.1 and up.  So it
>> seems odd to list 5.2 and 5.3 specifically.  Do you have a
>> suggestion to address this?
> 
> Say "used by" instead of "included in"?

Ok, so there are two things bothering me in the current text, and I am not sure if
we are in disagreement with both or just one.

- mi1 can be used by any GDB version between 5.1 and the present.  Why do we list
  specifically 5.1, 5.2 and 5.3?  It's not untrue to say that mi1 is available in
  5.1, 5.2 and 5.3, but it's ambiguous whether this is an exhaustive list or not.
  Better just say it's 5.1 and up.
- The term "included in", or "used by", regardless of whether it is coupled with "5.1"
  or 5.1, 5.2 and 5.3".  Since mi1 can be used with any version between 5.1 and the
  latest release, I think we should be looking for a phrasing that conveys that it's
  an half-open interval.  Saying that "mi1 is used by GDB 5.1" just tells you about
  5.1.  Saying that "mi1 is available starting with GDB 5.1" tells you about all
  versions between 5.1 and the current one.

Hopefully that clears up why I see the need to re-word this.


>>> P.S. I wonder how did we let this problem slip through the cracks when
>>> multiple-location breakpoints were introduced?  Maybe we should do
>>> something to avoid such mistakes in the future.  We really shouldn't
>>> be changing the MI syntax in incompatible ways so late into GDB
>>> development cycle.
>>
>> Well it's been known for a long time, as shows this bug from 2008 (AFAIU,
>> multiple location breakpoints were introduced around 2007?):
>>
>> https://sourceware.org/bugzilla/show_bug.cgi?id=9659
>>
>> I've been meaning to fix this for a few years, but always put it aside (until now)
>> because of the headache introducing backwards-incompatible MI changes represents.
> 
> That's not what bothered me.  What bothered me was that we released a
> GDB with this MI syntax without fixing it first.  I'm wondering how to
> prevent such mistakes in the future.

The problem here is that we had a tuple without name/id in a context where this
should be forbidden.  Maybe we could add some assert in ui_out::begin to catch
this.

Also, these MI commands and async events weren't properly tested in the testsuite.
But even if they were, our tests aren't bulletproof, since we match MI output textually.
We don't confirm that the output has valid MI syntax.

Instead of matching the MI output textually, we should have a proper MI output parser
written in TCL.  This would mean that any output produced by GDB in MI could be ran
through this parser, and any non-conforming output would cause an error.

I don't know if anybody is ready to write such a parser.  An idea out there would be
for GDB to use JSON for MI, instead of its own format (which is close to JSON anyway).
Then we could probably use an existing library, such as

  https://core.tcl.tk/tcllib/doc/trunk/embedded/www/tcllib/files/modules/json/json.html

making the process much easier.

Thanks,

Simon