* [RFC] Print MI prompt on interrupted command @ 2022-01-06 17:32 Tom Tromey 2022-01-26 18:48 ` Tom Tromey 2022-02-25 13:40 ` Andrew Burgess 0 siblings, 2 replies; 5+ messages in thread From: Tom Tromey @ 2022-01-06 17:32 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Print MI prompt on interrupted command 2022-01-06 17:32 [RFC] Print MI prompt on interrupted command Tom Tromey @ 2022-01-26 18:48 ` Tom Tromey 2022-01-31 19:33 ` Tom Tromey 2022-02-25 13:40 ` Andrew Burgess 1 sibling, 1 reply; 5+ messages in thread From: Tom Tromey @ 2022-01-26 18:48 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Print MI prompt on interrupted command 2022-01-26 18:48 ` Tom Tromey @ 2022-01-31 19:33 ` Tom Tromey 0 siblings, 0 replies; 5+ messages in thread From: Tom Tromey @ 2022-01-31 19:33 UTC (permalink / raw) To: Tom Tromey via Gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Print MI prompt on interrupted command 2022-01-06 17:32 [RFC] Print MI prompt on interrupted command Tom Tromey 2022-01-26 18:48 ` Tom Tromey @ 2022-02-25 13:40 ` Andrew Burgess 2022-02-25 13:59 ` Tom Tromey 1 sibling, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2022-02-25 13:40 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches * 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 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] Print MI prompt on interrupted command 2022-02-25 13:40 ` Andrew Burgess @ 2022-02-25 13:59 ` Tom Tromey 0 siblings, 0 replies; 5+ messages in thread From: Tom Tromey @ 2022-02-25 13:59 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey, gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-02-25 13:59 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-06 17:32 [RFC] Print MI prompt on interrupted command Tom Tromey 2022-01-26 18:48 ` Tom Tromey 2022-01-31 19:33 ` Tom Tromey 2022-02-25 13:40 ` Andrew Burgess 2022-02-25 13:59 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).