[PATCH] -stack-info-frames

[PATCH] -stack-info-frames

[Nick Roberts]

Here's a patch for -stack-info-frames.  Having just read Jason's e-mail
about how quick "-stack-list-frames 0 0" is, I'm not sure its necessary.
I thought I'd send it anyway.

Even if we decide not to implement -stack-info-frames, could you please
approve the other parts: Not testing for stack, and not allowing an argument
(mi_cmd_stack_select_frame).

Nick

2005-06-18  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-cmd-stack.c (mi_cmd_stack_info_frame): New function.
	(mi_cmd_stack_list_frames, mi_cmd_stack_info_depth):
	Don't test for stack.
	(mi_cmd_stack_select_frame): Do not allow an argument.
	Don't test for stack.

	* mi/mi-cmds.c (mi_cmds): Add mi_cmd_stack_info_frame to
	entry for -stack-info-frame.

	* mi/mi-cmds.h (mi_cmd_stack_info_frame): New declaration.

	* gdb.texinfo (GDB/MI Stack Manipulation): Update description
	of -stack-info-frame.


*** /home/nick/src/gdb/mi/mi-cmd-stack.c.~1.25.~	2005-02-13 00:36:20.000000000 +1300
--- /home/nick/src/gdb/mi/mi-cmd-stack.c	2005-06-18 01:20:30.000000000 +1200
***************
*** 47,55 ****
    struct cleanup *cleanup_stack;
    struct frame_info *fi;
  
-   if (!target_has_stack)
-     error (_("mi_cmd_stack_list_frames: No stack."));
- 
    if (argc > 2 || argc == 1)
      error (_("mi_cmd_stack_list_frames: Usage: [FRAME_LOW FRAME_HIGH]"));
  
--- 47,52 ----
***************
*** 104,112 ****
    int i;
    struct frame_info *fi;
  
-   if (!target_has_stack)
-     error (_("mi_cmd_stack_info_depth: No stack."));
- 
    if (argc > 1)
      error (_("mi_cmd_stack_info_depth: Usage: [MAX_DEPTH]"));
  
--- 101,106 ----
***************
*** 329,344 ****
  enum mi_cmd_result
  mi_cmd_stack_select_frame (char *command, char **argv, int argc)
  {
!   if (!target_has_stack)
!     error (_("mi_cmd_stack_select_frame: No stack."));
  
!   if (argc > 1)
!     error (_("mi_cmd_stack_select_frame: Usage: [FRAME_SPEC]"));
  
!   /* with no args, don't change frame */
!   if (argc == 0)
!     select_frame_command (0, 1 /* not used */ );
!   else
!     select_frame_command (argv[0], 1 /* not used */ );
    return MI_CMD_DONE;
  }
--- 323,341 ----
  enum mi_cmd_result
  mi_cmd_stack_select_frame (char *command, char **argv, int argc)
  {
!   if (argc == 0 || argc > 1)
!     error (_("mi_cmd_stack_select_frame: Usage: FRAME_SPEC"));
  
!   select_frame_command (argv[0], 1 /* not used */ );
!   return MI_CMD_DONE;
! }
  
! enum mi_cmd_result
! mi_cmd_stack_info_frame (char *command, char **argv, int argc)
! {
!   if (argc > 0)
!     error (_("mi_cmd_stack_info_frame: No arguments required"));
! 
!   print_stack_frame (get_selected_frame (NULL), 1, LOC_AND_ADDRESS);
    return MI_CMD_DONE;
  }

*** /home/nick/src/gdb/mi/mi-cmds.c.~1.16.~	2005-02-13 00:36:20.000000000 +1300
--- /home/nick/src/gdb/mi/mi-cmds.c	2005-06-18 00:55:55.000000000 +1200
***************
*** 107,113 ****
    { "signal-list-handle-actions", { NULL, 0 }, NULL, NULL },
    { "signal-list-signal-types", { NULL, 0 }, NULL, NULL },
    { "stack-info-depth", { NULL, 0 }, 0, mi_cmd_stack_info_depth},
!   { "stack-info-frame", { NULL, 0 }, NULL, NULL },
    { "stack-list-arguments", { NULL, 0 }, 0, mi_cmd_stack_list_args},
    { "stack-list-exception-handlers", { NULL, 0 }, NULL, NULL },
    { "stack-list-frames", { NULL, 0 }, 0, mi_cmd_stack_list_frames},
--- 107,113 ----
    { "signal-list-handle-actions", { NULL, 0 }, NULL, NULL },
    { "signal-list-signal-types", { NULL, 0 }, NULL, NULL },
    { "stack-info-depth", { NULL, 0 }, 0, mi_cmd_stack_info_depth},
!   { "stack-info-frame", { NULL, 0 }, 0, mi_cmd_stack_info_frame},
    { "stack-list-arguments", { NULL, 0 }, 0, mi_cmd_stack_list_args},
    { "stack-list-exception-handlers", { NULL, 0 }, NULL, NULL },
    { "stack-list-frames", { NULL, 0 }, 0, mi_cmd_stack_list_frames},


diff -c /home/nick/src/gdb/mi/mi-cmds.h.\~1.15.\~ /home/nick/src/gdb/mi/mi-cmds.h
*** /home/nick/src/gdb/mi/mi-cmds.h.~1.15.~	2005-01-25 22:30:39.000000000 +1300
--- /home/nick/src/gdb/mi/mi-cmds.h	2005-06-18 10:48:59.000000000 +1200
***************
*** 87,92 ****
--- 87,93 ----
  extern mi_cmd_argv_ftype mi_cmd_gdb_exit;
  extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
  extern mi_cmd_argv_ftype mi_cmd_stack_info_depth;
+ extern mi_cmd_argv_ftype mi_cmd_stack_info_frame;
  extern mi_cmd_argv_ftype mi_cmd_stack_list_args;
  extern mi_cmd_argv_ftype mi_cmd_stack_list_frames;
  extern mi_cmd_argv_ftype mi_cmd_stack_list_locals;


** /home/nick/src/gdb/doc/gdb.texinfo.~1.261.~	2005-06-16 14:36:11.000000000 +1200
--- /home/nick/src/gdb/doc/gdb.texinfo	2005-06-18 01:55:52.000000000 +1200
***************
*** 19205,19211 ****
  (without arguments).
  
  @subsubheading Example
! N.A.
  
  @subheading The @code{-stack-info-depth} Command
  @findex -stack-info-depth
--- 19205,19220 ----
  (without arguments).
  
  @subsubheading Example
! 
! @smallexample
! (@value{GDBP})
! -stack-info-frame
! ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
! args=[@{name="strarg",value="0x11940 \"A string argument.\""@}],
! file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
! fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
! (@value{GDBP})
! @end smallexample
  
  @subheading The @code{-stack-info-depth} Command
  @findex -stack-info-depth

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sat, Jun 18, 2005 at 10:52:09AM +1200, Nick Roberts wrote:
> 
> Here's a patch for -stack-info-frames.  Having just read Jason's e-mail
> about how quick "-stack-list-frames 0 0" is, I'm not sure its necessary.
> I thought I'd send it anyway.
> 
> Even if we decide not to implement -stack-info-frames, could you please
> approve the other parts: Not testing for stack, and not allowing an argument
> (mi_cmd_stack_select_frame).

How do you feel about putting an optional argument on _this_ one?
-stack-select-frame 0; -stack-info-frame is much like
-stack-list-frames 0 0; but "-stack-info-frame 2" doesn't have an
obvious analogue.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Fri, Jun 17, 2005 at 07:01:30PM -0400, Daniel Jacobowitz wrote:
> On Sat, Jun 18, 2005 at 10:52:09AM +1200, Nick Roberts wrote:
> > 
> > Here's a patch for -stack-info-frames.  Having just read Jason's e-mail
> > about how quick "-stack-list-frames 0 0" is, I'm not sure its necessary.
> > I thought I'd send it anyway.
> > 
> > Even if we decide not to implement -stack-info-frames, could you please
> > approve the other parts: Not testing for stack, and not allowing an argument
> > (mi_cmd_stack_select_frame).
> 
> How do you feel about putting an optional argument on _this_ one?
> -stack-select-frame 0; -stack-info-frame is much like
> -stack-list-frames 0 0; but "-stack-info-frame 2" doesn't have an
> obvious analogue.

Waitasec...

Nick, sorry, I've been giving you silly suggestions, because I have not
spent enough time with the MI manual.  Is "-stack-info-frame N" basically
the same as "-stack-list-frames N N"?

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > Is "-stack-info-frame N" basically the same as "-stack-list-frames N N"?

I didn't allow -stack-info-frame an argument.  OK, lets forget this
implementation.  Perhaps we could add something about "-stack-list-frames N N"
in the documentation.  I know its kind of obvious, but I didn't realise
initially.

How about the rest of the patch:

	(mi_cmd_stack_list_frames, mi_cmd_stack_info_depth):
	Don't test for stack.
	(mi_cmd_stack_select_frame): Do not allow an argument.
	Don't test for stack.

Nick

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sat, Jun 18, 2005 at 01:29:53PM +1200, Nick Roberts wrote:
> 
>  > Is "-stack-info-frame N" basically the same as "-stack-list-frames N N"?
> 
> I didn't allow -stack-info-frame an argument.  OK, lets forget this
> implementation.  Perhaps we could add something about "-stack-list-frames N N"
> in the documentation.  I know its kind of obvious, but I didn't realise
> initially.
> 
> How about the rest of the patch:
> 
> 	(mi_cmd_stack_list_frames, mi_cmd_stack_info_depth):
> 	Don't test for stack.
> 	(mi_cmd_stack_select_frame): Do not allow an argument.
> 	Don't test for stack.

OK (except the changelog is wrong - it's Require an argument, not Do
not allow, right?).

A nice followup if you're feeling inspired would be to remove the traces
of -stack-info-frame from both gdb/mi/ and the manual, since it really
does not sound useful at this point.

Not sure what we could add to the docs.  This is what's there:
  If invoked without arguments, this command prints a backtrace for the
whole stack.  If given two integer arguments, it shows the frames whose
levels are between the two arguments (inclusive).  If the two arguments
are equal, it shows the single frame at the corresponding level.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > > How about the rest of the patch:
 > > 
 > > 	(mi_cmd_stack_list_frames, mi_cmd_stack_info_depth):
 > > 	Don't test for stack.
 > > 	(mi_cmd_stack_select_frame): Do not allow an argument.
 > > 	Don't test for stack.
 > 
 > OK (except the changelog is wrong - it's Require an argument, not Do
 > not allow, right?).

Yes.  Committed.

 > A nice followup if you're feeling inspired would be to remove the traces
 > of -stack-info-frame from both gdb/mi/ and the manual, since it really
 > does not sound useful at this point.

OK.  I've committed this as obvious (?).

 > Not sure what we could add to the docs.  This is what's there:
 >   If invoked without arguments, this command prints a backtrace for the
 > whole stack.  If given two integer arguments, it shows the frames whose
 > levels are between the two arguments (inclusive).  If the two arguments
 > are equal, it shows the single frame at the corresponding level.

I guess that just proves I didn't read the manual properly.

Nick


2005-06-18  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-cmds.c (mi_cmds): Remove entry for -stack-info-frame.

	* mi/mi-cmd-stack.c (mi_cmd_stack_list_frames)
	(mi_cmd_stack_info_depth): Don't test for stack.
	(mi_cmd_stack_select_frame): Make the argument mandatory.
	Don't test for stack.

	* gdb.texinfo (GDB/MI Stack Manipulation): Remove reference to
	-stack-info-frame.


*** /home/nick/src/gdb/mi/mi-cmds.c.~1.16~	2005-06-18 14:58:00.000000000 +1200
--- /home/nick/src/gdb/mi/mi-cmds.c	2005-06-18 14:44:50.000000000 +1200
***************
*** 107,113 ****
    { "signal-list-handle-actions", { NULL, 0 }, NULL, NULL },
    { "signal-list-signal-types", { NULL, 0 }, NULL, NULL },
    { "stack-info-depth", { NULL, 0 }, 0, mi_cmd_stack_info_depth},
