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

[Pedro Alves <palves at redhat dot com>]

On 01/11/2019 12:34 PM, 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]


Thanks.  This was also recently-ish discussed in PR9659.

https://sourceware.org/bugzilla/show_bug.cgi?id=9659


>>
>> 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.


My original concern with MI bumps for individual MI fixes is that they
force an all-or-nothing approach on the frontends.  Let me expand.

Suppose a frontend developer only cares about the multi-location
fix, and not any of the other (supposed) fixes that go into MI3 that
make it backwards incompatible.  It was with that in mind that I had
suggested at <https://sourceware.org/bugzilla/show_bug.cgi?id=9659#c20>
to consider going with the "-fix-break-list-bug" solution first.
That would be usable with MI2 and also be enabled by default with
MI3 (with no way to disable).  Then later on, when we get rid of
MI2, the "-fix-break-list-bug" setting disappears.

But I suppose that that's really an unnecessary complication if we're
not really going to massively change MI every other release, and if
migrating a frontend to a new MI version isn't expected to be that
complicated.  We probably aren't and it probably isn't.

So all things considered, it's fine with me to go your route directly
without a "-fix-break-list-bug" step.

I agree with Andrew below though.  Bumping the MI version this late in
the cycle is probably not a good idea.  

If we want to fix this bug for 8.3, we could merge the fix while
leaving MI2 as the default, declare MI3 stable, and then bump the
WIP MI version to MI4.  I.e., the comments in the code that talk
about things to fix for MI3 should become references to MI4 instead.

Thanks,
Pedro Alves

> 
> 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.
> 
> 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.
> 
> 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...
> 
> 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.
> 
> Thoughts?
>