[RFA] Async mode fixes.

[RFA] Async mode fixes.

[Vladimir Prus]

Presently, gdb's async mode is not very healthy, as follows:

	# of expected passes            10129
	# of unexpected failures        537
	# of unexpected successes       2
	# of expected failures          40
	# of known failures             48
	# of unresolved testcases       301
	# of untested testcases         10
	# of unsupported tests          63

The attached patch improves that, bringing the results to:

	# of expected passes            10722
	# of unexpected failures        39
	# of unexpected successes       2
	# of expected failures          41
	# of known failures             51
	# of untested testcases         11
	# of unsupported tests          64

where only few failures are actually specific to async mode. There
are no regressions in non-async mode, on x86.

The patch has comments whenever appropriate, but some points need explicit
clarification.

To recap, current gdb, when operating with async-enabled target, allows
two modes for all commands that run target. The fully async mode, requested
by adding "&" to the command (say, "next &"), makes the target run, gives
you a prompt and allows you to type further commands. If no "&" is added,
then a sync execution is simulated -- whereby the prompt is suppressed
and GDB does not handle any events on stdin. It is my understanding that
we cannot kill this simulated sync mode because for console GDB,
simulated sync mode is the only way we can allow the debugged program to
read from stdin. (I haven't checked if the interaction with debugged program
indeed works, with Nick's linux async patch). This simulated sync mode 
is implemented by the sync_execution variable, and the 
async_enable_stdin and async_enable_stdout functions.

This issue of simulate sync is not relevant to MI -- for MI, gdb stdin
is generally a pipe, not a terminal, used only for reading 
commands from frontend and gdb has no way to route input to the application.

Another important detail of async mode are continuations -- when we resume
the target, we setup a function to be called when the target eventually
stops. The relevant functions are add_continuation and do_all_continuations.

The important problems with the current code are:

1. For MI mode, when we call a CLI command, in mi_cmd_interpreter_exec,
we set sync_execution to 1 and then set it back to 0.  This is just bogus --
setting that variable without also disabling stdin bring the entire
system in half-consistent state. Further, if the CLI command throws
an exception, we fail to restore sync_execution. I just removed that code.

2. If we ever throw an exception, throw_exception calls do_exec_error_cleanup,
which might call async_enable_stdin -- even though we're not yet done with
an operation that executes a target. I fixed that by moving async_enable_stdin
calls into event loop -- if no exception escapes to the event loop, when
async_enable_stdin is called normally, and otherwise we'll call it when we
get back to event loop.

3. When running breakpoints commands that includes 'continue' we have all
host of issues:
	- execute_command, which is directly used, does not set 
	continuation, and as result, the next event from inferior is not
	handled in a proper way. I've moved continuation setting to
	'execute_command'.
	- bpstat_do_action, after it sees that target has resumed,
	jumps to the top of itself. For sync mode, this works because
	at this point inferior is stopped again. For async mode,
	this obviously does not work -- I've made the function
	to just return.
	- inferior_event_handler tries to print the prompt even
	if the target has resumed

There are some remaining issues:
1. throw_exception calls do_exec_cleanups, and unlike ordinary
cleanup, the exec cleanup chain is not saved by TRY_CATCH. As
result, any exception does all cleanups from main to the current
point -- not from last TRY_CATCH to the current point. This
causes mi-pending.exp to fail.

2. When we execute a user-defined command that includes 'continue'
in the middle, we try to send the next command immediately -- and
in async mode the target is still running. This breaks define.exp

I'm also not 100% happy with the resulting code. In particular,
having exec_cleanup, exec_error_cleanup and continuations, which
all are used to implement calling code when something is done,
just screams for a unified callback-after-command-is-done-or-has-thrown
solution. However, before any big refactoring, we should bring
the number of failures down, and this patch does exactly that.

OK?

- Volodya


	Async mode fixes
	* Makefile.in (infcmd.o, inf-loop.o): Update dependencies.
	* breakpoint.c (bpstat_do_actions): In async mode,
	don't jump to top expecting stop_bpstat to be already
	updated.
	* event-loop.c (start_event_loop): Call async_enable_stdin
	on exception.
	* event-top.c (async_enable_stdin): Do nothing if sync_execution
	is not set.
	(command_handler): Do not setup continuation here.
	(command_line_handler_continuation): Move to...
	* top.c (command_line_handler_continuation): ... here.
	(execute_command): In async mode, register continuation.
	Don't check frame's language in running in async mode.
	* exceptions.c (throw_exception): Don't do exec_error_cleanups.
	* inf-loop.c (complete_execution): Inline into...
	(inferior_event_handler): ... here.  Clear target_executing before
	doing any cleanups.  Don't try to show prompt if the target was
	resumed.
	* infcmd.c (signal_command): Add support for async mode.
	(finish_command): Only add continuation if the target was
	successfully resumed.
	* remote.c (init_async_opts): Register to_get_thread_local_address
	handler.
	* mi/mi-interp.c (mi_cmd_interpreter_exec): Don't mess
	with sync_execution.
	* tui/tui-interp.c (tui_command_loop): Call async_enable_stdin
	on exception.
---
 gdb/Makefile.in      |    5 ++-
 gdb/breakpoint.c     |   29 +++++++++++++++---
 gdb/event-loop.c     |    4 ++
 gdb/event-top.c      |   75 ++++++-----------------------------------------
 gdb/exceptions.c     |    6 ++-
 gdb/inf-loop.c       |   78 ++++++++++++++++++++++++++++++--------------------
 gdb/infcmd.c         |   29 ++++++++++++++++--
 gdb/mi/mi-interp.c   |   24 ++++-----------
 gdb/remote.c         |    2 +
 gdb/top.c            |   78 +++++++++++++++++++++++++++++++++++++++++++++++++-
 gdb/tui/tui-interp.c |    4 ++
 11 files changed, 207 insertions(+), 127 deletions(-)

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 5b6f0d5..fcc8cb1 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -2291,9 +2291,10 @@ infcmd.o: infcmd.c $(defs_h) $(gdb_string_h) $(symtab_h) $(gdbtypes_h) \
 	$(objfiles_h) $(completer_h) $(ui_out_h) $(event_top_h) \
 	$(parser_defs_h) $(regcache_h) $(reggroups_h) $(block_h) \
 	$(solib_h) $(gdb_assert_h) $(observer_h) $(target_descriptions_h) \
-	$(user_regs_h)
+	$(user_regs_h) $(exceptions_h)
 inf-loop.o: inf-loop.c $(defs_h) $(inferior_h) $(target_h) $(event_loop_h) \
-	$(event_top_h) $(inf_loop_h) $(remote_h) $(exceptions_h)
+	$(event_top_h) $(inf_loop_h) $(remote_h) $(exceptions_h) \
+	$(language_h)
 inflow.o: inflow.c $(defs_h) $(frame_h) $(inferior_h) $(command_h) \
 	$(serial_h) $(terminal_h) $(target_h) $(gdbthread_h) $(gdb_string_h) \
 	$(inflow_h) $(gdb_select_h)
diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index a689e3a..78c01ff 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -2094,11 +2094,30 @@ top:
       do_cleanups (this_cmd_tree_chain);
 
       if (breakpoint_proceeded)
-	/* The inferior is proceeded by the command; bomb out now.
-	   The bpstat chain has been blown away by wait_for_inferior.
-	   But since execution has stopped again, there is a new bpstat
-	   to look at, so start over.  */
-	goto top;
+	{
+	  if (target_can_async_p ())
+	  /* If we are in async mode, then the target might
+	     be still running, not stopped at any breakpoint,
+	     so nothing for us to do here -- just return to
+	     the event loop.  */
+	    break;
+	  else
+	    /* In sync mode, when execute_control_command returns
+	       we're already standing on the next breakpoint.
+	       Breakpoint commands for that stop were not run,
+	       since execute_command does not run breakpoint
+	       commands -- only command_line_handler does, but
+	       that one is not involved in execution of breakpoint
+	       commands.  So, we can now execute breakpoint commands.
+	       There's an implicit assumption that we're called with
+	       stop_bpstat, so our parameter is the new bpstat to
+	       handle.  
+	       It should be noted that making execute_command do
+	       bpstat actions is not an option -- in this case we'll
+	       have recursive invocation of bpstat for each breakpoint
+	       with a command, and can easily blow up GDB stack.  */
+	    goto top;
+	}
     }
   do_cleanups (old_chain);
 }
diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index 94f07e9..348ffef 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -411,6 +411,10 @@ start_event_loop (void)
 
       if (gdb_result == 0)
 	{
+	  /* If any exception escaped to here, we better enable
+	     stdin.  Otherwise, any command that calls async_disable_stdin,
+	     and the can throw, will leave stdin inoperable.  */
+	  async_enable_stdin ((void *) 0);
 	  /* FIXME: this should really be a call to a hook that is
 	     interface specific, because interfaces can display the
 	     prompt in their own way. */
diff --git a/gdb/event-top.c b/gdb/event-top.c
index a3cc15c..9bd64f8 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -44,7 +44,6 @@
 
 static void rl_callback_read_char_wrapper (gdb_client_data client_data);
 static void command_line_handler (char *rl);
-static void command_line_handler_continuation (struct continuation_arg *arg);
 static void change_line_handler (void);
 static void change_annotation_level (void);
 static void command_handler (char *command);
@@ -438,13 +437,16 @@ stdin_event_handler (int error, gdb_client_data client_data)
 void
 async_enable_stdin (void *dummy)
 {
-  /* See NOTE in async_disable_stdin() */
-  /* FIXME: cagney/1999-09-27: Call this before clearing
-     sync_execution.  Current target_terminal_ours() implementations
-     check for sync_execution before switching the terminal. */
-  target_terminal_ours ();
-  pop_prompt ();
-  sync_execution = 0;
+  if (sync_execution)
+    {
+      /* See NOTE in async_disable_stdin() */
+      /* FIXME: cagney/1999-09-27: Call this before clearing
+	 sync_execution.  Current target_terminal_ours() implementations
+	 check for sync_execution before switching the terminal. */
+      target_terminal_ours ();
+      pop_prompt ();
+      sync_execution = 0;
+    }
 }
 
 /* Disable reads from stdin (the console) marking the command as
@@ -480,8 +482,6 @@ command_handler (char *command)
 {
   struct cleanup *old_chain;
   int stdin_is_tty = ISATTY (stdin);
-  struct continuation_arg *arg1;
-  struct continuation_arg *arg2;
   long time_at_cmd_start;
 #ifdef HAVE_SBRK
   long space_at_cmd_start = 0;
@@ -517,24 +517,6 @@ command_handler (char *command)
 
   execute_command (command, instream == stdin);
 
-  /* Set things up for this function to be compete later, once the
-     execution has completed, if we are doing an execution command,
-     otherwise, just go ahead and finish. */
-  if (target_can_async_p () && target_executing)
-    {
-      arg1 =
-	(struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
-      arg2 =
-	(struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
-      arg1->next = arg2;
-      arg2->next = NULL;
-      arg1->data.longint = time_at_cmd_start;
-#ifdef HAVE_SBRK
-      arg2->data.longint = space_at_cmd_start;
-#endif
-      add_continuation (command_line_handler_continuation, arg1);
-    }
-
   /* Do any commands attached to breakpoint we stopped at. Only if we
      are always running synchronously. Or if we have just executed a
      command that doesn't start the target. */
@@ -567,43 +549,6 @@ command_handler (char *command)
     }
 }
 
-/* Do any commands attached to breakpoint we stopped at. Only if we
-   are always running synchronously. Or if we have just executed a
-   command that doesn't start the target. */
-void
-command_line_handler_continuation (struct continuation_arg *arg)
-{
-  extern int display_time;
-  extern int display_space;
-
-  long time_at_cmd_start  = arg->data.longint;
-  long space_at_cmd_start = arg->next->data.longint;
-
-  bpstat_do_actions (&stop_bpstat);
-  /*do_cleanups (old_chain); *//*?????FIXME????? */
-
-  if (display_time)
-    {
-      long cmd_time = get_run_time () - time_at_cmd_start;
-
-      printf_unfiltered (_("Command execution time: %ld.%06ld\n"),
-			 cmd_time / 1000000, cmd_time % 1000000);
-    }
-  if (display_space)
-    {
-#ifdef HAVE_SBRK
-      char *lim = (char *) sbrk (0);
-      long space_now = lim - lim_at_start;
-      long space_diff = space_now - space_at_cmd_start;
-
-      printf_unfiltered (_("Space used: %ld (%c%ld for this command)\n"),
-			 space_now,
-			 (space_diff >= 0 ? '+' : '-'),
-			 space_diff);
-#endif
-    }
-}
-
 /* Handle a complete line of input. This is called by the callback
    mechanism within the readline library.  Deal with incomplete commands
    as well, by saving the partial input in a global buffer.  */
diff --git a/gdb/exceptions.c b/gdb/exceptions.c
index ae30367..89d1455 100644
--- a/gdb/exceptions.c
+++ b/gdb/exceptions.c
@@ -221,10 +221,12 @@ throw_exception (struct gdb_exception exception)
 
   disable_current_display ();
   do_cleanups (ALL_CLEANUPS);
+  /* When we implement non-stop mode, this should be redone.  If we get
+     exception in a command pertaining to one thread, or maybe even not
+     involving inferior at all, we should not do exec cleanups for all
+     threads.  */
   if (target_can_async_p () && !target_executing)
     do_exec_cleanups (ALL_CLEANUPS);
-  if (sync_execution)
-    do_exec_error_cleanups (ALL_CLEANUPS);
 
   /* Jump to the containing catch_errors() call, communicating REASON
      to that call via setjmp's return value.  Note that REASON can't
diff --git a/gdb/inf-loop.c b/gdb/inf-loop.c
index b6f8bb8..f789868 100644
--- a/gdb/inf-loop.c
+++ b/gdb/inf-loop.c
@@ -25,9 +25,9 @@
 #include "inf-loop.h"
 #include "remote.h"
 #include "exceptions.h"
+#include "language.h"
 
 static int fetch_inferior_event_wrapper (gdb_client_data client_data);
-static void complete_execution (void);
 
 void
 inferior_event_handler_wrapper (gdb_client_data client_data)
@@ -43,6 +43,7 @@ void
 inferior_event_handler (enum inferior_event_type event_type, 
 			gdb_client_data client_data)
 {
+  int was_sync = 0;
   switch (event_type)
     {
     case INF_ERROR:
@@ -70,11 +71,52 @@ inferior_event_handler (enum inferior_event_type event_type,
       break;
 
     case INF_EXEC_COMPLETE:
-      /* Is there anything left to do for the command issued to
-         complete? */
+
+      /* This is the first thing to do -- so that continuations know that
+	 the target is stopped.  For example, command_line_handler_continuation
+	 will run breakpoint commands, and if we thing that the target is
+	 running, we'll refuse to execute most command.  MI continuation
+	 presently is target_executing to either print or not print *stopped.  */
+      target_executing = 0;
+
+      /* Unregister the inferior from the event loop. This is done so that
+	 when the inferior is not running we don't get distracted by
+	 spurious inferior output.  */
+      if (target_has_execution)
+	target_async (NULL, 0);
+
+      /* Calls to do_exec_error_cleanup below will call async_enable_stdin,
+	 and that resets 'sync_execution'.  However, if we were running
+	 in sync execution mode, we also need to display the prompt.  */
+      was_sync = sync_execution;
+
+      if (was_sync)
+	do_exec_error_cleanups (ALL_CLEANUPS);
+
       do_all_continuations ();
-      /* Reset things after target has stopped for the async commands. */
-      complete_execution ();
+
+      if (current_language != expected_language)
+	{
+	  if (language_mode == language_mode_auto)
+	    {
+	      language_info (1);	/* Print what changed.  */
+	    }
+	}
+
+      /* If the continuation did not start the target again,
+	 prepare for interation with the user.  */
+      if (!target_executing)
+	{              
+	  if (was_sync)
+	    {
+	      display_gdb_prompt (0);
+	    }
+	  else
+	    {
+	      if (exec_done_display_p)
+		printf_unfiltered (_("completed.\n"));
+	    }
+	}
       break;
 
     case INF_EXEC_CONTINUE:
@@ -103,29 +145,3 @@ fetch_inferior_event_wrapper (gdb_client_data client_data)
   fetch_inferior_event (client_data);
   return 1;
 }
