Fix "attach" command vs user input race [Re: Regression for attach from stdin [Re: [pushed] Re: [PATCH v6 0/2] enable target-async by default]]
Commit Message
On 07/03/2014 04:38 PM, Pedro Alves wrote:
> On 07/02/2014 10:15 AM, Pedro Alves wrote:
>> On 07/02/2014 09:59 AM, Mark Wielaard wrote:
>>> Thanks for tracking this down Jan. Should this patch be reverted
>>
>> Unless it turns into a deep rat hole, I'd like to fix the issue
>> instead. I haven't investigated this particular issue yet, but I've
>> been busy fixing PR17072, and I ended up fixing several issues
>> related to execution commands entered directly in the command line,
>> before GDB's main event loop starts. This one sounds related.
>
> Looking at this now. The other series unfortunately doesn't fix this.
>
Here's a fix. Let me know whether it fixes guality testing.
You can also find it at:
git@github.com:palves/gdb.git palves/attach_from_stdin
I've also pushed it on top of the palves/pr17072_pagination_async
branch, in case the other fixes on that branch (posted yesterday)
are necessary.
Thanks,
Pedro Alves
8<----------
From f93645049c02459bedada7b61fea633f59a47817 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Fri, 4 Jul 2014 13:47:53 +0100
Subject: [PATCH] Fix "attach" command vs user input race
On async targets, a synchronous attach is done like this:
#1 - target_attach is called (PTRACE_ATTACH is issued)
#2 - a continuation is installed
#3 - we go back to the event loop
#4 - target reports stop (SIGSTOP), event loop wakes up, and
attach continuation is called
#5 - among other things, the continuation calls
target_terminal_inferior, which removes stdin from the event
loop
Note that in #3, GDB is still processing user input. If the user is
fast enough, e.g., with something like:
echo -e "attach PID\nset xxx=1" | gdb
... then the "set" command is processed before the attach completes.
We get worse behavior even, if input is a tty and therefore
readline/editing is enabled, with e.g.,:
(gdb) attach PID\nset xxx=1
we then crash readline/gdb, with:
Attaching to program: attach-wait-input, process 14537
readline: readline_callback_read_char() called with no handler!
Aborted
$
Fix this by calling target_terminal_inferior before #3 above.
The test covers both scenarios by running with editing/readline forced
to both on and off.
gdb/
2014-07-04 Pedro Alves <palves@redhat.com>
* infcmd.c (attach_command_post_wait): Don't call
target_terminal_inferior here.
(attach_command): Call it here instead.
gdb/testsuite/
2014-07-04 Pedro Alves <palves@redhat.com>
* gdb.base/attach-fg-no-stdin.exp: New file.
* gdb.base/attach-fg-no-stdin.c: New file.
---
gdb/infcmd.c | 9 +-
gdb/testsuite/gdb.base/attach-wait-input.c | 47 +++++++++++
gdb/testsuite/gdb.base/attach-wait-input.exp | 119 +++++++++++++++++++++++++++
3 files changed, 170 insertions(+), 5 deletions(-)
create mode 100644 gdb/testsuite/gdb.base/attach-wait-input.c
create mode 100644 gdb/testsuite/gdb.base/attach-wait-input.exp
Comments
On Fri, 2014-07-04 at 14:48 +0100, Pedro Alves wrote:
> Here's a fix. Let me know whether it fixes guality testing.
Yes, that patch works for me. Now I can run make check-c
RUNTESTFLAGS=guality.exp in gcc without needing any guality.h
workarounds.
Thanks,
Mark
Pedro Alves writes:
> gdb/
> 2014-07-04 Pedro Alves <palves@redhat.com>
>
> * infcmd.c (attach_command_post_wait): Don't call
> target_terminal_inferior here.
> (attach_command): Call it here instead.
> [...]
> diff --git a/gdb/infcmd.c b/gdb/infcmd.c
> index c4bb401..76ab12b 100644
> --- a/gdb/infcmd.c
> +++ b/gdb/infcmd.c
> @@ -2381,9 +2381,6 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
>
> post_create_inferior (¤t_target, from_tty);
>
> - /* Install inferior's terminal modes. */
> - target_terminal_inferior ();
> -
> if (async_exec)
> {
> /* The user requested an `attach&', so be sure to leave threads
> @@ -2495,9 +2492,11 @@ attach_command (char *args, int from_tty)
> shouldn't refer to attach_target again. */
> attach_target = NULL;
>
> - /* Set up the "saved terminal modes" of the inferior
> - based on what modes we are starting it with. */
> + /* Set up the "saved terminal modes" of the inferior based on what
> + modes we are starting it with, and remove stdin from the event
> + loop. */
> target_terminal_init ();
> + target_terminal_inferior ();
>
> /* Set up execution context to know that we should return from
> wait_for_inferior as soon as the target reports a stop. */
Our nomenclature here is problematic. I always do a double take when
I see attach and target_terminal_foo mentioned in the same sentence.
Bleah.
For the case at hand, and at least until we have something more
readable, can I ask for a slight change here?
target_terminal_inferior can do a lot more than just "remove stdin
from the event loop", thus as a reader I'm left being unsure
if there aren't still more bugs here.
Plus, while I can see how to map the comment to the code in the patch,
"Set up the "saved terminal modes" of the inferior based on what
modes we are starting it with"
-->
target_terminal_init ();
and
"remove stdin from the event loop"
-->
target_terminal_inferior ();
If I look at the result after the patch,
combining the two steps into one sentence doesn't help clarify
things given that target_terminal_inferior can do more than just
"remove stdin from the event loop".
E.g., this is a marginal improvement:
/* Set up the "saved terminal modes" of the inferior based on what
modes we are starting it with. */
target_terminal_init ();
/* And remove stdin from the event loop. */
target_terminal_inferior ();
But I'm hoping for more text in the second comment to explain things
better.
On Fri, 04 Jul 2014 15:48:40 +0200, Pedro Alves wrote:
> Here's a fix. Let me know whether it fixes guality testing.
Mark has already replied the same but I can also confirm it fixes the GCC
testsuite.
Thanks,
Jan
@@ -2381,9 +2381,6 @@ attach_command_post_wait (char *args, int from_tty, int async_exec)
post_create_inferior (¤t_target, from_tty);
- /* Install inferior's terminal modes. */
- target_terminal_inferior ();
-
if (async_exec)
{
/* The user requested an `attach&', so be sure to leave threads
@@ -2495,9 +2492,11 @@ attach_command (char *args, int from_tty)
shouldn't refer to attach_target again. */
attach_target = NULL;
- /* Set up the "saved terminal modes" of the inferior
- based on what modes we are starting it with. */
+ /* Set up the "saved terminal modes" of the inferior based on what
+ modes we are starting it with, and remove stdin from the event
+ loop. */
target_terminal_init ();
+ target_terminal_inferior ();
/* Set up execution context to know that we should return from
wait_for_inferior as soon as the target reports a stop. */
new file mode 100644
@@ -0,0 +1,47 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+ Copyright 2014 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. Thus, it simply spins in a loop. The loop
+ is exited when & if the variable 'should_exit' is non-zero. (It
+ is initialized to zero in this program, so the loop will never
+ exit unless/until gdb sets the variable to non-zero.)
+ */
+
+#include <sys/types.h>
+#include <unistd.h>
+
+volatile int should_exit = 0;
+int mypid;
+
+static void
+setup_done (void)
+{
+}
+
+int
+main (void)
+{
+ unsigned int counter = 1;
+
+ mypid = getpid ();
+ setup_done ();
+
+ for (counter = 0; !should_exit && counter < 100; counter++)
+ sleep (1);
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,119 @@
+# Copyright 1997-2014 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/>. */
+
+# Verify that GDB waits for the "attach" command to finish before
+# processing the following command.
+#
+# GDB used to have a race where on async targets, in the small window
+# between the attach request and the initial stop for the attach, GDB
+# was still processing user input.
+#
+# The issue was originally detected with:
+#
+# echo -e "attach PID\nset xxx=1" | gdb
+#
+# In that scenario, stdin is not a tty, which disables readline.
+# Explicitly turning off editing exercises the same code path, and is
+# simpler to do, so we test with both editing on and off.
+
+# The test uses the "attach" command.
+if [target_info exists use_gdb_stub] {
+ return
+}
+
+standard_testfile
+
+if {[build_executable "failed to build" $testfile $srcfile debug]} {
+ return -1
+}
+
+# Start the program running, and return its PID, ready for attaching.
+
+proc start_program {binfile} {
+ global gdb_prompt
+ global decimal
+
+ clean_restart $binfile
+
+ if ![runto setup_done] then {
+ fail "Can't run to setup_done"
+ return 0
+ }
+
+ # Get the PID of the test process.
+ set testpid ""
+ set test "get inferior process ID"
+ gdb_test_multiple "p mypid" $test {
+ -re " = ($decimal)\r\n$gdb_prompt $" {
+ set testpid $expect_out(1,string)
+ pass $test
+ }
+ }
+
+ gdb_test "detach" "Detaching from program: .*"
+
+ if {$testpid == ""} {
+ return
+ }
+
+ return $testpid
+}
+
+# Do test proper. EDITING indicates whether "set editing" is on or
+# off.
+
+proc test { editing } {
+ global gdb_prompt
+ global binfile
+ global decimal
+
+ with_test_prefix "editing $editing" {
+
+ set testpid [start_program $binfile]
+ if {$testpid == ""} {
+ return
+ }
+
+ # Enable/disable readline.
+ gdb_test_no_output "set editing $editing"
+
+ # Send both commands at once.
+ send_gdb "attach $testpid\nprint should_exit = 1\n"
+
+ # Use gdb_expect directly instead of gdb_test_multiple to
+ # avoid races with the double prompt.
+ set test "attach and print"
+ gdb_expect {
+ -re "Attaching to program.*process $testpid\r\n.*$gdb_prompt.*$decimal = 1\r\n$gdb_prompt $" {
+ pass "$test"
+ }
+ timeout {
+ fail "$test (timeout)"
+ }
+ }
+
+ # As we've used attach, on quit, we'll detach from the
+ # program. Explicitly kill it in case we failed above.
+ gdb_test "kill" \
+ "" \
+ "after attach, exit" \
+ "Kill the program being debugged.*y or n. $" \
+ "y"
+ }
+}
+
+foreach editing {"on" "off"} {
+ test $editing
+}