[1/2] gdb: Re-enable stdin for all UIs from start_event_loop

Message ID cbc0d85fce4b5be87f0cbd09c7c3e76bcc6b6752.1577065293.git.andrew.burgess@embecosm.com
State New
Headers show
Series
  • Re-enable stdin in various places
Related show

Commit Message

Andrew Burgess Dec. 23, 2019, 1:45 a.m.
If we catch an exception in start_event_loop's call to
gdb_do_one_event, then it is possible that the current_ui has changed
since we called async_disable_stdin.  If that's the case then calling
async_enable_stdin will be called on the wrong UI.

To solve this problem we wrap the call to async_enable_stdin with
SWITCH_THRU_ALL_UIS, this causes us to try and re-enable stdin for all
UIs, which will catch any for which we called async_disable_stdin.

gdb/ChangeLog:

	* event-loop.c (start_event_loop): Wrap async_enable_stdin with
	SWITCH_THRU_ALL_UIS.

gdb/testsuite/ChangeLog:

	* gdb.server/multi-ui-errors.c: New file.
	* gdb.server/multi-ui-errors.exp: New file.

Change-Id: I1e18deff2e6f4e17f7a13adce3553eb001cad93b
---
 gdb/ChangeLog                                |   5 ++
 gdb/event-loop.c                             |   5 +-
 gdb/testsuite/ChangeLog                      |   5 ++
 gdb/testsuite/gdb.server/multi-ui-errors.c   |  30 ++++++++
 gdb/testsuite/gdb.server/multi-ui-errors.exp | 107 +++++++++++++++++++++++++++
 5 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.c
 create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.exp

-- 
2.14.5

Comments

Pedro Alves Jan. 22, 2020, 7:26 p.m. | #1
This looks good to me, pending a few comments below.

On 12/23/19 1:45 AM, Andrew Burgess wrote:
> If we catch an exception in start_event_loop's call to

> gdb_do_one_event, then it is possible that the current_ui has changed

> since we called async_disable_stdin.  If that's the case then calling

> async_enable_stdin will be called on the wrong UI.

> 

> To solve this problem we wrap the call to async_enable_stdin with

> SWITCH_THRU_ALL_UIS, this causes us to try and re-enable stdin for all

> UIs, which will catch any for which we called async_disable_stdin.

> 

> gdb/ChangeLog:

> 

> 	* event-loop.c (start_event_loop): Wrap async_enable_stdin with

> 	SWITCH_THRU_ALL_UIS.

> 

> gdb/testsuite/ChangeLog:

> 

> 	* gdb.server/multi-ui-errors.c: New file.

> 	* gdb.server/multi-ui-errors.exp: New file.

> 


The test fails when run against --target_board=native-extended-gdbserver:

 target extended-remote localhost:2348
 Already connected to a remote target.  Disconnect? (y or n) [answered Y; input not from terminal]
 n
 Remote debugging using localhost:2348
 Reading symbols from /lib64/ld-linux-x86-64.so.2...
 Reading symbols from /usr/lib/debug/usr/lib64/ld-2.26.so.debug...
 0x00007ffff7dd8ed0 in _start () from /lib64/ld-linux-x86-64.so.2
 (gdb) FAIL: gdb.server/multi-ui-errors.exp: target extended-remote localhost:2348 (got interactive prompt)

Looks like it's just missing the standard "disconnect" most
gdb.server/ tests do.

> Change-Id: I1e18deff2e6f4e17f7a13adce3553eb001cad93b

> ---

>  gdb/ChangeLog                                |   5 ++

>  gdb/event-loop.c                             |   5 +-

>  gdb/testsuite/ChangeLog                      |   5 ++

>  gdb/testsuite/gdb.server/multi-ui-errors.c   |  30 ++++++++

>  gdb/testsuite/gdb.server/multi-ui-errors.exp | 107 +++++++++++++++++++++++++++

>  5 files changed, 151 insertions(+), 1 deletion(-)

>  create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.c

>  create mode 100644 gdb/testsuite/gdb.server/multi-ui-errors.exp

> 

> diff --git a/gdb/event-loop.c b/gdb/event-loop.c

> index affa00b4aa9..b9444153ff8 100644

> --- a/gdb/event-loop.c

> +++ b/gdb/event-loop.c

> @@ -376,7 +376,10 @@ start_event_loop (void)

>  	  /* If any exception escaped to here, we better enable

>  	     stdin.  Otherwise, any command that calls async_disable_stdin,

>  	     and then throws, will leave stdin inoperable.  */

> -	  async_enable_stdin ();

> +	  SWITCH_THRU_ALL_UIS ()

> +	    {

> +	      async_enable_stdin ();

> +	    }

>  	  /* If we long-jumped out of do_one_event, we probably didn't

>  	     get around to resetting the prompt, which leaves readline

>  	     in a messed-up state.  Reset it here.  */

> diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.c b/gdb/testsuite/gdb.server/multi-ui-errors.c

> new file mode 100644

> index 00000000000..0e6c0ea0fde

> --- /dev/null

> +++ b/gdb/testsuite/gdb.server/multi-ui-errors.c

> @@ -0,0 +1,30 @@

> +/* This testcase is part of GDB, the GNU debugger.

> +

> +   Copyright 2019 Free Software Foundation, Inc.


Remember to update this to "2019-2020".

> +

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

> +

> +#include <stdio.h>

> +#include <unistd.h>

> +

> +int

> +main (void)

> +{

> +  printf ("@@XX@@ Inferior Starting @@XX@@\n");

> +

> +  while (1)

> +    sleep (1);


Avoid infinite loops.  For example, replace the "while" with
a "for" loop that counts 60 seconds.

> +

> +  return 0;

> +}

> diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.exp b/gdb/testsuite/gdb.server/multi-ui-errors.exp

> new file mode 100644

> index 00000000000..5162a1a6f9c

> --- /dev/null

> +++ b/gdb/testsuite/gdb.server/multi-ui-errors.exp

> @@ -0,0 +1,107 @@

> +# This testcase is part of GDB, the GNU debugger.

> +#

> +# Copyright 2019 Free Software Foundation, Inc.


Ditto.

> +#

> +# 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/>.

> +

> +# Test what happens if we have multiple UIs in use, and an error

> +# occurs while running a GDB command.  Specifically, do both UIs

> +# return to an interactive state, or does one (or both) of them get

> +# stuck in a non-interactive state.

> +

> +load_lib gdbserver-support.exp

> +

> +standard_testfile

> +

> +if {[skip_gdbserver_tests]} {

> +    return 0

> +}

> +

> +if {[build_executable "failed to prepare" ${testfile} \

> +	 ${srcfile}] == -1} {

> +    return -1

> +}

> +

> +clean_restart $testfile


Use prepare_for_testing ?

> +

> +# Start gdbserver.

> +set res [gdbserver_spawn "${binfile}"]

> +set gdbserver_protocol [lindex $res 0]

> +set gdbserver_gdbport [lindex $res 1]

> +set gdbserver_pid [exp_pid -i $server_spawn_id]

> +

> +# Save the main UI's spawn ID.

> +set gdb_main_spawn_id $gdb_spawn_id

> +

> +# Create the new PTY for the secondary console UI, issue the 'new-ui'

> +# command, and wait for a prompt on the second UI.

> +spawn -pty

> +set extra_spawn_id $spawn_id

> +set extra_tty_name $spawn_out(slave,name)

> +gdb_test_multiple "new-ui console $extra_tty_name" "new-ui" {

> +    -re "New UI allocated\r\n$gdb_prompt $" {

> +	pass $gdb_test_name

> +    }

> +}

> +with_spawn_id $extra_spawn_id {

> +    gdb_test_multiple "" "initial prompt on extra console" {

> +	-re "$gdb_prompt $" {

> +	    pass $gdb_test_name

> +	}

> +    }

> +}

> +

> +# Connect to the remote and continue its execution from the other UI.

> +with_spawn_id $extra_spawn_id {

> +    gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*"

> +    send_gdb "continue\n"

> +}

> +

> +# We're going to kill the gdbserver, but before we do, lets make sure

> +# that the inferior has started executing.

> +with_spawn_id $server_spawn_id {

> +    gdb_expect 3 {

> +	-re "@@XX@@ Inferior Starting @@XX@@" {

> +	    verbose -log "APB: Spotted running pattern"

> +	}

> +	timeout {

> +	    verbose -log "APB: Didn't find running message"

> +	}

> +    }

> +}

> +

> +# Interact with the main UI.

> +with_spawn_id $gdb_main_spawn_id {

> +    gdb_test "echo hello\\n" "hello" "interact with GDB's main UI"

> +}

> +

> +# Now kill the gdbserver.

> +remote_exec target "kill -9 $gdbserver_pid"

> +

> +# We expect to land back at a GDB prompt in both UIs.  Right now there

> +# seems to be some "issues" with the prompt automatically being

> +# displayed... Or maybe this is intentional and I'm missunderstanding

> +# the expected behaviour.  


Please avoid "I" in comments.  Can you clarify what you mean by "issues"?

> +# Either way, getting a prompt isn't the

> +# point of this test.  The point is that we should not be able to

> +# interact with GDB from either interpreter now.


I guess you meant "should" instead of "should not"?

> +

> +with_spawn_id $gdb_main_spawn_id {

> +    gdb_test "echo" "" \

> +	"main UI, prompt after gdbserver dies"

> +}

> +

> +with_spawn_id $extra_spawn_id {

> +    gdb_test "echo" "" \

> +	"extra UI, prompt after gdbserver dies"

> +}

> 


Thanks,
Pedro Alves

Patch

diff --git a/gdb/event-loop.c b/gdb/event-loop.c
index affa00b4aa9..b9444153ff8 100644
--- a/gdb/event-loop.c
+++ b/gdb/event-loop.c
@@ -376,7 +376,10 @@  start_event_loop (void)
 	  /* If any exception escaped to here, we better enable
 	     stdin.  Otherwise, any command that calls async_disable_stdin,
 	     and then throws, will leave stdin inoperable.  */