-
-/* Reset proper settings after an asynchronous command has finished.
-   If the execution command was in synchronous mode, register stdin
-   with the event loop, and reset the prompt. */
-
-static void
-complete_execution (void)
-{
-  target_executing = 0;
-  
-  /* Unregister the inferior from the event loop. This is done so that
-     when the inferior is not running we don't get distracted by
-     spurious inferior output. */
-  target_async (NULL, 0);
-
-  if (sync_execution)
-    {
-      do_exec_error_cleanups (ALL_CLEANUPS);
-      display_gdb_prompt (0);
-    }
-  else
-    {
-      if (exec_done_display_p)
-	printf_unfiltered (_("completed.\n"));
-    }
-}
diff --git a/gdb/infcmd.c b/gdb/infcmd.c
index 70bf695..2ece41b 100644
--- a/gdb/infcmd.c
+++ b/gdb/infcmd.c
@@ -48,6 +48,7 @@
 #include "observer.h"
 #include "target-descriptions.h"
 #include "user-regs.h"
+#include "exceptions.h"
 
 /* Functions exported for general use, in inferior.h: */
 
@@ -1005,10 +1006,28 @@ static void
 signal_command (char *signum_exp, int from_tty)
 {
   enum target_signal oursig;
+  int async_exec = 0;
 
   dont_repeat ();		/* Too dangerous.  */
   ERROR_NO_INFERIOR;
 
+  /* Find out whether we must run in the background. */
+  if (signum_exp != NULL)
+    async_exec = strip_bg_char (&signum_exp);
+
+  /* If we must run in the background, but the target can't do it,
+     error out. */
+  if (async_exec && !target_can_async_p ())
+    error (_("Asynchronous execution not supported on this target."));
+
+  /* If we are not asked to run in the bg, then prepare to run in the
+     foreground, synchronously. */
+  if (!async_exec && target_can_async_p ())
+    {
+      /* Simulate synchronous execution */
+      async_disable_stdin ();
+    }
+
   if (!signum_exp)
     error_no_arg (_("signal number"));
 
@@ -1321,10 +1340,15 @@ finish_command (char *arg, int from_tty)
       print_stack_frame (get_selected_frame (NULL), 1, LOCATION);
     }
 
+  proceed_to_finish = 1;	/* We want stop_registers, please...  */
+  proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
+
   /* If running asynchronously and the target support asynchronous
      execution, set things up for the rest of the finish command to be
      completed later on, when gdb has detected that the target has
-     stopped, in fetch_inferior_event.  */
+     stopped, in fetch_inferior_event.  
+     Setup it only after proceed, so that if proceed throws, we don't
+     set continuation.  */
   if (target_can_async_p ())
     {
       arg1 =
@@ -1342,9 +1366,6 @@ finish_command (char *arg, int from_tty)
       add_continuation (finish_command_continuation, arg1);
     }
 
-  proceed_to_finish = 1;	/* We want stop_registers, please...  */
-  proceed ((CORE_ADDR) -1, TARGET_SIGNAL_DEFAULT, 0);
-
   /* Do this only if not running asynchronously or if the target
      cannot do async execution.  Otherwise, complete this command when
      the target actually stops, in fetch_inferior_event.  */
diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
index 143a29c..8a3a0a8 100644
--- a/gdb/mi/mi-interp.c
+++ b/gdb/mi/mi-interp.c
@@ -217,23 +217,13 @@ mi_cmd_interpreter_exec (char *command, char **argv, int argc)
 
   for (i = 1; i < argc; i++)
     {
-      /* We had to set sync_execution = 0 for the mi (well really for Project
-         Builder's use of the mi - particularly so interrupting would work.
-         But for console commands to work, we need to initialize it to 1 -
-         since that is what the cli expects - before running the command,
-         and then set it back to 0 when we are done. */
-      sync_execution = 1;
-      {
-	struct gdb_exception e = interp_exec (interp_to_use, argv[i]);
-	if (e.reason < 0)
-	  {
-	    mi_error_message = xstrdup (e.message);
-	    result = MI_CMD_ERROR;
-	    break;
-	  }
-      }
-      do_exec_error_cleanups (ALL_CLEANUPS);
-      sync_execution = 0;
+      struct gdb_exception e = interp_exec (interp_to_use, argv[i]);
+      if (e.reason < 0)
+	{
+	  mi_error_message = xstrdup (e.message);
+	  result = MI_CMD_ERROR;
+	  break;
+	}
     }
 
   mi_remove_notify_hooks ();
diff --git a/gdb/remote.c b/gdb/remote.c
index a5349b4..a0703e2 100644
--- a/gdb/remote.c
+++ b/gdb/remote.c
@@ -7385,6 +7385,8 @@ Specify the serial device it is connected to (e.g. /dev/ttya).";
   remote_async_ops.to_stop = remote_stop;
   remote_async_ops.to_xfer_partial = remote_xfer_partial;
   remote_async_ops.to_rcmd = remote_rcmd;
