mi_load_progress question

mi_load_progress question

[Vladimir Prus]

Hi,
I see some strange logic in the mi_load_progress function (file mi/mi-main.c). 
That function is responsible for printing progress report when downloading 
program to target, and the code in question is:

 if (current_interp_named_p (INTERP_MI))
    uiout = mi_out_new (2);
  else if (current_interp_named_p (INTERP_MI1))
    uiout = mi_out_new (1);
  else
    return;

When I run gdb with "--i=mi2", this code exists with "return", producing to 
progress information. Is this desired behaviour?

- Volodya

RE: mi_load_progress question

[Nick Roberts]

Vladimir Prus writes:
 > 
 > Hi,
 > I see some strange logic in the mi_load_progress function (file mi/mi-main.c). 
 > That function is responsible for printing progress report when downloading 
 > program to target, and the code in question is:
 > 
 >  if (current_interp_named_p (INTERP_MI))
 >     uiout = mi_out_new (2);
 >   else if (current_interp_named_p (INTERP_MI1))
 >     uiout = mi_out_new (1);
 >   else
 >     return;
 > 
 > When I run gdb with "--i=mi2", this code exists with "return", producing to 
 > progress information. Is this desired behaviour?

Looking through the change history, other values didn't exist when the original
code was written.  So I guess it's not desired behaviour.

 if (current_interp_named_p (INTERP_MI1))
   uiout = mi_out_new (1);
 else 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_MI3))
   uiout = mi_out_new (3);
 else
   return;

would work (until new MI levels are introduced!).  It would be best to generalise
this and similar code to make it future proof.

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

Re: mi_load_progress question

[Daniel Jacobowitz]

On Wed, Jul 12, 2006 at 09:32:34PM +1200, Nick Roberts wrote:
> Vladimir Prus writes:
>  > 
>  > Hi,
>  > I see some strange logic in the mi_load_progress function (file mi/mi-main.c). 
>  > That function is responsible for printing progress report when downloading 
>  > program to target, and the code in question is:
>  > 
>  >  if (current_interp_named_p (INTERP_MI))
>  >     uiout = mi_out_new (2);
>  >   else if (current_interp_named_p (INTERP_MI1))
>  >     uiout = mi_out_new (1);
>  >   else
>  >     return;
>  > 
>  > When I run gdb with "--i=mi2", this code exists with "return", producing to 
>  > progress information. Is this desired behaviour?

I am pretty sure that there was a bug report about this before, but I
can't find it now.  Ah, here:

http://sourceware.org/ml/gdb-patches/2005-11/msg00295.html

No one else commented at the time and I opted to stick with what we
already had.  Do you think we should just turn the progress reporting
back on for -i=mi2?  How about you, Nick?

> Looking through the change history, other values didn't exist when the original
> code was written.  So I guess it's not desired behaviour.
> 
>  if (current_interp_named_p (INTERP_MI1))
>    uiout = mi_out_new (1);
>  else 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_MI3))
>    uiout = mi_out_new (3);
>  else
>    return;
> 
> would work (until new MI levels are introduced!).  It would be best to generalise
> this and similar code to make it future proof.

Or come up with some other way to do this that avoids the need for the
hack.  The problem was that we were calling the MI load progress hook
through the wrong interpreter.  We need to figure out how to do this
right.

First question: Should typing "load" at the MI prompt issue +download
updates?  Currently it does.  It also issues the CLI's updates.

Second question: Should typing "-interpreter-exec console load" issue
+download updates?  Currently it does the same as "load" and
that's the right thing to do so this is the same as the first question.

Jim, maybe you've got an opinion on this?  Apple seems to make more use
of the CLI support than anyone else :-)

-- 
Daniel Jacobowitz
CodeSourcery

Re: mi_load_progress question

[Jim Ingham]

I haven't thought about this case specifically, because we don't use  
the "load" command - Xcode only supports native debuggers at present...

But in general, anything that might affect the UI should emit some  
kind of notification when done through -interpreter-exec.  If you had  
a little "load progress meter" it would be a bug if clicking the  
"load" button caused the meter to update, but typing "load" at the CLI  
didn't.  So I would say yes.

Jim

On Jul 12, 2006, at 7:04 AM, Daniel Jacobowitz wrote:

> Second question: Should typing "-interpreter-exec console load" issue
> +download updates?  Currently it does the same as "load" and
> that's the right thing to do so this is the same as the first  
> question.
>
> Jim, maybe you've got an opinion on this?  Apple seems to make more  
> use
> of the CLI support than anyone else :-)
>

Re: mi_load_progress question

[Nick Roberts]

 > I am pretty sure that there was a bug report about this before, but I
 > can't find it now.  Ah, here:
 > 
 > http://sourceware.org/ml/gdb-patches/2005-11/msg00295.html
 > 
 > No one else commented at the time and I opted to stick with what we
 > already had.  Do you think we should just turn the progress reporting
 > back on for -i=mi2?  How about you, Nick?

