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

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

On 2019-01-11 7:34 a.m., Andrew Burgess wrote:
> * Simon Marchi <simon.marchi@ericsson.com> [2019-01-11 00:15:34 +0000]:
> 
>> [CCing Pedro because we had some discussions earlier about that offline]
>>
>> Various MI commands or events related to breakpoints output invalid MI
>> records when printing information about a multi-location breakpoint.
>> For example:
>>
>>     -break-insert allo
>>     ^done,bkpt={...,addr="<MULTIPLE>",...},{number="1.1",...},{number="1.2",...}
>>
>> The problem is that according to the syntax [1], the top-level elements
>> are of type "result" and should be of the form "variable=value".
>>
>> This patch changes the output to wrap the locations in a list:
>>
>>     ^done,bkpt={...,addr="<MULTIPLE>",...},locations=[{number="1.1",...},{number="1.2",...}]
>>
>> The events =breakpoint-created, =breakpoint-modified, as well as the
>> -break-info command also suffer from this (and maybe others I didn't
>> find).
>>
>> Since this is a breaking change for MI, we have to deal somehow with
>> backwards compatibility.  The approach taken by this patch is to bump
>> the MI version, use the new syntax in MI3 while retaining the old syntax
>> in MI2.  Frontends are expected to use a precise MI version (-i=mi2), so
>> if they do that they should be unaffected.
>>
>> My previous attempts of this patch (not published) introduced new
>> commands (-break-insert2, -break-info2) or a flag to the original
>> commands to make them output the fixed syntax.  That doesn't really work
>> because async events need to be fixed too.
>>
>> Another solution would be to have a setting or command
>> (-use-fixed-breakpoint-output) to enable the fixed output, keeping the
>> broken one by default.  The only thing I don't like about that is that
>> we will keep the broken behavior by default, which means that somebody
>> writing a frontend who is not aware of that gotcha will go through the
>> trouble of stumbling on broken MI output, and then hopefully discover
>> that there is a command to un-break it.  I also don't see an easy way to
>> deprecate and remove the old output over time using this strategy.  With
>> a new MI version, somebody starting from scratch will use the latest MI
>> version available, and therefore the fixed version.  Also, we can
>> deprecate and then remove old MI versions after, let's say, 5 years of a
>> newer version being available.
> 
> I think fixing issues like this is a good thing, but I wonder if we
> should move a little slower.
> 
> With the next release only days away, could we not include this fix
> for the release, but leave MI2 as the default.
> 
> MI3 would still exist, and include the fix, but would be documented as
> unstable, or under-development, with the caveat that things could
> change in the MI3 output.
> 
> Then between the next release (few days) and the release after (~6
> months?) we can go crazy looking for MI fixes.  Then before the next
> release we bump the default version to MI3.

That's what I intended (I should have noted it in my original message), to
have this merge post-8.3.  It,s been broken for 12 years, nobody will cry
if 8.3 still has this bug.

> 
> UI developers can still use MI2 until they switch to MI3, but, when
> they do switch to MI3 they get more fixes than just one item.
> 
> Further, I think we should make the lives of UI developers easier, but
> having an explicit list in the documentation of what changed between
> MI versions (starting from MI2 -> MI3) including examples.  Yes we
> have the NEWS file, but (my personal opinion) the docs should stand on
> their own for these sort of changes.

Agreed, it's documentation that should be kept across releases (so, in the
doc), because frontend developers may upgrade from mi2 to mi3 a few versions
in the future, and we want the info about the backwards-incompatible changes
all in one place.

> The above only really makes sense if we think there might be other
> issues that could benefit from being fixed in the MI.  If we really
> feel this is the only bug out there, then maybe we should just push
> ahead with this patch as is...

As an example, I would be considering getting rid of BreakpointTable in the
-break-list output.

> In summary, I think we should:
> 
>   1. Merge this patch, but leave MI2 the default, add a new section to
>   the docs listing "Changes Between MI2 and MI3".
> 
>   2. Change GDB so that when a user starts with -i=mi3 they get a
>   warning that this version of the MI is under development, and liable
>   to change, this should be documented too.
> 
>   3. Between now and the next + 1 release we should find and fix as
>   many MI bugs as possible, documenting each.
> 
>   4. Just before the next + 1 release we should make MI3 the default,
>   create MI4, and mark MI4 as "unstable".
> 
>   5. (Optional) Maybe, officially mark MI1 as deprecated, and note
>   that it will be removed at some point.

Yes that makes sense.  Except that I would merge this patch after the 8.3
branch creation (until there's a compelling reason to have it in 8.3).

Simon