mi_load_progress question

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

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.n
z/~nickrob

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

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 
>

 > 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.n
z/~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;

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

 > > 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.n
z/~nickrob