Re: [patch] python prompt additions at first prompt.

Matt Rice
2011-08-11T12:03:50Z

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

On Wed, Aug 10, 2011 at 8:20 AM, Tom Tromey <tromey@redhat.com> wrote:
> Matt> So, I guess neither the current patch or adding the observer calls
> Matt> to the existing code is correct and we at least need to split up
> Matt> prompt displaying from the setting of the prompt parameter?
>
> I'm afraid I don't have an answer for you.

thats alright, updated patch, modifications from the previous patch include
on the code side we need to avoid calling the observer/prompt_hook
when sync_execution is enabled,
wrt tests check for double prompting in lib/prompt.exp, and add
sync_execution tests,
and tests for the prompt_hook function's argument.

if we don't check sync_execution when calling the prompt_hook:
in the sync_execution && is_running case this leads to prompt_hook
being called for a prompt that is never displayed (giving the
prompt_hook an empty string argument from async_disable_stdin).

in the sync_execution && !is_running case the fallthrough can lead to
a 'double prompt', if the prompt_hook overrides the empty prompt that
async_disable_stdin sets, then it prompts again in the future.

there is a concerted effort (see below for examples), that we force
the prompt to return early, avoiding readline stuff, and then
re-calling it in the future, when the signal handling won't get in the
way, the first prompt does not seem to be different in regard to this
than any other, thus the cli_command_loop call to display_gdb_prompt,
returns early, then later in fetch_inferior_event, we call it again
without sync_execution set, and its only then that we get the first
prompt and setup readline.

doing readline stuff in cli_command_loop (in the case of
sync_execution) was causing 10720.
because sync_execution is set during command handling which happens
before the first prompt.
and it wasn't testing that flag.

thus, I don't think returning early is a problem here, i believe it is
working as intended, though this entire mechanism does seem complex
and fragile... anyhow that is my understanding of it, thanks for your
patience.

infrun.c:fetch_inferior_event (~2815)
/* If the inferior was in sync execution mode, and now isn't,
restore the prompt. */
if (was_sync && !sync_execution)
display_gdb_prompt (0);

inf-loop.c:inferior_event_handler (~58)
async_enable_stdin ();
display_gdb_prompt (0);

inf-loop.c:inferior_event_handler (~73)
/* The call to async_enable_stdin below resets 'sync_execution'.
However, if sync_execution is 1 now, we also need to show the
prompt below, so save the current value. */
was_sync = sync_execution;
async_enable_stdin ();

P.S. I have seen some core dumps on exit in the sync_execution
versions of these tests, both with and without the patch.
along with random stopping after 'continue&', i'm not very familiar
with target-async so i'm not exactly sure what to make of it.

no new failures (not even a spurious one, weird), all new tests pass.

2011-08-11 Matt Rice <ratmice@gmail.com>

* event-top.c (cli_command_loop): Replace readline setup with
direct call to display_gdb_prompt.
(display_gdb_prompt): Do not call observer mechanism during
synchronous execution.

2011-08-11 Matt Rice <ratmice@gmail.com>

* lib/prompt.exp: New file for testing the first prompt.
* gdb.python/py-prompt.exp: Ditto.
* gdb.python/py-prompt.c: Ditto (copy of ext-attach.c).

