[PATCH] MI and MI2 should have identical behavior on -target-download command

[Maxim Grigoriev-2]

This patch has to be approved before I can commit it.

It fixes a small glitch in GDB MI. MI2 is a default. So
MI and MI2 should work identically. The MI2 being set as
a default is hard-coded in several places. I think one
place was missed:

    mi/mi-main.c::mi_load_progress().

Without this fix, MI "-target-download" command behaves
differently on MI versus MI2 not showing loading progress,
when "-i=mi2" is used.

GDB front-ends developers might want to stick with "-i=mi"
assuming they always use a default. It would be a bad idea
to force them to use explicit options like "-i=mi2".

-- Maxim



2007-08-20  Maxim Grigoriev  <maxim2405@...>

        * mi/mi-main.c (mi_load_progress): Make MI and MI2 behave
        identically on -target-download command as long as MI2 is a default.

Index: gdb/mi/mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.101
diff -u -r1.101 mi-main.c
--- gdb/mi/mi-main.c 10 Aug 2007 21:52:38 -0000 1.101
+++ gdb/mi/mi-main.c 20 Aug 2007 20:19:57 -0000
@@ -1465,7 +1465,8 @@
      of this function.  */
   saved_uiout = uiout;
 
-  if (current_interp_named_p (INTERP_MI))
+  if (current_interp_named_p (INTERP_MI)
+      || current_interp_named_p (INTERP_MI2))
     uiout = mi_out_new (2);
   else if (current_interp_named_p (INTERP_MI1))
     uiout = mi_out_new (1);

[Nick Roberts]

 > It fixes a small glitch in GDB MI. MI2 is a default. So
 > MI and MI2 should work identically. The MI2 being set as
 > a default is hard-coded in several places. I think one
 > place was missed:
 >
 >     mi/mi-main.c::mi_load_progress().
 >
 > Without this fix, MI "-target-download" command behaves
 > differently on MI versus MI2 not showing loading progress,
 > when "-i=mi2" is used.
 >
 > GDB front-ends developers might want to stick with "-i=mi"
 > assuming they always use a default. It would be a bad idea
 > to force them to use explicit options like "-i=mi2".

What behaviour do you want for mi3?

 >...
 > -  if (current_interp_named_p (INTERP_MI))
 > +  if (current_interp_named_p (INTERP_MI)
 > +      || current_interp_named_p (INTERP_MI2))
 >      uiout = mi_out_new (2);
 >    else if (current_interp_named_p (INTERP_MI1))
 >      uiout = mi_out_new (1);

--
Nick                                           http://www.inet.net.nz/~nickrob

[Daniel Jacobowitz-2]

On Wed, Aug 22, 2007 at 12:16:01AM +1200, Nick Roberts wrote:
>  > It fixes a small glitch in GDB MI. MI2 is a default. So
>  > MI and MI2 should work identically. The MI2 being set as
>  > a default is hard-coded in several places. I think one
>  > place was missed:
>  >
>  >     mi/mi-main.c::mi_load_progress().

Sorry, we (specifically Andrew Stubbs) noticed this problem in
November 2005, but no one got around to fixing it.

>  > Without this fix, MI "-target-download" command behaves
>  > differently on MI versus MI2 not showing loading progress,
>  > when "-i=mi2" is used.
>  >
>  > GDB front-ends developers might want to stick with "-i=mi"
>  > assuming they always use a default. It would be a bad idea
>  > to force them to use explicit options like "-i=mi2".
>
> What behaviour do you want for mi3?

MI3 should probably print the progress indicator too.  Like below.
Anyone disagree with this patch?

--
Daniel Jacobowitz
CodeSourcery

2007-08-21  Maxim Grigoriev  <maxim2405@...>
            Daniel Jacobowitz  <dan@...>

        * mi-main.c (mi_load_progress): Handle MI2 and MI3 interpreters.

