[PATCH] Have SIGTERM promptly quit GDB even when the dummy target is active

[PATCH] Have SIGTERM promptly quit GDB even when the dummy target is active

[Patrick Palka]

GDB currently does not promptly quit after receiving a SIGTERM while no
proper target is active.  This is because in handle_sigterm we currently
look at target_can_async_p to determine whether to asynchronously quit
GDB using an async signal handler or to asynchronously quit using the
quit flag.  However, target_can_async_p is always false under the dummy
target, so under this target we always use the quit flag and not the
async signal handler to signal that GDB should quit.  So GDB won't quit
until a code path that checks the quit flag is executed.

To fix this issue, this patch makes the SIGTERM handler no longer
inspect target_can_async_p, and instead makes the handler
unconditionally set the quit flag _and_ mark the corresponding async
signal handler, so that if the target is async (or if it's the dummy
target) then we will likely quit through the async signal handler, and
if it's not async then we will likely quit through the quit flag.  This
redundant approach is similar to how we handle SIGINT.

gdb/ChangeLog:

	* event-top.c (handle_sigterm): Don't inspect
	target_can_async_p.  Always set the quit flag and always mark
	the async signal handler.

gdb/testsuite/ChangeLog:

	* gdb.base/gdb-sigterm-2.exp: New test.
---
 gdb/event-top.c                          | 13 ++++--------
 gdb/testsuite/gdb.base/gdb-sigterm-2.exp | 35 ++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 9 deletions(-)
 create mode 100644 gdb/testsuite/gdb.base/gdb-sigterm-2.exp

diff --git a/gdb/event-top.c b/gdb/event-top.c
index e9cc2d7..1762e3b 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -876,15 +876,10 @@ handle_sigterm (int sig)
 {
   signal (sig, handle_sigterm);
 
-  /* Call quit_force in a signal safe way.
-     quit_force itself is not signal safe.  */
-  if (target_can_async_p ())
-    mark_async_signal_handler (async_sigterm_token);
-  else
-    {
-      sync_quit_force_run = 1;
-      set_quit_flag ();
-    }
+  sync_quit_force_run = 1;
+  set_quit_flag ();
+
+  mark_async_signal_handler (async_sigterm_token);
 }
 
 /* Do the quit.  All the checks have been done by the caller.  */
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm-2.exp b/gdb/testsuite/gdb.base/gdb-sigterm-2.exp
new file mode 100644
index 0000000..73313aa
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm-2.exp
@@ -0,0 +1,35 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2015 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/>.
+
+# Check that GDB promptly quits after receiving a SIGTERM while no proper
+# target is active.
+
+gdb_start
+
+set gdb_pid [exp_pid -i [board_info host fileid]]
+remote_exec host "kill -TERM $gdb_pid"
+
+set test "expect eof"
+
+gdb_expect 5 {
+    eof {
+	pass $test
+    }
+    timeout {
+	fail $test
+    }
+}
-- 
2.5.0.rc2.22.g69b1679.dirty

Re: [PATCH] Have SIGTERM promptly quit GDB even when the dummy target is active

[Pedro Alves]

On 07/24/2015 07:33 PM, Patrick Palka wrote:

> gdb/ChangeLog:
> 
> 	* event-top.c (handle_sigterm): Don't inspect
> 	target_can_async_p.  Always set the quit flag and always mark
> 	the async signal handler.

OK.

> +gdb_start
> +
> +set gdb_pid [exp_pid -i [board_info host fileid]]
> +remote_exec host "kill -TERM $gdb_pid"
> +
> +set test "expect eof"
> +
> +gdb_expect 5 {

Any reason not to use gdb_test_multiple, like:

 gdb_test_multiple "" $test {
   -timeout 5
   eof {
      pass $test
   }
 }

?

> +    eof {
> +	pass $test
> +    }
> +    timeout {
> +	fail $test
> +    }
> +}
> 

Thanks,
Pedro Alves

Re: [PATCH] Have SIGTERM promptly quit GDB even when the dummy target is active

[Pedro Alves]

On 07/27/2015 03:53 PM, Pedro Alves wrote:
> On 07/24/2015 07:33 PM, Patrick Palka wrote:
> 
>> gdb/ChangeLog:
>>
>> 	* event-top.c (handle_sigterm): Don't inspect
>> 	target_can_async_p.  Always set the quit flag and always mark
>> 	the async signal handler.
> 
> OK.

BTW, I notice that GDB leaves the terminal settings broken
after exiting on SIGTERM.  Is that something you're already
looking at?  (Mostly likely there's a target_terminal_ours
missing somewhere.)

Thanks,
Pedro Alves

Re: [PATCH] Have SIGTERM promptly quit GDB even when the dummy target is active

[Patrick Palka]

On Mon, Jul 27, 2015 at 11:21 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/27/2015 03:53 PM, Pedro Alves wrote:
>> On 07/24/2015 07:33 PM, Patrick Palka wrote:
>>
>>> gdb/ChangeLog:
>>>
>>>      * event-top.c (handle_sigterm): Don't inspect
>>>      target_can_async_p.  Always set the quit flag and always mark
>>>      the async signal handler.
>>
>> OK.
>
> BTW, I notice that GDB leaves the terminal settings broken
> after exiting on SIGTERM.  Is that something you're already
> looking at?  (Mostly likely there's a target_terminal_ours
> missing somewhere.)

I think we should do target_terminal_ours in quit_force.  (A similar
problem also exists in our SIGWINCH handler.)

Re: [PATCH] Have SIGTERM promptly quit GDB even when the dummy target is active

[Patrick Palka]

On Mon, Jul 27, 2015 at 10:53 AM, Pedro Alves <palves@redhat.com> wrote:
> On 07/24/2015 07:33 PM, Patrick Palka wrote:
>
>> gdb/ChangeLog:
>>
>>       * event-top.c (handle_sigterm): Don't inspect
>>       target_can_async_p.  Always set the quit flag and always mark
>>       the async signal handler.
>
> OK.
>
>> +gdb_start
>> +
>> +set gdb_pid [exp_pid -i [board_info host fileid]]
>> +remote_exec host "kill -TERM $gdb_pid"
>> +
>> +set test "expect eof"
>> +
>> +gdb_expect 5 {
>
> Any reason not to use gdb_test_multiple, like:
>
>  gdb_test_multiple "" $test {
>    -timeout 5
>    eof {
>       pass $test
>    }
>  }
>
> ?

No reason.  I'll change the test to use gdb_test_multiple and commit the patch.