bug in mi when setting breakpoint

bug in mi when setting breakpoint

[Dodji Seketeli]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

Hello,

With the mi2 interpreter, when one issues a break command
(either '-break-insert <classname::function-name>' or the cli
'break <classname::function-name>' command)
in the context of a C++ inferior, an overloaded
function resolution can happen.

In that case, the breakpoint setting code
asks the user to choose the overloaded function it wants to break in.
To do so, the breakpoint setting code displays something like:

~"[0] cancel\n[1] all\n"
~"[2] classname::function_name(int) at fooprog.cc:65\n"
~"[3] classname::function_name() at fooprog.cc:59\n"
~"> "

The last line of this "question" is the default prompt indicating the end
of the question.

In gdb 6.7.1, that prompt is missing *only* when using the MI interpreter.
It is present in the CLI interpreter. And this is a regression from 6.6
where the prompt was present with both interpreters.

The prompt is really important for graphical front-end tools willing
to parse that "question" so that they can display display it
back to the user in a nice windowed manner.
As the question does not really respect the GDB/MI output format where the
output should be ended by a "(gdb)" string, the prompt is the only way th
front end can detect the end of the "question".

So I tried to produce the attached patch to pinpoint the problem and hopefully
propose a fix.

Cheers,

-- 
Dodji Seketeli
http://www.seketeli.org/dodji


[-- Attachment #2: fix-prompt.patch --]
[-- Type: text/x-diff, Size: 6272 bytes --]


The spirit of the patch is to enable the prompt when using the MI
interpreter. I tried to enable the prompt for both "-break-insert"
and "break" commands because the "break" command offers features that the
"-break-insert" command does not offer yet, so it is very useful, even in an
MI context.

The patch applies to the gdb-6.7.1 tarball.

Index: gdb-6.7.1/gdb/interps.c
===================================================================
--- gdb-6.7.1.orig/gdb/interps.c	2007-12-16 08:56:50.000000000 +0100
+++ gdb-6.7.1/gdb/interps.c	2007-12-16 10:13:50.000000000 +0100
@@ -289,10 +289,13 @@
     return current_interpreter->quiet_p;
 }
 
-static int
+int
 interp_set_quiet (struct interp *interp, int quiet)
 {
-  int old_val = interp->quiet_p;
+  int old_val = 0;
+  if (!interp)
+    interp = current_interpreter;
+  old_val = interp->quiet_p;
   interp->quiet_p = quiet;
   return old_val;
 }
Index: gdb-6.7.1/gdb/interps.h
===================================================================
--- gdb-6.7.1.orig/gdb/interps.h	2007-12-16 08:55:24.000000000 +0100
+++ gdb-6.7.1/gdb/interps.h	2007-12-16 09:03:11.000000000 +0100
@@ -34,6 +34,7 @@
 extern struct gdb_exception interp_exec (struct interp *interp,
 					 const char *command);
 extern int interp_quiet_p (struct interp *interp);
+int interp_set_quiet (struct interp *interp, int quiet);
 
 typedef void *(interp_init_ftype) (void);
 typedef int (interp_resume_ftype) (void *data);
Index: gdb-6.7.1/gdb/mi/mi-cmd-break.c
===================================================================
--- gdb-6.7.1.orig/gdb/mi/mi-cmd-break.c	2007-12-16 10:04:45.000000000 +0100
+++ gdb-6.7.1/gdb/mi/mi-cmd-break.c	2007-12-16 10:36:11.000000000 +0100
@@ -26,6 +26,7 @@
 #include "mi-getopt.h"
 #include "gdb-events.h"
 #include "gdb.h"
