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

Andrew Burgess
2019-01-12T01:40:36Z

* Simon Marchi <simon.marchi@ericsson.com> [2019-01-11 23:36:16 +0000]:

> On 2019-01-11 1:39 p.m., Pedro Alves wrote:
> > 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
>
> Ahh thanks for the reference, I couldn't remember where you had already
> wrote about that.
>
> > 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.
>
> I agree this would be nice.
>
> > 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.
>
> Well this addresses my concern that frontends won't need to use
> -fix-break-list-bug until the end of time, so I am ok with it.
>
> > 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.
>
> I'll at least give it a try, implementing it is probably not hard. If it doesn't
> add too much maintenance burden, I'm not against it. If I do it for this bug, it
> will pave the path for future bug fixes, so hopefully it will be smoother next time.
>
> > So all things considered, it's fine with me to go your route directly
> > without a "-fix-break-list-bug" step.
>
> As I said, I'll give it a try. I intend to name it -fix-multi-location-breakpoint-output.
>

Instead of adding a flag for this specific issue, should we consider
adding a generic mechanism that allows single commands to be run with
a different MI version?

My first thought was to add (in mi-parse.c:mi_parse) a new flag
'--mi', like we already have '--thread' and '--language', which would
let you pick a different MI version just for this command. So you
could say:

-break-insert --mi 3 LOCATION

And get MI3 for this command, even if you are currently running at MI2
by default. Conversely, if a UI developer has mostly moved to MI3,
but break has not been updated yet, they could (assuming their default
is now MI3) do this:

-break-insert --mi 2 LOCATION

and get the old behaviour.

The problem with the above, is that a user can also do:

break LOCATION

and run the console command, but also get the formatted output.

I'm slightly tempted so say that we could ignore this case. If you
use a CLI command then you get whatever the default is, only pure MI
commands would allow per-command switching...

An alternative, but similarly generic approach would be to allow
recursive MI invocation, with something like this (assuming MI2 is the
current default):

-interpreter-exec mi3 "-break-insert LOCATION"

Again, this would allow the interpreter to be switched up and down as
needed on a command-by-command basis. The problem with the second
approach is that it currently segfaults, I assume we don't currently
expect recursive MI invocation.

I started working on a patch for the first approach before realising
the problem with CLI commands. I haven't looked at the cause of the
segfault in the second approach yet.

Do you think there's any benefit to adopting a more general solution
to this issue?

Thanks,
Andrew

> > I agree with Andrew below though. Bumping the MI version this late in
> > the cycle is probably not a good idea.
>
> I agree, I intend to merge a fix for this after 8.3 has branched.
>
> > 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.
>
> Yes, although I would wait until 8.3 is branched before merging it.
>
> Btw I realized the output with this patch is not good. For -break-list with two multi-location
> breakpoints, it results in something like:
>
> body=[
> bkpt={ ... },
> locations={ ... },
> bkpt={ ... },
> locations={ ... },
> ]
>
> Where I was aiming for:
>
> body=[
> bkpt={
> ...,
> locations={ ... },
> },
> bkpt={
> ...,
> locations={ ... },
> },
> ]
>
> The next version will fix this.
>
> Simon