Re: [PATCH v2 9/9] enable target-async

[Doug Evans <dje at google dot com>]

Tom Tromey writes:
 > This enables target-async by default.
 > 
 > Unlike the CLI, MI chose to treat target-async specially -- setting it
 > changes the default behavior of commands.  So, we can't get rid of the
 > option.  Instead we have to make it MI-only.
 > 
 > The hardest part of this patch, to my surprise, was getting the MI
 > prompt to work properly.  It was reasonably easy, and clean, to get
 > close to what the test suite expects; but to fix the last remaining
 > failure (mi-async.exp), I had to resort to a hack.
 > 
 > It seems to me that the MI grammar was never updated to account for
 > changes implied by async.
 > 
 > Perhaps some future MI can dispense with the prompt entirely.
 > 
 > Built and regtested on x86-64 Fedora 18.
 > 
 > 	PR gdb/15712:
 > 	* infrun.c (set_observer_mode): Don't set target_async_permitted.
 > 	* linux-nat.c (linux_nat_is_async_p): Always return 1.
 > 	(linux_nat_can_async_p): Likewise.
 > 	* mi/mi-interp.c (mi_interpreter_prompt_p): Maybe print the MI
 > 	prompt.
 > 	(mi_cmd_interpreter_exec): Set mi_last_was_cli.
 > 	(mi_execute_command_input_handler): Conditionally print prompt.
 > 	(mi_on_resume): Check sync_execution before printing prompt.
 > 	* mi/mi-main.h (mi_last_was_cli): Declare.
 > 	* mi/mi-main.c (mi_last_was_cli): New global.
 > 	(mi_target_can_async_p): New function.
 > 	(exec_continue): Maybe call async_disable_stdin.
 > 	(run_one_inferior, mi_cmd_exec_run, mi_cmd_list_target_features):
 > 	Use mi_target_can_async_p.
 > 	(captured_mi_execute_command): Clear mi_last_was_cli.
 > 	(mi_execute_async_cli_command): Use mi_target_can_async_p.
 > 	* remote.c (remote_open_1, remote_terminal_inferior)
 > 	(remote_terminal_ours, remote_can_async_p, remote_is_async_p):
 > 	Don't check target_async_permitted.
 > 
 > 	* gdb.texinfo (Non-Stop Mode): Remove "set target-async 1"
 > 	from example.
 > 	(Background Execution): Move target-async docs...
 > 	(Asynchronous and non-stop modes): ... here.  Rewrite to
 > 	MI form.
 > 
 > 	* gdb.mi/mi-cli.exp: Don't check "$async".
 > ---
 >  gdb/cli/cli-interp.c            |  1 +
 >  gdb/doc/gdb.texinfo             | 29 ++++++++++---------------
 >  gdb/infrun.c                    |  1 -
 >  gdb/linux-nat.c                 | 10 ++-------
 >  gdb/mi/mi-interp.c              | 28 +++++++++++++++++++-----
 >  gdb/mi/mi-main.c                | 29 +++++++++++++++++++------
 >  gdb/mi/mi-main.h                |  1 +
 >  gdb/remote.c                    | 48 +++++++++++------------------------------
 >  gdb/testsuite/gdb.mi/mi-cli.exp | 15 +------------
 >  gdb/tui/tui-interp.c            |  1 +
 >  10 files changed, 76 insertions(+), 87 deletions(-)
 > 
 > diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
 > index 1003cc7..ef1e65b 100644
 > --- a/gdb/cli/cli-interp.c
 > +++ b/gdb/cli/cli-interp.c
 > @@ -25,6 +25,7 @@
 >  #include "top.h"		/* for "execute_command" */
 >  #include "gdb_string.h"
 >  #include "exceptions.h"
 > +#include "target.h"
 >  
 >  struct ui_out *cli_uiout;

Needed?

 > diff --git a/gdb/infrun.c b/gdb/infrun.c
 > index 0052ef2..be12a02 100644
 > --- a/gdb/infrun.c
 > +++ b/gdb/infrun.c
 > @@ -242,7 +242,6 @@ set_observer_mode (char *args, int from_tty,
 >       going out we leave it that way.  */
 >    if (observer_mode)
 >      {
 > -      target_async_permitted = 1;
 >        pagination_enabled = 0;
 >        non_stop = non_stop_1 = 1;
 >      }
 > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
 > index e9f3266..4a1ab16 100644
 > --- a/gdb/linux-nat.c
 > +++ b/gdb/linux-nat.c
 > @@ -4764,10 +4764,7 @@ linux_trad_target (CORE_ADDR (*register_u_offset)(struct gdbarch *, int, int))
 >  static int
 >  linux_nat_is_async_p (struct target_ops *ops)
 >  {
 > -  /* NOTE: palves 2008-03-21: We're only async when the user requests
 > -     it explicitly with the "set target-async" command.
 > -     Someday, linux will always be async.  */
 > -  return target_async_permitted;
 > +  return 1;
 >  }
 >  
 >  /* target_can_async_p implementation.  */
 > @@ -4775,10 +4772,7 @@ linux_nat_is_async_p (struct target_ops *ops)
 >  static int
 >  linux_nat_can_async_p (struct target_ops *ops)
 >  {
 > -  /* NOTE: palves 2008-03-21: We're only async when the user requests
 > -     it explicitly with the "set target-async" command.
 > -     Someday, linux will always be async.  */
 > -  return target_async_permitted;
 > +  return 1;
 >  }
 >  
 >  static int
 > diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c
 > index b4515d9..886fde1 100644
 > --- a/gdb/mi/mi-interp.c
 > +++ b/gdb/mi/mi-interp.c
 > @@ -222,11 +222,25 @@ mi_interpreter_exec (void *data, const char *command)
 >    return exception_none;
 >  }
 >  
 > +int mi_last_was_cli;
 > +

