[PATCH] Re: bug in mi when setting breakpoint

[PATCH] Re: bug in mi when setting breakpoint

[Nick Roberts]

[continued from gdb mailing list]

 > How about doing something similar in FSF GDB?

Like below?

Apple also have a MI command called -break-commands which I want to add to FSF
GDB to avoid similar prompting problems in MI with "commands".

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


2008-01-17  Nick Roberts  <nickrob@snap.net.nz>

	    Based on work by Apple Computer, Inc.

	* linespec.c: Include mi/mi-cmds.h.
	(decode_objc, find_method): When using MI, set accept_all to 1.
	(decode_line_2): Add argument accept_all.  Accept all breakpoints
	when non-zero.

	* mi/mi-interp.c (mi_underlying): New variable
	(mi_insert_notify_hooks, mi_remove_notify_hooks): Use it.

	* mi/mi-cmds.h (mi_underlying): New extern.


*** mi-cmds.h.~1.27.~	2008-01-04 10:24:35.000000000 +1300
--- mi-cmds.h	2008-01-15 17:25:11.000000000 +1300
*************** extern struct ui_file *raw_stdout;
*** 158,161 ****
--- 158,164 ----
  extern char *mi_error_message;
  extern void mi_execute_command (char *cmd, int from_tty);
  
+ /* From mi-interp.c */
+ extern int mi_underlying;
+ 
  #endif



*** mi-interp.c.~1.23.~	2008-01-04 10:24:35.000000000 +1300
--- mi-interp.c	2008-01-17 11:12:57.000000000 +1300
*************** struct mi_interp
*** 47,52 ****
--- 47,55 ----
    struct interp *mi_interp;
  };
  
+ /* Set to 1 to make CLI commands behave as MI would like.  */
+ int mi_underlying = 0;
+ 
  /* These are the interpreter setup, etc. functions for the MI interpreter */
  static void mi_execute_command_wrapper (char *cmd);
  static void mi_command_loop (int mi_version);
*************** static void
*** 264,275 ****
--- 267,280 ----
  mi_insert_notify_hooks (void)
  {
    deprecated_query_hook = mi_interp_query_hook;
+   mi_underlying = 1;
  }
  
  static void
  mi_remove_notify_hooks (void)
  {
    deprecated_query_hook = NULL;
+   mi_underlying = 0;
  }
  
  static int

*** linespec.c.~1.74.~	2008-01-04 10:24:14.000000000 +1300
--- linespec.c	2008-01-15 17:42:11.000000000 +1300
***************
*** 36,41 ****
--- 36,42 ----
  #include "linespec.h"
  #include "exceptions.h"
  #include "language.h"
+ #include "mi/mi-cmds.h"
  
  /* We share this one with symtab.c, but it is not exported widely. */
  
