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

Simon Marchi
2019-01-11T21:07:32Z

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