Delete?  It's declared in mi-main.h.

 >  /* Never display the default GDB prompt in MI case.  */
 >  
 >  static int
 >  mi_interpreter_prompt_p (void *data)
 >  {
 > +  if (!interp_quiet_p (NULL))
 > +    {
 > +      if (!target_is_async_p ()
 > +	  || (!sync_execution && (!target_async_permitted || !mi_last_was_cli)))
 > +	      /* && (!running_result_record_printed */
 > +	      /* 	  || !mi_proceeded )))) */

Add comment to indicate why the commented out code is present?

 > +	{
 > +	  fputs_unfiltered ("(gdb) \n", raw_stdout);
 > +	  gdb_flush (raw_stdout);
 > +	}
 > +    }
 > +
 >    return 0;
 >  }
 >  
 > @@ -252,6 +266,8 @@ mi_cmd_interpreter_exec (char *command, char **argv, int argc)
 >  	     "does not support command execution"),
 >  	      argv[0]);
 >  
 > +  mi_last_was_cli = strcmp (argv[0], "console") == 0;
 > +
 >    /* Note that unlike the CLI version of this command, we don't
 >       actually set INTERP_TO_USE as the current interpreter, as we
 >       still want gdb_stdout, etc. to point at MI streams.  */
 > @@ -323,8 +339,11 @@ mi_execute_command_input_handler (char *cmd)
 >  {
 >    mi_execute_command_wrapper (cmd);
 >  
 > -  fputs_unfiltered ("(gdb) \n", raw_stdout);
 > -  gdb_flush (raw_stdout);
 > +  if (!target_is_async_p () || !sync_execution)

Please add a comment explaining the condition here.
It loosely reads as !async || async.

 > +    {
 > +      fputs_unfiltered ("(gdb) \n", raw_stdout);
 > +      gdb_flush (raw_stdout);
 > +    }
 >  }
 >  
 >  static void
 > @@ -855,9 +874,8 @@ mi_on_resume (ptid_t ptid)
 >        /* 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 earler.  */
 > -      /* FIXME: review the use of target_is_async_p here -- is that
 > -	 what we want? */
 > -      if (!target_is_async_p ())
 > +      if (/* !target_async_permitted ||  */
 > +	  !target_is_async_p () || sync_execution)

Why the addition of "/* !target_async_permitted ||  */" ?

