diff mbox

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

Message ID 86h9isx92q.fsf@gmail.com
State New, archived
Headers

Commit Message

Yao Qi Jan. 5, 2016, 4:52 p.m. UTC 
  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.

Comments

Pedro Alves Jan. 7, 2016, 5:24 p.m. UTC | #1 
On 01/05/2016 04:52 PM, Yao Qi wrote:
> 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".

Indeed.

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

This LGTM.

> Your patch looks good to me.

I'll push it after yours goes in.

Thanks,
Pedro Alves
Yao Qi Jan. 8, 2016, 11:08 a.m. UTC | #2 
Pedro Alves <palves@redhat.com> writes:

> This LGTM.
>

Patch is pushed in.

>> Your patch looks good to me.
>
> I'll push it after yours goes in.

Please do that.  Thanks for new test.
Pedro Alves Jan. 12, 2016, 1:11 p.m. UTC | #3 
On 01/08/2016 11:08 AM, Yao Qi wrote:
> Pedro Alves <palves@redhat.com> writes:

>>> Your patch looks good to me.
>>
>> I'll push it after yours goes in.
> 
> Please do that.  Thanks for new test.
> 

Done.

Thanks,
Pedro Alves
diff mbox

Patch

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