Bug 20418 - Problems with synchronous commands and new-ui
Summary: Problems with synchronous commands and new-ui
Status: RESOLVED FIXED
Alias: None
Product: gdb
Classification: Unclassified
Component: gdb (show other bugs)
Version: HEAD
: P2 normal
Target Milestone: 7.12
Assignee: Pedro Alves
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-27 20:45 UTC by Simon Marchi
Modified: 2016-09-28 21:57 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed: 2016-07-28 00:00:00


Attachments
Test exp (1.19 KB, text/plain)
2016-07-29 21:25 UTC, Simon Marchi
Details
Test c (518 bytes, text/x-csrc)
2016-07-29 21:25 UTC, Simon Marchi
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Marchi 2016-07-27 20:45:40 UTC
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.
Comment 1 Pedro Alves 2016-07-28 09:35:27 UTC
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...
Comment 2 Pedro Alves 2016-07-28 09:39:20 UTC
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.
Comment 3 Pedro Alves 2016-07-28 09:43:23 UTC
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.
Comment 4 Pedro Alves 2016-07-28 23:44:13 UTC
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.
Comment 5 Simon Marchi 2016-07-29 19:51:55 UTC
It works with the few cases I tested by hand.
Comment 6 Simon Marchi 2016-07-29 20:33:24 UTC
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.
Comment 7 Simon Marchi 2016-07-29 21:25:11 UTC
Created attachment 9409 [details]
Test exp
Comment 8 Simon Marchi 2016-07-29 21:25:35 UTC
Created attachment 9410 [details]
Test c
Comment 9 Simon Marchi 2016-07-29 21:26:09 UTC
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.
Comment 10 Pedro Alves 2016-07-29 21:44:16 UTC
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.
Comment 11 Pedro Alves 2016-07-29 21:44:58 UTC
forgot to say that i pushed mine to the branch now.
Comment 12 Simon Marchi 2016-07-29 21:50:27 UTC
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 ;).
Comment 13 Simon Marchi 2016-07-29 22:10:55 UTC
I think the .c for the test is missing in your branch.
Comment 14 Marc Khouzam 2016-08-01 13:37:28 UTC
(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!
Comment 15 Pedro Alves 2016-08-01 15:32:28 UTC
@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.
Comment 16 Pedro Alves 2016-08-01 16:31:27 UTC
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...
Comment 17 Pedro Alves 2016-08-02 11:59:40 UTC
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.
Comment 18 Pedro Alves 2016-08-02 23:23:17 UTC
Fix posted here:
  https://sourceware.org/ml/gdb-patches/2016-08/msg00043.html
Comment 19 cvs-commit@gcc.gnu.org 2016-08-09 21:53:06 UTC
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.
Comment 20 cvs-commit@gcc.gnu.org 2016-08-09 21:57:21 UTC
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.
Comment 21 Pedro Alves 2016-08-09 22:02:15 UTC
Fixed.
Comment 22 cvs-commit@gcc.gnu.org 2016-09-28 21:55:37 UTC
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.
Comment 23 cvs-commit@gcc.gnu.org 2016-09-28 21:57:50 UTC
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.