-   { "stack-info-frame", { NULL, 0 }, NULL, NULL },
    { "stack-list-arguments", { NULL, 0 }, 0, mi_cmd_stack_list_args},
    { "stack-list-exception-handlers", { NULL, 0 }, NULL, NULL },
    { "stack-list-frames", { NULL, 0 }, 0, mi_cmd_stack_list_frames},
--- 107,112 ----


*** /home/nick/src/gdb/mi/mi-cmd-stack.c.~1.25~	2005-06-18 14:57:26.000000000 +1200
--- /home/nick/src/gdb/mi/mi-cmd-stack.c	2005-06-18 14:31:45.000000000 +1200
***************
*** 47,55 ****
    struct cleanup *cleanup_stack;
    struct frame_info *fi;
  
-   if (!target_has_stack)
-     error (_("mi_cmd_stack_list_frames: No stack."));
- 
    if (argc > 2 || argc == 1)
      error (_("mi_cmd_stack_list_frames: Usage: [FRAME_LOW FRAME_HIGH]"));
  
--- 47,52 ----
***************
*** 104,112 ****
    int i;
    struct frame_info *fi;
  
-   if (!target_has_stack)
-     error (_("mi_cmd_stack_info_depth: No stack."));
- 
    if (argc > 1)
      error (_("mi_cmd_stack_info_depth: Usage: [MAX_DEPTH]"));
  
--- 101,106 ----
***************
*** 329,344 ****
  enum mi_cmd_result
  mi_cmd_stack_select_frame (char *command, char **argv, int argc)
  {
!   if (!target_has_stack)
!     error (_("mi_cmd_stack_select_frame: No stack."));
  
!   if (argc > 1)
!     error (_("mi_cmd_stack_select_frame: Usage: [FRAME_SPEC]"));
! 
!   /* with no args, don't change frame */
!   if (argc == 0)
!     select_frame_command (0, 1 /* not used */ );
!   else
!     select_frame_command (argv[0], 1 /* not used */ );
    return MI_CMD_DONE;
  }
--- 323,331 ----
  enum mi_cmd_result
  mi_cmd_stack_select_frame (char *command, char **argv, int argc)
  {
!   if (argc == 0 || argc > 1)
!     error (_("mi_cmd_stack_select_frame: Usage: FRAME_SPEC"));
  
!   select_frame_command (argv[0], 1 /* not used */ );
    return MI_CMD_DONE;
  }


*** /home/nick/src/gdb/doc/gdb.texinfo.~1.261~	2005-06-18 15:04:56.000000000 +1200
--- /home/nick/src/gdb/doc/gdb.texinfo	2005-06-18 14:44:18.000000000 +1200
***************
*** 19187,19212 ****
  @node GDB/MI Stack Manipulation
  @section @sc{gdb/mi} Stack Manipulation Commands
  
- 
- @subheading The @code{-stack-info-frame} Command
- @findex -stack-info-frame
- 
- @subsubheading Synopsis
- 
- @smallexample
-  -stack-info-frame
- @end smallexample
- 
- Get info on the current frame.
- 
- @subsubheading @value{GDBN} Command
- 
- The corresponding @value{GDBN} command is @samp{info frame} or @samp{frame}
- (without arguments).
- 
- @subsubheading Example
- N.A.
- 
  @subheading The @code{-stack-info-depth} Command
  @findex -stack-info-depth
  
--- 19187,19192 ----

Re: [PATCH] -stack-info-frames

[Eli Zaretskii]

> Date: Fri, 17 Jun 2005 21:57:57 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: gdb-patches@sources.redhat.com
> 
> Not sure what we could add to the docs.  This is what's there:
>   If invoked without arguments, this command prints a backtrace for the
> whole stack.  If given two integer arguments, it shows the frames whose
> levels are between the two arguments (inclusive).  If the two arguments
> are equal, it shows the single frame at the corresponding level.

Is the actual/new behavior any different, and if so, how?

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > > Not sure what we could add to the docs.  This is what's there:
 > >   If invoked without arguments, this command prints a backtrace for the
 > > whole stack.  If given two integer arguments, it shows the frames whose
 > > levels are between the two arguments (inclusive).  If the two arguments
 > > are equal, it shows the single frame at the corresponding level.
 > 
 > Is the actual/new behavior any different, and if so, how?

The only behaviour that I've changed is that -stack-selected-frame must
have an argument.  This now accurately reflects existing documentation.

I've talked myself out of implementing -stack-info-frame to give the selected
frame because I thought that "-stack-list-frames 0 0" gave this.  Now I see
that it gives the innermost frame where execution has stopped (the current
frame?).  I'm not sure that I should have done that because if the user type a
CLI command like "up" in the GUD buffer, I don't see how Emacs could keep
track of the selected frame.  Apple's implementation of GDB/MI presumably has
something extra (frame-changed notification?) to do that.

Nick

Re: [PATCH] -stack-info-frames

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Sat, 18 Jun 2005 20:53:32 +1200
> Cc: gdb-patches@sources.redhat.com
> 
>  > > Not sure what we could add to the docs.  This is what's there:
>  > >   If invoked without arguments, this command prints a backtrace for the
>  > > whole stack.  If given two integer arguments, it shows the frames whose
>  > > levels are between the two arguments (inclusive).  If the two arguments
>  > > are equal, it shows the single frame at the corresponding level.
>  > 
>  > Is the actual/new behavior any different, and if so, how?
> 
> The only behaviour that I've changed is that -stack-selected-frame must
> have an argument.  This now accurately reflects existing documentation.

But the above snippet is taken from the documentation of
"-stack-list-frames", not from "-stack-selected-frame".  How is that
relevant?

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sat, Jun 18, 2005 at 01:19:55PM +0200, Eli Zaretskii wrote:
> > From: Nick Roberts <nickrob@snap.net.nz>
> > Date: Sat, 18 Jun 2005 20:53:32 +1200
> > Cc: gdb-patches@sources.redhat.com
> > 
> >  > > Not sure what we could add to the docs.  This is what's there:
> >  > >   If invoked without arguments, this command prints a backtrace for the
> >  > > whole stack.  If given two integer arguments, it shows the frames whose
> >  > > levels are between the two arguments (inclusive).  If the two arguments
> >  > > are equal, it shows the single frame at the corresponding level.
> >  > 
> >  > Is the actual/new behavior any different, and if so, how?
> > 
> > The only behaviour that I've changed is that -stack-selected-frame must
> > have an argument.  This now accurately reflects existing documentation.
> 
> But the above snippet is taken from the documentation of
> "-stack-list-frames", not from "-stack-selected-frame".  How is that
> relevant?

It isn't, really - Nick and I have been discussing -stack-info-frame
versus -stack-list-frames.  We didn't realize that you could use
-stack-list-frame to list a single frame.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Nick Roberts]

Eli Zaretskii writes:
 > >  > Is the actual/new behavior any different, and if so, how?
 > > 
 > > The only behaviour that I've changed is that -stack-selected-frame must
 > > have an argument.  This now accurately reflects existing documentation.

(I mean -stack-select-frame, of course)

 > But the above snippet is taken from the documentation of
 > "-stack-list-frames", not from "-stack-selected-frame".  How is that
 > relevant?

As you maintain the documentation, I took the question to mean
"Does the documentation reflect the actual/new behaviour".  I replied
about new behaviour. -stack-list-frames hasn't been changed and AFAIK
the documentation about it is accurate.

Nick

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sat, Jun 18, 2005 at 08:53:32PM +1200, Nick Roberts wrote:
> I've talked myself out of implementing -stack-info-frame to give the selected
> frame because I thought that "-stack-list-frames 0 0" gave this.  Now I see
> that it gives the innermost frame where execution has stopped (the current
> frame?).  I'm not sure that I should have done that because if the user type a
> CLI command like "up" in the GUD buffer, I don't see how Emacs could keep
> track of the selected frame.  Apple's implementation of GDB/MI presumably has
> something extra (frame-changed notification?) to do that.

Yes, the current frame.

Talk to me about this GUD buffer for a second.  How does it work -
-interpreter-exec?

I think it's becoming clear that we need to have the MI output for
commands whether or not we also have the CLI output.  Right now we've
got these:

(gdb)
up
&"up\n"
^done,frame={level="2",addr="0x0813f48d",func="gdb_wait_for_event",args=[],file="/big/fsf/local/src/gdb/event-loop.c",line="753"},line="753",file="/big/fsf/local/src/gdb/event-loop.c"
(gdb) 

(gdb)
-interpreter-exec console up
~"#1  0xb7d621ae in poll () from /lib/tls/i686/cmov/libc.so.6\n"
^done

But wouldn't this be better?

(gdb)
-interpreter-exec console up
~"#1  0xb7d621ae in poll () from /lib/tls/i686/cmov/libc.so.6\n"
^done,frame={level="2",addr="0x0813f48d",func="gdb_wait_for_event",args=[],file="/big/fsf/local/src/gdb/event-loop.c",line="753"},line="753",file="/big/fsf/local/src/gdb/event-loop.c"

Meanwhile, we don't have this ability.  So maybe we do need
-stack-info-frame, without an argument.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > Talk to me about this GUD buffer for a second.  How does it work -
 > -interpreter-exec?

Currently I input CLI commands directly but I know that the plan is to
remove this capability and just leave "-interpreter-exec"

 > I think it's becoming clear that we need to have the MI output for
 > commands whether or not we also have the CLI output.  Right now we've
 > got these:
 > 
 > (gdb)
 > up
 > &"up\n"
 > ^done,frame={level="2",addr="0x0813f48d",func="gdb_wait_for_event",args=[],file="/big/fsf/local/src/gdb/event-loop.c",line="753"},line="753",file="/big/fsf/local/src/gdb/event-loop.c"
 > (gdb) 

This is what used to happen (I get this with GNU gdb 5.2.1-2mdk, for example)
But now with GDB in CVS, I get the same output as with
"-interpreter-exec console up" (apart from "up\n" in the log stream) i.e.

up
&"up\n"
~"#1  0x08048641 in main (argc=1, argv=0xbffff794) at myprog.c:81\n"
~"81\t    myprint (i,a[i] /* hello */);\n"
^done

I don't know why you don't also get this.

 > (gdb)
 > -interpreter-exec console up
 > ~"#1  0xb7d621ae in poll () from /lib/tls/i686/cmov/libc.so.6\n"
 > ^done
 > 
 > But wouldn't this be better?
 > 
 > (gdb)
 > -interpreter-exec console up
 > ~"#1  0xb7d621ae in poll () from /lib/tls/i686/cmov/libc.so.6\n"
 > ^done,frame={level="2",addr="0x0813f48d",func="gdb_wait_for_event",args=[],file="/big/fsf/local/src/gdb/event-loop.c",line="753"},line="753",file="/big/fsf/local/src/gdb/event-loop.c"

Yes.  The same is true for the commands that control execution:

(gdb) 
-exec-next 
^running
(gdb) 
*stopped,reason="end-stepping-range",thread-id="0",frame={addr="0x080484dd",func="main",args=[{name="argc",value="1"},{name="argv",value="0xbffff794"}],file="myprog.c",fullname="/home/nick/myprog.c",line="50"}
(gdb) 

but

(gdb) 
-interpreter-exec console next
~"50\t  int n1=7, n2=8, n3=9;\n"
^done
(gdb) 

doesn't give the frontend any information.

Before I looked at the code, I presumed that GDB detected when execution had
stopped and the printed out "*stopped" (in which case this would happen also
with CLI commands). Looking at it, I see it is printed in
mi_execute_async_cli_command so that it appears as command output rather than
event notification

 > Meanwhile, we don't have this ability.  So maybe we do need
 > -stack-info-frame, without an argument.

Yes, unless Apple's proposed merge will provide the necessary information.

Nick

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sun, Jun 19, 2005 at 10:49:29AM +1200, Nick Roberts wrote:
> This is what used to happen (I get this with GNU gdb 5.2.1-2mdk, for example)
> But now with GDB in CVS, I get the same output as with
> "-interpreter-exec console up" (apart from "up\n" in the log stream) i.e.
> 
> up
> &"up\n"
> ~"#1  0x08048641 in main (argc=1, argv=0xbffff794) at myprog.c:81\n"
> ~"81\t    myprint (i,a[i] /* hello */);\n"
> ^done
> 
> I don't know why you don't also get this.