Also a comment explaining why the sync_execution test is present
would be helpful.

 >  	fputs_unfiltered ("(gdb) \n", raw_stdout);
 >      }
 >    gdb_flush (raw_stdout);
 > diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c
 > index c2d8501..115308b 100644
 > --- a/gdb/mi/mi-main.c
 > +++ b/gdb/mi/mi-main.c
 > @@ -94,6 +94,10 @@ int running_result_record_printed = 1;
 >     command was issued.  */
 >  int mi_proceeded;
 >  
 > +/* Flag indicating whether the most recent command was executed via
 > +   the CLI interpreter.  */
 > +int mi_last_was_cli;
 > +
 >  extern void _initialize_mi_main (void);
 >  static void mi_cmd_execute (struct mi_parse *parse);
 >  
 > @@ -106,6 +110,15 @@ static int register_changed_p (int regnum, struct regcache *,
 >  static void output_register (struct frame_info *, int regnum, int format,
 >  			     int skip_unavailable);
 >  
 > +/* A wrapper for target_can_async_p that takes the MI setting into
 > +   account.  */
 > +
 > +static int
 > +mi_target_can_async_p (void)
 > +{
 > +  return target_async_permitted && target_can_async_p ();
 > +}
 > +
 >  /* Command implementations.  FIXME: Is this libgdb?  No.  This is the MI
 >     layer that calls libgdb.  Any operation used in the below should be
 >     formalized.  */
 > @@ -262,6 +275,9 @@ exec_continue (char **argv, int argc)
 >      {
 >        struct cleanup *back_to = make_cleanup_restore_integer (&sched_multi);
 >  
 > +      if (!target_async_permitted && target_can_async_p ())
 > +	async_disable_stdin ();
 > +

A comment explaining why this is here would be helpful.
I thought target_can_async_p was just a capability flag, not something
specifying the current state of anything.

 >        if (current_context->all)
 >  	{
 >  	  sched_multi = 1;
 > @@ -386,8 +402,8 @@ run_one_inferior (struct inferior *inf, void *arg)
 >        switch_to_thread (null_ptid);
 >        set_current_program_space (inf->pspace);
 >      }
 > -  mi_execute_cli_command ("run", target_can_async_p (),
 > -			  target_can_async_p () ? "&" : NULL);
 > +  mi_execute_cli_command ("run", mi_target_can_async_p (),
 > +			  mi_target_can_async_p () ? "&" : NULL);
 >    return 0;
 >  }
 >  
 > @@ -403,8 +419,8 @@ mi_cmd_exec_run (char *command, char **argv, int argc)
 >      }
 >    else
 >      {
 > -      mi_execute_cli_command ("run", target_can_async_p (),
 > -			      target_can_async_p () ? "&" : NULL);
 > +      mi_execute_cli_command ("run", mi_target_can_async_p (),
 > +			      mi_target_can_async_p () ? "&" : NULL);
 >      }
 >  }
 >  
 > @@ -1789,7 +1805,7 @@ mi_cmd_list_target_features (char *command, char **argv, int argc)
 >        struct ui_out *uiout = current_uiout;
 >  
 >        cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features");      
 > -      if (target_can_async_p ())
 > +      if (mi_target_can_async_p ())
 >  	ui_out_field_string (uiout, NULL, "async");
 >        if (target_can_execute_reverse)
 >  	ui_out_field_string (uiout, NULL, "reverse");
 > @@ -1886,6 +1902,7 @@ captured_mi_execute_command (struct ui_out *uiout, struct mi_parse *context)
 >  
 >    running_result_record_printed = 0;
 >    mi_proceeded = 0;
 > +  mi_last_was_cli = 0;
 >    switch (context->op)
 >      {
 >      case MI_COMMAND:
 > @@ -2204,7 +2221,7 @@ mi_execute_async_cli_command (char *cli_command, char **argv, int argc)
 >    struct cleanup *old_cleanups;
 >    char *run;
 >  
 > -  if (target_can_async_p ())
 > +  if (mi_target_can_async_p ())
 >      run = xstrprintf ("%s %s&", cli_command, argc ? *argv : "");
 >    else
 >      run = xstrprintf ("%s %s", cli_command, argc ? *argv : "");
 > diff --git a/gdb/mi/mi-main.h b/gdb/mi/mi-main.h
 > index d75526a..22f8827 100644
 > --- a/gdb/mi/mi-main.h
 > +++ b/gdb/mi/mi-main.h
 > @@ -32,6 +32,7 @@ extern char *current_token;
 >  
 >  extern int running_result_record_printed;
 >  extern int mi_proceeded;
 > +extern int mi_last_was_cli;
 >  
 >  struct mi_suppress_notification
 >  {
 > diff --git a/gdb/remote.c b/gdb/remote.c
 > index 0892ae4..92ba239 100644
 > --- a/gdb/remote.c
 > +++ b/gdb/remote.c
 > @@ -4252,8 +4252,7 @@ remote_open_1 (char *name, int from_tty,
 >  	   "(e.g. /dev/ttyS0, /dev/ttya, COM1, etc.)."));
 >  
 >    /* See FIXME above.  */
 > -  if (!target_async_permitted)
 > -    wait_forever_enabled_p = 1;
 > +  wait_forever_enabled_p = 1;
 >  
 >    /* If we're connected to a running target, target_preopen will kill it.
 >       Ask this question first, before target_preopen has a chance to kill
 > @@ -4339,20 +4338,17 @@ remote_open_1 (char *name, int from_tty,
 >    use_threadinfo_query = 1;
 >    use_threadextra_query = 1;
 >  
 > -  if (target_async_permitted)
 > -    {
 > -      /* With this target we start out by owning the terminal.  */
 > -      remote_async_terminal_ours_p = 1;
 > +  /* With this target we start out by owning the terminal.  */
 > +  remote_async_terminal_ours_p = 1;
 >  
 > -      /* FIXME: cagney/1999-09-23: During the initial connection it is
 > -	 assumed that the target is already ready and able to respond to
 > -	 requests.  Unfortunately remote_start_remote() eventually calls
 > -	 wait_for_inferior() with no timeout.  wait_forever_enabled_p gets
 > -	 around this.  Eventually a mechanism that allows
 > -	 wait_for_inferior() to expect/get timeouts will be
 > -	 implemented.  */
 > -      wait_forever_enabled_p = 0;
 > -    }
 > +  /* FIXME: cagney/1999-09-23: During the initial connection it is
 > +     assumed that the target is already ready and able to respond to
 > +     requests.  Unfortunately remote_start_remote() eventually calls
 > +     wait_for_inferior() with no timeout.  wait_forever_enabled_p gets
 > +     around this.  Eventually a mechanism that allows
 > +     wait_for_inferior() to expect/get timeouts will be
 > +     implemented.  */
 > +  wait_forever_enabled_p = 0;
 >  
 >    /* First delete any symbols previously loaded from shared libraries.  */
 >    no_shared_libraries (NULL, 0);
 > @@ -4388,14 +4384,12 @@ remote_open_1 (char *name, int from_tty,
 >  	   already before throwing the exception.  */
 >  	if (remote_desc != NULL)
 >  	  remote_unpush_target ();
 > -	if (target_async_permitted)
 > -	  wait_forever_enabled_p = 1;
 > +	wait_forever_enabled_p = 1;
 >  	throw_exception (ex);
 >        }
 >    }
 >  
 > -  if (target_async_permitted)
 > -    wait_forever_enabled_p = 1;
 > +  wait_forever_enabled_p = 1;
 >  }
 >  
 >  /* This takes a program previously attached to and detaches it.  After
 > @@ -5190,10 +5184,6 @@ Give up (and stop debugging it)? ")))
 >  static void
 >  remote_terminal_inferior (void)
 >  {
 > -  if (!target_async_permitted)
 > -    /* Nothing to do.  */
 > -    return;
 > -
 >    /* FIXME: cagney/1999-09-27: Make calls to target_terminal_*()
 >       idempotent.  The event-loop GDB talking to an asynchronous target
 >       with a synchronous command calls this function from both
 > @@ -5213,10 +5203,6 @@ remote_terminal_inferior (void)
 >  static void
 >  remote_terminal_ours (void)
 >  {
 > -  if (!target_async_permitted)
 > -    /* Nothing to do.  */
 > -    return;
 > -
 >    /* See FIXME in remote_terminal_inferior.  */
 >    if (remote_async_terminal_ours_p)
 >      return;
 > @@ -11625,10 +11611,6 @@ Specify the serial device it is connected to (e.g. /dev/ttya).";
 >  static int
 >  remote_can_async_p (struct target_ops *ops)
 >  {
 > -  if (!target_async_permitted)
 > -    /* We only enable async when the user specifically asks for it.  */
 > -    return 0;
 > -
 >    /* We're async whenever the serial device is.  */
 >    return serial_can_async_p (remote_desc);
 >  }
 > @@ -11636,10 +11618,6 @@ remote_can_async_p (struct target_ops *ops)
 >  static int
 >  remote_is_async_p (struct target_ops *ops)
 >  {
 > -  if (!target_async_permitted)
 > -    /* We only enable async when the user specifically asks for it.  */
 > -    return 0;
 > -
 >    /* We're async whenever the serial device is.  */
 >    return serial_is_async_p (remote_desc);
 >  }
 > diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp
 > index bee296d..08e443e 100644
 > --- a/gdb/testsuite/gdb.mi/mi-cli.exp
 > +++ b/gdb/testsuite/gdb.mi/mi-cli.exp
 > @@ -134,20 +134,7 @@ mi_gdb_test "500-stack-select-frame 0" \
 >    {500\^done} \
 >    "-stack-select-frame 0"
 >  
 > -# When a CLI command is entered in MI session, the respose is different in
 > -# sync and async modes. In sync mode normal_stop is called when current
 > -# interpreter is CLI. So:
 > -#   - print_stop_reason prints stop reason in CLI uiout, and we don't show it
 > -#     in MI
 > -#   - The stop position is printed, and appears in MI 'console' channel.
 > -#
 > -# In async mode the stop event is processed when we're back to MI interpreter,
 > -# so the stop reason is printed into MI uiout an.
 > -if {$async} {
 > -    set reason "end-stepping-range"
 > -} else {
 > -    set reason ""
 > -}
 > +set reason "end-stepping-range"
 >  
 >  mi_execute_to "interpreter-exec console step" $reason "callee4" "" ".*basics.c" $line_callee4_next \
 >      "" "check *stopped from CLI command"
 > diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c
 > index 42526e6..296519b 100644
 > --- a/gdb/tui/tui-interp.c
 > +++ b/gdb/tui/tui-interp.c
 > @@ -30,6 +30,7 @@
 >  #include "tui/tui.h"
 >  #include "tui/tui-io.h"
 >  #include "exceptions.h"
 > +#include "target.h"

Needed?

 >  
 >  /* Set to 1 when the TUI mode must be activated when we first start
 >     gdb.  */
 > --