Re: [RFC] Print MI prompt on interrupted command

[Andrew Burgess <aburgess@redhat.com>]

* 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
>