I do now.  It didn't even occur to me that this would change between
6.3 and CVS.  I'll assume that was a deliberate change... is this going
to bust frontends?

> Before I looked at the code, I presumed that GDB detected when execution had
> stopped and the printed out "*stopped" (in which case this would happen also
> with CLI commands). Looking at it, I see it is printed in
> mi_execute_async_cli_command so that it appears as command output rather than
> event notification
> 
>  > Meanwhile, we don't have this ability.  So maybe we do need
>  > -stack-info-frame, without an argument.
> 
> Yes, unless Apple's proposed merge will provide the necessary information.

Let's not wait on that.  You've demonstrated a use for
-stack-info-frame relative to the current source base.  That's plenty
good enough for me.

Of course now we need to re-add the documentation (with example this
time).  A test case would be nice too.  Since we've decided that we do
want this feature, could you put that together?

I'm going to be travelling all next week, but I should have time to
review the occasional patch anyway.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > > Yes, unless Apple's proposed merge will provide the necessary information.
 > 
 > Let's not wait on that.  You've demonstrated a use for
 > -stack-info-frame relative to the current source base.  That's plenty
 > good enough for me.
 > 
 > Of course now we need to re-add the documentation (with example this
 > time).  A test case would be nice too.  Since we've decided that we do
 > want this feature, could you put that together?

OK.  I've committed the -stack-info-frame part of the change that posted (Sat,
18 Jun 2005 10:52:09 +1200).  Perhaps that doesn't follow the letter of the
law but I hope it follows the spirit.  In any case, I find it easier to make
further changes to the repository than juggle patches (as demonstrated shown
with my earlier mangling).

This commit is slightly different in two respects:

1) mi_cmd_stack_info_frame uses print_frame_info instead of print_stack_frame.
   This follows mi_cmd_stack_list_frames and means that the argument values
   aren't printed.

2) The documentation for -stack-info-frame previously said (before I removed
   it) "Get info on the current frame.".  I've corrected this to
   "Get info on the selected frame."  I've also removed the argument values
   from the example as explained in 1).

I hope this is acceptable.

Nick


2005-06-19  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-cmd-stack.c (mi_cmd_stack_info_frame): New function.

	* mi/mi-cmds.c (mi_cmds): Replace entry for -stack-info-frame.
	Make it use mi_cmd_stack_info_frame

	* mi/mi-cmds.h (mi_cmd_stack_info_frame): New declaration.

	* gdb.texinfo (GDB/MI Stack Manipulation):
	Re-instate -stack-info-frame with example.  Say that it gets
	info on selected frame, not current frame.


*** /home/nick/src/gdb/mi/mi-cmd-stack.c.~1.26~	2005-06-19 15:34:00.000000000 +1200
--- /home/nick/src/gdb/mi/mi-cmd-stack.c	2005-06-19 14:45:13.000000000 +1200
***************
*** 329,331 ****
--- 329,341 ----
    select_frame_command (argv[0], 1 /* not used */ );
    return MI_CMD_DONE;
  }
+ 
+ enum mi_cmd_result
+ mi_cmd_stack_info_frame (char *command, char **argv, int argc)
+ {
+   if (argc > 0)
+     error (_("mi_cmd_stack_info_frame: No arguments required"));
+   
+   print_frame_info (get_selected_frame (NULL), 1, LOC_AND_ADDRESS, 0);
+   return MI_CMD_DONE;
+ }


*** /home/nick/src/gdb/mi/mi-cmds.c.~1.17~	2005-06-19 15:33:32.000000000 +1200
--- /home/nick/src/gdb/mi/mi-cmds.c	2005-06-19 13:57:36.000000000 +1200
***************
*** 107,112 ****
--- 107,113 ----
    { "signal-list-handle-actions", { NULL, 0 }, NULL, NULL },
    { "signal-list-signal-types", { NULL, 0 }, NULL, NULL },
    { "stack-info-depth", { NULL, 0 }, 0, mi_cmd_stack_info_depth},
+   { "stack-info-frame", { NULL, 0 }, 0, mi_cmd_stack_info_frame},
    { "stack-list-arguments", { NULL, 0 }, 0, mi_cmd_stack_list_args},
    { "stack-list-exception-handlers", { NULL, 0 }, NULL, NULL },
    { "stack-list-frames", { NULL, 0 }, 0, mi_cmd_stack_list_frames},


*** /home/nick/src/gdb/mi/mi-cmds.h.~1.15~	2005-06-19 15:34:22.000000000 +1200
--- /home/nick/src/gdb/mi/mi-cmds.h	2005-06-19 13:58:18.000000000 +1200
***************
*** 87,92 ****
--- 87,93 ----
  extern mi_cmd_argv_ftype mi_cmd_gdb_exit;
  extern mi_cmd_argv_ftype mi_cmd_interpreter_exec;
  extern mi_cmd_argv_ftype mi_cmd_stack_info_depth;
+ extern mi_cmd_argv_ftype mi_cmd_stack_info_frame;
  extern mi_cmd_argv_ftype mi_cmd_stack_list_args;
  extern mi_cmd_argv_ftype mi_cmd_stack_list_frames;
  extern mi_cmd_argv_ftype mi_cmd_stack_list_locals;


*** /home/nick/src/gdb/doc/gdb.texinfo.~1.269~	2005-06-19 15:03:43.000000000 +1200
--- /home/nick/src/gdb/doc/gdb.texinfo~	2005-06-19 14:57:04.000000000 +1200
***************
*** 19223,19228 ****
--- 19223,19257 ----
  @node GDB/MI Stack Manipulation
  @section @sc{gdb/mi} Stack Manipulation Commands
  
+ 
+ @subheading The @code{-stack-info-frame} Command
+ @findex -stack-info-frame
+ 
+ @subsubheading Synopsis
+ 
+ @smallexample
+  -stack-info-frame
+ @end smallexample
+ 
+ Get info on the selected frame.
+ 
+ @subsubheading @value{GDBN} Command
+ 
+ The corresponding @value{GDBN} command is @samp{info frame} or @samp{frame}
+ (without arguments).
+ 
+ @subsubheading Example
+ 
+ @smallexample
+ (@value{GDBP})
+ -stack-info-frame
+ ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
+ args=[@{name="strarg",value="0x11940 \"A string argument.\""@}],
+ file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
+ fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
+ (@value{GDBP})
+ @end smallexample
+ 
  @subheading The @code{-stack-info-depth} Command
  @findex -stack-info-depth

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sun, Jun 19, 2005 at 03:39:42PM +1200, Nick Roberts wrote:
>  > > Yes, unless Apple's proposed merge will provide the necessary information.
>  > 
>  > Let's not wait on that.  You've demonstrated a use for
>  > -stack-info-frame relative to the current source base.  That's plenty
>  > good enough for me.
>  > 
>  > Of course now we need to re-add the documentation (with example this
>  > time).  A test case would be nice too.  Since we've decided that we do
>  > want this feature, could you put that together?
> 
> OK.  I've committed the -stack-info-frame part of the change that posted (Sat,
> 18 Jun 2005 10:52:09 +1200).  Perhaps that doesn't follow the letter of the
> law but I hope it follows the spirit.  In any case, I find it easier to make
> further changes to the repository than juggle patches (as demonstrated shown
> with my earlier mangling).

No, Nick.  You don't get to make up rules as you go along, no matter
how much the current ones irk you.  That patch was never reviewed and
never approved.  Don't do that again; if you won't wait for approval,
we'll remove you from write-after-approval.

If you have trouble juggling patches, have a complete checkout for each
independent project you are working on.  That's not hard to do.

+enum mi_cmd_result
+mi_cmd_stack_info_frame (char *command, char **argv, int argc)
+{
+  if (argc > 0)
+    error (_("mi_cmd_stack_info_frame: No arguments required"));
+
+  print_frame_info (get_selected_frame (NULL), 1, LOC_AND_ADDRESS, 0);
+  return MI_CMD_DONE;
+}

"No arguments required" doesn't make much sense as an error message; it
suggests that no arguments are necessary, but not that any arguments
are invalid.  But I see there are two uses of it already, and none of
any other format for functions which take no arguements.  So the code
parts of the patch are belatedly OK...

> This commit is slightly different in two respects:
> 
> 1) mi_cmd_stack_info_frame uses print_frame_info instead of print_stack_frame.
>    This follows mi_cmd_stack_list_frames and means that the argument values
>    aren't printed.
> 
> 2) The documentation for -stack-info-frame previously said (before I removed
>    it) "Get info on the current frame.".  I've corrected this to
>    "Get info on the selected frame."  I've also removed the argument values
>    from the example as explained in 1).

Despite the fact that you made it up as you went along.  Why did you
decide that this change was a better idea?

The documentation is up to Eli, but I can say with some confidence that
it is NOT ok, since you didn't really remove argument values from the
examples.  I still see one:

> + @smallexample
> + (@value{GDBP})
> + -stack-info-frame
> + ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
> + args=[@{name="strarg",value="0x11940 \"A string argument.\""@}],
> + file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
> + fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
> + (@value{GDBP})
> + @end smallexample
> + 


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Eli Zaretskii]

> Date: Sun, 19 Jun 2005 10:56:13 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
> 
> The documentation is up to Eli, but I can say with some confidence that
> it is NOT ok, since you didn't really remove argument values from the
> examples.  I still see one:
> 
> > + @smallexample
> > + (@value{GDBP})
> > + -stack-info-frame
> > + ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
> > + args=[@{name="strarg",value="0x11940 \"A string argument.\""@}],
> > + file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
> > + fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
> > + (@value{GDBP})
> > + @end smallexample
> > + 

Yes, Nick, please fix this.

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > > Date: Sun, 19 Jun 2005 10:56:13 -0400
 > > From: Daniel Jacobowitz <drow@false.org>
 > > Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
 > > 
 > > The documentation is up to Eli, but I can say with some confidence that
 > > it is NOT ok, since you didn't really remove argument values from the
 > > examples.  I still see one:
 > > 
 > > > + @smallexample
 > > > + (@value{GDBP})
 > > > + -stack-info-frame
 > > > + ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
 > > > + args=[@{name="strarg",value="0x11940 \"A string argument.\""@}],
 > > > + file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
 > > > + fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
 > > > + (@value{GDBP})
 > > > + @end smallexample
 > > > + 
 > 
 > Yes, Nick, please fix this.

The patch was OK but the diff wasn't.  I picked up the backup copy by mistake:

*** /home/nick/src/gdb/doc/gdb.texinfo.~1.269~	2005-06-19...
--- /home/nick/src/gdb/doc/gdb.texinfo~	2005-06-19...
***************
                                     ^^^

In future, I'll diff on the repository (and wait for approval!).

Nick


*** /home/nick/src/gdb/doc/gdb.texinfo.~1.269~	2005-06-19 15:03:43.000000000 +1200
--- /home/nick/src/gdb/doc/gdb.texinfo	2005-06-19 15:00:52.000000000 +1200
***************
*** 19223,19228 ****
--- 19223,19256 ----
  @node GDB/MI Stack Manipulation
  @section @sc{gdb/mi} Stack Manipulation Commands
  
+ 
+ @subheading The @code{-stack-info-frame} Command
+ @findex -stack-info-frame
+ 
+ @subsubheading Synopsis
+ 
+ @smallexample
+  -stack-info-frame
+ @end smallexample
+ 
+ Get info on the selected frame.
+ 
+ @subsubheading @value{GDBN} Command
+ 
+ The corresponding @value{GDBN} command is @samp{info frame} or @samp{frame}
+ (without arguments).
+ 
+ @subsubheading Example
+ 
+ @smallexample
+ (@value{GDBP})
+ -stack-info-frame
+ ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
+ file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
+ fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
+ (@value{GDBP})
+ @end smallexample
+ 
  @subheading The @code{-stack-info-depth} Command
  @findex -stack-info-depth
  