I have no experience with remote debugging, so I can only suggest that it's
consistent i.e have progress reporting for all cases or turn it off completely
(I don't know how useful it is or why it should only exist in MI).

 > > Looking through the change history, other values didn't exist when the
 > > original code was written.  So I guess it's not desired behaviour.
 > > 
 > >  if (current_interp_named_p (INTERP_MI1))
 > >    uiout = mi_out_new (1);
 > >  else 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_MI3))
 > >    uiout = mi_out_new (3);
 > >  else
 > >    return;
 > > 
 > > would work (until new MI levels are introduced!).  It would be best to generalise
 > > this and similar code to make it future proof.
 > 
 > Or come up with some other way to do this that avoids the need for the
 > hack.  The problem was that we were calling the MI load progress hook
 > through the wrong interpreter.  We need to figure out how to do this
 > right.
 >
 > First question: Should typing "load" at the MI prompt issue +download
 > updates?  Currently it does.  It also issues the CLI's updates.
 > 
 > Second question: Should typing "-interpreter-exec console load" issue
 > +download updates?  Currently it does the same as "load" and
 > that's the right thing to do so this is the same as the first question.

If the answer is yes, I suggest something like the patch below.  Unless I'm
missing something mi_load_progress only gets called in MI.  I would like to
make similar but more fiddly changes to mi-interp.c, removing
mi`N'_command_loop, N = 1,2,3, and just using mi_command_loop.

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


*** mi-main.c	13 May 2006 16:42:07 +1200	1.84
--- mi-main.c	13 Jul 2006 18:41:37 +1200	
*************** mi_load_progress (const char *section_na
*** 1386,1397 ****
       of this function.  */
    saved_uiout = uiout;
  
!   if (current_interp_named_p (INTERP_MI))
!     uiout = mi_out_new (2);
!   else if (current_interp_named_p (INTERP_MI1))
!     uiout = mi_out_new (1);
!   else
!     return;
  
    update_threshold.tv_sec = 0;
    update_threshold.tv_usec = 500000;
--- 1386,1392 ----
       of this function.  */
    saved_uiout = uiout;
  
!   uiout = mi_out_new (mi_version (uiout));
  
    update_threshold.tv_sec = 0;
    update_threshold.tv_usec = 500000;

Re: mi_load_progress question

[Daniel Jacobowitz]

On Thu, Jul 13, 2006 at 06:46:16PM +1200, Nick Roberts wrote:
> I have no experience with remote debugging, so I can only suggest that it's
> consistent i.e have progress reporting for all cases or turn it off completely
> (I don't know how useful it is or why it should only exist in MI).

There's similar output from the CLI including speed and section name.

> If the answer is yes, I suggest something like the patch below.  Unless I'm
> missing something mi_load_progress only gets called in MI.  I would like to
> make similar but more fiddly changes to mi-interp.c, removing
> mi`N'_command_loop, N = 1,2,3, and just using mi_command_loop.

You're wrong; take a look at the URL from my message.  If you
type "load" at the MI prompt, the hook remains installed, but we get
called while the CLI is temporarily active.  Doing this will probably
crash.

You can test this using gdbserver; "load" isn't useful with gdbserver,
but if you load the same binary gdbserver started, it works well enough
to test "load".

-- 
Daniel Jacobowitz
CodeSourcery

Re: mi_load_progress question

[Nick Roberts]

 > > I have no experience with remote debugging, so I can only suggest that
 > > it's consistent i.e have progress reporting for all cases or turn it off
 > > completely (I don't know how useful it is or why it should only exist in
 > > MI).
 > 
 > There's similar output from the CLI including speed and section name.

The progress reporting is described for -target-download but not mentioned
for load.

 > > If the answer is yes, I suggest something like the patch below.  Unless I'm
 > > missing something mi_load_progress only gets called in MI.  I would like to
 > > make similar but more fiddly changes to mi-interp.c, removing
 > > mi`N'_command_loop, N = 1,2,3, and just using mi_command_loop.
 > 
 > You're wrong; take a look at the URL from my message.  If you
 > type "load" at the MI prompt, the hook remains installed, but we get
 > called while the CLI is temporarily active.  Doing this will probably
 > crash.

OK.  I can see a problem now: if I start GDB normally and use a command like
"interpreter mi -environment-pwd".  Otherwise deprecated_show_load_progress is
presumably only set to mi_load_progress if GDB is invoked with "-i=mi[N]" as
there is currently there is no ability to switch interpreters permanently in
FSF GDB.  Or is the problem more general than that?

When I read the thread I thought it was referring to executing CLI commands
from MI, in which case you presumably would want the MI output.

Perhaps mi_load_progress could just do:

  if (mi_version (uiout) != 0)
    uiout = mi_out_new (mi_version (uiout));
  else
    return;

 > You can test this using gdbserver; "load" isn't useful with gdbserver,
 > but if you load the same binary gdbserver started, it works well enough
 > to test "load".

I don't see how mi_load_progress would run in this case.

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