Re: [RFC 2/2] Change SIGINT handler for extension languages only when target terminal is ours

[Pedro Alves <palves at redhat dot com>]

On 12/21/2015 11:30 AM, Yao Qi wrote:
> I see a timeout in gdb.base/random-signal.exp,
> 
>  Continuing.^M
>  PASS: gdb.base/random-signal.exp: continue
>  ^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type
>  exceptions.KeyboardInterrupt'>: ^M
>  FAIL: gdb.base/random-signal.exp: stop with control-c (timeout)
> 
> it can be reproduced by running random-signal.exp with native-gdbserver
> in a loop, like this, and the fail will be shown in about 20 runs,
> 
> $ (set -e; while true; do make check RUNTESTFLAGS="--target_board=native-gdbserver random-signal.exp"; done)

The reason this doesn't trigger with native debugging is that
in that case, gdb is sharing the terminal with gdb, and with job
control, then the ctrl-c always reaches the inferior instead.

We can trigger the issue with native debugging as well, if
we "attach" instead of "run", like in the patch at the bottom:

$ (set -e; while true; do make check RUNTESTFLAGS="random-signal.exp"; done)
...
FAIL: gdb.base/random-signal.exp: attach: stop with control-c (timeout)
FAIL: gdb.base/random-signal.exp: attach: p wait_gdb = 0 (timeout)
...

gdb.log:

(gdb) PASS: gdb.base/random-signal.exp: attach: watch v
continue
Continuing.
PASS: gdb.base/random-signal.exp: attach: continue
^CPython Exception <type 'exceptions.KeyboardInterrupt'> <type 'exceptions.KeyboardInterrupt'>: 
FAIL: gdb.base/random-signal.exp: attach: stop with control-c (timeout)
p wait_gdb = 0
FAIL: gdb.base/random-signal.exp: attach: p wait_gdb = 0 (timeout)