Re: [PATCH] -stack-info-frames

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Mon, 20 Jun 2005 10:32:38 +1200
> Cc: gdb-patches@sources.redhat.com
> 
> + 
> + @subheading The @code{-stack-info-frame} Command
> + @findex -stack-info-frame
> + 
> + @subsubheading Synopsis
> + 
> + @smallexample
> +  -stack-info-frame
> + @end smallexample
> + 
> + Get info on the selected frame.
> + 
> + @subsubheading @value{GDBN} Command
> + 
> + The corresponding @value{GDBN} command is @samp{info frame} or @samp{frame}
> + (without arguments).
> + 
> + @subsubheading Example
> + 
> + @smallexample
> + (@value{GDBP})
> + -stack-info-frame
> + ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
> + file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
> + fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
> + (@value{GDBP})
> + @end smallexample
> + 
>   @subheading The @code{-stack-info-depth} Command
>   @findex -stack-info-depth

This is okay, thanks.

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > > OK.  I've committed the -stack-info-frame part of the change that posted
 > > (Sat, 18 Jun 2005 10:52:09 +1200).  Perhaps that doesn't follow the
 > > letter of the law but I hope it follows the spirit.  In any case, I find
 > > it easier to make further changes to the repository than juggle patches
 > > (as demonstrated shown with my earlier mangling).
 > 
 > No, Nick.  You don't get to make up rules as you go along, no matter
 > how much the current ones irk you.  That patch was never reviewed and
 > never approved.  Don't do that again; if you won't wait for approval,
 > we'll remove you from write-after-approval.

I wasn't trying to make up the rules, just interpret them.  I posted a very
similar patch earlier which was reviewed.  As no branch/release is imminent,
it seemed a safe thing to do.  I know not to apply judgement again.  Sorry.

 > If you have trouble juggling patches, have a complete checkout for each
 > independent project you are working on.  That's not hard to do.

I'll have to work out a new routine.  Contributing to Emacs works differently.

 > +enum mi_cmd_result
 > +mi_cmd_stack_info_frame (char *command, char **argv, int argc)
 > +{
 > +  if (argc > 0)
 > +    error (_("mi_cmd_stack_info_frame: No arguments required"));
 > +
 > +  print_frame_info (get_selected_frame (NULL), 1, LOC_AND_ADDRESS, 0);
 > +  return MI_CMD_DONE;
 > +}
 > 
 > "No arguments required" doesn't make much sense as an error message; it
 > suggests that no arguments are necessary, but not that any arguments
 > are invalid.  But I see there are two uses of it already, and none of
 > any other format for functions which take no arguements.  So the code
 > parts of the patch are belatedly OK...

Where possible, I just copy what is already there.

 > > This commit is slightly different in two respects:
 > > 
 > > 1) mi_cmd_stack_info_frame uses print_frame_info instead of
 > >    print_stack_frame.  This follows mi_cmd_stack_list_frames and means
 > >    that the argument values aren't printed.
 > > 
 > > 2) The documentation for -stack-info-frame previously said (before I
 > >    removed it) "Get info on the current frame.".  I've corrected this to
 > >    "Get info on the selected frame."  I've also removed the argument
 > >    values from the example as explained in 1).
 > 
 > Despite the fact that you made it up as you went along.  Why did you
 > decide that this change was a better idea?

Which change?

 > The documentation is up to Eli, but I can say with some confidence that
 > it is NOT ok, since you didn't really remove argument values from the
 > examples.  I still see one:
 > 
 > > + @smallexample
 > > + (@value{GDBP})
 > > + -stack-info-frame
 > > + ^done,frame=@{level="1",addr="0x0001076c",func="callee3",
 > > + args=[@{name="strarg",value="0x11940 \"A string argument.\""@}],
 > > + file="../../../devo/gdb/testsuite/gdb.mi/basics.c",
 > > + fullname="/home/foo/bar/devo/gdb/testsuite/gdb.mi/basics.c",line="17"@}
 > > + (@value{GDBP})
 > > + @end smallexample
 > > + 

The patch was OK but the diff wasn't.  I picked up the backup copy by mistake:

*** /home/nick/src/gdb/doc/gdb.texinfo.~1.269~	2005-06-19...
--- /home/nick/src/gdb/doc/gdb.texinfo~	2005-06-19...
***************
                                     ^^^

Nick

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Mon, Jun 20, 2005 at 10:24:19AM +1200, Nick Roberts wrote:
> I wasn't trying to make up the rules, just interpret them.  I posted a very
> similar patch earlier which was reviewed.  As no branch/release is imminent,
> it seemed a safe thing to do.  I know not to apply judgement again.  Sorry.

OK, thanks.

>  > If you have trouble juggling patches, have a complete checkout for each
>  > independent project you are working on.  That's not hard to do.
> 
> I'll have to work out a new routine.  Contributing to Emacs works differently.

It's a serious suggestion - I often have a tree for everything I'm
working on, and that lets me make sure that I keep things independent.

>  > > This commit is slightly different in two respects:
>  > > 
>  > > 1) mi_cmd_stack_info_frame uses print_frame_info instead of
>  > >    print_stack_frame.  This follows mi_cmd_stack_list_frames and means
>  > >    that the argument values aren't printed.
>  > > 
>  > > 2) The documentation for -stack-info-frame previously said (before I
>  > >    removed it) "Get info on the current frame.".  I've corrected this to
>  > >    "Get info on the selected frame."  I've also removed the argument
>  > >    values from the example as explained in 1).
>  > 
>  > Despite the fact that you made it up as you went along.  Why did you
>  > decide that this change was a better idea?
> 
> Which change?

#1.  Why not print the arguments?  I guess your logic is that if you
want them, you can find them by a round trip through
-stack-list-frames, is that it?

> The patch was OK but the diff wasn't.  I picked up the backup copy by mistake:

Great!  Thank you.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > >  > > 1) mi_cmd_stack_info_frame uses print_frame_info instead of
 > >  > >    print_stack_frame.  This follows mi_cmd_stack_list_frames and means
 > >  > >    that the argument values aren't printed.
 ...

 > > Which change?
 > 
 > #1.  Why not print the arguments?  I guess your logic is that if you
 > want them, you can find them by a round trip through
 > -stack-list-frames, is that it?