+  remote_async_ops.to_get_thread_local_address 
+    = remote_get_thread_local_address;
   remote_async_ops.to_stratum = process_stratum;
   remote_async_ops.to_has_all_memory = 1;
   remote_async_ops.to_has_memory = 1;
diff --git a/gdb/top.c b/gdb/top.c
index 9ad2f54..7f31f6b 100644
--- a/gdb/top.c
+++ b/gdb/top.c
@@ -364,6 +364,43 @@ do_chdir_cleanup (void *old_dir)
 }
 #endif
 
+/* Do any commands attached to breakpoint we stopped at. Only if we
+   are always running synchronously. Or if we have just executed a
+   command that doesn't start the target. */
+static void
+command_line_handler_continuation (struct continuation_arg *arg)
+{
+  extern int display_time;
+  extern int display_space;
+
+  long time_at_cmd_start  = arg->data.longint;
+  long space_at_cmd_start = arg->next->data.longint;
+
+  bpstat_do_actions (&stop_bpstat);
+  /*do_cleanups (old_chain); *//*?????FIXME????? */
+
+  if (display_time)
+    {
+      long cmd_time = get_run_time () - time_at_cmd_start;
+
+      printf_unfiltered (_("Command execution time: %ld.%06ld\n"),
+			 cmd_time / 1000000, cmd_time % 1000000);
+    }
+  if (display_space)
+    {
+#ifdef HAVE_SBRK
+      char *lim = (char *) sbrk (0);
+      long space_now = lim - lim_at_start;
+      long space_diff = space_now - space_at_cmd_start;
+
+      printf_unfiltered (_("Space used: %ld (%c%ld for this command)\n"),
+			 space_now,
+			 (space_diff >= 0 ? '+' : '-'),
+			 space_diff);
+#endif
+    }
+}
+
 /* Execute the line P as a command.
    Pass FROM_TTY as second argument to the defining function.  */
 
@@ -374,6 +411,27 @@ execute_command (char *p, int from_tty)
   enum language flang;
   static int warned = 0;
   char *line;
+  struct continuation_arg *arg1;
+  struct continuation_arg *arg2;
+  long time_at_cmd_start;
+#ifdef HAVE_SBRK
+  long space_at_cmd_start = 0;
+#endif
+  extern int display_time;
+  extern int display_space;
+
+  if (target_can_async_p ())
+    {
+      time_at_cmd_start = get_run_time ();
+
+      if (display_space)
+	{
+#ifdef HAVE_SBRK
+	  char *lim = (char *) sbrk (0);
+	  space_at_cmd_start = lim - lim_at_start;
+#endif
+	}
+    }
   
   free_all_values ();
 
@@ -469,7 +527,7 @@ execute_command (char *p, int from_tty)
   /* FIXME:  This should be cacheing the frame and only running when
      the frame changes.  */
 
-  if (target_has_stack)
+  if (!target_executing && target_has_stack)
     {
       flang = get_frame_language ();
       if (!warned
@@ -480,6 +538,24 @@ execute_command (char *p, int from_tty)
 	  warned = 1;
 	}
     }
