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

[Andrew Burgess <andrew dot burgess at embecosm dot com>]

* 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