+#include "interps.h"
 
 enum
   {
@@ -76,6 +77,7 @@
   char *condition = NULL;
   enum gdb_rc rc;
   struct gdb_events *old_hooks;
+  int was_cur_interp_quiet = 0;
   enum opt
     {
       HARDWARE_OPT, TEMP_OPT /*, REGEXP_OPT */ , CONDITION_OPT,
@@ -133,6 +135,14 @@
 
   /* Now we have what we need, let's insert the breakpoint! */
   old_hooks = deprecated_set_gdb_event_hooks (&breakpoint_hooks);
+
+  /*
+   * don't quiet the current interpreter because otherwise,
+   * if the user is asked (by the breakpoint setting code) to resolve
+   * function overload ambiguity, the question won't be followed by a
+   * prompt.
+   */
+  was_cur_interp_quiet = interp_set_quiet (NULL, 0);
   switch (type)
     {
     case REG_BP:
@@ -161,6 +171,8 @@
 		      _("mi_cmd_break_insert: Bad switch."));
     }
   deprecated_set_gdb_event_hooks (old_hooks);
+  /*set the interpreter back to its previous quiet-ness state*/
+  interp_set_quiet (NULL, was_cur_interp_quiet);
 
   if (rc == GDB_RC_FAIL)
     return MI_CMD_ERROR;
Index: gdb-6.7.1/gdb/mi/mi-interp.c
===================================================================
--- gdb-6.7.1.orig/gdb/mi/mi-interp.c	2007-12-16 09:57:56.000000000 +0100
+++ gdb-6.7.1/gdb/mi/mi-interp.c	2007-12-16 10:33:33.000000000 +0100
@@ -31,6 +31,14 @@
 #include "mi-out.h"
 #include "mi-console.h"
 
+/*
+ * this is a global variable
+ * to detect if we are in the context
+ * of setting a breakpoint using a cli command, while being initially
+ * in an MI interpreter.
+ */
+extern int in_cli_break_context;
+
 struct mi_interp
 {
   /* MI's output channels */
@@ -156,7 +164,10 @@
 static int
 mi_interpreter_prompt_p (void *data)
 {
-  return 0;
+  if (interp_quiet_p (NULL)) {
+    return 0;
+  }
+  return 1;
 }
 
 static void
@@ -185,6 +196,7 @@
   enum mi_cmd_result result = MI_CMD_DONE;
   int i;
   struct interp_procs *procs;
+  int was_interp_quiet = 0;
 
   if (argc < 2)
     {
@@ -199,6 +211,17 @@
       return MI_CMD_ERROR;
     }
 
+  /*
+   * if we are executing a cli "break" command, don't be quiet.
+   * Otherwise, c++ functions overload resolution questions asked to the
+   * user won't be followed by the mandatory prompt.
+   */
+  if (in_cli_break_context)
+    {
+      /*save the previouse quiet-ness status*/
+      was_interp_quiet = interp_set_quiet (interp_to_use, 0);
+    }
+
   if (!interp_exec_p (interp_to_use))
     {
       mi_error_message = xstrprintf ("mi_cmd_interpreter_exec: interpreter \"%s\" does not support command execution",
@@ -206,6 +229,7 @@
       return MI_CMD_ERROR;
     }
 
+
   /* Insert the MI out hooks, making sure to also call the interpreter's hooks
      if it has any. */
   /* KRS: We shouldn't need this... Events should be installed and they should
@@ -235,8 +259,15 @@
       sync_execution = 0;
     }
 
+  if (in_cli_break_context)
+    {
+      /*restore the previouse quiet-ness status*/
+      interp_set_quiet (interp_to_use, was_interp_quiet);
+    }
+
   mi_remove_notify_hooks ();
 
+
   /* Okay, now let's see if the command set the inferior going...
      Tricky point - have to do this AFTER resetting the interpreter, since
      changing the interpreter will clear out all the continuations for
Index: gdb-6.7.1/gdb/mi/mi-main.c
===================================================================
--- gdb-6.7.1.orig/gdb/mi/mi-main.c	2007-12-16 09:41:12.000000000 +0100
+++ gdb-6.7.1/gdb/mi/mi-main.c	2007-12-16 10:33:08.000000000 +0100
@@ -120,6 +120,14 @@
 static void print_diff_now (struct mi_timestamp *start);
 static void print_diff (struct mi_timestamp *start, struct mi_timestamp *end);
 
+/*
+ * this is a global variable
+ * to detect if we are in the context
+ * of setting a breakpoint using a cli command, while being initially
+ * in an MI interpreter.
+ */
+int in_cli_break_context;
+
 enum mi_cmd_result
 mi_cmd_gdb_exit (char *command, char **argv, int argc)
 {
@@ -1170,7 +1178,16 @@
 	/* Call the "console" interpreter.  */
 	argv[0] = "console";
 	argv[1] = context->command;
+	if (context->command && (strncmp (context->command, "b", 1) == 0))
+	  {
+	    /*
+	     * we are being asked to execute a break command, using the cli
+	     * interpreter, while being in a MI interpreter.
+	     */
+	    in_cli_break_context = 1;
+	  }
 	args->rc = mi_cmd_interpreter_exec ("-interpreter-exec", argv, 2);
+	in_cli_break_context = 0;
 
 	/* If we changed interpreters, DON'T print out anything.  */
 	if (current_interp_named_p (INTERP_MI)

Re: bug in mi when setting breakpoint

[Nick Roberts]

 > In that case, the breakpoint setting code
 > asks the user to choose the overloaded function it wants to break in.
 > To do so, the breakpoint setting code displays something like:
 > 
 > ~"[0] cancel\n[1] all\n"
 > ~"[2] classname::function_name(int) at fooprog.cc:65\n"
 > ~"[3] classname::function_name() at fooprog.cc:59\n"
 > ~"> "
 > 
 > The last line of this "question" is the default prompt indicating the end
 > of the question.
 > 
 > In gdb 6.7.1, that prompt is missing *only* when using the MI interpreter.
 > It is present in the CLI interpreter. And this is a regression from 6.6
 > where the prompt was present with both interpreters.

Your patch appears to introduce new behaviour.  The question to ask is what
change broke this behaviour?  I suspect it was the change to readline made at
the start to this year.  GDB seems to go into gdb_readline_wrapper from
decode_line_2 and stay there.

 > The prompt is really important for graphical front-end tools willing
 > to parse that "question" so that they can display display it
 > back to the user in a nice windowed manner.
 > As the question does not really respect the GDB/MI output format where the
 > output should be ended by a "(gdb)" string, the prompt is the only way th
 > front end can detect the end of the "question".
 > 
 > So I tried to produce the attached patch to pinpoint the problem and
 > hopefully propose a fix.

Unfortunately CLI also uses sub-prompts for several other commands: queries e.g
pending breakpoints, exiting after exevution has started; the "commands"
command.  I don't think that they fit well with the MI paradigm: MI expects
MI output.  With queries, GDB takes affirmative action, e.g., creates pending
breakpoints regardless of the value of "show breakpoint pending" and exits
regardless of the value of "show confirm".

Perhaps, for now, GDB could do something similar, i.e., set all the breakpoints
in the breakpoint menu.

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

Re: bug in mi when setting breakpoint

[Vladimir Prus]

Nick Roberts wrote:

>  > In that case, the breakpoint setting code
>  > asks the user to choose the overloaded function it wants to break in.
>  > To do so, the breakpoint setting code displays something like:
>  > 
>  > ~"[0] cancel\n[1] all\n"
>  > ~"[2] classname::function_name(int) at fooprog.cc:65\n"
>  > ~"[3] classname::function_name() at fooprog.cc:59\n"
>  > ~"> "
>  > 
>  > The last line of this "question" is the default prompt indicating the
>  > end of the question.
>  > 
>  > In gdb 6.7.1, that prompt is missing *only* when using the MI
>  > interpreter. It is present in the CLI interpreter. And this is a
>  > regression from 6.6 where the prompt was present with both
>  > interpreters.
> 
> Your patch appears to introduce new behaviour.  The question to ask is
> what
> change broke this behaviour?  I suspect it was the change to readline made
> at
> the start to this year.  GDB seems to go into gdb_readline_wrapper from
> decode_line_2 and stay there.
> 
>  > The prompt is really important for graphical front-end tools willing
>  > to parse that "question" so that they can display display it
>  > back to the user in a nice windowed manner.
>  > As the question does not really respect the GDB/MI output format where
>  > the output should be ended by a "(gdb)" string, the prompt is the only
>  > way th front end can detect the end of the "question".
>  > 
>  > So I tried to produce the attached patch to pinpoint the problem and
>  > hopefully propose a fix.
> 
> Unfortunately CLI also uses sub-prompts for several other commands:
> queries e.g pending breakpoints, exiting after exevution has started; the
> "commands"
> command.  I don't think that they fit well with the MI paradigm: MI
> expects
> MI output.  With queries, GDB takes affirmative action, e.g., creates
> pending breakpoints regardless of the value of "show breakpoint pending"

I don't think that's true. Week old CVS HEAD won't ever create pending
breakpoint when using MI. Today's CVS HEAD will create pending breakpoint
if you use the -f option to -break-insert. Without that option, pending
breakpoint won't ever be created.

> and exits regardless of the value of "show confirm".
> 
> Perhaps, for now, GDB could do something similar, i.e., set all the
> breakpoints in the breakpoint menu.

In general, I think that any command that requires any interactivity
should explicitly document how it would handle that.

- Volodya
 

Re: bug in mi when setting breakpoint

[Nick Roberts]

 > > Unfortunately CLI also uses sub-prompts for several other commands:
 > > queries e.g pending breakpoints, exiting after exevution has started; the
 > > "commands" command.  I don't think that they fit well with the MI
 > > paradigm: MI expects MI output.  With queries, GDB takes affirmative
 > > action, e.g., creates pending breakpoints regardless of the value of "show
 > > breakpoint pending"
 > 
 > I don't think that's true. Week old CVS HEAD won't ever create pending
 > breakpoint when using MI. Today's CVS HEAD will create pending breakpoint
 > if you use the -f option to -break-insert. Without that option, pending
 > breakpoint won't ever be created.

It is true with CLI commands in MI:

(gdb) 
-interpreter-exec console "break fgfg"
&"Function \"fgfg\" not defined.\n"
~"Breakpoint 1 (fgfg) pending.\n"
^done
(gdb) 

 > > and exits regardless of the value of "show confirm".
 > > 
 > > Perhaps, for now, GDB could do something similar, i.e., set all the
 > > breakpoints in the breakpoint menu.
 > 
 > In general, I think that any command that requires any interactivity
 > should explicitly document how it would handle that.

We don't appear to have any provision for such commands in MI and ISTR that Jim
Ingham has described difficulties with MI.  In, particular they have
-break-commands to deal with the "commands" command.  I was just proposing a
possible immediate fix, not to solve the general problem.

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

Re: bug in mi when setting breakpoint

[Dodji Seketeli]

On Mon, Dec 17, 2007 at 09:41:49AM +1300, Nick Roberts wrote:
[...]
> 
> Your patch appears to introduce new behaviour.  The question to ask is what
> change broke this behaviour?  I suspect it was the change to readline made at
> the start to this year.  GDB seems to go into gdb_readline_wrapper from
> decode_line_2 and stay there.

I am not aware of the exact changeset that changed the behaviour. Could you
point me to it please ? I am only aware of what is in gdb 6.7.1 now, sorry.

But yeah, it actually boils down to the decode_line_2() that calls
command_line_input() to get input from the user (and display a prompt at
the same time).

That code patch It eventually ends up in display_gdb_prompt() which job is
to (surprise!) display our now famous prompt.

One of the first thing that display_gdb_prompt() does is to check if the
current interpreter has disabled prompts or not, by calling
current_interp_display_prompt_p(). This last function calls returns 0,
meaning that the current interpreter (in my case MI) disallows prompts so
the prompt is simply not displayed.

So in short, MI disallows prompts. It does this by setting the member
prompt_proc_p of the struct interp_procs to a function that always returns
0. This is done in _initialize_mi_interp() (in gdb/mi/mi-interp.c).

[...]

> 
> Unfortunately CLI also uses sub-prompts for several other commands: queries e.g
> pending breakpoints, exiting after exevution has started; the "commands"
> command.  I don't think that they fit well with the MI paradigm: MI expects
> MI output.  With queries, GDB takes affirmative action, e.g., creates pending
> breakpoints regardless of the value of "show breakpoint pending" and exits
> regardless of the value of "show confirm".

I am not sure to understand why are saying this here :-)
But for what it is worth, the CLI interpreter allows prompt, yes. I am not
sure to understand why it is unfortunatel

> 
> Perhaps, for now, GDB could do something similar, i.e., set all the breakpoints
> in the breakpoint menu.

Hmmh, no. I think it is important to stay consistent with the format of the
"question" asked to the user in the pre 6.7.1 and make that question be
followed by a prompt. Otherwise this would break existing front ends.

Cheers,

-- 
Dodji Seketeli
http://www.seketeli.org/dodji

Re: bug in mi when setting breakpoint

[Nick Roberts]

 > > Your patch appears to introduce new behaviour.  The question to ask is
 > > what change broke this behaviour?  I suspect it was the change to readline
 > > made at the start to this year.  GDB seems to go into gdb_readline_wrapper
 > > from decode_line_2 and stay there.
 > 
 > I am not aware of the exact changeset that changed the behaviour. Could you
 > point me to it please ? I am only aware of what is in gdb 6.7.1 now, sorry.
 
I think it's this one:

2007-01-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
	    Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (top.o): Update.
	* top.c (gdb_readline_wrapper_done, gdb_readline_wrapper_result)
	(saved_after_char_processing_hook, gdb_readline_wrapper_line)
	(struct gdb_readline_wrapper_cleanup, gdb_readline_wrapper_cleanup):
	New.
	(gdb_readline_wrapper): Rewrite to use asynchronous readline.

I see behaviour changed around that time.

 > But yeah, it actually boils down to the decode_line_2() that calls
 > command_line_input() to get input from the user (and display a prompt at
 > the same time).
 > 
 > That code patch It eventually ends up in display_gdb_prompt() which job is
 > to (surprise!) display our now famous prompt.
 > 
 > One of the first thing that display_gdb_prompt() does is to check if the
 > current interpreter has disabled prompts or not, by calling
 > current_interp_display_prompt_p(). This last function calls returns 0,
 > meaning that the current interpreter (in my case MI) disallows prompts so
 > the prompt is simply not displayed.
 > 
 > So in short, MI disallows prompts. It does this by setting the member
 > prompt_proc_p of the struct interp_procs to a function that always returns
 > 0. This is done in _initialize_mi_interp() (in gdb/mi/mi-interp.c).

Why does MI disallow prompts?  Presumably it's for good reason and perhaps
you're trying to subvert that.  The function gdb_readline_wrapper used to
be just that: a wrapper for readline.  It didn't used to do things like
call display_gdb_prompt.  Perhaps its name should be changed.

 > [...]
 > 
 > > 
 > > Unfortunately CLI also uses sub-prompts for several other commands: queries e.g
 > > pending breakpoints, exiting after exevution has started; the "commands"
 > > command.  I don't think that they fit well with the MI paradigm: MI expects
 > > MI output.  With queries, GDB takes affirmative action, e.g., creates pending
 > > breakpoints regardless of the value of "show breakpoint pending" and exits
 > > regardless of the value of "show confirm".
 > 
 > I am not sure to understand why are saying this here :-)
 > But for what it is worth, the CLI interpreter allows prompt, yes. I am not
 > sure to understand why it is unfortunatel

I'm trying to say that sub-prompts are really for the command line.

 > > 
 > > Perhaps, for now, GDB could do something similar, i.e., set all the breakpoints
 > > in the breakpoint menu.
 > 
 > Hmmh, no. I think it is important to stay consistent with the format of the
 > "question" asked to the user in the pre 6.7.1 and make that question be
 > followed by a prompt. Otherwise this would break existing front ends.

It's probably important not to break existing use, if possible, but you
appear to be parsing CLI output that hasn't really been designed for use
with front ends.  So I don't think it's a question of being consistent.

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

Re: bug in mi when setting breakpoint

[Dodji Seketeli]

On Tue, Dec 18, 2007 at 12:04:29AM +1300, Nick Roberts wrote:

[...]

>  > 
>  > I am not aware of the exact changeset that changed the behaviour. Could you
>  > point me to it please ? I am only aware of what is in gdb 6.7.1 now, sorry.
>  
> I think it's this one:
> 
> 2007-01-03  Jan Kratochvil  <jan.kratochvil@redhat.com>
> 	    Daniel Jacobowitz  <dan@codesourcery.com>
> 
> 	* Makefile.in (top.o): Update.
> 	* top.c (gdb_readline_wrapper_done, gdb_readline_wrapper_result)
> 	(saved_after_char_processing_hook, gdb_readline_wrapper_line)
> 	(struct gdb_readline_wrapper_cleanup, gdb_readline_wrapper_cleanup):
> 	New.
> 	(gdb_readline_wrapper): Rewrite to use asynchronous readline.
> 
> I see behaviour changed around that time.

Ah okay Nick. This is really valuable info. Thank you.

[...]

>  > 
>  > So in short, MI disallows prompts. It does this by setting the member
>  > prompt_proc_p of the struct interp_procs to a function that always returns
>  > 0. This is done in _initialize_mi_interp() (in gdb/mi/mi-interp.c).
> 
> Why does MI disallow prompts?  Presumably it's for good reason and perhaps
> you're trying to subvert that.  The function gdb_readline_wrapper used to
> be just that: a wrapper for readline.  It didn't used to do things like
> call display_gdb_prompt.  Perhaps its name should be changed.

I am not sure why MI disallows prompts. But I would think it is because MI has
its own prompt, which is the string "(gdb)", and which should appear at the
very end of the MI output. So allowing prompt spitted of other parts of the code
could possibly break the MI output format.

I guess Daniel or Jan could shed some light on this matter ?

[...]

>  > 
>  > I am not sure to understand why are saying this here :-)
>  > But for what it is worth, the CLI interpreter allows prompt, yes. I am not
>  > sure to understand why it is unfortunatel
> 
> I'm trying to say that sub-prompts are really for the command line.

Ah, okay.

> 
>  > > 
>  > > Perhaps, for now, GDB could do something similar, i.e., set all the breakpoints
>  > > in the breakpoint menu.
>  > 
>  > Hmmh, no. I think it is important to stay consistent with the format of the
>  > "question" asked to the user in the pre 6.7.1 and make that question be
>  > followed by a prompt. Otherwise this would break existing front ends.
> 
> It's probably important not to break existing use, if possible, but you
> appear to be parsing CLI output that hasn't really been designed for use
> with front ends.  So I don't think it's a question of being consistent.

Well, all the good graphical debuggers out there let the user choose
between function overloads when the user sets a breakpoint by specifier a
function name. So there must be a way to let the user choose.

I tried to parse the sub-prompt *MI* was spitting because I had not other
choice but parse the only thing the mi interpreter was outputing, even if
that output was not compliant with the documented MI format.

And all the front ends willing to provide that user experience are
forced to do the same.

So the consistency is maybe not a requirement, but I think it would be
really nice to have it.

Cheers,

-- 
Dodji Seketeli
http://www.seketeli.org/dodji

Re: bug in mi when setting breakpoint

[Nick Roberts]

 > Unfortunately CLI also uses sub-prompts for several other commands: queries
 > e.g pending breakpoints, exiting after exevution has started; the "commands"
 > command.  I don't think that they fit well with the MI paradigm: MI expects
 > MI output.  With queries, GDB takes affirmative action, e.g., creates
 > pending breakpoints regardless of the value of "show breakpoint pending" and
 > exits regardless of the value of "show confirm".
 > 
 > Perhaps, for now, GDB could do something similar, i.e., set all the
 > breakpoints in the breakpoint menu.

I see now that this is what Apple do:

  if (i1 > 0)
    {
      int accept_all;
      if (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, i1, funfirstline, accept_all,
                             canonical);
    }
 
How about doing something similar in FSF GDB?

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