-	  async_enable_stdin ();
+	  SWITCH_THRU_ALL_UIS ()
+	    {
+	      async_enable_stdin ();
+	    }
 	  /* If we long-jumped out of do_one_event, we probably didn't
 	     get around to resetting the prompt, which leaves readline
 	     in a messed-up state.  Reset it here.  */
diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.c b/gdb/testsuite/gdb.server/multi-ui-errors.c
new file mode 100644
index 00000000000..0e6c0ea0fde
--- /dev/null
+++ b/gdb/testsuite/gdb.server/multi-ui-errors.c
@@ -0,0 +1,30 @@ 
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 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/>.  */
+
+#include <stdio.h>
+#include <unistd.h>
+
+int
+main (void)
+{
+  printf ("@@XX@@ Inferior Starting @@XX@@\n");
+
+  while (1)
+    sleep (1);
+
+  return 0;
+}
diff --git a/gdb/testsuite/gdb.server/multi-ui-errors.exp b/gdb/testsuite/gdb.server/multi-ui-errors.exp
new file mode 100644
index 00000000000..5162a1a6f9c
--- /dev/null
+++ b/gdb/testsuite/gdb.server/multi-ui-errors.exp
@@ -0,0 +1,107 @@ 
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2019 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/>.
+
+# Test what happens if we have multiple UIs in use, and an error
+# occurs while running a GDB command.  Specifically, do both UIs
+# return to an interactive state, or does one (or both) of them get
+# stuck in a non-interactive state.
+
+load_lib gdbserver-support.exp
+
+standard_testfile
+
+if {[skip_gdbserver_tests]} {
+    return 0
+}
+
+if {[build_executable "failed to prepare" ${testfile} \
+	 ${srcfile}] == -1} {
+    return -1
+}
+
+clean_restart $testfile
+
+# Start gdbserver.
+set res [gdbserver_spawn "${binfile}"]
+set gdbserver_protocol [lindex $res 0]
+set gdbserver_gdbport [lindex $res 1]
+set gdbserver_pid [exp_pid -i $server_spawn_id]
+
+# Save the main UI's spawn ID.
+set gdb_main_spawn_id $gdb_spawn_id
+
+# Create the new PTY for the secondary console UI, issue the 'new-ui'
+# command, and wait for a prompt on the second UI.
+spawn -pty
+set extra_spawn_id $spawn_id
+set extra_tty_name $spawn_out(slave,name)
+gdb_test_multiple "new-ui console $extra_tty_name" "new-ui" {
+    -re "New UI allocated\r\n$gdb_prompt $" {
+	pass $gdb_test_name
+    }
+}
+with_spawn_id $extra_spawn_id {
+    gdb_test_multiple "" "initial prompt on extra console" {
+	-re "$gdb_prompt $" {
+	    pass $gdb_test_name
+	}
+    }
+}
+
+# Connect to the remote and continue its execution from the other UI.
+with_spawn_id $extra_spawn_id {
+    gdb_test "target $gdbserver_protocol $gdbserver_gdbport" ".*"
+    send_gdb "continue\n"
+}
+
+# We're going to kill the gdbserver, but before we do, lets make sure
+# that the inferior has started executing.
+with_spawn_id $server_spawn_id {
+    gdb_expect 3 {
+	-re "@@XX@@ Inferior Starting @@XX@@" {
+	    verbose -log "APB: Spotted running pattern"
+	}
+	timeout {
+	    verbose -log "APB: Didn't find running message"
+	}
+    }
+}
+
+# Interact with the main UI.
+with_spawn_id $gdb_main_spawn_id {
+    gdb_test "echo hello\\n" "hello" "interact with GDB's main UI"
+}
+
+# Now kill the gdbserver.
+remote_exec target "kill -9 $gdbserver_pid"
+
+# We expect to land back at a GDB prompt in both UIs.  Right now there
+# seems to be some "issues" with the prompt automatically being
+# displayed... Or maybe this is intentional and I'm missunderstanding
+# the expected behaviour.  Either way, getting a prompt isn't the
+# point of this test.  The point is that we should not be able to
+# interact with GDB from either interpreter now.
+
+with_spawn_id $gdb_main_spawn_id {
+    gdb_test "echo" "" \
+	"main UI, prompt after gdbserver dies"
+}
+
+with_spawn_id $extra_spawn_id {
+    gdb_test "echo" "" \
+	"extra UI, prompt after gdbserver dies"
+}