From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21161 invoked by alias); 31 Jul 2013 20:20:18 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 21128 invoked by uid 89); 31 Jul 2013 20:20:17 -0000 X-Spam-SWARE-Status: No, score=-3.5 required=5.0 tests=AWL,BAYES_00,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RDNS_NONE,SPF_PASS autolearn=ham version=3.3.1 Received: from Unknown (HELO mail-ye0-f201.google.com) (209.85.213.201) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Wed, 31 Jul 2013 20:20:14 +0000 Received: by mail-ye0-f201.google.com with SMTP id m14so122267yen.0 for ; Wed, 31 Jul 2013 13:20:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=from:mime-version:content-type:content-transfer-encoding:message-id :date:to:cc:subject:in-reply-to:references:x-mailer :x-gm-message-state; bh=MS1Lvg9quNXlTZuCdAOs7I+tB0Ut026wXWoa+ENcXC8=; b=PTq0BSsPC7PHCSkj0MReVf5tZaIo4T8xjuP1HRclMsNkuS3xikrFe9CsZInKwQ0A3D x5NzEd4eVvGwzzDDYvGwf1kVfr+oVq1+IXdfvuC59/GlwZQF1L9MdfCSo/ilv9e67L3P NWyXNTYI++eS7E/d5dxFOpvIwXtOpkFba0fTZsQrv8ik5E6K3jK7TPHe4vb0ZnclhEhK rev9j6QBZ5z98SOF2s5c+tX1CaQxSuoieW+ZW+yp4qu+4k0rn4Esi/2k3vZ0BfS2XMhu B7gMbyq9oYqQ145JGSDIQob/Zx1wUMrNoUA3NvtUTbNFy3xXfmtjYhidlLdzMutLhO50 6cNg== X-Received: by 10.236.198.240 with SMTP id v76mr711716yhn.10.1375302006849; Wed, 31 Jul 2013 13:20:06 -0700 (PDT) Received: from corp2gmr1-1.hot.corp.google.com (corp2gmr1-1.hot.corp.google.com [172.24.189.92]) by gmr-mx.google.com with ESMTPS id n70si1331084yhj.7.2013.07.31.13.20.06 for (version=TLSv1.1 cipher=AES128-SHA bits=128/128); Wed, 31 Jul 2013 13:20:06 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-1.hot.corp.google.com (Postfix) with ESMTP id 353AE31C03C; Wed, 31 Jul 2013 13:20:06 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <20985.29045.586199.345765@ruffy.mtv.corp.google.com> Date: Wed, 31 Jul 2013 20:20:00 -0000 To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v2 9/9] enable target-async In-Reply-To: <1375295281-7040-10-git-send-email-tromey@redhat.com> References: <1375295281-7040-1-git-send-email-tromey@redhat.com> <1375295281-7040-10-git-send-email-tromey@redhat.com> X-Gm-Message-State: ALoCoQljquj0DnuShcMtW69fkIxLGQt+kA/OmG2X4LWNM1YhxAEg79iowXTavBoKIKykjTwqfIXzGIPaBfO4pGQIMZj3BhhDWrDxBbOmzKxq1oCFg38nzzNSgoVXk/ukmtdMRL52ls2wRfgagLbjK0+eBPTM1rXxzBcwa0VTm7DAEcw0OC5LYdHUSYvHg4oenlViZtT1DIAG5xhyojCbR6GttEPVmqtarw== X-SW-Source: 2013-07/txt/msg00842.txt.bz2 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. */ > --