> 
> In the test, the program is being single-stepped for software watchpoint,
> and in each internal stop, python unwinder sniffer is used,
> 
>  #0  pyuw_sniffer (self=<optimised out>, this_frame=<optimised out>, cache_ptr=0xd554f8) at /home/yao/SourceCode/gnu/gdb/git/gdb/python/py-unwind.c:608
>  #1  0x00000000006a10ae in frame_unwind_try_unwinder (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8, unwinder=0xecd540)
>      at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:107
>  #2  0x00000000006a143f in frame_unwind_find_by_frame (this_frame=this_frame@entry=0xd554e0, this_cache=this_cache@entry=0xd554f8)
>      at /home/yao/SourceCode/gnu/gdb/git/gdb/frame-unwind.c:163
>  #3  0x000000000069dc6b in compute_frame_id (fi=0xd554e0) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:454
>  #4  get_prev_frame_if_no_cycle (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1781
>  #5  0x000000000069fdb9 in get_prev_frame_always_1 (this_frame=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1955
>  #6  get_prev_frame_always (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:1971
>  #7  0x00000000006a04b1 in get_prev_frame (this_frame=this_frame@entry=0xd55410) at /home/yao/SourceCode/gnu/gdb/git/gdb/frame.c:2213
> 
> when GDB goes to python extension, or other extension language, the
> SIGINT handler is changed, and is restored when GDB leaves extension
> language.  GDB only stays in extension language for a very short period
> in this case, but if ctrl-c is pressed at that moment, python extension
> will handle the SIGINT, and exceptions.KeyboardInterrupt is shown.
> 
> Language extension is used in GDB side rather than inferior side,
> so GDB should only change SIGINT handler for extension language when
> the terminal is ours (not inferior's).  This is what this patch does.
> With this patch applied, I run random-signal.exp in a loop for 18
> hours, and no fail is shown.
> 
> gdb:
> 
> 2015-12-21  Yao Qi  <yao.qi@linaro.org>
> 
> 	* extension.c: Include target.h.
> 	(set_active_ext_lang): Only call install_gdb_sigint_handler,
> 	check_quit_flag, and set_quit_flag if target_terminal_is_inferior
> 	returns false.
> 	(restore_active_ext_lang): Likewise.
> ---
>  gdb/extension.c | 51 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 30 insertions(+), 21 deletions(-)
> 
> diff --git a/gdb/extension.c b/gdb/extension.c
> index 1b5365a..1147df9 100644
> --- a/gdb/extension.c
> +++ b/gdb/extension.c
> @@ -22,6 +22,7 @@
>  
>  #include "defs.h"
>  #include <signal.h>
> +#include "target.h"
>  #include "auto-load.h"
>  #include "breakpoint.h"
>  #include "event-top.h"
> @@ -746,19 +747,24 @@ set_active_ext_lang (const struct extension_language_defn *now_active)
>      = XCNEW (struct active_ext_lang_state);
>  
>    previous->ext_lang = active_ext_lang;
> +  previous->sigint_handler.handler_saved = 0;
>    active_ext_lang = now_active;
>  
> -  /* If the newly active extension language uses cooperative SIGINT handling
> -     then ensure GDB's SIGINT handler is installed.  */
> -  if (now_active->language == EXT_LANG_GDB
> -      || now_active->ops->check_quit_flag != NULL)
> -    install_gdb_sigint_handler (&previous->sigint_handler);
> -
> -  /* If there's a SIGINT recorded in the cooperative extension languages,
> -     move it to the new language, or save it in GDB's global flag if the newly
> -     active extension language doesn't use cooperative SIGINT handling.  */
> -  if (check_quit_flag ())
> -    set_quit_flag ();
> +  if (!target_terminal_is_inferior ())

I think this should check for terminal_is_ours instead.  We also
don't  want this with terminal_ours_for_output.  In that case, _input_
is still sent to the inferior.

Here's the test tweak:

>From 2cf334c489d6278ea73e1f82905b9726f7d502fc Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 4 Jan 2016 14:25:31 +0000
Subject: [PATCH] Test gdb.base/random-signal.exp with "attach"

This exposes the issued fixed by:
  https://sourceware.org/ml/gdb-patches/2015-12/msg00423.html
to native debugging as well.

gdb/testsuite/ChangeLog:
2016-01-04  Pedro Alves  <palves@redhat.com>

	* gdb.base/random-signal.c (wait): New global.
	(main): Use it.
	* gdb.base/random-signal.exp (do_test): New procedure, with body
	of testcase moved in.
	(top level) Call it twice, once with "run" and once with "attach".
---
 gdb/testsuite/gdb.base/random-signal.c   |  7 +++-
 gdb/testsuite/gdb.base/random-signal.exp | 58 ++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/gdb/testsuite/gdb.base/random-signal.c b/gdb/testsuite/gdb.base/random-signal.c
index f4c4b9f..45adbf0 100644
--- a/gdb/testsuite/gdb.base/random-signal.c
+++ b/gdb/testsuite/gdb.base/random-signal.c
@@ -19,11 +19,16 @@
 
 int v;
 
+/* GDB clears this.  */
+volatile int wait_gdb = 1;
+
 int main()
 {
   /* Don't let the test case run forever.  */
   alarm (60);
 
-  for (;;)
+  while (wait_gdb)
     ;
+
+  return 0;
 }
diff --git a/gdb/testsuite/gdb.base/random-signal.exp b/gdb/testsuite/gdb.base/random-signal.exp
index a6170cc..9aa5843 100644
--- a/gdb/testsuite/gdb.base/random-signal.exp
+++ b/gdb/testsuite/gdb.base/random-signal.exp
@@ -30,19 +30,55 @@ if {[prepare_for_testing $testfile.exp $testfile $srcfile debug]} {
     return -1
 }
 
-if {![runto_main]} {
-    return -1
+# Set a software watchpoint, continue, wait a bit and stop the target
+# with ctrl-c.  A software watchpoint forces the target to
+# single-step.
+proc do_test {} {
+    global binfile
+
+    gdb_test_no_output "set can-use-hw-watchpoints 0"
+    gdb_test "watch v" "Watchpoint .*"
+    gdb_test_multiple "continue" "continue" {
+	-re "Continuing" {
+	    pass "continue"
+	}
+    }
+
+    # For this to work we must be sure to consume the "Continuing."
+    # message first, or GDB's signal handler may not be in place.
+    after 500 {send_gdb "\003"}
+    gdb_test "" "Program received signal SIGINT.*" "stop with control-c"
+
+    gdb_test "p wait_gdb = 0" " = 0"
 }
 
-gdb_test_no_output "set can-use-hw-watchpoints 0"
-gdb_test "watch v" "Watchpoint .*"
-gdb_test_multiple "continue" "continue" {
-    -re "Continuing" {
-	pass "continue"
+# With native debugging and "run" (with job control), the ctrl-c
+# always reaches the inferior, not gdb, even if ctrl-c is pressed
+# while gdb is processing the internal software watchtpoint
+# single-step.  With remote debugging, the ctrl-c reaches GDB first.
+with_test_prefix "run" {
+    clean_restart $binfile
+
+    if {![runto_main]} {
+	return -1
     }
+
+    do_test
 }
 
-# For this to work we must be sure to consume the "Continuing."
-# message first, or GDB's signal handler may not be in place.
-after 500 {send_gdb "\003"}
-gdb_test "" "Program received signal SIGINT.*" "stop with control-c"
+# With "attach" however, even with native debugging, the ctrl-c always
+# reaches GDB first.  Test that as well.
+with_test_prefix "attach" {
+    if {[can_spawn_for_attach]} {
+	clean_restart $binfile
+
+	set test_spawn_id [spawn_wait_for_attach $binfile]
+	set testpid [spawn_id_get_pid $test_spawn_id]
+
+	gdb_test "attach $testpid" "Attaching to.*process $testpid.*libc.*" "attach"
+
+	do_test
+
+	kill_wait_spawned_process $test_spawn_id
+    }
+}
-- 
1.9.3