*************** static char *find_toplevel_char (char *s
*** 102,108 ****
  static int is_objc_method_format (const char *s);
  
  static struct symtabs_and_lines decode_line_2 (struct symbol *[],
! 					       int, int, char ***);
  
  static struct symtab *symtab_from_filename (char **argptr,
  					    char *p, int is_quote_enclosed,
--- 103,109 ----
  static int is_objc_method_format (const char *s);
  
  static struct symtabs_and_lines decode_line_2 (struct symbol *[],
! 					       int, int, int, char ***);
  
  static struct symtab *symtab_from_filename (char **argptr,
  					    char *p, int is_quote_enclosed,
*************** is_objc_method_format (const char *s)
*** 482,487 ****
--- 483,489 ----
  
  static struct symtabs_and_lines
  decode_line_2 (struct symbol *sym_arr[], int nelts, int funfirstline,
+ 	       int accept_all,
  	       char ***canonical)
  {
    struct symtabs_and_lines values, return_values;
*************** decode_line_2 (struct symbol *sym_arr[],
*** 507,512 ****
--- 509,515 ----
      }
  
    i = 0;
+   if (!accept_all)
    printf_unfiltered (_("[0] cancel\n[1] all\n"));
    while (i < nelts)
      {
*************** decode_line_2 (struct symbol *sym_arr[],
*** 515,547 ****
        if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
  	{
  	  values.sals[i] = find_function_start_sal (sym_arr[i], funfirstline);
! 	  if (values.sals[i].symtab)
! 	    printf_unfiltered ("[%d] %s at %s:%d\n",
! 			       (i + 2),
! 			       SYMBOL_PRINT_NAME (sym_arr[i]),
! 			       values.sals[i].symtab->filename,
! 			       values.sals[i].line);
! 	  else
! 	    printf_unfiltered (_("[%d] %s at ?FILE:%d [No symtab? Probably broken debug info...]\n"),
! 			       (i + 2),
! 			       SYMBOL_PRINT_NAME (sym_arr[i]),
! 			       values.sals[i].line);
! 
  	}
        else
  	printf_unfiltered (_("?HERE\n"));
        i++;
      }
  
!   prompt = getenv ("PS2");
!   if (prompt == NULL)
      {
!       prompt = "> ";
      }
!   args = command_line_input (prompt, 0, "overload-choice");
! 
!   if (args == 0 || *args == 0)
!     error_no_arg (_("one or more choice numbers"));
  
    i = 0;
    while (*args)
--- 518,565 ----
        if (sym_arr[i] && SYMBOL_CLASS (sym_arr[i]) == LOC_BLOCK)
  	{
  	  values.sals[i] = find_function_start_sal (sym_arr[i], funfirstline);
! 	  if (!accept_all)
! 	    {
! 	      if (values.sals[i].symtab)
! 		printf_unfiltered ("[%d] %s at %s:%d\n",
! 				   (i + 2),
! 				   SYMBOL_PRINT_NAME (sym_arr[i]),
! 				   values.sals[i].symtab->filename,
! 				   values.sals[i].line);
! 	      else
! 		printf_unfiltered ("[%d] %s at ?FILE:%d [No symtab? Probably broken debug info...]\n",
! 				   (i + 2),
! 				   SYMBOL_PRINT_NAME (sym_arr[i]),
! 				   values.sals[i].line);
! 	    }
! 	}
!       else if (!accept_all)
! 	{
! 	  /* APPLE LOCAL: We can do a little better than just printing ?HERE?...  */
!           printf_filtered ("[%d]    %s\n",
!                            (i + 2),
!                            (sym_arr[i] && SYMBOL_PRINT_NAME (sym_arr[i])) ?
!                            SYMBOL_PRINT_NAME (sym_arr[i]) : "?HERE?");
  	}
        else
  	printf_unfiltered (_("?HERE\n"));
        i++;
      }
  
!   if (!accept_all)
      {
!       prompt = getenv ("PS2");
!       if (prompt == NULL)
! 	{
! 	  prompt = "> ";
! 	}
!       args = command_line_input (prompt, 0, "overload-choice");
!       
!       if (args == 0 || *args == 0)
! 	error_no_arg ("one or more choice numbers");
      }
!   else
!     args = "1";
  
    i = 0;
    while (*args)
*************** decode_objc (char **argptr, int funfirst
*** 1154,1161 ****
  
    if (i1 > 1)
      {
        /* More than one match. The user must choose one or more.  */
!       return decode_line_2 (sym_arr, i2, funfirstline, canonical);
      }
  
    return values;
--- 1172,1184 ----
  
    if (i1 > 1)
      {
+       int accept_all;
+       if (mi_underlying == 1 || ui_out_is_mi_like_p (uiout))
+ 	accept_all = 1;
+       else
+ 	accept_all = 0;
        /* More than one match. The user must choose one or more.  */
!       return decode_line_2 (sym_arr, i2, funfirstline, accept_all, canonical);
      }
  
    return values;
*************** find_method (int funfirstline, char ***c
*** 1442,1450 ****
      }
    if (i1 > 0)
      {
        /* There is more than one field with that name
  	 (overloaded).  Ask the user which one to use.  */
!       return decode_line_2 (sym_arr, i1, funfirstline, canonical);
      }
    else
      {
--- 1465,1479 ----
      }
    if (i1 > 0)
      {
+       int accept_all;
+       if (mi_underlying == 1 || ui_out_is_mi_like_p (uiout))
+ 	accept_all = 1;
+       else
+ 	accept_all = 0;
+ 
        /* There is more than one field with that name
  	 (overloaded).  Ask the user which one to use.  */
!       return decode_line_2 (sym_arr, i1, funfirstline, accept_all, canonical);
      }
    else
      {

Re: [PATCH] Re: bug in mi when setting breakpoint

[Vladimir Prus]

Nick Roberts wrote:

> [continued from gdb mailing list]
> 
>  > How about doing something similar in FSF GDB?
> 
> Like below?
> 
> Apple also have a MI command called -break-commands which I want to add to
> FSF GDB to avoid similar prompting problems in MI with "commands".

This is different to what I've proposed -- namely reporting 
"there are several overloaded functions" and then allowing the frontend to select
a specific one. The solution you propose does not give a frontend a way to control
which overloaded function to set breakpoint on, which seems like an important
limitation for me.

- Volodya

Re: [PATCH] Re: bug in mi when setting breakpoint

[Nick Roberts]

 > This is different to what I've proposed -- namely reporting "there are
 > several overloaded functions" and then allowing the frontend to select a
 > specific one. 

I must have missed that.  Can you please point to a reference?

 >               The solution you propose does not give a frontend a way to
 > control which overloaded function to set breakpoint on, which seems like an
 > important limitation for me.

After selecting all breakpoints GDB prints:

  warning: Multiple breakpoints were set.
  Use the "delete" command to delete unwanted breakpoints.

I can see it may be an inconvenience but not how it can be a limitation.
IMHO, it seems preferable to generating prompts that are not compatible with
rules for MI output.

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

Re: [PATCH] Re: bug in mi when setting breakpoint

[Vladimir Prus]

Nick Roberts wrote:

>  > This is different to what I've proposed -- namely reporting "there are
>  > several overloaded functions" and then allowing the frontend to select
>  > a specific one.
> 
> I must have missed that.  Can you please point to a reference?

I can't immediately find that; the point was that -break-insert would
list all possible function on which breakpoint can be set, without setting
any. Then frontend will then specifically set breakpoints on the function
it wants.

> 
>  >               The solution you propose does not give a frontend a way
>  >               to
>  > control which overloaded function to set breakpoint on, which seems
>  > like an important limitation for me.
> 
> After selecting all breakpoints GDB prints:
> 
>   warning: Multiple breakpoints were set.
>   Use the "delete" command to delete unwanted breakpoints.
> 
> I can see it may be an inconvenience but not how it can be a limitation.
> IMHO, it seems preferable to generating prompts that are not compatible
> with rules for MI output.

To clarify -- are you suggesting what we should first create breakpoints
for all overloaded function, and then remove those we don't need, in MI?

- Volodya

Re: [PATCH] Re: bug in mi when setting breakpoint

[Nick Roberts]

 > To clarify -- are you suggesting what we should first create breakpoints
 > for all overloaded function, and then remove those we don't need, in MI?

In Emacs, multiple breakpoints are only created when the user specifies the
overloaded function in the GUD buffer (if he clicks on the fringe in an
overloaded function in the source buffer he only gets one breakpoint at the
line on which he clicked).

If GDB sets all these breakpoints, unwanted ones could then be deleted manually
either by typing "delete BKPTNO" in the GUD buffer, or clicking on individual
breakpoint icons in the fringe of the source buffers.

(I think the answer to your question is "Yes.".)

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

Re: [PATCH] Re: bug in mi when setting breakpoint

[Vladimir Prus]

On Sunday 20 January 2008 00:45:06 Nick Roberts wrote:
>  > To clarify -- are you suggesting what we should first create breakpoints
>  > for all overloaded function, and then remove those we don't need, in MI?
> 
> In Emacs, multiple breakpoints are only created when the user specifies the
> overloaded function in the GUD buffer (if he clicks on the fringe in an
> overloaded function in the source buffer he only gets one breakpoint at the
> line on which he clicked).
> 
> If GDB sets all these breakpoints, unwanted ones could then be deleted manually
> either by typing "delete BKPTNO" in the GUD buffer, or clicking on individual
> breakpoint icons in the fringe of the source buffers.

I don't think such solution is entirely satisfactory, for the following reason:

1. UI should not require to type GDB command for such basic task as adding
breakpoints.
2. Overloaded functions can be scattered over several source file, so clicking
on fringe is highly inconvenient -- the files might not be even open.
3. Assuming we have a list of breakpoints (which Eclipse and KDevelop do), we
surely can delete breakpoints there. However, adding unwanted breakpoints right
away will not be good in non-stop mode:

  - Some threads might already stop on unwanted breakpoints before you delete them
  - You might run out of hardware resources while setting unwanted breakpoints

I think it's more clear to set only those breakpoints that user want to set,
as opposed to setting all of them, and then removing undesired ones.

- Volodya

Re: [PATCH] Re: bug in mi when setting breakpoint

[Nick Roberts]

 > > In Emacs, multiple breakpoints are only created when the user specifies
 > > the overloaded function in the GUD buffer (if he clicks on the fringe in
 > > an overloaded function in the source buffer he only gets one breakpoint at
 > > the line on which he clicked).
 > > 
 > > If GDB sets all these breakpoints, unwanted ones could then be deleted
 > > manually either by typing "delete BKPTNO" in the GUD buffer, or clicking
 > > on individual breakpoint icons in the fringe of the source buffers.
 > 
 > I don't think such solution is entirely satisfactory, for the following
 > reason:
 > 
 > 1. UI should not require to type GDB command for such basic task as adding
 > breakpoints.

The GUD buffer is generally additional to the graphical like features: for
those who like to use the command line and for more obscure features which have
no graphical counterpart.  Also, of course, for environments that only support
tarminal based applications.

 > 2. Overloaded functions can be scattered over several source file, so
 > clicking on fringe is highly inconvenient -- the files might not be even
 > open.  

For such cases there is also a breakpoints buffer from which all breakpoints
can be controlled.

 > 3. Assuming we have a list of breakpoints (which Eclipse and KDevelop
 > do), we surely can delete breakpoints there. However, adding unwanted
 > breakpoints right away will not be good in non-stop mode:
 >
 >   - Some threads might already stop on unwanted breakpoints before you
 >   delete them 
 >   - You might run out of hardware resources while setting unwanted
 >    breakpoints

Running GDB in "non-stop mode" sounds highly ambitious and may happen a long
time in the future, if at all.  I'm talking about implementing a simple
solution, for now.  Even if non-stop mode does become a reality, it presumably
won't be the default behaviour, so it won't break existing front ends.

 > I think it's more clear to set only those breakpoints that user want to set,
 > as opposed to setting all of them, and then removing undesired ones.

Yes, but I still haven't see how you propose to do this.

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

Re: [PATCH] Re: bug in mi when setting breakpoint

[Vladimir Prus]

On Sunday 20 January 2008 13:38:05 Nick Roberts wrote:

>  > 3. Assuming we have a list of breakpoints (which Eclipse and KDevelop
>  > do), we surely can delete breakpoints there. However, adding unwanted
>  > breakpoints right away will not be good in non-stop mode:
>  >
>  >   - Some threads might already stop on unwanted breakpoints before you
>  >   delete them 
>  >   - You might run out of hardware resources while setting unwanted
>  >    breakpoints
> 
> Running GDB in "non-stop mode" sounds highly ambitious and may happen a long
> time in the future, if at all.

I'm not sure if you saw an announcement about non-stop mode project. It's
rather near future.

> I'm talking about implementing a simple 
> solution, for now.  Even if non-stop mode does become a reality, it presumably
> won't be the default behaviour, so it won't break existing front ends.

This does not mean that all MI commands should behave differently in non-stop mode.

>  > I think it's more clear to set only those breakpoints that user want to set,
>  > as opposed to setting all of them, and then removing undesired ones.
> 
> Yes, but I still haven't see how you propose to do this.

-break-insert, in case when there are several overloaded functions, should return
this:

   ^error,msg="Several overloaded functions found",
	functions=["int f(int)", "int f(double)"]

then, break-insert should have an -o parameter that specifies a list of overloaded
functions to set breakpoints on:

  -break-insert -o 0 f

- Volodya

Re: [PATCH] Re: bug in mi when setting breakpoint

[Nick Roberts]

 > > Running GDB in "non-stop mode" sounds highly ambitious and may happen a
 > > long time in the future, if at all.
 > 
 > I'm not sure if you saw an announcement about non-stop mode project. It's
 > rather near future.

You probably know more than me, it didn't give any timescales.

 > > I'm talking about implementing a simple solution, for now.  Even if
 > > non-stop mode does become a reality, it presumably won't be the default
 > > behaviour, so it won't break existing front ends.
 > 
 > This does not mean that all MI commands should behave differently in
 > non-stop mode.

They wouldn't.  I don't see why we should design MI commands around a feature
that doesn't exist yet and will presumably be optional.

 > >  > I think it's more clear to set only those breakpoints that user want to
 > >  > set, as opposed to setting all of them, and then removing undesired
 > >  > ones.
 > > 
 > > Yes, but I still haven't see how you propose to do this.
 > 
 > -break-insert, in case when there are several overloaded functions, should
 > return this:
 > 
 >    ^error,msg="Several overloaded functions found",
 > 	functions=["int f(int)", "int f(double)"]

In Emacs, multiple breakpoints for overloaded functions would be specified
from the GUD buffer.  How do you specify them graphically?

 > then, break-insert should have an -o parameter that specifies a list of
 > overloaded functions to set breakpoints on:
 > 
 >   -break-insert -o 0 f

It all seems rather elaborate, but I don't feel that strongly about it as I'm
not yet in a position to fully migrate Emacs to GDB/MI.

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

Re: [PATCH] Re: bug in mi when setting breakpoint

[Vladimir Prus]

On Sunday 20 January 2008 23:32:22 Nick Roberts wrote:
>  > > Running GDB in "non-stop mode" sounds highly ambitious and may happen a
>  > > long time in the future, if at all.
>  > 
>  > I'm not sure if you saw an announcement about non-stop mode project. It's
>  > rather near future.
> 
> You probably know more than me, it didn't give any timescales.

We expect that basic non-stop mode will be available in about three months
from now.

>  > > I'm talking about implementing a simple solution, for now.  Even if
>  > > non-stop mode does become a reality, it presumably won't be the default
>  > > behaviour, so it won't break existing front ends.
>  > 
>  > This does not mean that all MI commands should behave differently in
>  > non-stop mode.
> 
> They wouldn't.  I don't see why we should design MI commands around a feature
> that doesn't exist yet and will presumably be optional.

As I've mentioned, in non-stop mode, adding all breakpoint and then removing
undesired ones won't work. So, we need MI support for adding only needed
breakpoints. And once we implement such support, and use it in non-stop mode,
it won't make much sense to use some different mechanism in all-stop mode -- that
will only complicate logic without much benefit.

> 
>  > >  > I think it's more clear to set only those breakpoints that user want to
>  > >  > set, as opposed to setting all of them, and then removing undesired
>  > >  > ones.
>  > > 
>  > > Yes, but I still haven't see how you propose to do this.
>  > 
>  > -break-insert, in case when there are several overloaded functions, should
>  > return this:
>  > 
>  >    ^error,msg="Several overloaded functions found",
>  > 	functions=["int f(int)", "int f(double)"]
> 
> In Emacs, multiple breakpoints for overloaded functions would be specified
> from the GUD buffer.  How do you specify them graphically?

You present a list of overloaded function, with checkboxes next to each
name, that user would check and uncheck as he see fit.

>  > then, break-insert should have an -o parameter that specifies a list of
>  > overloaded functions to set breakpoints on:
>  > 
>  >   -break-insert -o 0 f
> 
> It all seems rather elaborate, but I don't feel that strongly about it as I'm
> not yet in a position to fully migrate Emacs to GDB/MI.

BTW, insert-all-and-remove-undesired would be as complex. First, you need to
detect that several breakpoints were inserted (-break-insert will return several
breakpoint ids in that case). Then, you better tell the user about this fact,
as he might not be looking at gdb console. And ideally, you'll actually pop up
a list of just inserted breakpoints and let the user delete those he does not
want -- which is just as complex for frontend as the scheme I've suggested.

- Volodya

Re: [PATCH] Re: bug in mi when setting breakpoint

[Nick Roberts]

 > We expect that basic non-stop mode will be available in about three months
 > from now.

OK, thanks.

 > > They wouldn't.  I don't see why we should design MI commands around a
 > > feature that doesn't exist yet and will presumably be optional.
 > 
 > As I've mentioned, in non-stop mode, adding all breakpoint and then removing
 > undesired ones won't work. So, we need MI support for adding only needed
 > breakpoints. And once we implement such support, and use it in non-stop
 > mode, it won't make much sense to use some different mechanism in all-stop
 > mode -- that will only complicate logic without much benefit.

If your timescale is realistic and your proposal practical then I agree.

 > > In Emacs, multiple breakpoints for overloaded functions would be specified
 > > from the GUD buffer.  How do you specify them graphically?
 > 
 > You present a list of overloaded function, with checkboxes next to each
 > name, that user would check and uncheck as he see fit.

You previously said:

   1. UI should not require to type GDB command for such basic task as adding
   breakpoints.

My point was that in the GUD buffer you can specify an overloaded function,
A::f, by typing

(gdb) break A::f

and get breakpoints in A::f(double), A::f(int), say, but if you click on a line
inside an overloaded function, generally you will only get one breakpoint.

 > > It all seems rather elaborate, but I don't feel that strongly about it as
 > > I'm not yet in a position to fully migrate Emacs to GDB/MI.
 > 
 > BTW, insert-all-and-remove-undesired would be as complex. First, you need to
 > detect that several breakpoints were inserted (-break-insert will return
 > several breakpoint ids in that case). Then, you better tell the user about
 > this fact, as he might not be looking at gdb console. And ideally, you'll
 > actually pop up a list of just inserted breakpoints and let the user delete
 > those he does not want -- which is just as complex for frontend as the
 > scheme I've suggested.

I can only see such breakpoints being set from the GUD buffer and the
current warning gets printed there.  He can look at the breakpoints buffer if
he wishes to know more.

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

Re: [PATCH] Re: bug in mi when setting breakpoint

[Daniel Jacobowitz]

On Mon, Jan 21, 2008 at 10:28:06AM +0300, Vladimir Prus wrote:
> As I've mentioned, in non-stop mode, adding all breakpoint and then removing
> undesired ones won't work.

I don't think this is right.  "break f" -> [f(int), f(long)] is exactly
the same as "break f" -> [f<int>(), f<long>()] or "break f" ->
[f inlined into g, f inlined into h].  The fact that we handle all
these things differently in GDB today seems like a bug to me.

If the IDE wants to set a breakpoint on one of a group of overloaded
functions, where it only knows the name, can't it query GDB for the
list separately from -break-insert?

We have got a problem today: we print out prompts for GDB/MI clients
in a completely invalid way.  Let's figure out what those prompts
should do in MI mode, and make them do it instead of prompting.

-- 
Daniel Jacobowitz
CodeSourcery