[RFC] Print MI prompt on interrupted command

[RFC] Print MI prompt on interrupted command

[Tom Tromey]

Joel noticed that if the remote dies unexpectedly during a command --
you can simulate this by using "continue" and then killing gdbserver
-- then the CLI will print a new prompt, but MI will not.

The output looks something like this:

    | (gdb)
    | cont
    | &"cont\n"
    | ~"Continuing.\n"
    | ^running
    | *running,thread-id="all"
    | (gdb)
    | [... some output from GDB during program startup...]
    | =thread-exited,id="1",group-id="i1"
    | =thread-group-exited,id="i1"
    | &"Remote connection closed\n"

Now, what about that "(gdb)" in the middle?

That prompt comes from this questionable code in
mi-interp.c:mi_on_resume_1:

      /* This is what gdb used to do historically -- printing prompt
	 even if it cannot actually accept any input.  This will be
	 surely removed for MI3, and may be removed even earlier.  */
      if (current_ui->prompt_state == PROMPT_BLOCKED)
	fputs_unfiltered ("(gdb) \n", mi->raw_stdout);

... which seems like something to remove.  But maybe the intent here
is that this prompt is sufficient, and MI clients must be ready to
handle output coming after a prompt.  On the other hand, if this code
*is* removed, then nothing would print a prompt in this scenario.

Anyway, the CLI and the TUI handle emitting the prompt here by hooking
into gdb::observers::command_error, but MI doesn't install an observer
here.

This patch adds the missing observer and arranges to show the MI
prompt.  While this passes both the gdb test suite and the AdaCore
internal test suite (and improves the results on the one particular
target that caused us to notice this), I'm not certain of the
ramifications of this patch, so I thought I'd ask for advice.

Does it seems like a reasonable idea?  It is likely to break existing
clients?  Or perhaps should start_event_loop (that's what notifies the
command_error observer) loop over all UIs and emit a prompt for each?
---
 gdb/mi/mi-interp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index e69ad9aff2d..58ca2fadf1b 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -112,6 +112,16 @@ as_mi_interp (struct interp *interp)
   return dynamic_cast<mi_interp *> (interp);
 }
 
+/* Observer for the command_error notification.  */
+
+static void
+mi_on_command_error ()
+{
+  mi_interp *mi = as_mi_interp (top_level_interpreter ());
+  if (mi != nullptr)
+    display_mi_prompt (mi);
+}
+
 void
 mi_interp::init (bool top_level)
 {
@@ -1369,6 +1379,7 @@ _initialize_mi_interp ()
 					      "mi-interp");
   gdb::observers::command_param_changed.attach (mi_command_param_changed,
 						"mi-interp");
+  gdb::observers::command_error.attach (mi_on_command_error, "mi-interp");
   gdb::observers::memory_changed.attach (mi_memory_changed, "mi-interp");
   gdb::observers::sync_execution_done.attach (mi_on_sync_execution_done,
 					      "mi-interp");
-- 
2.31.1

Re: [RFC] Print MI prompt on interrupted command

[Tom Tromey]

Tom> Does it seems like a reasonable idea?  It is likely to break existing
Tom> clients?  Or perhaps should start_event_loop (that's what notifies the
Tom> command_error observer) loop over all UIs and emit a prompt for each?

I'm leaning toward this latter plan now.

But also I still wonder if this will break existing clients.  Maybe it
has to be an MI3 thing, combined with removing the hack that prints the
prompt earlier.

Tom

Re: [RFC] Print MI prompt on interrupted command

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Does it seems like a reasonable idea?  It is likely to break existing
Tom> clients?  Or perhaps should start_event_loop (that's what notifies the
Tom> command_error observer) loop over all UIs and emit a prompt for each?

>> I'm leaning toward this latter plan now.

I did some experimenting with new-ui and I think that it would be weird
to emit a prompt in this situation.  So now I'm back to thinking the
original patch is probably ok.

Tom

Re: [RFC] Print MI prompt on interrupted command

[Andrew Burgess]

* Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> [2022-01-06 10:32:13 -0700]:

> Joel noticed that if the remote dies unexpectedly during a command --
> you can simulate this by using "continue" and then killing gdbserver
> -- then the CLI will print a new prompt, but MI will not.
> 
> The output looks something like this:
> 
>     | (gdb)
>     | cont
>     | &"cont\n"
>     | ~"Continuing.\n"
>     | ^running
>     | *running,thread-id="all"
>     | (gdb)
>     | [... some output from GDB during program startup...]
>     | =thread-exited,id="1",group-id="i1"
>     | =thread-group-exited,id="i1"
>     | &"Remote connection closed\n"
> 
> Now, what about that "(gdb)" in the middle?
> 
> That prompt comes from this questionable code in
> mi-interp.c:mi_on_resume_1:
> 
>       /* This is what gdb used to do historically -- printing prompt
> 	 even if it cannot actually accept any input.  This will be
> 	 surely removed for MI3, and may be removed even earlier.  */
>       if (current_ui->prompt_state == PROMPT_BLOCKED)
> 	fputs_unfiltered ("(gdb) \n", mi->raw_stdout);
> 
> ... which seems like something to remove.  But maybe the intent here
> is that this prompt is sufficient, and MI clients must be ready to
> handle output coming after a prompt.  On the other hand, if this code
> *is* removed, then nothing would print a prompt in this scenario.
> 
> Anyway, the CLI and the TUI handle emitting the prompt here by hooking
> into gdb::observers::command_error, but MI doesn't install an observer
> here.
> 
> This patch adds the missing observer and arranges to show the MI
> prompt.  While this passes both the gdb test suite and the AdaCore
> internal test suite (and improves the results on the one particular
> target that caused us to notice this), I'm not certain of the
> ramifications of this patch, so I thought I'd ask for advice.
> 
> Does it seems like a reasonable idea?  It is likely to break existing
> clients?  Or perhaps should start_event_loop (that's what notifies the
> command_error observer) loop over all UIs and emit a prompt for each?

I can't speak to whether this would break existing clients or not, for
me, I'd be happy to have this go in, and see how things go.

On the code change itself, again, I'm happy with what you have, it
matches the existing code.  But, I do find the existing pattern rather
odd:

In start_event_loop we have to do:

  current_ui->prompt_state = PROMPT_NEEDED;

then each of the hooks in cli/tui/mi checks to see if the current_ui
is "theirs", and if so, calls display_gdb_prompt.

For me, the line in start_event_loop indicates we are already
hard-coding in a desire for the prompt to be displayed (but in the
current_ui only, so not looping over all UIs) which we expect the
command_error hook to take care of for us.

Why not cut out the command_error hook completely, and in
start_event_loop just do:

  current_ui->prompt_state = PROMPT_NEEDED;
  display_gdb_prompt (nullptr);

If we really wanted to leave the door open for the command_error hook
to do something different, we could have this in start_event_loop:

  current_ui->prompt_state = PROMPT_NEEDED;
  gdb::observers::command_error.notify ();
  if (current_ui->prompt_state == PROMPT_NEEDED)
    display_gdb_prompt (nullptr);

Then a command_error hook can change 'prompt_state' to, e.g. PROMPTED,
and avoid having start_event_loop print a prompt at all.

But, honestly, I don't think I'd bother right now.

Just my thoughts.

Thanks,
Andrew

> ---
>  gdb/mi/mi-interp.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
> index e69ad9aff2d..58ca2fadf1b 100644
> --- a/gdb/mi/mi-interp.c
> +++ b/gdb/mi/mi-interp.c
> @@ -112,6 +112,16 @@ as_mi_interp (struct interp *interp)
>    return dynamic_cast<mi_interp *> (interp);
>  }
>  
> +/* Observer for the command_error notification.  */
> +
> +static void
> +mi_on_command_error ()
> +{
> +  mi_interp *mi = as_mi_interp (top_level_interpreter ());
> +  if (mi != nullptr)
> +    display_mi_prompt (mi);
> +}
> +
>  void
>  mi_interp::init (bool top_level)
>  {
> @@ -1369,6 +1379,7 @@ _initialize_mi_interp ()
>  					      "mi-interp");
>    gdb::observers::command_param_changed.attach (mi_command_param_changed,
>  						"mi-interp");
> +  gdb::observers::command_error.attach (mi_on_command_error, "mi-interp");
>    gdb::observers::memory_changed.attach (mi_memory_changed, "mi-interp");
>    gdb::observers::sync_execution_done.attach (mi_on_sync_execution_done,
>  					      "mi-interp");
> -- 
> 2.31.1
> 

Re: [RFC] Print MI prompt on interrupted command

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> I can't speak to whether this would break existing clients or not, for
Andrew> me, I'd be happy to have this go in, and see how things go.

Thanks for looking at this.

FWIW, yesterday I was looking through bugzilla and found out that this
was reported as a bug a few years ago:

https://sourceware.org/bugzilla/show_bug.cgi?id=23820

Andrew> On the code change itself, again, I'm happy with what you have, it
Andrew> matches the existing code.  But, I do find the existing pattern rather
Andrew> odd:
[...]

Yeah, me too.
I managed to convince myself that checking the other UIs isn't needed.
At the same time, it seems like the sort of code that should be
direct, as opposed to indirectly calling via an observer.

Andrew> But, honestly, I don't think I'd bother right now.

Alright.  I'm going to update the commit message and push it in.

Tom