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

[Yao Qi <qiyaoltc at gmail dot com>]

Pedro Alves <palves@redhat.com> writes:

> The reason this doesn't trigger with native debugging is that
> in that case, gdb is sharing the terminal with gdb, and with job

I think you meant "gdb is sharing the terminal with the inferior".

> 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)
>
>

Yes, it is a good test case.


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

OK, target_terminal_is_ours is added and is used here.  Patch is updated
as below,

>
> 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".

Your patch looks good to me.

-- 
Yao (éå)

From 43887b575d10f65def86f80794094404b6a3709e Mon Sep 17 00:00:00 2001
From: Yao Qi <yao.qi@linaro.org>
Date: Fri, 18 Dec 2015 10:58:45 +0000
Subject: [PATCH] Change SIGINT handler for extension languages only when
 target terminal is ours

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)

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 language extension, 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:

2016-01-05  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_ours
	returns false.
	(restore_active_ext_lang): Likewise.
	* target.c (target_terminal_is_ours): New function.
	* target.h (target_terminal_is_ours): Declare.

diff --git a/gdb/extension.c b/gdb/extension.c
index 5c23bcc..d2c5669 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_ours ())
+    {
+      /* 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 ();
+    }
 
   return previous;
 }
@@ -772,16 +778,19 @@ restore_active_ext_lang (struct active_ext_lang_state *previous)
 
   active_ext_lang = previous->ext_lang;
 
-  /* Restore the previous SIGINT handler if one was saved.  */
-  if (previous->sigint_handler.handler_saved)
-    install_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_ours ())
+    {
+      /* Restore the previous SIGINT handler if one was saved.  */
+      if (previous->sigint_handler.handler_saved)
+	install_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 ();
+    }
   xfree (previous);
 }
 
diff --git a/gdb/target.c b/gdb/target.c
index d331fe4..e88d60c 100644
--- a/gdb/target.c
+++ b/gdb/target.c
@@ -466,6 +466,14 @@ target_terminal_is_inferior (void)
 
 /* See target.h.  */
 
+int
+target_terminal_is_ours (void)
+{
+  return (terminal_state == terminal_is_ours);
+}
+
+/* See target.h.  */
+
 void
 target_terminal_inferior (void)
 {
diff --git a/gdb/target.h b/gdb/target.h
index dd2896f..e1419a9 100644
--- a/gdb/target.h
+++ b/gdb/target.h
@@ -1503,6 +1503,10 @@ extern int target_remove_breakpoint (struct gdbarch *gdbarch,
 
 extern int target_terminal_is_inferior (void);
 
+/* Returns true if our terminal settings are in effect.  */
+
+extern int target_terminal_is_ours (void);
+
 /* Initialize the terminal settings we record for the inferior,
    before we actually run the inferior.  */