[PATCH] Avoid breakpoint query in MI

[PATCH] Avoid breakpoint query in MI

[Nick Roberts]

This updates my earlier patch
(http://sourceware.org/ml/gdb-patches/2008-01/msg00421.html) from the thread
"bug in mi when setting breakpoint" started last year by Dodji Seketeli.  It is
simplified by Joel's recent patch for multiple symbols and Vladimir's concept
of top-level interpreter.

I see the default for multiple-symbols is 'all' (it used to be 'ask', perhaps
this change should be mentioned in NEWS) which might make this change more
acceptable.

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


2008-04-18  Nick Roberts  <nickrob@snap.net.nz>

	* interps.c (interp_top_level): New function.

	* interps.h: New extern.

	* linespec.c: Include interps.h and mi/mi-cmds.h.
	(decode_line_2): When using MI, always set all breakpoints in menu.


*** interps.h	15 Mar 2008 18:14:50 +1300	1.14
--- interps.h	18 Apr 2008 00:47:56 +1200	
*************** extern int current_interp_display_prompt
*** 66,71 ****
--- 66,72 ----
  extern void current_interp_command_loop (void);
  /* Returns opaque data associated with the top-level interpreter.  */
  extern void *top_level_interpreter_data (void);
+ extern void *interp_top_level (void);
  
  extern void clear_interpreter_hooks (void);
  

*** interps.c	15 Mar 2008 18:14:50 +1300	1.23
--- interps.c	16 Apr 2008 15:37:01 +1200	
*************** interpreter_completer (char *text, char 
*** 476,482 ****
    return matches;
  }
  
! extern void *
  top_level_interpreter_data (void)
  {
    gdb_assert (top_level_interpreter);
--- 476,488 ----
    return matches;
  }
  
! void *
! interp_top_level (void)
! {
!   return top_level_interpreter;  
! }
! 
! void *
  top_level_interpreter_data (void)
  {
    gdb_assert (top_level_interpreter);


*** linespec.c	18 Apr 2008 00:10:58 +1200	1.75
--- linespec.c	18 Apr 2008 00:21:29 +1200	
***************
*** 36,41 ****
--- 36,43 ----
  #include "linespec.h"
  #include "exceptions.h"
  #include "language.h"
+ #include "interps.h"
+ #include "mi/mi-cmds.h"
  
  /* We share this one with symtab.c, but it is not exported widely. */
  
*************** See set/show multiple-symbol."));
*** 524,530 ****
  
    /* If select_mode is "all", then do not print the multiple-choice
       menu and act as if the user had chosen choice "1" (all).  */
!   if (select_mode == multiple_symbols_all)
      args = "1";
    else
      {
--- 526,533 ----
  
    /* If select_mode is "all", then do not print the multiple-choice
       menu and act as if the user had chosen choice "1" (all).  */
!   if (select_mode == multiple_symbols_all
!       || ui_out_is_mi_like_p (interp_ui_out (interp_top_level ())))
      args = "1";
    else
      {

Re: [PATCH] Avoid breakpoint query in MI

[Vladimir Prus]

Nick Roberts wrote:

> This updates my earlier patch
> (http://sourceware.org/ml/gdb-patches/2008-01/msg00421.html) from the thread
> "bug in mi when setting breakpoint" started last year by Dodji Seketeli.  It is
> simplified by Joel's recent patch for multiple symbols and Vladimir's concept
> of top-level interpreter.
> 
> I see the default for multiple-symbols is 'all' (it used to be 'ask', perhaps
> this change should be mentioned in NEWS) which might make this change more
> acceptable.
> 
> Nick                                           http://www.inet.net.nz/~nickrob
> 
> 
> 2008-04-18  Nick Roberts  <nickrob@snap.net.nz>
> 
>         * interps.c (interp_top_level): New function.
> 
>         * interps.h: New extern.
> 
>         * linespec.c: Include interps.h and mi/mi-cmds.h.
>         (decode_line_2): When using MI, always set all breakpoints in menu.
> 
> 
> *** interps.h   15 Mar 2008 18:14:50 +1300      1.14
> --- interps.h   18 Apr 2008 00:47:56 +1200      
> *************** extern int current_interp_display_prompt
> *** 66,71 ****
> --- 66,72 ----
>   extern void current_interp_command_loop (void);
>   /* Returns opaque data associated with the top-level interpreter.  */
>   extern void *top_level_interpreter_data (void);
> + extern void *interp_top_level (void);

Please name this top_level_interpreter. Also, the return type
should be "struct interp*". In fact, I have a not-yet-submitted
patches which does just that :-)


>   
>   extern void clear_interpreter_hooks (void);
>   
> 
> *** interps.c   15 Mar 2008 18:14:50 +1300      1.23
> --- interps.c   16 Apr 2008 15:37:01 +1200      
> *************** interpreter_completer (char *text, char 
> *** 476,482 ****
>     return matches;
>   }
>   
> ! extern void *
>   top_level_interpreter_data (void)
>   {
>     gdb_assert (top_level_interpreter);
> --- 476,488 ----
>     return matches;
>   }
>   
> ! void *
> ! interp_top_level (void)
> ! {
> !   return top_level_interpreter;  
> ! }
> ! 
> ! void *
>   top_level_interpreter_data (void)
>   {
>     gdb_assert (top_level_interpreter);
> 
> 
> *** linespec.c  18 Apr 2008 00:10:58 +1200      1.75
> --- linespec.c  18 Apr 2008 00:21:29 +1200      
> ***************
> *** 36,41 ****
> --- 36,43 ----
>   #include "linespec.h"
>   #include "exceptions.h"
>   #include "language.h"
> + #include "interps.h"
> + #include "mi/mi-cmds.h"
>   
>   /* We share this one with symtab.c, but it is not exported widely. */
>   
> *************** See set/show multiple-symbol."));
> *** 524,530 ****
>   
>     /* If select_mode is "all", then do not print the multiple-choice
>        menu and act as if the user had chosen choice "1" (all).  */
> !   if (select_mode == multiple_symbols_all)
>       args = "1";
>     else
>       {
> --- 526,533 ----
>   
>     /* If select_mode is "all", then do not print the multiple-choice
>        menu and act as if the user had chosen choice "1" (all).  */
> !   if (select_mode == multiple_symbols_all
> !       || ui_out_is_mi_like_p (interp_ui_out (interp_top_level ())))
>       args = "1";
>     else
>       {

I think that adding breakpoints on all symbols is better than silently grabbing 
random symbol, so the above seems fine to me. Can somebody approve the linespec.c
bit?

- Volodya

Re: [PATCH] Avoid breakpoint query in MI

[Nick Roberts]

 > >   extern void current_interp_command_loop (void);
 > >   /* Returns opaque data associated with the top-level interpreter.  */
 > >   extern void *top_level_interpreter_data (void);
 > > + extern void *interp_top_level (void);
 > 
 > Please name this top_level_interpreter. Also, the return type
 > should be "struct interp*". In fact, I have a not-yet-submitted
 > patches which does just that :-)

OK.

 > > *************** See set/show multiple-symbol."));
 > > *** 524,530 ****
 > >   
 > >     /* If select_mode is "all", then do not print the multiple-choice
 > >        menu and act as if the user had chosen choice "1" (all).  */
 > > !   if (select_mode == multiple_symbols_all)
 > >       args = "1";
 > >     else
 > >       {
 > > --- 526,533 ----
 > >   
 > >     /* If select_mode is "all", then do not print the multiple-choice
 > >        menu and act as if the user had chosen choice "1" (all).  */
 > > !   if (select_mode == multiple_symbols_all
 > > !       || ui_out_is_mi_like_p (interp_ui_out (interp_top_level ())))
 > >       args = "1";
 > >     else
 > >       {
 > 
 > I think that adding breakpoints on all symbols is better than silently
 > grabbing random symbol, so the above seems fine to me. Can somebody approve
 > the linespec.c bit?

I'm not sure I understand.  When multiple-symbols is 'all' (the default) there
is no problem.  When it's 'ask', it currently queries which we don't want to
happen in MI.

This appears to me to be an MI only change and within your domain.  I don't
think authority is devolved on a file by file basis, although clearly I'm
not qualified to make such remarks.

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

Re: [PATCH] Avoid breakpoint query in MI

[Nick Roberts]

 > Please name this top_level_interpreter. Also, the return type
 > should be "struct interp*". In fact, I have a not-yet-submitted
 > patches which does just that :-)

Actually I can't cal it top_level_interpreter because you've already used
that name:

static struct interp *top_level_interpreter = NULL;

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

Re: [PATCH] Avoid breakpoint query in MI

[Vladimir Prus]

On Friday 18 April 2008 02:32:13 Nick Roberts wrote:
>  > Please name this top_level_interpreter. Also, the return type
>  > should be "struct interp*". In fact, I have a not-yet-submitted
>  > patches which does just that :-)
> 
> Actually I can't cal it top_level_interpreter because you've already used
> that name:
> 
> static struct interp *top_level_interpreter = NULL;

This is 'static', nobody cares how it's called. My patch has this bit:


@@ -78,13 +78,13 @@ static char **interpreter_completer (char *text, char *word);
 void _initialize_interpreter (void);
 
 /* Variables local to this file: */
 
 static struct interp *interp_list = NULL;
 static struct interp *current_interpreter = NULL;
-static struct interp *top_level_interpreter = NULL;
+static struct interp *top_level_interpreter_ptr = NULL;
 
 static int interpreter_initialized = 0;
 
 /* interp_new - This allocates space for a new interpreter,
    fills the fields from the inputs, and returns a pointer to the
    interpreter. */

- Volodya

Re: [PATCH] Avoid breakpoint query in MI

[Nick Roberts]

 > I'm not qualified either to decide if I'm qualified to approve this patch :-)
 > As far as I'm concerned, this patch is OK.
 > 
 > - Volodya

OK.  This is what I've committed.

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


2008-04-19  Nick Roberts  <nickrob@snap.net.nz>

	* interps.c (top_level_interpreter): Rename static variable...
	(top_level_interpreter_ptr): ...to this.
 	(top_level_interpreter): New function.

	* interps.h: New extern for top_level_interpreter.

	* linespec.c: Include interps.h and mi/mi-cmds.h.
	(decode_line_2): When using MI, always set all breakpoints in menu.

	* Makefile.in (linespec.o, mi-interp.o): Add dependencies.


*** interps.c.~1.23.~	2008-03-15 22:34:07.000000000 +1300
--- interps.c	2008-04-19 16:16:43.000000000 +1200
*************** void _initialize_interpreter (void);
*** 81,87 ****
  
  static struct interp *interp_list = NULL;
  static struct interp *current_interpreter = NULL;
! static struct interp *top_level_interpreter = NULL;
  
  static int interpreter_initialized = 0;
  
--- 81,87 ----
  
  static struct interp *interp_list = NULL;
  static struct interp *current_interpreter = NULL;
! static struct interp *top_level_interpreter_ptr = NULL;
  
  static int interpreter_initialized = 0;
  
*************** interp_set (struct interp *interp, int t
*** 144,150 ****
    /* If we already have an interpreter, then trying to
       set top level interpreter is kinda pointless.  */
    gdb_assert (!top_level || !current_interpreter);
!   gdb_assert (!top_level || !top_level_interpreter);
  
    if (current_interpreter != NULL)
      {
--- 144,150 ----
    /* If we already have an interpreter, then trying to
       set top level interpreter is kinda pointless.  */
    gdb_assert (!top_level || !current_interpreter);
!   gdb_assert (!top_level || !top_level_interpreter_ptr);
  
    if (current_interpreter != NULL)
      {
*************** interp_set (struct interp *interp, int t
*** 165,171 ****
  
    current_interpreter = interp;
    if (top_level)
!     top_level_interpreter = interp;
  
    /* We use interpreter_p for the "set interpreter" variable, so we need
       to make sure we have a malloc'ed copy for the set command to free. */
--- 165,171 ----
  
    current_interpreter = interp;
    if (top_level)
!     top_level_interpreter_ptr = interp;
  
    /* We use interpreter_p for the "set interpreter" variable, so we need
       to make sure we have a malloc'ed copy for the set command to free. */
*************** interpreter_completer (char *text, char 
*** 476,486 ****
    return matches;
  }
  
! extern void *
  top_level_interpreter_data (void)
  {
!   gdb_assert (top_level_interpreter);
!   return top_level_interpreter->data;  
  }
  
  /* This just adds the "interpreter-exec" command.  */
--- 476,492 ----
    return matches;
  }
  
! struct interp *
! top_level_interpreter (void)
! {
!   return top_level_interpreter_ptr;  
! }
! 
! void *
  top_level_interpreter_data (void)
  {
!   gdb_assert (top_level_interpreter_ptr);
!   return top_level_interpreter_ptr->data;  
  }
  
  /* This just adds the "interpreter-exec" command.  */


*** interps.h.~1.14.~	2008-03-15 22:34:07.000000000 +1300
--- interps.h	2008-04-19 16:17:10.000000000 +1200
*************** extern int current_interp_display_prompt
*** 66,71 ****
--- 66,72 ----
  extern void current_interp_command_loop (void);
  /* Returns opaque data associated with the top-level interpreter.  */
  extern void *top_level_interpreter_data (void);
+ extern struct interp *top_level_interpreter (void);
  
  extern void clear_interpreter_hooks (void);
  

*** linespec.c.~1.75~	2008-04-19 17:54:40.000000000 +1200
--- linespec.c	2008-04-19 09:56:08.000000000 +1200
***************
*** 36,41 ****
--- 36,43 ----
  #include "linespec.h"
  #include "exceptions.h"
  #include "language.h"
+ #include "interps.h"
+ #include "mi/mi-cmds.h"
  
  /* We share this one with symtab.c, but it is not exported widely. */
  
*************** See set/show multiple-symbol."));
*** 524,530 ****
  
    /* If select_mode is "all", then do not print the multiple-choice
       menu and act as if the user had chosen choice "1" (all).  */
!   if (select_mode == multiple_symbols_all)
      args = "1";
    else
      {
--- 526,533 ----
  
    /* If select_mode is "all", then do not print the multiple-choice
       menu and act as if the user had chosen choice "1" (all).  */
!   if (select_mode == multiple_symbols_all
!       || ui_out_is_mi_like_p (interp_ui_out (top_level_interpreter ())))
      args = "1";
    else
      {


*** Makefile.in.~1.1001.~	2008-04-19 10:53:43.000000000 +1200
--- Makefile.in	2008-04-19 18:03:52.000000000 +1200
*************** libunwind-frame.o: libunwind-frame.c $(d
*** 2365,2372 ****
  	$(gdb_string_h) $(libunwind_frame_h) $(complaints_h)
  linespec.o: linespec.c $(defs_h) $(symtab_h) $(frame_h) $(command_h) \
  	$(symfile_h) $(objfiles_h) $(source_h) $(demangle_h) $(value_h) \
! 	$(completer_h) $(cp_abi_h) $(parser_defs_h) $(block_h) \
! 	$(objc_lang_h) $(linespec_h) $(exceptions_h) $(language_h)
  linux-fork.o: linux-fork.c $(defs_h) $(inferior_h) $(regcache_h) $(gdbcmd_h) \
  	$(infcall_h) $(gdb_assert_h) $(gdb_string_h) $(linux_fork_h) \
  	$(linux_nat_h) $(gdb_wait_h) $(gdb_dirent_h)
--- 2365,2372 ----
  	$(gdb_string_h) $(libunwind_frame_h) $(complaints_h)
  linespec.o: linespec.c $(defs_h) $(symtab_h) $(frame_h) $(command_h) \
  	$(symfile_h) $(objfiles_h) $(source_h) $(demangle_h) $(value_h) \
! 	$(completer_h) $(cp_abi_h) $(parser_defs_h) $(block_h) $(interps_h) \
! 	$(objc_lang_h) $(linespec_h) $(exceptions_h) $(language_h) $(mi_cmds_h)
  linux-fork.o: linux-fork.c $(defs_h) $(inferior_h) $(regcache_h) $(gdbcmd_h) \
  	$(infcall_h) $(gdb_assert_h) $(gdb_string_h) $(linux_fork_h) \
  	$(linux_nat_h) $(gdb_wait_h) $(gdb_dirent_h)
*************** mi-getopt.o: $(srcdir)/mi/mi-getopt.c $(
*** 3253,3259 ****
  mi-interp.o: $(srcdir)/mi/mi-interp.c $(defs_h) $(gdb_string_h) $(interps_h) \
  	$(event_top_h) $(event_loop_h) $(inferior_h) $(ui_out_h) $(top_h) \
  	$(exceptions_h) $(mi_main_h) $(mi_cmds_h) $(mi_out_h) \
! 	$(mi_console_h) $(observer_h) $(gdbthread_h)
  	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/mi/mi-interp.c
  mi-main.o: $(srcdir)/mi/mi-main.c $(defs_h) $(target_h) $(inferior_h) \
  	$(gdb_string_h) $(exceptions_h) $(top_h) $(gdbthread_h) $(mi_cmds_h) \
--- 3253,3259 ----
  mi-interp.o: $(srcdir)/mi/mi-interp.c $(defs_h) $(gdb_string_h) $(interps_h) \
  	$(event_top_h) $(event_loop_h) $(inferior_h) $(ui_out_h) $(top_h) \
  	$(exceptions_h) $(mi_main_h) $(mi_cmds_h) $(mi_out_h) \
! 	$(mi_console_h) $(observer_h) $(gdbthread_h) $(interps_h)
  	$(CC) -c $(INTERNAL_CFLAGS) $(srcdir)/mi/mi-interp.c
  mi-main.o: $(srcdir)/mi/mi-main.c $(defs_h) $(target_h) $(inferior_h) \
  	$(gdb_string_h) $(exceptions_h) $(top_h) $(gdbthread_h) $(mi_cmds_h) \

Re: [PATCH] Avoid breakpoint query in MI

[Daniel Jacobowitz]

On Sat, Apr 19, 2008 at 06:17:16PM +1200, Nick Roberts wrote:
>  > I'm not qualified either to decide if I'm qualified to approve this patch :-)
>  > As far as I'm concerned, this patch is OK.

As far as I'm concerned, MI maintenance is not limited by strict file
boundaries; this is fine for Volodya to approve if he's comfortable
with the change.

This is a bug that MI frontend developers have certainly encountered
before.  Should it get a NEWS entry?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [PATCH] Avoid breakpoint query in MI

[Nick Roberts]

 > This is a bug that MI frontend developers have certainly encountered
 > before.  Should it get a NEWS entry?

No, I don't think so.  It doesn't fix all queries, just this one.  AFAIK
it's only been reported by Dodji Seketeli and I've already cc'ed him.  The
big change is setting the default for multiple-symbols to 'all' and I've
already suggested mentioning that in NEWS.

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

Re: [PATCH] Avoid breakpoint query in MI

[Eli Zaretskii]

> Date: Sat, 19 Apr 2008 10:19:04 -0400
> From: Daniel Jacobowitz <drow@false.org>
> Cc: Vladimir Prus <ghost@cs.msu.su>, gdb-patches@sources.redhat.com
> 
> This is a bug that MI frontend developers have certainly encountered
> before.  Should it get a NEWS entry?

Sorry, I didn't follow this thread.  If someone explains the bug and
its solution, I could tell what I think about mentioning it in NEWS.