When executing commands on an external UI, some commands that should be synchronous are not. Simple test program: int main() { for (;;); } On shell #1: $ ./gdb -nx ~/test (gdb) new-ui mi /dev/pts/X (gdb) On shell #2: $ sleep 9999999 ... output from gdb ... -exec-run ... output from "-exec-run" ... help ... output from "help" ... Here, we should not be able to type "help" and get the output, since the interpreter should be synchronously executing -exec-run. Doing the same with "native" mi works correctly.
Hmm. Running the console interpreter on the extra UI instead does not trigger the problem. I checked now that the MI ui's ui->prompt_state ends up correctly set to PROMPT_BLOCKED. It looks like when we emit MI async events (=library-loaded, etc.), and we go about restoring the previous terminal state, we end up calling target_terminal_ours, which reinstalls MI's input_fd in the event loop...
Specifically, code like this: old_chain = make_cleanup_restore_target_terminal (); target_terminal_ours_for_output (); fprintf_unfiltered (mi->event_channel, "library-loaded"); ... do_cleanups (old_chain); Hmm. If we're going to output to a secondary UI, then we don't really need to mess with save/restore of terminal states, since that only matters for the main UI.
Maybe it's time to pull the add_file_handler/delete_file_handler out of target_terminal_foo somewhere else, and make target_terminal_foo assert that it's only called on the main UI.
I've pushed a fix to the users/palves/PR18077-new-ui-sync-commands branch. It pulss the add_file_handler/delete_file_handler calls out of target_terminal_foo. It does not add the assert, because it turns out it's simpler to leave the target_terminal_foo functions as no-ops for non-main UIs. Could you give it a try? It passes regtesting here, with and without FORCE_SEPARATE_MI_TTY=1. Still needs a new test though.
It works with the few cases I tested by hand.
I might have found a corner case: - start the new MI ui, but don't start the program - in the new MI ui, type -exec-continue You should get "^error,msg="The program is not being run."". At this point, it seems like the fd for that output is not in the event loop because I can't type anything else after that. If I type "start" in the main ui, then the fd eventually gets back in the event loop so it comes back to normal. Basically, it seems like error handling is not quite right.
Created attachment 9409 [details] Test exp
Created attachment 9410 [details] Test c
Here's a first shot at a test. If you have suggestions on how not to make it timing dependent (or about anything else), it would be nice.
gah, i wrote a test today too, but then had to rush out before having a chance to say it here. :-/ from a first quick skim, your looks great . let me see if keeping both tests might be useful. maybe not.
forgot to say that i pushed mine to the branch now.
Ok, I'll take a look at it. I'm sure yours will be nicer! I should take more the habit of saying what I'm working on when there's a risk of overlap. But then I don't want to say "I'm going to write a test", because if it ends up being too complicated and I can't do it, it looks bad ;).
I think the .c for the test is missing in your branch.
(In reply to Pedro Alves from comment #4) > I've pushed a fix to the users/palves/PR18077-new-ui-sync-commands branch. This fixes the problem I had seen in eclipse. Thanks!
@Simon, I think your test looks great, and bettern than mine on a couple aspects. Though I think it's also useful to test "run" as well, because that exercises several other async MI events. So I'm merging both testcases. I think using interrupt as in mine is a little better than a breakpoint as it avoids the need for the 1s sleep. Also, while at it, I made the test use mi_expect_interrupt and mi_send_resuming_command now. I've pushed the current version to the users/palves/PR20418-new-ui-sync-commands branch now (and the deleted the old branch as it referred the wrong bug number...). Haven't looked at the corner case case you mentioned in comment #6 yet.
I see the problem. It's sort of a latent bug now exposed by this bug's patch. On error, we used to end up with the prompt_state set to PROMPT_BLOCKED, but it had no real consequence, because stdin was still happened to be left registered in the event loop. I.e., the prompt_state and stdin event registration got out of sync. Now we "correctly" end up with both PROMPT_BLOCKED and no stdin registered. Fixing...
It's not really latent, actually. Before the patch, MI forgets to output the prompt when -exec-continue fails, and forgets printing the prompt for all subsequent commands until you next run a suceeding sync command. It's a 7.10.1 -> 7.11.1 regression that can be triggered with plain "gdb -i=mi", and that precedes all the new-ui changes, even. I've filed Bug #20431 for it now.
Fix posted here: https://sourceware.org/ml/gdb-patches/2016-08/msg00043.html
The master branch has been updated by Pedro Alves <palves@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=3eb7562a983bab4c768983bcd85708852d171121 commit 3eb7562a983bab4c768983bcd85708852d171121 Author: Pedro Alves <palves@redhat.com> Date: Tue Aug 9 22:45:40 2016 +0100 Fix PR gdb/20418 - Problems with synchronous commands and new-ui When executing commands on a secondary UI running the MI interpreter, some commands that should be synchronous are not. MI incorrectly continues processing input right after the synchronous command is sent, before the target stops. The problem happens when we emit MI async events (=library-loaded, etc.), and we go about restoring the previous terminal state, we end up calling target_terminal_ours, which incorrectly always installs the current UI's input_fd in the event loop... That is, code like this: old_chain = make_cleanup_restore_target_terminal (); target_terminal_ours_for_output (); fprintf_unfiltered (mi->event_channel, "library-loaded"); ... do_cleanups (old_chain); The fix is to move the add_file_handler/delete_file_handler calls out of target_terminal_$foo, making these completely no-ops unless called with the main UI as current UI. gdb/ChangeLog: 2016-08-09 Pedro Alves <palves@redhat.com> PR gdb/20418 * event-top.c (ui_register_input_event_handler) (ui_unregister_input_event_handler): New functions. (async_enable_stdin): Register input in the event loop. (async_disable_stdin): Unregister input from the event loop. (gdb_setup_readline): Register input in the event loop. * infrun.c (check_curr_ui_sync_execution_done): Register input in the event loop. * target.c (target_terminal_inferior): Don't unregister input from the event loop. (target_terminal_ours): Don't register input in the event loop. * target.h (target_terminal_inferior) (target_terminal_ours_for_output, target_terminal_ours): Update comments. * top.h (ui_register_input_event_handler) (ui_unregister_input_event_handler): New declarations. * utils.c (ui_unregister_input_event_handler_cleanup) (prepare_to_handle_input): New functions. (defaulted_query, prompt_for_continue): Use prepare_to_handle_input. gdb/testsuite/ChangeLog: 2016-08-09 Pedro Alves <palves@redhat.com> Simon Marchi <simon.marchi@ericsson.com> PR gdb/20418 * gdb.mi/new-ui-mi-sync.c, gdb.mi/new-ui-mi-sync.exp: New files. * lib/mi-support.exp (mi_expect_interrupt): Remove anchors.
The gdb-7.12-branch branch has been updated by Pedro Alves <palves@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=b24bdfa398beba87b48fffeb3b1f9bcfe7bf924d commit b24bdfa398beba87b48fffeb3b1f9bcfe7bf924d Author: Pedro Alves <palves@redhat.com> Date: Tue Aug 9 22:52:46 2016 +0100 Fix PR gdb/20418 - Problems with synchronous commands and new-ui When executing commands on a secondary UI running the MI interpreter, some commands that should be synchronous are not. MI incorrectly continues processing input right after the synchronous command is sent, before the target stops. The problem happens when we emit MI async events (=library-loaded, etc.), and we go about restoring the previous terminal state, we end up calling target_terminal_ours, which incorrectly always installs the current UI's input_fd in the event loop... That is, code like this: old_chain = make_cleanup_restore_target_terminal (); target_terminal_ours_for_output (); fprintf_unfiltered (mi->event_channel, "library-loaded"); ... do_cleanups (old_chain); The fix is to move the add_file_handler/delete_file_handler calls out of target_terminal_$foo, making these completely no-ops unless called with the main UI as current UI. gdb/ChangeLog: 2016-08-09 Pedro Alves <palves@redhat.com> PR gdb/20418 * event-top.c (ui_register_input_event_handler) (ui_unregister_input_event_handler): New functions. (async_enable_stdin): Register input in the event loop. (async_disable_stdin): Unregister input from the event loop. (gdb_setup_readline): Register input in the event loop. * infrun.c (check_curr_ui_sync_execution_done): Register input in the event loop. * target.c (target_terminal_inferior): Don't unregister input from the event loop. (target_terminal_ours): Don't register input in the event loop. * target.h (target_terminal_inferior) (target_terminal_ours_for_output, target_terminal_ours): Update comments. * top.h (ui_register_input_event_handler) (ui_unregister_input_event_handler): New declarations. * utils.c (ui_unregister_input_event_handler_cleanup) (prepare_to_handle_input): New functions. (defaulted_query, prompt_for_continue): Use prepare_to_handle_input. gdb/testsuite/ChangeLog: 2016-08-09 Pedro Alves <palves@redhat.com> Simon Marchi <simon.marchi@ericsson.com> PR gdb/20418 * gdb.mi/new-ui-mi-sync.c, gdb.mi/new-ui-mi-sync.exp: New files. * lib/mi-support.exp (mi_expect_interrupt): Remove anchors.
Fixed.
The master branch has been updated by Pedro Alves <palves@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=6d61dee599fb314f0561c3bd0dd17ac0cfa05e35 commit 6d61dee599fb314f0561c3bd0dd17ac0cfa05e35 Author: Pedro Alves <palves@redhat.com> Date: Wed Sep 28 17:44:57 2016 -0400 Fix PR 20345 - call_function_by_hand_dummy: Assertion `tp->thread_fsm == &sm->thread_fsm' failed If you run an infcall from the command line, and immediately after run some other command, GDB incorrectly processes the other command before the infcall finishes. The problem is that the fix for PR gdb/20418 (Problems with synchronous commands and new-ui, git 3eb7562a983b) moved the add_file_handler/delete_file_handler calls out of target_terminal_$foo, and missed adjusting the infcall code. gdb/ChangeLog: 2016-09-28 Pedro Alves <palves@redhat.com> * infcall.c (run_inferior_call): Remove input from the event loop while running the infcall. gdb/testsuite/ChangeLog: 2016-09-28 Pedro Alves <palves@redhat.com> * gdb.base/infcall-input.c: New file. * gdb.base/infcall-input.exp: New file.
The gdb-7.12-branch branch has been updated by Pedro Alves <palves@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=629ad95de4e61ac78f49e1abf4592dbc1fe84d96 commit 629ad95de4e61ac78f49e1abf4592dbc1fe84d96 Author: Pedro Alves <palves@redhat.com> Date: Wed Sep 28 17:55:29 2016 -0400 Fix PR 20345 - call_function_by_hand_dummy: Assertion `tp->thread_fsm == &sm->thread_fsm' failed If you run an infcall from the command line, and immediately after run some other command, GDB incorrectly processes the other command before the infcall finishes. The problem is that the fix for PR gdb/20418 (Problems with synchronous commands and new-ui, git 3eb7562a983b) moved the add_file_handler/delete_file_handler calls out of target_terminal_$foo, and missed adjusting the infcall code. gdb/ChangeLog: 2016-09-28 Pedro Alves <palves@redhat.com> * infcall.c (run_inferior_call): Remove input from the event loop while running the infcall. gdb/testsuite/ChangeLog: 2016-09-28 Pedro Alves <palves@redhat.com> * gdb.base/infcall-input.c: New file. * gdb.base/infcall-input.exp: New file.