+
+  /* Set things up for this function to be compete later, once the
+     execution has completed, if we are doing an execution command,
+     otherwise, just go ahead and finish. */
+  if (target_can_async_p () && target_executing)
+    {
+      arg1 =
+	(struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
+      arg2 =
+	(struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
+      arg1->next = arg2;
+      arg2->next = NULL;     
+      arg1->data.longint = time_at_cmd_start;
+#ifdef HAVE_SBRK
+      arg2->data.longint = space_at_cmd_start;
+#endif
+      add_continuation (command_line_handler_continuation, arg1);
+    }
 }
 
 /* Read commands from `instream' and execute them
diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
index 9abecd4..dcee46f 100644
--- a/gdb/tui/tui-interp.c
+++ b/gdb/tui/tui-interp.c
@@ -164,6 +164,10 @@ tui_command_loop (void *data)
       
       if (result == 0)
 	{
+	  /* If any exception escaped to here, we better enable
+	     stdin.  Otherwise, any command that calls async_disable_stdin,
+	     and the can throw, will leave stdin inoperable.  */
+	  async_enable_stdin ((void *) 0);
 	  /* FIXME: this should really be a call to a hook that is
 	     interface specific, because interfaces can display the
 	     prompt in their own way.  */
-- 
1.5.3.5

Re: [RFA] Async mode fixes.

[Nick Roberts]

 > Presently, gdb's async mode is not very healthy, as follows:
 > 
 > 	# of expected passes            10129
 > 	# of unexpected failures        537
 > 	# of unexpected successes       2
 > 	# of expected failures          40
 > 	# of known failures             48
 > 	# of unresolved testcases       301
 > 	# of untested testcases         10
 > 	# of unsupported tests          63
 > 
 > The attached patch improves that, bringing the results to:
 > 
 > 	# of expected passes            10722
 > 	# of unexpected failures        39
 > 	# of unexpected successes       2
 > 	# of expected failures          41
 > 	# of known failures             51
 > 	# of untested testcases         11
 > 	# of unsupported tests          64
 > 
 > where only few failures are actually specific to async mode. There
 > are no regressions in non-async mode, on x86.

Like my patch, IMO this one looks a bit like a dog's breakfast.  It's got some
good ideas, though, and has certainly increased my understanding of the
asynchronous code.  Perhaps a combination of the two patches would create
something useful.

It may reduce the failures, but I suspect that's partly because it's not really
running asynchronously.  I don't understand how it really could without adding
another file handler for inferior events.  That doesn't mean it can't, just
that I don't understand how it could.

 > The patch has comments whenever appropriate, but some points need explicit
 > clarification.
 > 
 > To recap, current gdb, when operating with async-enabled target, allows
 > two modes for all commands that run target. The fully async mode, requested
 > by adding "&" to the command (say, "next &"), makes the target run, gives
 > you a prompt and allows you to type further commands. If no "&" is added,
 > then a sync execution is simulated -- whereby the prompt is suppressed
 > and GDB does not handle any events on stdin. It is my understanding that
 > we cannot kill this simulated sync mode because for console GDB,
 > simulated sync mode is the only way we can allow the debugged program to
 > read from stdin.

I think sync mode is also needed for command files.  Also if you look at
top.c, only a few commands, e.g. pwd, can execute while the target is running.

 >               (I haven't checked if the interaction with debugged program
 > indeed works, with Nick's linux async patch). This simulated sync mode 
 > is implemented by the sync_execution variable, and the 
 > async_enable_stdin and async_enable_stdout functions.
 >
 > This issue of simulate sync is not relevant to MI -- for MI, gdb stdin
 > is generally a pipe, not a terminal, used only for reading 
 > commands from frontend and gdb has no way to route input to the application.

Emacs uses a tty for MI when one is available.

 > Another important detail of async mode are continuations -- when we resume
 > the target, we setup a function to be called when the target eventually
 > stops. The relevant functions are add_continuation and do_all_continuations.

There's another continuation in the mi directory:

  else if (target_can_async_p ())
    {
      add_continuation (mi_interpreter_exec_continuation, NULL);
    }

You don't appear to have changed that.  This is where I have issue with the
current `async' mode: it doesn't appear to use this continuation.

 > The important problems with the current code are:
 > 
 > 1. For MI mode, when we call a CLI command, in mi_cmd_interpreter_exec,
 > we set sync_execution to 1 and then set it back to 0.  This is just bogus --
 > setting that variable without also disabling stdin bring the entire
 > system in half-consistent state. Further, if the CLI command throws
 > an exception, we fail to restore sync_execution. I just removed that code.

I think that _may_ be there because mi_cmd_interpreter_exec can process more
than one CLI command at a time:

-interpreter-exec console pwd dir
~"Working directory /home/nickrob.\n"
~"Source directories searched: $cdir:$cwd\n"
^done
(gdb) 

Perhaps that should be changed to just allow one command.

> ...

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Async mode fixes.

[Daniel Jacobowitz]

Hi Nick, thanks for looking at this.  I was hoping you would.

On Sat, Mar 08, 2008 at 11:18:47AM +1300, Nick Roberts wrote:
> Like my patch, IMO this one looks a bit like a dog's breakfast.  It's got some
> good ideas, though, and has certainly increased my understanding of the
> asynchronous code.  Perhaps a combination of the two patches would create
> something useful.

As I understand it, it already is combined - I know Vladimir started
with a copy of your most recent async patch.  I haven't looked at his
patch yet but it may need to be broken down into some pieces.
 
> It may reduce the failures, but I suspect that's partly because it's not really
> running asynchronously.  I don't understand how it really could without adding
> another file handler for inferior events.  That doesn't mean it can't, just
> that I don't understand how it could.

Vladimir, what target were you using to test - was it "target async"?
If so, there's already a file handler relevant to inferior events;
"target async" uses the remote protocol to talk to gdbserver, so it
receives events every time there's a byte on the TCP socket.

> I think sync mode is also needed for command files.

Shouldn't you be able to use async operations from a command file?

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Async mode fixes.

[Vladimir Prus]

On Saturday 08 March 2008 01:32:47 Daniel Jacobowitz wrote:
> Hi Nick, thanks for looking at this.  I was hoping you would.
> 
> On Sat, Mar 08, 2008 at 11:18:47AM +1300, Nick Roberts wrote:
> > Like my patch, IMO this one looks a bit like a dog's breakfast.  It's got some
> > good ideas, though, and has certainly increased my understanding of the
> > asynchronous code.  Perhaps a combination of the two patches would create
> > something useful.
> 
> As I understand it, it already is combined - I know Vladimir started
> with a copy of your most recent async patch.

Yes. Nick patch has two bits:
1. Linux native async mode (which Pedro is looking at as we speak)
2. Core async mode changes.

My patches addresses (2), and I've used both Nick's patch and Apple
branch source as reference.

> I haven't looked at his 
> patch yet but it may need to be broken down into some pieces.

I surely can break it into 10 separate patches, *if* that will make it
more easy to review for you.

> > It may reduce the failures, but I suspect that's partly because it's not really
> > running asynchronously.  I don't understand how it really could without adding
> > another file handler for inferior events.  That doesn't mean it can't, just
> > that I don't understand how it could.
> 
> Vladimir, what target were you using to test - was it "target async"?

Yes, it's target async, and it's really running asynchronously.

- Volodya

Re: [RFA] Async mode fixes.

[Vladimir Prus]

On Saturday 08 March 2008 01:18:47 Nick Roberts wrote:
>  > Presently, gdb's async mode is not very healthy, as follows:
>  > 
>  > 	# of expected passes            10129
>  > 	# of unexpected failures        537
>  > 	# of unexpected successes       2
>  > 	# of expected failures          40
>  > 	# of known failures             48
>  > 	# of unresolved testcases       301
>  > 	# of untested testcases         10
>  > 	# of unsupported tests          63
>  > 
>  > The attached patch improves that, bringing the results to:
>  > 
>  > 	# of expected passes            10722
>  > 	# of unexpected failures        39
>  > 	# of unexpected successes       2
>  > 	# of expected failures          41
>  > 	# of known failures             51
>  > 	# of untested testcases         11
>  > 	# of unsupported tests          64
>  > 
>  > where only few failures are actually specific to async mode. There
>  > are no regressions in non-async mode, on x86.
> 
> Like my patch, IMO this one looks a bit like a dog's breakfast.  

I have only a cat, so I'm afraid I don't know what a dog's breakfast look like :-/

> It's got some 
> good ideas, though, and has certainly increased my understanding of the
> asynchronous code.  Perhaps a combination of the two patches would create
> something useful.

You patch has both general async changes, and Linux native async support.
I've looked at your general async changes, but as you might notice, the
changes in my patch are more extensive -- because I wanted to fix all
failures. The Linux part is independent, and better be addressed separately --
otherwise, we won't be able to easily say if a test failure is a problem with
general code, or with Linux support. 
 
> It may reduce the failures, but I suspect that's partly because it's not really
> running asynchronously.  I don't understand how it really could without adding
> another file handler for inferior events.  That doesn't mean it can't, just
> that I don't understand how it could.

Well, "target async" exists and has existed for a long time. I presume that when
it was added, it fully worked. However, as it was not routinely tested, it has
regressed to its current state.

It surely *is* running asynchronously -- say, when I do -exec-continue, then
gdb does indeed respond to -exec-interrupt. The corresponding event handlers
indeed run, and the processing goes via async code paths. 

>  > The patch has comments whenever appropriate, but some points need explicit
>  > clarification.
>  > 
>  > To recap, current gdb, when operating with async-enabled target, allows
>  > two modes for all commands that run target. The fully async mode, requested
>  > by adding "&" to the command (say, "next &"), makes the target run, gives
>  > you a prompt and allows you to type further commands. If no "&" is added,
>  > then a sync execution is simulated -- whereby the prompt is suppressed
>  > and GDB does not handle any events on stdin. It is my understanding that
>  > we cannot kill this simulated sync mode because for console GDB,
>  > simulated sync mode is the only way we can allow the debugged program to
>  > read from stdin.
> 
> I think sync mode is also needed for command files. 

Possibly -- except there's no prompt in the middle of execution of command file
anyway.

> Also if you look at 
> top.c, only a few commands, e.g. pwd, can execute while the target is running.

Yes, and in MI, only "exec-interrupt" is allowed. Anyway, clearly, async mode
where one can use a total of 5 commands, and not very interesting ones,
is not very attractive, so that list will will grow.

>  >               (I haven't checked if the interaction with debugged program
>  > indeed works, with Nick's linux async patch). This simulated sync mode 
>  > is implemented by the sync_execution variable, and the 
>  > async_enable_stdin and async_enable_stdout functions.
>  >
>  > This issue of simulate sync is not relevant to MI -- for MI, gdb stdin
>  > is generally a pipe, not a terminal, used only for reading 
>  > commands from frontend and gdb has no way to route input to the application.
> 
> Emacs uses a tty for MI when one is available.

Can you clarify? Why don't you use external tty (set with the 'tty' command)?

>  > Another important detail of async mode are continuations -- when we resume
>  > the target, we setup a function to be called when the target eventually
>  > stops. The relevant functions are add_continuation and do_all_continuations.
> 
> There's another continuation in the mi directory:

Err, I'm giving the functions to set and execute continuations, I'm not 
trying to list all existing continuations.

>   else if (target_can_async_p ())
>     {
>       add_continuation (mi_interpreter_exec_continuation, NULL);
>     }
> 
> You don't appear to have changed that.  This is where I have issue with the
> current `async' mode: it doesn't appear to use this continuation.

Are you sure? I get the following with my patch:

	(gdb)
	-interpreter-exec console continue
	~"Continuing.\n"
	^running
	*stopped,reason="breakpoint-hit",bkptno="1",thread-id="1",.....

That continuation is called, even in pristine GDB. However, in pristine GDB,
inferior_event_handler first calls do_all_continuation and then calls
complete_execution, which sets target_executing to 0. So, 
mi_interpreter_exec_continuation thinks that the target is still executing, and
does not print anything.

In my patch, target_executing is set to 0 right away, and we get the above output.

> 
>  > The important problems with the current code are:
>  > 
>  > 1. For MI mode, when we call a CLI command, in mi_cmd_interpreter_exec,
>  > we set sync_execution to 1 and then set it back to 0.  This is just bogus --
>  > setting that variable without also disabling stdin bring the entire
>  > system in half-consistent state. Further, if the CLI command throws
>  > an exception, we fail to restore sync_execution. I just removed that code.
> 
> I think that _may_ be there because mi_cmd_interpreter_exec can process more
> than one CLI command at a time:
> 
> -interpreter-exec console pwd dir
> ~"Working directory /home/nickrob.\n"
> ~"Source directories searched: $cdir:$cwd\n"
> ^done
> (gdb) 
> 
> Perhaps that should be changed to just allow one command.

Oh, I think we indeed better allow just one command. However, even if
we wanted to allow several commands, setting sync_executing would be wrong:
1. It should only be set if stdin is really disabled -- otherwise 
async_enable_stdin can just crash, thinking that stdin is disbled and trying
to pop prompt that was never pushed.
2. For a command that actually runs the target, executing of several command
is broken already. Say, "-interpreter-exec console next next" will
try to execute second next before we got an event indicating that the first
next is done. sync_execution does not cause us to wait for event in
any way, it merely makes it impossible for user to type next command.

- Volodya

Re: [RFA] Async mode fixes.

[Nick Roberts]

 > Well, "target async" exists and has existed for a long time. I presume that
 > when it was added, it fully worked. However, as it was not routinely tested,
 > it has regressed to its current state.

I think the asynchronous mode wasn't completed and that it only ever worked for
remote targets.  I'm starting to understand your patch now and see that your
changes just make the asynchronous mode of remote targets work - and that's a
sensible place to start.

 >...
 > > You don't appear to have changed that.  This is where I have issue with the
 > > current `async' mode: it doesn't appear to use this continuation.
 > 
 > Are you sure? I get the following with my patch:
 > 
 > 	(gdb)
 > 	-interpreter-exec console continue
 > 	~"Continuing.\n"
 > 	^running
 > 	*stopped,reason="breakpoint-hit",bkptno="1",thread-id="1",.....

I've not tested your patch, just looked at it.  This doesn't work in the
current code but I see now, as you say, that it's because target_executing is
not reset to 0, not because the continuation isn't called.

You say:

> Yes. Nick patch has two bits:
> 1. Linux native async mode (which Pedro is looking at as we speak)
> 2. Core async mode changes.

It really has three bits:

1. Core async mode changes.
2. Exec async mode changes.
3. Linux native async mode.

I think the command "target async" is a bit misleading and it should really be
called "target remote-async" (I'm not really suggesting changing it).

My changes aren't just for Linux, but an exec target.  I mean native debugging
(I think) with just one implementation - linux.  It sounds like I'm inflating
what I've done but other native targets can presumably be adapted to make use
of the changes in inf-ptrace.c, exec.c etc.

If all targets could run asynchronously, then it seems that that mode should
be specified at startup:

   gdb --async myprog

in which case

  (gdb) target remote host:2222

would do what

  (gdb) target async host:2222

currently does and

  (gdb) run

would run the native applications asynchronously.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Async mode fixes.

[Daniel Jacobowitz]

On Sun, Mar 09, 2008 at 09:36:15AM +1300, Nick Roberts wrote:
> I think the command "target async" is a bit misleading and it should really be
> called "target remote-async" (I'm not really suggesting changing it).

I agree that the name is misleading.  Also, it's not documented, and
we can be pretty sure that no one uses it.  So I think we can rename
it if we want - and what I really want is to get rid of it entirely.

> My changes aren't just for Linux, but an exec target.  I mean native debugging
> (I think) with just one implementation - linux.  It sounds like I'm inflating
> what I've done but other native targets can presumably be adapted to make use
> of the changes in inf-ptrace.c, exec.c etc.

I think you mean "child" target?  target exec just reads an exec file
on disk.

> If all targets could run asynchronously, then it seems that that mode should
> be specified at startup:
> 
>    gdb --async myprog

Can it just be the default, once we get it working?  If I understand
correctly, the async-ness of the target should not affect the CLI at
all.  It lets you use "continue&" in addition to "continue", but
existing commands should keep working as before.

Async mode seems inherently superior.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Async mode fixes.

[Nick Roberts]

 > > My changes aren't just for Linux, but an exec target.  I mean native
 > > debugging (I think) with just one implementation - linux.  It sounds like
 > > I'm inflating what I've done but other native targets can presumably be
 > > adapted to make use of the changes in inf-ptrace.c, exec.c etc.
 > 
 > I think you mean "child" target?  target exec just reads an exec file
 > on disk.

Well, my changes are in exec.c to exec_ops methods, not to target methods in
inf-child.c.  Then ISTR these get inherited by target methods in inf-ptrace.c

Are you saying that these changes should really be in inf-child.c or just that
I've misunderstood the concept of an exec target?

 > > If all targets could run asynchronously, then it seems that that mode should
 > > be specified at startup:
 > > 
 > >    gdb --async myprog
 > 
 > Can it just be the default, once we get it working?  If I understand
 > correctly, the async-ness of the target should not affect the CLI at
 > all.  It lets you use "continue&" in addition to "continue", but
 > existing commands should keep working as before.
 > 
 > Async mode seems inherently superior.

You're probably right.  I just don't really have the bigger picture to be able
to say that.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Async mode fixes.

[Daniel Jacobowitz]

On Sun, Mar 09, 2008 at 12:08:31PM +1300, Nick Roberts wrote:
>  > > My changes aren't just for Linux, but an exec target.  I mean native
>  > > debugging (I think) with just one implementation - linux.  It sounds like
>  > > I'm inflating what I've done but other native targets can presumably be
>  > > adapted to make use of the changes in inf-ptrace.c, exec.c etc.
>  > 
>  > I think you mean "child" target?  target exec just reads an exec file
>  > on disk.
> 
> Well, my changes are in exec.c to exec_ops methods, not to target methods in
> inf-child.c.  Then ISTR these get inherited by target methods in inf-ptrace.c
> 
> Are you saying that these changes should really be in inf-child.c or just that
> I've misunderstood the concept of an exec target?

An exec target is the bit that owns the executable file.  Then other
targets live on top of it (on the "target stack") - target remote,
target sim, target child (which is just "run").  Putting things in the
exec target and not overriding them in any other target means that
they will work whenever there is an executable file, regardless of
target, and not work if we are attached without an executable file.

So if they go in exec.c, perhaps they shouldn't be in the target
vector at all.  Or perhaps they should be the default versions in
target.c if no target overrides them (like
default_region_ok_for_hw_watchpoint).

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Async mode fixes.

[Vladimir Prus]

On Saturday 08 March 2008 23:36:15 Nick Roberts wrote:

> I think the command "target async" is a bit misleading and it should really be
> called "target remote-async" (I'm not really suggesting changing it).

Well, ideally, it would be called 'target remote' ;-) Having two different modes,
where one mode has strictly more features than the other, is a way to have
one of the modes bit-rot.

> My changes aren't just for Linux, but an exec target.  I mean native debugging
> (I think) with just one implementation - linux.  It sounds like I'm inflating
> what I've done but other native targets can presumably be adapted to make use
> of the changes in inf-ptrace.c, exec.c etc.
> 
> If all targets could run asynchronously, then it seems that that mode should
> be specified at startup:
> 
>    gdb --async myprog
> 
> in which case
> 
>   (gdb) target remote host:2222
> 
> would do what
> 
>   (gdb) target async host:2222
> 
> currently does and
> 
>   (gdb) run
> 
> would run the native applications asynchronously.

Likewise, I think it's better to always use async mode,
if the target supports it. We're not at the point when we can do
it -- even with my patch, 'target async' has a couple of extra
failures relatively to 'target remote', and I never tried getting
test results for linux native -- but I think having async mode
always on should be end goal.

- Volodya

 

Re: [RFA] Async mode fixes.

[Nick Roberts]

 > Likewise, I think it's better to always use async mode,
 > if the target supports it. We're not at the point when we can do
 > it -- even with my patch, 'target async' has a couple of extra
 > failures relatively to 'target remote', and I never tried getting
 > test results for linux native -- but I think having async mode
 > always on should be end goal.

The patch isn't perfect, e.g., you say:

  I get the following with my patch:

	(gdb)
	-interpreter-exec console continue
	~"Continuing.\n"
	^running
	*stopped,reason="breakpoint-hit",bkptno="1",thread-id="1",.....

but that should be:

	(gdb)
	-interpreter-exec console continue
	~"Continuing.\n"
	^running
	(gdb)
	*stopped,reason="breakpoint-hit",bkptno="1",thread-id="1",.....

because in mi_execute_command, args.action == EXECUTE_COMMAND_SUPRESS_PROMPT
when it should be EXECUTE_COMMAND_DISPLAY_PROMPT.  This in turn is because
sync_execution is 1 when it should be 0.  Note that in Apple's code
sync_execution doesn't get toggled by async_enable/disable_stdin,

but, if Daniel is prepared to approve it, I would suggest committing it on
the trunk as there is plenty of time to polish it and it breaks things into
manageable chunks.

Then, at some stage, I could repost my patch as a much smaller diff and with
further improvements.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Async mode fixes.

[Vladimir Prus]

On Monday 10 March 2008 12:11:58 Nick Roberts wrote:
>  > Likewise, I think it's better to always use async mode,
>  > if the target supports it. We're not at the point when we can do
>  > it -- even with my patch, 'target async' has a couple of extra
>  > failures relatively to 'target remote', and I never tried getting
>  > test results for linux native -- but I think having async mode
>  > always on should be end goal.
> 
> The patch isn't perfect, e.g., you say:
> 
>   I get the following with my patch:
> 
> 	(gdb)
> 	-interpreter-exec console continue
> 	~"Continuing.\n"
> 	^running
> 	*stopped,reason="breakpoint-hit",bkptno="1",thread-id="1",.....
> 
> but that should be:
> 
> 	(gdb)
> 	-interpreter-exec console continue
> 	~"Continuing.\n"
> 	^running
> 	(gdb)
> 	*stopped,reason="breakpoint-hit",bkptno="1",thread-id="1",.....
> 
> because in mi_execute_command, args.action == EXECUTE_COMMAND_SUPRESS_PROMPT
> when it should be EXECUTE_COMMAND_DISPLAY_PROMPT.  This in turn is because
> sync_execution is 1 when it should be 0.  Note that in Apple's code
> sync_execution doesn't get toggled by async_enable/disable_stdin,

You mean this bit of code:

     else if (sync_execution)
	{
	  /* Don't print the prompt. We are executing the target in
	     synchronous mode.  */
	  args->action = EXECUTE_COMMAND_SUPRESS_PROMPT;
	  return;
	}

? I actually planned to remove it without further ado, after this async
patch goes in. My reasoning is that the current behaviour is just bizarre. For sync
target, any exec commands results in 

	^running
	(gdb) 

whereas for async target, due to above fragment, you can sometimes not get
a prompt -- and this is backward.

Of course, this code only deals with printing the prompt, whereas we still have the
bigger issue -- namely that if we execute "continue" using -interpreter-exec,
then GDB does not accept the input while continue runs, and -exec-interrupt
does not work. Apple branch fixes that by not setting sync_execution inside
async_disable_stdin, but I find the fix suspect -- if we give up
terminal to inferior and then try to read something, I'm not sure it will
work. It does work with "target async", but target async does not actually
mess with terminal.

One solution is probably to never disable stdin when an external tty
is used for program output. Or to never disable stdin when top-level
interpreter is MI. I must admit I don't know the right solution, yet.

- Volodya

Re: [RFA] Async mode fixes.

[Nick Roberts]

 > You mean this bit of code:
 > 
 >      else if (sync_execution)
 > 	{
 > 	  /* Don't print the prompt. We are executing the target in
 > 	     synchronous mode.  */
 > 	  args->action = EXECUTE_COMMAND_SUPRESS_PROMPT;
 > 	  return;
 > 	}
 > 
 > ?

Yes.

 > I actually planned to remove it without further ado, after this async
 > patch goes in. My reasoning is that the current behaviour is just bizarre. For sync
 > target, any exec commands results in 
 > 
 > 	^running
 > 	(gdb) 
 > 
 > whereas for async target, due to above fragment, you can sometimes not get
 > a prompt -- and this is backward.

That would seem to work.

 > Of course, this code only deals with printing the prompt, whereas we still
 > have the bigger issue -- namely that if we execute "continue" using
 > -interpreter-exec, then GDB does not accept the input while continue runs,
 > and -exec-interrupt does not work.

I don't think this is right but I don't want to work on your patch while it's
not part of GDB.

 >                                    Apple branch fixes that by not setting
 > sync_execution inside async_disable_stdin, but I find the fix suspect -- if
 > we give up terminal to inferior and then try to read something, I'm not sure
 > it will work. It does work with "target async", but target async does not
 > actually mess with terminal.
 > 
 > One solution is probably to never disable stdin when an external tty
 > is used for program output. Or to never disable stdin when top-level
 > interpreter is MI. I must admit I don't know the right solution, yet.
 > 
 > - Volodya

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: [RFA] Async mode fixes.

[Daniel Jacobowitz]

On Wed, Mar 05, 2008 at 10:27:28AM +0300, Vladimir Prus wrote:
> +	  /* If any exception escaped to here, we better enable
> +	     stdin.  Otherwise, any command that calls async_disable_stdin,
> +	     and the can throw, will leave stdin inoperable.  */

and can throw?

>      case INF_EXEC_COMPLETE:
> -      /* Is there anything left to do for the command issued to
> -         complete? */
> +
> +      /* This is the first thing to do -- so that continuations know that
> +	 the target is stopped.  For example, command_line_handler_continuation
> +	 will run breakpoint commands, and if we thing that the target is
> +	 running, we'll refuse to execute most command.  MI continuation
> +	 presently is target_executing to either print or not print *stopped.  */
> +      target_executing = 0;

"if we think", "most commands".  "is" -> "uses" in the last line, right?

> @@ -1005,10 +1006,28 @@ static void
>  signal_command (char *signum_exp, int from_tty)

Two spaces after periods in this function :-)

> +/* Do any commands attached to breakpoint we stopped at. Only if we
> +   are always running synchronously. Or if we have just executed a
> +   command that doesn't start the target. */

Here too.

> +static void
> +command_line_handler_continuation (struct continuation_arg *arg)
> +{
> +  extern int display_time;
> +  extern int display_space;
> +
> +  long time_at_cmd_start  = arg->data.longint;
> +  long space_at_cmd_start = arg->next->data.longint;
> +
> +  bpstat_do_actions (&stop_bpstat);
> +  /*do_cleanups (old_chain); *//*?????FIXME????? */

Clean up this, please.

Other than that, this can go in now.  Thanks.  Between you and Nick
and Pedro I'm sure we'll have async mode working again soon.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [RFA] Async mode fixes.

[Vladimir Prus]

On Friday 14 March 2008 21:26:29 Daniel Jacobowitz wrote:
> On Wed, Mar 05, 2008 at 10:27:28AM +0300, Vladimir Prus wrote:
> > +	  /* If any exception escaped to here, we better enable
> > +	     stdin.  Otherwise, any command that calls async_disable_stdin,
> > +	     and the can throw, will leave stdin inoperable.  */
> 
> and can throw?

"and then throws", actually.
> 
> >      case INF_EXEC_COMPLETE:
> > -      /* Is there anything left to do for the command issued to
> > -         complete? */
> > +
> > +      /* This is the first thing to do -- so that continuations know that
> > +	 the target is stopped.  For example, command_line_handler_continuation
> > +	 will run breakpoint commands, and if we thing that the target is
> > +	 running, we'll refuse to execute most command.  MI continuation
> > +	 presently is target_executing to either print or not print *stopped.  */
> > +      target_executing = 0;
> 
> "if we think", "most commands".  "is" -> "uses" in the last line, right?

right.


> 
> > @@ -1005,10 +1006,28 @@ static void
> >  signal_command (char *signum_exp, int from_tty)
> 
> Two spaces after periods in this function :-)
> 
> > +/* Do any commands attached to breakpoint we stopped at. Only if we
> > +   are always running synchronously. Or if we have just executed a
> > +   command that doesn't start the target. */
> 
> Here too.
> 
> > +static void
> > +command_line_handler_continuation (struct continuation_arg *arg)
> > +{
> > +  extern int display_time;
> > +  extern int display_space;
> > +
> > +  long time_at_cmd_start  = arg->data.longint;
> > +  long space_at_cmd_start = arg->next->data.longint;
> > +
> > +  bpstat_do_actions (&stop_bpstat);
> > +  /*do_cleanups (old_chain); *//*?????FIXME????? */
> 
> Clean up this, please.

Oh, this is not mine, I've just moved command_line_handler_continuation
to a different module. Anyway, some of later patches of mine removes
that line as not necessary, and I might as well do it now.

> 
> Other than that, this can go in now.  Thanks.  Between you and Nick
> and Pedro I'm sure we'll have async mode working again soon.

Thanks, checked in.

- Volodya
 

[commit] Fix compile error (Re: [RFA] Async mode fixes.)

[Ulrich Weigand]

Vladimir Prus wrote:

> +  long time_at_cmd_start;
[snip]
> +  if (target_can_async_p ())
> +    {
> +      time_at_cmd_start = get_run_time ();
[snip]
> +  if (target_can_async_p () && target_executing)
> +    {
> +      arg1 =
> +	(struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
> +      arg2 =
> +	(struct continuation_arg *) xmalloc (sizeof (struct continuation_arg));
> +      arg1->next = arg2;
> +      arg2->next = NULL;     
> +      arg1->data.longint = time_at_cmd_start;


this breaks compilation with:

/home/uweigand/fsf/gdb-head/gdb/top.c: In function `execute_command':
/home/uweigand/fsf/gdb-head/gdb/top.c:415: warning: 'time_at_cmd_start' might be used uninitialized in this function

and the compiler error seems somewhat justified as the compiler cannot
know that target_can_async_p () always returns the same value ...

Fixed by adding a dummy initializer, as already done for space_at_cmd_start.

Committed to mainline.

Bye,
Ulrich


ChangeLog:

	* top.c (execute_command): Fix uninitialized variable error.


Index: gdb/top.c
===================================================================
RCS file: /cvs/src/src/gdb/top.c,v
retrieving revision 1.133
diff -u -p -r1.133 top.c
--- gdb/top.c	14 Mar 2008 18:57:43 -0000	1.133
+++ gdb/top.c	17 Mar 2008 13:54:19 -0000
@@ -412,7 +412,7 @@ execute_command (char *p, int from_tty)
   char *line;
   struct continuation_arg *arg1;
   struct continuation_arg *arg2;
-  long time_at_cmd_start;
+  long time_at_cmd_start = 0;
 #ifdef HAVE_SBRK
   long space_at_cmd_start = 0;
 #endif


-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com

Re: [commit] Fix compile error (Re: [RFA] Async mode fixes.)

[Daniel Jacobowitz]

On Mon, Mar 17, 2008 at 03:10:32PM +0100, Ulrich Weigand wrote:
> > +  long time_at_cmd_start;
> [snip]
> > +  if (target_can_async_p ())
> > +    {
> > +      time_at_cmd_start = get_run_time ();
> [snip]
> > +  if (target_can_async_p () && target_executing)
> > +    {

> and the compiler error seems somewhat justified as the compiler cannot
> know that target_can_async_p () always returns the same value ...
> 
> Fixed by adding a dummy initializer, as already done for space_at_cmd_start.

Thanks, I was just looking at this.  But it made me wonder: what if
target_can_async_p changes when we execute the command?  For instance,
a "target" command?

Also, it looks like an async command which starts the target will
print out the time it took twice.

-- 
Daniel Jacobowitz
CodeSourcery