[-- Attachment #2: foo.diff --]
[-- Type: text/x-patch, Size: 10731 bytes --]

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 37882728..0210600 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -185,27 +185,7 @@ rl_callback_read_char_wrapper (gdb_client_data client_data)
void
cli_command_loop (void)
{
- /* If we are using readline, set things up and display the first
- prompt, otherwise just print the prompt. */
- if (async_command_editing_p)
- {
- int length;
- char *a_prompt;
- char *gdb_prompt = get_prompt (0);
-
- /* Tell readline what the prompt to display is and what function
- it will need to call after a whole line is read. This also
- displays the first prompt. */
- length = strlen (get_prefix (0))
- + strlen (gdb_prompt) + strlen (get_suffix(0)) + 1;
- a_prompt = (char *) alloca (length);
- strcpy (a_prompt, get_prefix (0));
- strcat (a_prompt, gdb_prompt);
- strcat (a_prompt, get_suffix (0));
- rl_callback_handler_install (a_prompt, input_handler);
- }
- else
- display_gdb_prompt (0);
+ display_gdb_prompt (0);

/* Now it's time to start the event loop. */
start_event_loop ();
@@ -273,7 +253,7 @@ display_gdb_prompt (char *new_prompt)
functions may change the prompt. Do not call observers on an
explicit prompt change as passed to this function, as this forms
a temporary prompt, IE, displayed but not set. */
- if (! new_prompt)
+ if (! sync_execution && ! new_prompt)
{
char *post_gdb_prompt = NULL;
char *pre_gdb_prompt = xstrdup (get_prompt (0));
diff --git a/gdb/testsuite/gdb.python/py-prompt.c b/gdb/testsuite/gdb.python/py-prompt.c
new file mode 100644
index 0000000..8d84f09
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-prompt.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2007, 2009, 2010, 2011 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <http://www.gnu.org/licenses/>. */
+
+/* This program is intended to be started outside of gdb, and then
+ attached to by gdb. It loops for a while, but not forever. */
+
+#include <unistd.h>
+
+int main ()
+{
+ int i;
+
+ for (i = 0; i < 120; i++)
+ sleep (1);
+
+ return 0;
+}
diff --git a/gdb/testsuite/gdb.python/py-prompt.exp b/gdb/testsuite/gdb.python/py-prompt.exp
new file mode 100644
index 0000000..c1ca105
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-prompt.exp
@@ -0,0 +1,131 @@
+# Copyright (C) 2011 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# This file is part of the GDB testsuite. It tests the mechanism
+# for defining the prompt in Python.
+
+if $tracelevel then {
+ strace $tracelevel
+}
+
+set testfile "py-prompt"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+
+load_lib gdb-python.exp
+load_lib prompt.exp
+
+# Start with a fresh gdb.
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+
+# Skip all tests if Python scripting is not enabled.
+if { [skip_python_tests] } { continue }
+gdb_exit
+
+if { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {debug}] != "" } {
+ untested py-prompt.exp
+ return -1
+}
+
+global GDBFLAGS
+set saved_gdbflags $GDBFLAGS
+set GDBFLAGS [concat $GDBFLAGS " -ex \"set height 0\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"set width 0\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"python p = list()\""]
+set prompt_func "python def foo(x): global p; p.append(x); return \'(Foo) \'"
+set GDBFLAGS [concat $GDBFLAGS " -ex \"$prompt_func\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"python gdb.prompt_hook=foo\""]
+
+set tmp_gdbflags $GDBFLAGS
+set gdb_prompt_fail $gdb_prompt
+
+global gdb_prompt
+# Does not include the space at the end of the prompt.
+# gdb_test expects it not to be there.
+set gdb_prompt "\[(\]Foo\[)\]"
+
+set GDBFLAGS [concat $tmp_gdbflags " -ex \"set editing on\""]
+prompt_gdb_start
+gdb_test "python x = len(p); print gdb.execute(\"show prompt\", to_string = True)" \
+ ".*prompt is \"$gdb_prompt \".*" \
+ "show prompt gets the correct result"
+gdb_test "python print x, len(p)" "1 2" \
+ "retrieving the prompt causes no extra prompt_hook calls"
+gdb_test "python print \"'\" + str(p\[0\]) + \"'\"" "'$gdb_prompt_fail '" \
+ "prompt_hook argument is default prompt."
+gdb_exit
+
+
+set GDBFLAGS [concat $tmp_gdbflags " -ex \"set editing off\""]
+prompt_gdb_start
+gdb_test "python x = len(p); print gdb.execute(\"show prompt\", to_string = True)" \
+ ".*prompt is \"$gdb_prompt \".*" \
+ "show prompt gets the correct result 2"
+gdb_test "python print x, len(p)" "1 2" \
+ "retrieving the prompt causes no extra prompt_hook calls 2"
+gdb_test "python print \"'\" + str(p\[0\]) + \"'\"" "'$gdb_prompt_fail '" \
+ "prompt_hook argument is default prompt. 2"
+gdb_exit
+
+# Start the program running and then wait for a bit, to be sure
+# that it can be attached to.
+set testpid [eval exec $binfile &]
+exec sleep 2
+if { [istarget "*-*-cygwin*"] } {
+ # testpid is the Cygwin PID, GDB uses the Windows PID, which might be
+ # different due to the way fork/exec works.
+ set testpid [ exec ps -e | gawk "{ if (\$1 == $testpid) print \$4; }" ]
+}
+
+set GDBFLAGS [concat $tmp_gdbflags " -ex \"set target-async on\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"set pagination off\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"set editing on\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"attach $testpid\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"continue&\""]
+
+# sync_execution = 1 is_running = 1
+prompt_gdb_start
+gdb_test "python x = len(p); print gdb.execute(\"show prompt\", to_string = True)" \
+ ".*prompt is \"$gdb_prompt \".*" \
+ "show prompt gets the correct result 3"
+gdb_test "python print x, len(p)" "1 2" \
+ "retrieving the prompt causes no extra prompt_hook calls 3"
+gdb_test "python print \"'\" + str(p\[0\]) + \"'\"" "'$gdb_prompt_fail '" \
+ "prompt_hook argument is default prompt. 3"
+gdb_exit
+
+set GDBFLAGS [concat $tmp_gdbflags " -ex \"set target-async on\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"set pagination off\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"set editing on\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"attach $testpid\""]
+set GDBFLAGS [concat $GDBFLAGS " -ex \"interrupt\""]
+
+# sync_execution = 1 is_running = 0
+prompt_gdb_start
+gdb_test "python x = len(p); print gdb.execute(\"show prompt\", to_string = True)" \
+ ".*prompt is \"$gdb_prompt \".*" \
+ "show prompt gets the correct result 4"
+gdb_test "python print x, len(p)" "1 2" \
+ "retrieving the prompt causes no extra prompt_hook calls 4"
+gdb_test "python print \"'\" + str(p\[0\]) + \"'\"" "'$gdb_prompt_fail '" \
+ "prompt_hook argument is default prompt. 4"
+gdb_exit
+
+set GDBFLAGS $saved_gdbflags
+return 0
diff --git a/gdb/testsuite/lib/prompt.exp b/gdb/testsuite/lib/prompt.exp
new file mode 100644
index 0000000..727bbb3
--- /dev/null
+++ b/gdb/testsuite/lib/prompt.exp
@@ -0,0 +1,92 @@
+# Copyright (C) 2011 Free Software Foundation, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+
+# Specialized subroutines for launching gdb and testing the very first prompt.
+
+
+#
+# start gdb -- start gdb running, prompt procedure
+# this procedure differs from the default in that you must pass 'set height 0',
+# and 'set width 0', yourself in GDBFLAGS, and it has a gdb_prompt_fail variable,
+#
+# uses pass if it sees $gdb_prompt, and fail if it sees $gdb_prompt_fail.
+#
+proc default_prompt_gdb_start { } {
+ global verbose
+ global GDB
+ global INTERNAL_GDBFLAGS GDBFLAGS
+ global gdb_prompt
+ global gdb_prompt_fail
+ global timeout
+ global gdb_spawn_id;
+
+ gdb_stop_suppressing_tests;
+
+ verbose "Spawning $GDB $INTERNAL_GDBFLAGS $GDBFLAGS"
+
+ if [info exists gdb_spawn_id] {
+ return 0;
+ }
+
+ if ![is_remote host] {
+ if { [which $GDB] == 0 } then {
+ perror "$GDB does not exist."
+ exit 1
+ }
+ }
+ set res [remote_spawn host "$GDB $INTERNAL_GDBFLAGS $GDBFLAGS [host_info gdb_opts]"];
+ if { $res < 0 || $res == "" } {
+ perror "Spawning $GDB failed."
+ return 1;
+ }
+ gdb_expect 360 {
+ -re ".*$gdb_prompt_fail.*$gdb_prompt_fail.*" {
+ fail "double prompted fail prompt"
+ }
+ -re ".*$gdb_prompt.*$gdb_prompt.*" {
+ fail "double prompted"
+ }
+ -re "\[\r\n\]$gdb_prompt_fail $" {
+ fail "GDB initializing first prompt"
+ }
+ -re "\[\r\n\]$gdb_prompt $" {
+ pass "GDB initializing first prompt"
+ }
+ -re "$gdb_prompt $" {
+ perror "GDB never initialized."
+ return -1
+ }
+ -re "$gdb_prompt_fail $" {
+ perror "GDB never initialized."
+ return -1
+ }
+ timeout {
+ perror "(timeout) GDB never initialized after 10 seconds."
+ remote_close host;
+ return -1
+ }
+ }
+ set gdb_spawn_id -1;
+ return 0;
+}
+
+#
+# Overridable function. You can override this function in your
+# baseboard file.
+#
+proc prompt_gdb_start { } {
+ default_prompt_gdb_start
+}
+