Well, through -stack-list-arguments (mi_cmd_stack_list_frames doesn't print
arguments: I've used the same call to print_frame_info in -stack-info-frame).
Also print_stack_frame calls eventually calls print_frame_info after a call to
catch_errors, so it seemed more efficient.

Nick

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Mon, Jun 20, 2005 at 01:25:28PM +1200, Nick Roberts wrote:
>  > >  > > 1) mi_cmd_stack_info_frame uses print_frame_info instead of
>  > >  > >    print_stack_frame.  This follows mi_cmd_stack_list_frames and means
>  > >  > >    that the argument values aren't printed.
>  ...
> 
>  > > Which change?
>  > 
>  > #1.  Why not print the arguments?  I guess your logic is that if you
>  > want them, you can find them by a round trip through
>  > -stack-list-frames, is that it?
> 
> Well, through -stack-list-arguments (mi_cmd_stack_list_frames doesn't print
> arguments: I've used the same call to print_frame_info in -stack-info-frame).
> Also print_stack_frame calls eventually calls print_frame_info after a call to
> catch_errors, so it seemed more efficient.

OK, makes sense.  Thank you.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Mon, 20 Jun 2005 10:24:19 +1200
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
> 
> Contributing to Emacs works differently.

Yep.

>  > "No arguments required" doesn't make much sense as an error message; it
>  > suggests that no arguments are necessary, but not that any arguments
>  > are invalid.  But I see there are two uses of it already, and none of
>  > any other format for functions which take no arguements.  So the code
>  > parts of the patch are belatedly OK...
> 
> Where possible, I just copy what is already there.

That is not always a good idea.  Sometimes it is better to modify old
code/docs, instead of proliferating past blunders in the name of
consistency.

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > >  > "No arguments required" doesn't make much sense as an error message; it
 > >  > suggests that no arguments are necessary, but not that any arguments
 > >  > are invalid.  But I see there are two uses of it already, and none of
 > >  > any other format for functions which take no arguements.  So the code
 > >  > parts of the patch are belatedly OK...
 > > 
 > > Where possible, I just copy what is already there.
 > 
 > That is not always a good idea.  Sometimes it is better to modify old
 > code/docs, instead of proliferating past blunders in the name of
 > consistency.

Currently:

(gdb)
-stack-info-frame 4
&"mi_cmd_stack_info_frame: No arguments required\n"
^error,msg="mi_cmd_stack_info_frame: No arguments required"
(gdb) 

(gdb) 
-stack-select-frame
&"mi_cmd_stack_select_frame: Usage: FRAME_SPEC\n"
^error,msg="mi_cmd_stack_select_frame: Usage: FRAME_SPEC"
(gdb) 


The other thing I find inappropriate about these messages is that they
print out the name of the procedure which is not of immediate interest.

How about changing the format (for all MI commands) to

-stack-info-frame 4
&"Usage: -stack-info-frame\n"
^error,msg="Usage: -stack-info-frame"
(gdb) 

(gdb) 
-stack-select-frame
&"Usage: -stack-select-frame FRAME_SPEC\n"
^error,msg="Usage: -stack-select-frame FRAME_SPEC"
(gdb) 

etc.

They should only occur when a developer is writing a frontend, not when it is
being used.  The procedure name is only of interest to someone writing MI code
For that person it should be quite easy to find the relevant code and, even if
it was not, it should only be printed if a debug flag is set.

Nick

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Mon, Jun 20, 2005 at 05:03:05PM +1200, Nick Roberts wrote:
> The other thing I find inappropriate about these messages is that they
> print out the name of the procedure which is not of immediate interest.

I agree.  I don't think the function or command name in all these error
messages is valuable.

> How about changing the format (for all MI commands) to
> 
> -stack-info-frame 4
> &"Usage: -stack-info-frame\n"
> ^error,msg="Usage: -stack-info-frame"
> (gdb) 
> 
> (gdb) 
> -stack-select-frame
> &"Usage: -stack-select-frame FRAME_SPEC\n"
> ^error,msg="Usage: -stack-select-frame FRAME_SPEC"
> (gdb) 
> 
> etc.
> 
> They should only occur when a developer is writing a frontend, not when it is
> being used.  The procedure name is only of interest to someone writing MI code
> For that person it should be quite easy to find the relevant code and, even if
> it was not, it should only be printed if a debug flag is set.

I like this idea; in fact, this is the style I was going to recommend
to you (until I went through and noticed that it is only used for
functions with arguments, not without, at the moment).

-- 
Daniel Jacobowitz
CodeSourcery, LLC

[PATCH] MI error messages

[Nick Roberts]

 > I like this idea; in fact, this is the style I was going to recommend
 > to you (until I went through and noticed that it is only used for
 > functions with arguments, not without, at the moment).

Here's a patch for mi-cmd-stack.c and mi-cmd-var.c to start with. Only the
word "Usage" should be translated so e.g

    error (_("Usage: -stack-list-locals PRINT_VALUES"));

should be something like:

    error (_("Usage")(": -stack-list-locals PRINT_VALUES"));

but I don't what the correct way to do this is.

Also, in mi-cmd-stack.c, I've given get_selected_frame the message string
"No stack." but presumably this also gets translated so should it be:

    get_selected_frame (_("No stack.")) ?

Some error messages are terminated with a period, others aren't.  I don't
know which style is preferred but clearly it would be best to just use
one.

I'll go through the rest, but I thought I'd try to get these right first.

Nick


2005-06-21  Nick Roberts  <nickrob@snap.net.nz>

	* mi/mi-cmd-stack.c, mi/mi-cmd-var.c: Re-style error strings for
	usage of MI commands.


Index: mi/mi-cmd-stack.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-stack.c,v
retrieving revision 1.27
diff -c -r1.27 mi-cmd-stack.c
*** mi/mi-cmd-stack.c	19 Jun 2005 03:11:47 -0000	1.27
--- mi/mi-cmd-stack.c	20 Jun 2005 21:30:28 -0000
***************
*** 48,54 ****
    struct frame_info *fi;
  
    if (argc > 2 || argc == 1)
!     error (_("mi_cmd_stack_list_frames: Usage: [FRAME_LOW FRAME_HIGH]"));
  
    if (argc == 2)
      {
--- 48,54 ----
    struct frame_info *fi;
  
    if (argc > 2 || argc == 1)
!     error (_("Usage: -stack-list-frames [FRAME_LOW FRAME_HIGH]"));
  
    if (argc == 2)
      {
***************
*** 102,108 ****
    struct frame_info *fi;
  
    if (argc > 1)
!     error (_("mi_cmd_stack_info_depth: Usage: [MAX_DEPTH]"));
  
    if (argc == 1)
      frame_high = atoi (argv[0]);
--- 102,108 ----
    struct frame_info *fi;
  
    if (argc > 1)
!     error (_("Usage: -stack-info-depth [MAX_DEPTH]"));
  
    if (argc == 1)
      frame_high = atoi (argv[0]);
***************
*** 131,139 ****
    enum print_values print_values;
  
    if (argc != 1)
!     error (_("mi_cmd_stack_list_locals: Usage: PRINT_VALUES"));
  
!    frame = get_selected_frame (NULL);
  
     if (strcmp (argv[0], "0") == 0
         || strcmp (argv[0], "--no-values") == 0)
--- 131,139 ----
    enum print_values print_values;
  
    if (argc != 1)
!     error (_("Usage: -stack-list-locals PRINT_VALUES"));
  
!    frame = get_selected_frame ("No stack.");
  
     if (strcmp (argv[0], "0") == 0
         || strcmp (argv[0], "--no-values") == 0)
***************
*** 163,169 ****
    struct cleanup *cleanup_stack_args;
  
    if (argc < 1 || argc > 3 || argc == 2)
!     error (_("mi_cmd_stack_list_args: Usage: PRINT_VALUES [FRAME_LOW FRAME_HIGH]"));
  
    if (argc == 3)
      {
--- 163,169 ----
    struct cleanup *cleanup_stack_args;
  
    if (argc < 1 || argc > 3 || argc == 2)
!     error (_("Usage: -stack-list-arguments PRINT_VALUES [FRAME_LOW FRAME_HIGH]"));
  
    if (argc == 3)
      {
***************
*** 324,330 ****
  mi_cmd_stack_select_frame (char *command, char **argv, int argc)
  {
    if (argc == 0 || argc > 1)
!     error (_("mi_cmd_stack_select_frame: Usage: FRAME_SPEC"));
  
    select_frame_command (argv[0], 1 /* not used */ );
    return MI_CMD_DONE;
--- 324,330 ----
  mi_cmd_stack_select_frame (char *command, char **argv, int argc)
  {
    if (argc == 0 || argc > 1)
!     error (_("Usage: -stack-select-frame FRAME_SPEC"));
  
    select_frame_command (argv[0], 1 /* not used */ );
    return MI_CMD_DONE;
***************
*** 334,341 ****
  mi_cmd_stack_info_frame (char *command, char **argv, int argc)
  {
    if (argc > 0)
!     error (_("mi_cmd_stack_info_frame: No arguments required"));
    
!   print_frame_info (get_selected_frame (NULL), 1, LOC_AND_ADDRESS, 0);
    return MI_CMD_DONE;
  }
--- 334,341 ----
  mi_cmd_stack_info_frame (char *command, char **argv, int argc)
  {
    if (argc > 0)
!     error (_("Usage: -stack-info-frame"));
    
!   print_frame_info (get_selected_frame ("No stack."), 1, LOC_AND_ADDRESS, 0);
    return MI_CMD_DONE;
  }


Index: mi/mi-cmd-var.c
===================================================================
RCS file: /cvs/src/src/gdb/mi/mi-cmd-var.c,v
retrieving revision 1.21
diff -c -r1.21 mi-cmd-var.c
*** mi/mi-cmd-var.c	11 Feb 2005 04:06:11 -0000	1.21
--- mi/mi-cmd-var.c	20 Jun 2005 21:31:08 -0000
***************
*** 52,58 ****
      {
        /* mi_error_message = xstrprintf ("mi_cmd_var_create: Usage:
           ...."); return MI_CMD_ERROR; */
!       error (_("mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."));
      }
  
    name = xstrdup (argv[0]);
--- 52,58 ----
      {
        /* mi_error_message = xstrprintf ("mi_cmd_var_create: Usage:
           ...."); return MI_CMD_ERROR; */
!       error (_("Usage: -var-create NAME FRAME EXPRESSION."));
      }
  
    name = xstrdup (argv[0]);
***************
*** 119,125 ****
    struct cleanup *old_cleanups;
  
    if (argc < 1 || argc > 2)
!     error (_("mi_cmd_var_delete: Usage: [-c] EXPRESSION."));
  
    name = xstrdup (argv[0]);
    /* Add cleanup for name. Must be free_current_contents as
--- 119,125 ----
    struct cleanup *old_cleanups;
  
    if (argc < 1 || argc > 2)
!     error (_("Usage: -var-delete [-c] EXPRESSION."));
  
    name = xstrdup (argv[0]);
    /* Add cleanup for name. Must be free_current_contents as
***************
*** 174,180 ****
    char *formspec;
  
    if (argc != 2)
!     error (_("mi_cmd_var_set_format: Usage: NAME FORMAT."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 174,180 ----
    char *formspec;
  
    if (argc != 2)
!     error (_("Usage: -var-set-format NAME FORMAT."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 216,222 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_show_format: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 216,222 ----
    struct varobj *var;
  
    if (argc != 1)
!     error (_("Usage: -var-show-format NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 236,242 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_info_num_children: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 236,242 ----
    struct varobj *var;
  
    if (argc != 1)
!     error (_("Usage: -var-info-num-children NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 259,265 ****
    enum print_values print_values;
  
    if (argc != 1 && argc != 2)
!     error (_("mi_cmd_var_list_children: Usage: [PRINT_VALUES] NAME"));
  
    /* Get varobj handle, if a valid var obj name was specified */
    if (argc == 1) var = varobj_get_handle (argv[0]);
--- 259,265 ----
    enum print_values print_values;
  
    if (argc != 1 && argc != 2)
!     error (_("Usage: -var-list-children [PRINT_VALUES] NAME"));
  
    /* Get varobj handle, if a valid var obj name was specified */
    if (argc == 1) var = varobj_get_handle (argv[0]);
***************
*** 315,321 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_info_type: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 315,321 ----
    struct varobj *var;
  
    if (argc != 1)
!     error (_("Usage: -var-info-type NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 333,339 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_info_expression: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 333,339 ----
    struct varobj *var;
  
    if (argc != 1)
!     error (_("Usage: -var-info-expression NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 355,361 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_show_attributes: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 355,361 ----
    struct varobj *var;
  
    if (argc != 1)
!     error (_("Usage: -var-show-attributes NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 379,385 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_evaluate_expression: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 379,385 ----
    struct varobj *var;
  
    if (argc != 1)
!     error (_("Usage: -var-evaluate-expression NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 397,403 ****
    char *expression;
  
    if (argc != 2)
!     error (_("mi_cmd_var_assign: Usage: NAME EXPRESSION."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
--- 397,403 ----
    char *expression;
  
    if (argc != 2)
!     error (_("Usage: -var-assign NAME EXPRESSION."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
***************
*** 428,434 ****
    int nv;
  
    if (argc != 1)
!     error (_("mi_cmd_var_update: Usage: NAME."));
  
    name = argv[0];
  
--- 428,434 ----
    int nv;
  
    if (argc != 1)
!     error (_("Usage: -var-update NAME."));
  
    name = argv[0];
  

Re: [PATCH] MI error messages

[Andreas Schwab]

Nick Roberts <nickrob@snap.net.nz> writes:

> should be something like:
>
>     error (_("Usage")(": -stack-list-locals PRINT_VALUES"));
>
> but I don't what the correct way to do this is.

Try this:

     error (_("Usage: %s"), "-stack-list-locals PRINT_VALUES");

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] MI error messages

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Tue, 21 Jun 2005 09:42:10 +1200
> Cc: Eli Zaretskii <eliz@gnu.org>, gdb-patches@sources.redhat.com
> 
>  > I like this idea; in fact, this is the style I was going to recommend
>  > to you (until I went through and noticed that it is only used for
>  > functions with arguments, not without, at the moment).
> 
> Here's a patch for mi-cmd-stack.c and mi-cmd-var.c to start with. Only the
> word "Usage" should be translated so e.g
> 
>     error (_("Usage: -stack-list-locals PRINT_VALUES"));
> 
> should be something like:
> 
>     error (_("Usage")(": -stack-list-locals PRINT_VALUES"));
> 
> but I don't what the correct way to do this is.

Andreas suggested one way to do this.  But I think it would be better
to add a function called, say, mi_error, that would do

      error (_("Usage: %s."), msg);

where `msg' is a char * string passed as its argument, and then
replace each call to `error' with a call to `mi_error', like so:

      mi_error ("-stack-list-locals PRINT_VALUES");

The advantage of this is that "Usage: %s" is not repeated dozens of
times in the message catalog and in the program.

> Also, in mi-cmd-stack.c, I've given get_selected_frame the message string
> "No stack." but presumably this also gets translated so should it be:
> 
>     get_selected_frame (_("No stack.")) ?

Yes.

> Some error messages are terminated with a period, others aren't.  I don't
> know which style is preferred but clearly it would be best to just use
> one.

They should all end with a period, I think.

Re: [PATCH] MI error messages

[Andreas Schwab]

Eli Zaretskii <eliz@gnu.org> writes:

> The advantage of this is that "Usage: %s" is not repeated dozens of
> times in the message catalog and in the program.

xgettext merges identical strings, so that would not be a problem.  But the
duplication in the program is still a valid point.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] MI error messages

[Nick Roberts]

 > Andreas suggested one way to do this.  But I think it would be better
 > to add a function called, say, mi_error, that would do
 > 
 >       error (_("Usage: %s."), msg);
 > 
 > where `msg' is a char * string passed as its argument, and then
 > replace each call to `error' with a call to `mi_error', like so:
 > 
 >       mi_error ("-stack-list-locals PRINT_VALUES");
 > 
 > The advantage of this is that "Usage: %s" is not repeated dozens of
 > times in the message catalog and in the program.

Which file should mi_error/mi_usage_error go in?  mi-cmds.c seems the best
option to me as the mi-cmd-*.c files include mi-cmds.h.

I think it would be better to call it something like mi_usage_error, as there
are many other errors generated in MI e.g.

    error (_("mi_cmd_var_delete: Variable object not found."));

which, I guess should just be:

    error (_("Variable object not found."));

However, unlike the usage error messages which should only be seen by the
person writing the frontend, these messages _will_ be seen by the user.
Perhaps they should be made more transparent.  For example, if I inadvertantly
selected the word warranty before clicking on the tool bar to create a watch
expression.  The frontend would send:

-var-create - * warranty

GDB would reply:

&"mi_cmd_var_create: unable to create variable object\n"
^error,msg="mi_cmd_var_create: unable to create variable object"

which the frontend could parse, but a more helpful message would be:

"No symbol warranty in current context."

 > > Also, in mi-cmd-stack.c, I've given get_selected_frame the message string
 > > "No stack." but presumably this also gets translated so should it be:
 > > 
 > >     get_selected_frame (_("No stack.")) ?
 > 
 > Yes.

OK

 > > Some error messages are terminated with a period, others aren't.  I don't
 > > know which style is preferred but clearly it would be best to just use
 > > one.
 > 
 > They should all end with a period, I think.

OK

Nick

Re: [PATCH] MI error messages

[Eli Zaretskii]

> From: Nick Roberts <nickrob@snap.net.nz>
> Date: Tue, 21 Jun 2005 21:40:38 +1200
> Cc: gdb-patches@sources.redhat.com
> 
> Which file should mi_error/mi_usage_error go in?  mi-cmds.c seems the best
> option to me as the mi-cmd-*.c files include mi-cmds.h.

I don't care much, but isn't mi-common.c a better place?

> I think it would be better to call it something like mi_usage_error

I have no problem with mi_usage_error.

> there are many other errors generated in MI e.g.
> 
>     error (_("mi_cmd_var_delete: Variable object not found."));
> 
> which, I guess should just be:
> 
>     error (_("Variable object not found."));
> 
> However, unlike the usage error messages which should only be seen by the
> person writing the frontend, these messages _will_ be seen by the user.

Yes.  And that is why they should go through `error', not through
`mi_usage_error'.

> Perhaps they should be made more transparent.

I guess you mean ``more self-explanatory''.  Yes, I agree.  One way of
doing that is not to hide relevant context information that is
available at the locus of the error message; in this case, that
context is the name of the object.  Thus,

	Variable object `warranty' not found.

is IMHO much better, even though your suggestion is an additional
improvement.

Re: [PATCH] MI error messages

[Nick Roberts]

Eli Zaretskii writes:
 > > Which file should mi_error/mi_usage_error go in?  mi-cmds.c seems the best
 > > option to me as the mi-cmd-*.c files include mi-cmds.h.
 > 
 > I don't care much, but isn't mi-common.c a better place?

mi-common.c seems a bit of a misnomer.  Perhap common here means frequent
rather than shared.  mi-common.h isn't included in any of the c files apart
from mi-common.c

 > > I think it would be better to call it something like mi_usage_error
 > 
 > I have no problem with mi_usage_error.
 > 
 > > there are many other errors generated in MI e.g.
 > > 
 > >     error (_("mi_cmd_var_delete: Variable object not found."));
 > > 
 > > which, I guess should just be:
 > > 
 > >     error (_("Variable object not found."));
 > > 
 > > However, unlike the usage error messages which should only be seen by the
 > > person writing the frontend, these messages _will_ be seen by the user.
 > 
 > Yes.  And that is why they should go through `error', not through
 > `mi_usage_error'.

I presumed mi_usage_error would call error.  That way the error is caught and
any cleanups and rewinds are done.  However, you are right, it would be nicer
just to get:

(gdb) 
-stack-select-frame
Usage: FRAME_SPEC.
(gdb) 

instead of

(gdb) 
-stack-select-frame
&"Usage: -stack-select-frame FRAME_SPEC\n"
^error,msg="Usage: -stack-select-frame FRAME_SPEC"
(gdb) 

as these errors aren't intended for the user when the frontend is being used.

 > > Perhaps they should be made more transparent.
 > 
 > I guess you mean ``more self-explanatory''.  Yes, I agree.  One way of
 > doing that is not to hide relevant context information that is
 > available at the locus of the error message; in this case, that
 > context is the name of the object.  Thus,
 > 
 > 	Variable object `warranty' not found.
 > 
 > is IMHO much better, even though your suggestion is an additional
 > improvement.

I'll look at submitting a revised patch for mi-cmd-stack.c and mi-cmd-var.c

Nick

Re: [PATCH] MI error messages

[Jason Molenda]

On Jun 21, 2005, at 2:44 PM, Nick Roberts wrote:

> I presumed mi_usage_error would call error.  That way the error is  
> caught and
> any cleanups and rewinds are done.  However, you are right, it  
> would be nicer
> just to get:
>
> (gdb)
> -stack-select-frame
> Usage: FRAME_SPEC.
> (gdb)
>
> instead of
>
> (gdb)
> -stack-select-frame
> &"Usage: -stack-select-frame FRAME_SPEC\n"
> ^error,msg="Usage: -stack-select-frame FRAME_SPEC"
> (gdb)
>
> as these errors aren't intended for the user when the frontend is  
> being used.


Yeah, we came to the same decision at Apple.  When you throw error()  
while in MI mode, you only get the ^error message, you don't get the  
console-style &"..." with the same message.  Our GUI has a little  
status line at the bottom of the window where it shows the error  
message, and if you have the "gdb console" window open, it shows the  
error text in a different color to differentiate the error message.   
It makes sense to let the GUI show the error in whatever way is most  
appropriate for it, instead of blotting back plain old text.

J

Re: [PATCH] MI error messages

[Daniel Jacobowitz]

On Tue, Jun 21, 2005 at 02:59:11PM -0700, Jason Molenda wrote:
> 
> On Jun 21, 2005, at 2:44 PM, Nick Roberts wrote:
> 
> >I presumed mi_usage_error would call error.  That way the error is  
> >caught and
> >any cleanups and rewinds are done.  However, you are right, it  
> >would be nicer
> >just to get:
> >
> >(gdb)
> >-stack-select-frame
> >Usage: FRAME_SPEC.
> >(gdb)
> >
> >instead of
> >
> >(gdb)
> >-stack-select-frame
> >&"Usage: -stack-select-frame FRAME_SPEC\n"
> >^error,msg="Usage: -stack-select-frame FRAME_SPEC"
> >(gdb)
> >
> >as these errors aren't intended for the user when the frontend is  
> >being used.
> 
> 
> Yeah, we came to the same decision at Apple.  When you throw error()  
> while in MI mode, you only get the ^error message, you don't get the  
> console-style &"..." with the same message.  Our GUI has a little  
> status line at the bottom of the window where it shows the error  
> message, and if you have the "gdb console" window open, it shows the  
> error text in a different color to differentiate the error message.   
> It makes sense to let the GUI show the error in whatever way is most  
> appropriate for it, instead of blotting back plain old text.

I'm not sure what you're agreeing with here.  I think it makes plenty
of sense to update error handling to just show the ^error, not &"..."
also - though that is an incompatible change and would need some
wider testing.  I strongly disagree with outputing unformatted errors
as in Nick's first example, though.

We have a syntax for error messages... let's use it?
-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] MI error messages

[Jason Molenda]

On Jun 21, 2005, at 8:32 PM, Daniel Jacobowitz wrote:

> I'm not sure what you're agreeing with here.

Sorry, you're right, I wasn't being clear.

> I think it makes plenty
> of sense to update error handling to just show the ^error, not &"..."
> also - though that is an incompatible change and would need some
> wider testing.

Agreed.  Honestly I don't know if there are any deployed GUIs that  
use MI except for Xcode -- I hear Eclipse does -- but I can imagine a  
(poor) implementation dropping the ^error message text on the floor  
and assuming that the user will see the error message in the gdb \x05\x12I/ 
O window.


> I strongly disagree with outputing unformatted errors
> as in Nick's first example, though.

Yes, I agree that outputting unformatted errors is a bad idea.


J

Re: [PATCH] MI error messages

[Daniel Jacobowitz]

On Tue, Jun 21, 2005 at 08:41:26PM -0700, Jason Molenda wrote:
> >I think it makes plenty
> >of sense to update error handling to just show the ^error, not &"..."
> >also - though that is an incompatible change and would need some
> >wider testing.
> 
> Agreed.  Honestly I don't know if there are any deployed GUIs that  
> use MI except for Xcode -- I hear Eclipse does -- but I can imagine a  

Yes, it does - more or less.  I imagine there are a few others by now.

> (poor) implementation dropping the ^error message text on the floor  
> and assuming that the user will see the error message in the gdb \x05\x12I/ 
> O window.

Exactly; that's what I'm worried about.  Maybe not worth worrying
about.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] MI error messages

[Nick Roberts]

 > We have a syntax for error messages... let's use it?

Are you referring to the return value MI_CMD_ERROR?  Interestingly this isn't
used at all in mi-cmd-var.c or mi-cmd-stack.c but I think it is more
appropriate for the errors generated there.  It only goes back to
captured_mi_execute_command and doesn't generate any output in the log stream
(&"...").  I'm not too keen on procedures with lots of return points though.

Nick

Re: [PATCH] MI error messages

[Daniel Jacobowitz]

On Wed, Jun 22, 2005 at 07:28:17PM +1200, Nick Roberts wrote:
>  > We have a syntax for error messages... let's use it?
> 
> Are you referring to the return value MI_CMD_ERROR?  Interestingly this isn't
> used at all in mi-cmd-var.c or mi-cmd-stack.c but I think it is more
> appropriate for the errors generated there.  It only goes back to
> captured_mi_execute_command and doesn't generate any output in the log stream
> (&"...").  I'm not too keen on procedures with lots of return points though.

I'm referring to the syntax, which is ^error.  If returning
MI_CMD_ERROR works, then that way is fine.

I don't get your objection.  error() is a return point anyway.

-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] MI error messages

[Bob Rossi]

On Tue, Jun 21, 2005 at 02:59:11PM -0700, Jason Molenda wrote:
> 
> On Jun 21, 2005, at 2:44 PM, Nick Roberts wrote:
> 
> >I presumed mi_usage_error would call error.  That way the error is  
> >caught and
> >any cleanups and rewinds are done.  However, you are right, it  
> >would be nicer
> >just to get:
> >
> >(gdb)
> >-stack-select-frame
> >Usage: FRAME_SPEC.
> >(gdb)
> >
> >instead of
> >
> >(gdb)
> >-stack-select-frame
> >&"Usage: -stack-select-frame FRAME_SPEC\n"
> >^error,msg="Usage: -stack-select-frame FRAME_SPEC"
> >(gdb)
> >
> >as these errors aren't intended for the user when the frontend is  
> >being used.
> 
> 
> Yeah, we came to the same decision at Apple.  When you throw error()  
> while in MI mode, you only get the ^error message, you don't get the  
> console-style &"..." with the same message.  Our GUI has a little  
> status line at the bottom of the window where it shows the error  
> message, and if you have the "gdb console" window open, it shows the  
> error text in a different color to differentiate the error message.   
> It makes sense to let the GUI show the error in whatever way is most  
> appropriate for it, instead of blotting back plain old text.

It would probably even make more sense if it the error message said
something like,
   ^error,msg="Usage: -stack-select-frame FRAME_SPEC",reason=USAGE_NOTE
That way, there is text associated with the error message, but the FE
can also easily understand what type of error it is receiving, and
perform different operations on that data. I can only guess that someone
is doing a strcmp on that Usage message ...

Bob Rossi

Re: [PATCH] MI error messages

[Bob Rossi]

On Wed, Jun 22, 2005 at 09:44:17AM +1200, Nick Roberts wrote:
> Eli Zaretskii writes:
>  > > Which file should mi_error/mi_usage_error go in?  mi-cmds.c seems the best
>  > > option to me as the mi-cmd-*.c files include mi-cmds.h.
>  > 
>  > I don't care much, but isn't mi-common.c a better place?
> 
> mi-common.c seems a bit of a misnomer.  Perhap common here means frequent
> rather than shared.  mi-common.h isn't included in any of the c files apart
> from mi-common.c

Well, I just added mi-common. It was intended to be an interface for the
rest of GDB to be able to get constant string data or MI specific
enumeration values from the MI code. 

It could also be used for common data for the MI internals itself, but I
would probably advice against that. It might be nice to add another file
that allows the internals of the MI code to share data and such.

Bob Rossi

Re: [PATCH] MI error messages

[Nick Roberts]

Nick Roberts writes:
 > I'll look at submitting a revised patch for mi-cmd-stack.c and mi-cmd-var.c

I've defined two functions:

enum mi_cmd_result
mi_error (char* message)
{
  mi_error_message = xstrprintf ("%s", message);
  return MI_CMD_ERROR;
}

enum mi_cmd_result
mi_usage_error (char* message)
{
  mi_error_message = xstrprintf (_("Usage: %s"), message);
  return MI_CMD_ERROR;
}

and used them in mi-cmd-var.c as shown below.

WDYT?

Nick

*** mi/mi-cmd-var.c.~1.21.~	2005-06-21 09:02:37.000000000 +1200
--- mi/mi-cmd-var.c	2005-06-22 22:56:03.000000000 +1200
***************
*** 52,58 ****
      {
        /* mi_error_message = xstrprintf ("mi_cmd_var_create: Usage:
           ...."); return MI_CMD_ERROR; */
!       error (_("mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."));
      }
  
    name = xstrdup (argv[0]);
--- 52,58 ----
      {
        /* mi_error_message = xstrprintf ("mi_cmd_var_create: Usage:
           ...."); return MI_CMD_ERROR; */
!       return mi_usage_error ("-var-create NAME FRAME EXPRESSION.");
      }
  
    name = xstrdup (argv[0]);
***************
*** 71,77 ****
        name = varobj_gen_name ();
      }
    else if (!isalpha (*name))
!     error (_("mi_cmd_var_create: name of object must begin with a letter"));
  
    if (strcmp (frame, "*") == 0)
      var_type = USE_CURRENT_FRAME;
--- 71,77 ----
        name = varobj_gen_name ();
      }
    else if (!isalpha (*name))
!     return mi_error (_("Name of object must begin with a letter."));
  
    if (strcmp (frame, "*") == 0)
      var_type = USE_CURRENT_FRAME;
***************
*** 91,98 ****
    var = varobj_create (name, expr, frameaddr, var_type);
  
    if (var == NULL)
!     error (_("mi_cmd_var_create: unable to create variable object"));
! 
    ui_out_field_string (uiout, "name", name);
    ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
    type = varobj_get_type (var);
--- 91,101 ----
    var = varobj_create (name, expr, frameaddr, var_type);
  
    if (var == NULL)
!     {
!       mi_error_message
! 	= xstrprintf (_("No symbol %s in current context."), expr);
!       return MI_CMD_ERROR;
!     }
    ui_out_field_string (uiout, "name", name);
    ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
    type = varobj_get_type (var);
***************
*** 119,125 ****
    struct cleanup *old_cleanups;
  
    if (argc < 1 || argc > 2)
!     error (_("mi_cmd_var_delete: Usage: [-c] EXPRESSION."));
  
    name = xstrdup (argv[0]);
    /* Add cleanup for name. Must be free_current_contents as
--- 122,128 ----
    struct cleanup *old_cleanups;
  
    if (argc < 1 || argc > 2)
!     return mi_usage_error ("-var-delete [-c] EXPRESSION.");
  
    name = xstrdup (argv[0]);
    /* Add cleanup for name. Must be free_current_contents as
***************
*** 131,139 ****
    if (argc == 1)
      {
        if (strcmp (name, "-c") == 0)
! 	error (_("mi_cmd_var_delete: Missing required argument after '-c': variable object name"));
        if (*name == '-')
! 	error (_("mi_cmd_var_delete: Illegal variable object name"));
      }
  
    /* If we have 2 arguments they must be '-c' followed by a string
--- 134,142 ----
    if (argc == 1)
      {
        if (strcmp (name, "-c") == 0)
! 	return mi_error (_("Missing required argument after '-c': variable object name."));
        if (*name == '-')
! 	return mi_error (_("Illegal variable object name."));
      }
  
    /* If we have 2 arguments they must be '-c' followed by a string
***************
*** 142,148 ****
      {
        expr = xstrdup (argv[1]);
        if (strcmp (name, "-c") != 0)
! 	error (_("mi_cmd_var_delete: Invalid option."));
        children_only_p = 1;
        xfree (name);
        name = xstrdup (expr);
--- 145,151 ----
      {
        expr = xstrdup (argv[1]);
        if (strcmp (name, "-c") != 0)
! 	return mi_error (_("Invalid option."));
        children_only_p = 1;
        xfree (name);
        name = xstrdup (expr);
***************
*** 155,161 ****
    var = varobj_get_handle (name);
  
    if (var == NULL)
!     error (_("mi_cmd_var_delete: Variable object not found."));
  
    numdel = varobj_delete (var, NULL, children_only_p);
  
--- 158,164 ----
    var = varobj_get_handle (name);
  
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    numdel = varobj_delete (var, NULL, children_only_p);
  
***************
*** 174,190 ****
    char *formspec;
  
    if (argc != 2)
!     error (_("mi_cmd_var_set_format: Usage: NAME FORMAT."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
  
    if (var == NULL)
!     error (_("mi_cmd_var_set_format: Variable object not found"));
  
    formspec = xstrdup (argv[1]);
    if (formspec == NULL)
!     error (_("mi_cmd_var_set_format: Must specify the format as: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
  
    len = strlen (formspec);
  
--- 177,193 ----
    char *formspec;
  
    if (argc != 2)
!     return mi_usage_error ("-var-set-format NAME FORMAT.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
  
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    formspec = xstrdup (argv[1]);
    if (formspec == NULL)
!     return mi_error (_("Must specify the format as: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\"."));
  
    len = strlen (formspec);
  
***************
*** 199,205 ****
    else if (strncmp (formspec, "octal", len) == 0)
      format = FORMAT_OCTAL;
    else
!     error (_("mi_cmd_var_set_format: Unknown display format: must be: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\""));
  
    /* Set the format of VAR to given format */
    varobj_set_display_format (var, format);
--- 202,208 ----
    else if (strncmp (formspec, "octal", len) == 0)
      format = FORMAT_OCTAL;
    else
!     return mi_error (_("Unknown display format: must be: \"natural\", \"binary\", \"decimal\", \"hexadecimal\", or \"octal\"."));
  
    /* Set the format of VAR to given format */
    varobj_set_display_format (var, format);
***************
*** 216,227 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_show_format: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_show_format: Variable object not found"));
  
    format = varobj_get_display_format (var);
  
--- 219,230 ----
    struct varobj *var;
  
    if (argc != 1)
!     return mi_usage_error ("-var-show-format NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    format = varobj_get_display_format (var);
  
***************
*** 236,247 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_info_num_children: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_info_num_children: Variable object not found"));
  
    ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
    return MI_CMD_DONE;
--- 239,250 ----
    struct varobj *var;
  
    if (argc != 1)
!     return mi_usage_error ("-var-info-num-children NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found"));
  
    ui_out_field_int (uiout, "numchild", varobj_get_num_children (var));
    return MI_CMD_DONE;
***************
*** 259,271 ****
    enum print_values print_values;
  
    if (argc != 1 && argc != 2)
!     error (_("mi_cmd_var_list_children: Usage: [PRINT_VALUES] NAME"));
  
    /* Get varobj handle, if a valid var obj name was specified */
    if (argc == 1) var = varobj_get_handle (argv[0]);
    else var = varobj_get_handle (argv[1]);
    if (var == NULL)
!     error (_("Variable object not found"));
  
    numchild = varobj_list_children (var, &childlist);
    ui_out_field_int (uiout, "numchild", numchild);
--- 262,274 ----
    enum print_values print_values;
  
    if (argc != 1 && argc != 2)
!     return mi_usage_error ("-var-list-children [PRINT_VALUES] NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    if (argc == 1) var = varobj_get_handle (argv[0]);
    else var = varobj_get_handle (argv[1]);
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    numchild = varobj_list_children (var, &childlist);
    ui_out_field_int (uiout, "numchild", numchild);
***************
*** 277,283 ****
  	     || strcmp (argv[0], "--all-values") == 0)
        print_values = PRINT_ALL_VALUES;
      else
!      error (_("Unknown value for PRINT_VALUES: must be: 0 or \"--no-values\", 1 or \"--all-values\""));
    else print_values = PRINT_NO_VALUES;
  
    if (numchild <= 0)
--- 280,286 ----
  	     || strcmp (argv[0], "--all-values") == 0)
        print_values = PRINT_ALL_VALUES;
      else
!      return mi_error (_("Unknown value for PRINT_VALUES: must be: 0 or \"--no-values\", 1 or \"--all-values\"."));
    else print_values = PRINT_NO_VALUES;
  
    if (numchild <= 0)
***************
*** 315,326 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_info_type: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_info_type: Variable object not found"));
  
    ui_out_field_string (uiout, "type", varobj_get_type (var));
    return MI_CMD_DONE;
--- 318,329 ----
    struct varobj *var;
  
    if (argc != 1)
!     return mi_usage_error ("-var-info-type NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    ui_out_field_string (uiout, "type", varobj_get_type (var));
    return MI_CMD_DONE;
***************
*** 333,344 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_info_expression: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_info_expression: Variable object not found"));
  
    lang = varobj_get_language (var);
  
--- 336,347 ----
    struct varobj *var;
  
    if (argc != 1)
!     return mi_usage_error ("-var-info-expression NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    lang = varobj_get_language (var);
  
***************
*** 355,366 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_show_attributes: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_show_attributes: Variable object not found"));
  
    attr = varobj_get_attributes (var);
    /* FIXME: define masks for attributes */
--- 358,369 ----
    struct varobj *var;
  
    if (argc != 1)
!     return mi_usage_error ("-var-show-attributes NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    attr = varobj_get_attributes (var);
    /* FIXME: define masks for attributes */
***************
*** 379,390 ****
    struct varobj *var;
  
    if (argc != 1)
!     error (_("mi_cmd_var_evaluate_expression: Usage: NAME."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_evaluate_expression: Variable object not found"));
  
    ui_out_field_string (uiout, "value", varobj_get_value (var));
    return MI_CMD_DONE;
--- 382,393 ----
    struct varobj *var;
  
    if (argc != 1)
!     return mi_usage_error ("-var-evaluate-expression NAME.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found."));
  
    ui_out_field_string (uiout, "value", varobj_get_value (var));
    return MI_CMD_DONE;
***************
*** 397,417 ****
    char *expression;
  
    if (argc != 2)
!     error (_("mi_cmd_var_assign: Usage: NAME EXPRESSION."));
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     error (_("mi_cmd_var_assign: Variable object not found"));
  
    /* FIXME: define masks for attributes */
    if (!(varobj_get_attributes (var) & 0x00000001))
!     error (_("mi_cmd_var_assign: Variable object is not editable"));
  
    expression = xstrdup (argv[1]);
  
    if (!varobj_set_value (var, expression))
!     error (_("mi_cmd_var_assign: Could not assign expression to varible object"));
  
    ui_out_field_string (uiout, "value", varobj_get_value (var));
    return MI_CMD_DONE;
--- 400,420 ----
    char *expression;
  
    if (argc != 2)
!     return mi_usage_error ("-var-assign NAME EXPRESSION.");
  
    /* Get varobj handle, if a valid var obj name was specified */
    var = varobj_get_handle (argv[0]);
    if (var == NULL)
!     return mi_error (_("Variable object not found"));
  
    /* FIXME: define masks for attributes */
    if (!(varobj_get_attributes (var) & 0x00000001))
!     return mi_error (_("Variable object is not editable."));
  
    expression = xstrdup (argv[1]);
  
    if (!varobj_set_value (var, expression))
!     return mi_error (_("Could not assign expression to variable object."));
  
    ui_out_field_string (uiout, "value", varobj_get_value (var));
    return MI_CMD_DONE;
***************
*** 428,434 ****
    int nv;
  
    if (argc != 1)
!     error (_("mi_cmd_var_update: Usage: NAME."));
  
    name = argv[0];
  
--- 431,437 ----
    int nv;
  
    if (argc != 1)
!     return mi_usage_error ("-var-update NAME.");
  
    name = argv[0];
  
***************
*** 461,467 ****
        /* Get varobj handle, if a valid var obj name was specified */
        var = varobj_get_handle (name);
        if (var == NULL)
! 	error (_("mi_cmd_var_update: Variable object not found"));
  
        if (mi_version (uiout) <= 1)
          cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");
--- 464,470 ----
        /* Get varobj handle, if a valid var obj name was specified */
        var = varobj_get_handle (name);
        if (var == NULL)
! 	return mi_error (_("Variable object not found."));
  
        if (mi_version (uiout) <= 1)
          cleanup = make_cleanup_ui_out_tuple_begin_end (uiout, "changelist");

Re: [PATCH] MI error messages

[Andreas Schwab]

Nick Roberts <nickrob@snap.net.nz> writes:

> Nick Roberts writes:
>  > I'll look at submitting a revised patch for mi-cmd-stack.c and mi-cmd-var.c
>
> I've defined two functions:
>
> enum mi_cmd_result
> mi_error (char* message)
> {
>   mi_error_message = xstrprintf ("%s", message);

xstrdup?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] MI error messages

[Jason Molenda]

Hi Nick,

This patch is changing two things.  You're not using error () when  
there is an error, and you're changing the error message texsts.

Why are you avoiding error()?  It's use throughout gdb is  
established; there are people who like this style of error handling  
and people who don't, but consistency is valuable and we're using  
error().  I'm not clear on why you're patching it out of mi-cmd- 
var.c.  Is your goal to have the error message output as just ^error  
w/o the console-quoted error text?  Using your mi_error will result  
in some error messages (i.e. from the rest of gdb) going through error 
() and some other error messages (from the mi/ sub directory) using  
mi_error(), so the front end will have both.

Second, the error messages are being changed from e.g.

"mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."

to

"-var-create NAME FRAME EXPRESSION."

Why is this better?  Neither one means anything to an end user --  
they're just strings for programmers to grep for in the gdb sources,  
and to disambiguate the same error message from different mi  
commands.  Some of the error message changes are good improvements  
(e.g. adding the symbol that was being var-create'd in  
"mi_cmd_var_create: unable to create variable object." but you  
dropped the mi command name, either function or MI command, entirely  
in that case), but there are a lot of changes in there.

(I didn't mention the third change, which is to flag mi command usage  
error separately, but you can indicate that in the error text already  
so I didn't see the compelling reason for that change either...)

We changed error () here at Apple so it is only printed as an MI  
^error msg; our error_stream() reads


   /* Copy the stream into the GDB_LASTERR buffer.  */
   ui_file_rewind (gdb_lasterr);
   ui_file_put (stream, do_write, gdb_lasterr);

   /* Write the message plus any error_pre_print to gdb_stderr.
      Don't do it for mi like interpreters, however, since
      the will get the error from ui_file_rewind. */
   if (!ui_out_is_mi_like_p (uiout))
     {
       target_terminal_ours ();
       wrap_here ("");           /* Force out any buffered output */
       gdb_flush (gdb_stdout);
       annotate_error_begin ();
       if (error_pre_print)
         fputs_filtered (error_pre_print, gdb_stderr);
       ui_file_put (stream, do_write, gdb_stderr);
       fprintf_filtered (gdb_stderr, "\n");
     }

Jason

PS-  If you add "-p" to your $HOME/.cvsrc, e.g.
diff -up
it will make the diffs a tiny bit easier to read.  -p makes diff try  
to figure out the function name and include it in the diff so people  
can easily tell which function the change is against.

Re: [PATCH] MI error messages

[Nick Roberts]

Hi Jason,

 > This patch is changing two things.  You're not using error () when  
 > there is an error, and you're changing the error message texsts.
 > 
 > Why are you avoiding error()?  

I see two, maybe three kinds of error:

1) MI developer
2) Frontend developer
3) User

Type 3 should only be generated, if a debug flag is set perhaps.

Type 2 errors are like:

(gdb)
-var-create
&"mi_cmd_var_create: Usage: NAME FRAME EXPRESSION.\n"
^error,msg="mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."
(gdb) 

The user won't type in -var-create directly, so this error can only be
generated by th frontend and doesn't need log stream output.  Since it
is specific to MI, I don't see why error () need be called.

Type 3 errors are like:

(gdb) 
-stack-select-frame 0
&"mi_cmd_stack_select_frame: No stack.\n"
^error,msg="mi_cmd_stack_select_frame: No stack."
(gdb) 

which in CVS is now reported by:

(gdb) 
-stack-select-frame 0
&"No registers.\n"
^error,msg="No registers."
(gdb) 

It is in the nature of GDB that these errors call error () (through
get_selected_frame).  Actually I would rather this particular situatin was
treated normal, I don't see why "No stack." should flash up as an error
on the status bar if the user opens a window for registers, for example
before execution has started.

 > It's use throughout gdb is established; there are people who like this
 > style of error handling and people who don't, but consistency is valuable
 > and we're using error().  I'm not clear on why you're patching it out of
 > mi-cmd- var.c.

I'm just using mi-cmd- var.c as an example.

 > Is your goal to have the error message output as just ^error w/o the
 > console-quoted error text?

Thats one benefit.  

 > Using your mi_error will result in some error messages (i.e. from the rest
 > of gdb) going through error () and some other error messages (from the mi/
 > sub directory) using mi_error(), so the front end will have both.

When do you see it as appropriate to use MI_CMD_ERROR?

 > Second, the error messages are being changed from e.g.
 > 
 > "mi_cmd_var_create: Usage: NAME FRAME EXPRESSION."
 > 
 > to
 > 
 > "-var-create NAME FRAME EXPRESSION."
 > 
 > Why is this better?

If someone is writing a frontend he might not know anything about the
GDB source code.  If the frontend sends -var-create to GDB without an
argument, he presumably wants to know which MI command generated that
error, not where in the source code it occurred.

 > Neither one means anything to an end user --  

Not to an end user, no, but to a frontend developer.

 > they're just strings for programmers to grep for in the gdb sources,  
 > and to disambiguate the same error message from different mi  
 > commands.

I disagree.

 > Some of the error message changes are good improvements (e.g. adding the
 > symbol that was being var-create'd in "mi_cmd_var_create: unable to create
 > variable object." but you dropped the mi command name, either function or
 > MI command, entirely in that case), but there are a lot of changes in
 > there.

Its for discussion.  I'm not clear what I need to do for Emacs, so I'm making
it up as I go along.  In the three years, I've contributed to Emacs, there
hasn't been a release, so I've not had to deal with compatibility with
existing code before.  Which changes will present problems for you?  In this
situation clearly it doesn't make sense to commit anything until its clear
exactly how it will be used.

 > (I didn't mention the third change, which is to flag mi command usage  
 > error separately, but you can indicate that in the error text already  
 > so I didn't see the compelling reason for that change either...)
 > 
 > We changed error () here at Apple so it is only printed as an MI  
 > ^error msg; our error_stream() reads
 > 
 > 
 >    /* Copy the stream into the GDB_LASTERR buffer.  */
 >    ui_file_rewind (gdb_lasterr);
 >    ui_file_put (stream, do_write, gdb_lasterr);
 > 
 >    /* Write the message plus any error_pre_print to gdb_stderr.
 >       Don't do it for mi like interpreters, however, since
 >       the will get the error from ui_file_rewind. */
 >    if (!ui_out_is_mi_like_p (uiout))
 >      {
 >        target_terminal_ours ();
 >        wrap_here ("");           /* Force out any buffered output */
 >        gdb_flush (gdb_stdout);
 >        annotate_error_begin ();
 >        if (error_pre_print)
 >          fputs_filtered (error_pre_print, gdb_stderr);
 >        ui_file_put (stream, do_write, gdb_stderr);
 >        fprintf_filtered (gdb_stderr, "\n");
 >      }

I thought the line:

      exception_print (gdb_stderr, result);

in  mi_execute_command printed this.  For user errors, isn't the message
in the log stream helpful?  I see now that its not always the same as
the MI error:

-var-create - * value
&"No symbol \"value\" in current context.\n"
&"mi_cmd_var_create: unable to create variable object\n"
^error,msg="mi_cmd_var_create: unable to create variable object"
(gdb) 

 > PS-  If you add "-p" to your $HOME/.cvsrc, e.g.
 > diff -up
 > it will make the diffs a tiny bit easier to read.  -p makes diff try  
 > to figure out the function name and include it in the diff so people  
 > can easily tell which function the change is against.

OK, thanks.

Nick

Re: [PATCH] -stack-info-frames

[Jason Molenda]

On Sun, Jun 19, 2005 at 10:49:29AM +1200, Nick Roberts wrote:

>  > (gdb)
>  > -interpreter-exec console up
>  > ~"#1  0xb7d621ae in poll () from /lib/tls/i686/cmov/libc.so.6\n"
>  > ^done
>  > 


> Yes.  The same is true for the commands that control execution:
> 

> (gdb) 
> -interpreter-exec console next
> ~"50\t  int n1=7, n2=8, n3=9;\n"
> ^done
> (gdb) 
> 
> doesn't give the frontend any information.



For what it's worth, here's what these look like with the Apple gdb:

86-interpreter-exec console-quoted "up"
(gdb) 
~"#1  0x0003eaf0 in -[SKTGraphicView createGraphicOfClass:withEvent:] (self=0x341380, _cmd=0x3411c0, theClass=0xb2014, theEvent=0x3afc90) at /Developer/Examples/AppKit/Sketch/SKTGraphicView.m:345\n"
~"345\t    _creatingGraphic = [[theClass allocWithZone:[document zone]] init];\n"
86^done,MI_HOOK_RESULT=[HOOK_TYPE="frame_changed",frame="1"]



In the case of a 'next',

78-interpreter-exec console-quoted "next"
(gdb) 
^stepping
~"Current language:  auto; currently objective-c\n"
78^running
78*stopped,reason="end-stepping-range",thread-id="1"


These, and a handful of other notifications, are essential
if you have a GUI with a "gdb console" and you want to allow
the user to change program state in the console.


You'll notice that we use "console-quoted" for these.  We keep
the meaning of "console" to actually mean console and have a
separate interpreter for the MI-quoted console output.  We also
have the increidbly useful ability to switch the current
interpreter on the fly.  e.g.

(gdb) b main
Breakpoint 1 at 0x2d04: file a.c, line 3.
(gdb) r
Starting program: /private/tmp/a.out 
Reading symbols for shared libraries . done

Breakpoint 1, main () at a.c:3
3         puts ("");
(gdb) set interpreter  mi
-stack-select-frame 0
^done
(gdb) 
set interpreter console
&"set interpreter console\n"
Switching to interpreter "console".
^done
(gdb) 
fr 0
#0  main () at a.c:3
3         puts ("");
(gdb) 

So when we're working on an MI problem, we can navigate to the
area of interest via console, then switch to an MI interpreter
to work the problem.

>  > Meanwhile, we don't have this ability.  So maybe we do need
>  > -stack-info-frame, without an argument.
> 
> Yes, unless Apple's proposed merge will provide the necessary information.

Yeah, I would agree with Daniel that you shouldn't wait.  We're working
on the latest FSF -> Apple merge right now.  That will take a while
to get all the bugs shaken out, but even when it's finished we'll have
accomplished nothing for the desirable Apple -> FSF contributions --
it just sets the stage to make those easier to do.

J

Re: [PATCH] -stack-info-frames

[Daniel Jacobowitz]

On Sun, Jun 19, 2005 at 02:55:05PM -0700, Jason Molenda wrote:
> 86^done,MI_HOOK_RESULT=[HOOK_TYPE="frame_changed",frame="1"]

Yes, I am absolutely positive that we're going to need something like
this, though I'm not sure the model will be quite the same.

On and off I have been working on a better interface for binding
scripting languages to GDB.  I have guile mostly working - the only
hassle is that I don't know squat about actually programming in guile,
so I can't write any of the script-side support code that it needs.  It
should be a cinch to add other languages, e.g. Python or to merge Kip's
Perl bindings.

But the way my interface works, it talks to GDB over an MI interpreter.
We need to find some way to keep an MI frontend informed of what
happens while "its" interpreter is not connected.  What I'm thinking is
that we should be able to have a primary interpreter and multiple
less-primary ones; only one accepts input at a time; most commands only
respond to the console they got input from; and certain notifications,
sort of like annotations in principle, go to any open channels.


-- 
Daniel Jacobowitz
CodeSourcery, LLC

Re: [PATCH] -stack-info-frames

[Jason Molenda]

On Jun 17, 2005, at 3:52 PM, Nick Roberts wrote:

>
> Here's a patch for -stack-info-frames.  Having just read Jason's e- 
> mail
> about how quick "-stack-list-frames 0 0" is, I'm not sure its  
> necessary.


NB: I don't have any objection to -stack-info-frame; some other GUI  
designer may find it a convenient command.  And it's hardly a burden  
code-wise in gdb to support the command.  It might be nice if there  
was a simple test case added for it at the same time, but that's my  
only comment.

J

Re: [PATCH] -stack-info-frames

[Nick Roberts]

 > NB: I don't have any objection to -stack-info-frame; some other GUI  
 > designer may find it a convenient command.  And it's hardly a burden  
 > code-wise in gdb to support the command.  It might be nice if there  
 > was a simple test case added for it at the same time, but that's my  
 > only comment.

Sure.  Perhaps adding the test could be a condition to approving the patch.
I'm finding my way a bit, and I don't want to have write more tests than I
need to.

Nick