Index: mi-main.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-main.c,v
retrieving revision 1.101
diff -u -p -r1.101 mi-main.c
--- mi-main.c 10 Aug 2007 21:52:38 -0000 1.101
+++ mi-main.c 21 Aug 2007 13:26:30 -0000
@@ -1465,10 +1465,13 @@ mi_load_progress (const char *section_na
      of this function.  */
   saved_uiout = uiout;
 
-  if (current_interp_named_p (INTERP_MI))
+  if (current_interp_named_p (INTERP_MI)
+      || current_interp_named_p (INTERP_MI2))
     uiout = mi_out_new (2);
   else if (current_interp_named_p (INTERP_MI1))
     uiout = mi_out_new (1);
+  else if (current_interp_named_p (INTERP_MI3))
+    uiout = mi_out_new (3);
   else
     return;
 

[Maxim Grigoriev-2]

>> What behavior do you want for mi3?

I think it's reasonable to make mi2 and mi3 work similarly
on -target-download. At least, until MI3 becomes a default,
and GDB community comes up with a different ideas about it.

I understood Daniel has the same opinion (see the follow-up email).

-- Maxim

Nick Roberts wrote:

>  > It fixes a small glitch in GDB MI. MI2 is a default. So
>  > MI and MI2 should work identically. The MI2 being set as
>  > a default is hard-coded in several places. I think one
>  > place was missed:
>  >
>  >     mi/mi-main.c::mi_load_progress().
>  >
>  > Without this fix, MI "-target-download" command behaves
>  > differently on MI versus MI2 not showing loading progress,
>  > when "-i=mi2" is used.
>  >
>  > GDB front-ends developers might want to stick with "-i=mi"
>  > assuming they always use a default. It would be a bad idea
>  > to force them to use explicit options like "-i=mi2".
>
> What behaviour do you want for mi3?
>
>  >...
>  > -  if (current_interp_named_p (INTERP_MI))
>  > +  if (current_interp_named_p (INTERP_MI)
>  > +      || current_interp_named_p (INTERP_MI2))
>  >      uiout = mi_out_new (2);
>  >    else if (current_interp_named_p (INTERP_MI1))
>  >      uiout = mi_out_new (1);
>
>  

[Nick Roberts]

 > -  if (current_interp_named_p (INTERP_MI))
 > +  if (current_interp_named_p (INTERP_MI)
 > +      || current_interp_named_p (INTERP_MI2))
 >      uiout = mi_out_new (2);
 >    else if (current_interp_named_p (INTERP_MI1))
 >      uiout = mi_out_new (1);
 > +  else if (current_interp_named_p (INTERP_MI3))
 > +    uiout = mi_out_new (3);
 >    else
 >      return;

Sorry, my comment was meant to be more general: What behaviour do you want for
miN?  where N >= 3.

Elsewhere GDB uses:

if (mi_version (uiout) < 2)

although that probably wouldn't work here.  I don't really no why GDB goes
through so many hoops with this.

--
Nick                                           http://www.inet.net.nz/~nickrob

[Daniel Jacobowitz-2]

On Wed, Aug 22, 2007 at 05:33:44PM +1200, Nick Roberts wrote:
> Sorry, my comment was meant to be more general: What behaviour do you want for
> miN?  where N >= 3.

The same as the current MI2 behavior, I think.  There's only a
difference because of an accident and because of the strange
properties of how this hook is called.  That's part of why it's
deprecated, I guess.  It gets called sometimes when the global uiout
is wrong.

--
Daniel Jacobowitz
CodeSourcery

[Nick Roberts]

 > > Sorry, my comment was meant to be more general: What behaviour do you want
 > > for miN?  where N >= 3.
 >
 > The same as the current MI2 behavior, I think.  

But your patch only gives that behaviour for N = 3.

 >                                                 There's only a
 > difference because of an accident and because of the strange
 > properties of how this hook is called.  That's part of why it's
 > deprecated, I guess.  It gets called sometimes when the global uiout
 > is wrong.

--
Nick                                           http://www.inet.net.nz/~nickrob

[Daniel Jacobowitz-2]

On Thu, Aug 23, 2007 at 06:11:42PM +1200, Nick Roberts wrote:
>  > > Sorry, my comment was meant to be more general: What behaviour do you want
>  > > for miN?  where N >= 3.
>  >
>  > The same as the current MI2 behavior, I think.  
>
> But your patch only gives that behaviour for N = 3.

For each version it creates a uiout with the same level.  I can't
exactly call mi_out_new (4) yet...

--
Daniel Jacobowitz
CodeSourcery

[Daniel Jacobowitz-2]

On Tue, Aug 21, 2007 at 09:27:53AM -0400, Daniel Jacobowitz wrote:
> 2007-08-21  Maxim Grigoriev  <maxim2405@...>
>    Daniel Jacobowitz  <dan@...>
>
> * mi-main.c (mi_load_progress): Handle MI2 and MI3 interpreters.

I checked this in.

--
Daniel Jacobowitz
CodeSourcery