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

Doug Evans
2013-07-31T20:20:18Z

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. */
> --