[patch] Fix SIGTERM signal safety (PR gdb/15358) [refresh]

[patch] Fix SIGTERM signal safety (PR gdb/15358) [refresh]

[Jan Kratochvil]

Hi,

reposting with no changes, the last post is a year old one, it had been already
approved by Tom that time so I will check it in in some time.
	[patch] Fix SIGTERM signal safety (PR gdb/15358)
	https://sourceware.org/ml/gdb-patches/2013-07/msg00094.html
	Message-ID: <20130702200010.GA23478@host2.jankratochvil.net>

The statement about PR remote/15297 dependency is IMO false, this patch works
on its own.  The PR remote/15297 patch remains unfinished/pending, but that is
off-topic here.

------------------------------------------------------------------------------

gdb deadlock due to gdb calling calloc() in signal handler
http://sourceware.org/bugzilla/show_bug.cgi?id=15358

Patch depends in its functionality on:
	[patchv2 2/2] Fix CTRL-C for remote.c (PR remote/15297)
	http://sourceware.org/ml/gdb-patches/2013-06/msg00943.html
	Message-ID: <20130630181110.GB29548@host2.jankratochvil.net>

async mode seems easy.

The sync mode is a bit difficult - assuming it is safe to call quit_force from
any place of QUIT;.  OTOH the patch above assumes it can do:
	  if (check_quit_flag ())
	    send_interrupt_sequence ();
which clears quit_flag but we used set_quit_flag () to call quit_force, not
just to throw the quit exception.  This is why QUIT now checks also for
SYNC_QUIT_FORCE_RUN.

The change in linux-nat.c comes from testing i386-linux-nat.c (therefore
32-bit host GDB).  i386_linux_resume there calls QUIT; via target_read ().
This is a bug on its own, filed as:
	http://sourceware.org/bugzilla/show_bug.cgi?id=15713

But I have seen another bug in linux-nat.c, it was depending on PTRACE_KILL
but at least Linux kernel ptrace expert Oleg Nesterov considers PTRACE_KILL
superseded by kill(SIGKILL).  Therefore I used there (also) more safe SIGKILL 
so that the possibly inconsistent state of inferior from i386_linux_resume
does not matter.


No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and in
gdbserver mode.


Thanks,
Jan


gdb/
2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* defs.h (sync_quit_force_run): New declaration.
	(QUIT): Check also SYNC_QUIT_FORCE_RUN.
	* event-top.c (async_sigterm_handler): New declaration.
	(async_sigterm_token): New variable.
	(async_init_signals): Create also async_sigterm_token.
	(async_sigterm_handler): New function.
	(sync_quit_force_run): New variable.
	(handle_sigterm): Replace quit_force call by other calls.
	* linux-nat.c (linux_nat_kill): Use kill_callback first.
	Extend the comment for stop_callback.
	* utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.

gdb/testsuite/
2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* gdb.base/gdb-sigterm.c: New file.
	* gdb.base/gdb-sigterm.exp: New file.

diff --git a/gdb/defs.h b/gdb/defs.h
index 480133d..47da43a 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -171,6 +171,9 @@ extern int check_quit_flag (void);
 /* * Set the quit flag.  */
 extern void set_quit_flag (void);
 
+/* Flag that function quit should call quit_force.  */
+extern volatile int sync_quit_force_run;
+
 extern int immediate_quit;
 
 extern void quit (void);
@@ -183,7 +186,7 @@ extern void quit (void);
    needed.  */
 
 #define QUIT { \
-  if (check_quit_flag ()) quit (); \
+  if (check_quit_flag () || sync_quit_force_run) quit (); \
   if (deprecated_interactive_hook) deprecated_interactive_hook (); \
 }
 
diff --git a/gdb/event-top.c b/gdb/event-top.c
index fbe89bd..af1562c 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -72,6 +72,7 @@ static void async_float_handler (gdb_client_data);
 #ifdef STOP_SIGNAL
 static void async_stop_sig (gdb_client_data);
 #endif
+static void async_sigterm_handler (gdb_client_data arg);
 
 /* Readline offers an alternate interface, via callback
    functions.  These are all included in the file callback.c in the
@@ -135,6 +136,7 @@ static struct async_signal_handler *sigfpe_token;
 #ifdef STOP_SIGNAL
 static struct async_signal_handler *sigtstp_token;
 #endif
+static struct async_signal_handler *async_sigterm_token;
 
 /* Structure to save a partially entered command.  This is used when
    the user types '\' at the end of a command line.  This is necessary
@@ -770,6 +772,8 @@ async_init_signals (void)
     create_async_signal_handler (async_stop_sig, NULL);
 #endif
 
+  async_sigterm_token =
+    create_async_signal_handler (async_sigterm_handler, NULL);
 }
 
 /* Tell the event loop what to do if SIGINT is received.
@@ -797,13 +801,31 @@ handle_sigint (int sig)
   gdb_call_async_signal_handler (sigint_token, immediate_quit);
 }
 
+/* Handle GDB exit upon receiving SIGTERM if target_can_async_p ().  */
+
+static void
+async_sigterm_handler (gdb_client_data arg)
+{
+  quit_force (NULL, stdin == instream);
+}
+
+/* See defs.h.  */
+volatile int sync_quit_force_run;
+
 /* Quit GDB if SIGTERM is received.
    GDB would quit anyway, but this way it will clean up properly.  */
 void
 handle_sigterm (int sig)
 {
   signal (sig, handle_sigterm);
-  quit_force ((char *) 0, stdin == instream);
+
+  if (target_can_async_p ())
+    mark_async_signal_handler (async_sigterm_token);
+  else
+    {
+      sync_quit_force_run = 1;
+      set_quit_flag ();
+    }
 }
 
 /* Do the quit.  All the checks have been done by the caller.  */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b615423..ec84188 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3777,8 +3777,15 @@ linux_nat_kill (struct target_ops *ops)
     {
       ptid_t ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
 
+      /* Kill all LWP's before trying to stop them.  In rare cases the
+	 lwp_info state may not match the inferior and
+	 stop_wait_callback could lock up.  */
+      iterate_over_lwps (ptid, kill_callback, NULL);
+
       /* Stop all threads before killing them, since ptrace requires
-	 that the thread is stopped to sucessfully PTRACE_KILL.  */
+	 that the thread is stopped to sucessfully PTRACE_KILL.
+	 kill_callback normally already turned the inferior into a zombie
+	 except for old Linux kernels 2.4.x.  */
       iterate_over_lwps (ptid, stop_callback, NULL);
       /* ... and wait until all of them have reported back that
 	 they're no longer running.  */
diff --git a/gdb/utils.c b/gdb/utils.c
index 9d068c5..364470c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -999,6 +999,12 @@ print_sys_errmsg (const char *string, int errcode)
 void
 quit (void)
 {
+  if (sync_quit_force_run)
+    {
+      sync_quit_force_run = 0;
+      quit_force (NULL, stdin == instream);
+    }
+
 #ifdef __MSDOS__
   /* No steenking SIGINT will ever be coming our way when the
      program is resumed.  Don't lie.  */
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.c b/gdb/testsuite/gdb.base/gdb-sigterm.c
new file mode 100644
index 0000000..ffa09e4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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 <unistd.h>
+
+int
+main (void)
+{
+  alarm (60);
+
+  for (;;); /* loop-line */
+}
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
new file mode 100644
index 0000000..02d93ce
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
@@ -0,0 +1,94 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 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/>.
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
+    return -1
+}
+
+proc do_test { pass } {
+    global testfile gdb_prompt binfile pf_prefix
+
+    if ![runto_main] {
+	return -1
+    }
+
+    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
+		   temporary
+
+    # gdb_continue_to_breakpoint would print a pass message.
+    gdb_test "continue" "Temporary breakpoint .* loop-line .*" ""
+
+    gdb_test_no_output "set range-stepping off" ""
+    gdb_test_no_output "set debug infrun 1" ""
+    gdb_test_no_output "set debug lin-lwp 1" ""
+
+    set test "run a bit #$pass"
+    set abort 1
+    gdb_test_multiple "step" $test {
+	-re "infrun: stepping inside range" {
+	    # Suppress pass $test
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+    }
+    if $abort {
+	return
+    }
+
+    set gdb_pid [exp_pid -i [board_info host fileid]]
+    remote_exec host "kill -TERM ${gdb_pid}"
+
+    set test "expect eof #$pass"
+    set abort 1
+    set stepping 0
+    gdb_test_multiple "" $test {
+	eof {
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+	-re "infrun: stepping inside range" {
+	    incr stepping
+	    if { $stepping > 200 } {
+		fail "$test (stepping inside range $stepping times)"
+	    } else {
+		exp_continue
+	    }
+	}
+    }
+    if $abort {
+	return
+    }
+}
+
+
+for {set pass 0} {$pass < 50} {incr pass} {
+
+#    clean_restart ${testfile}
+#    gdb_test_no_output "set target-async off" ""
+#    with_test_prefix "sync" {
+#        do_test $pass
+#    }
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async on" ""
+    with_test_prefix "async" {
+        do_test $pass
+    }
+}
+pass "$pass SIGTERM passes"

[patchv2] Fix SIGTERM signal safety (PR gdb/15358) [refresh]

[Jan Kratochvil]

Hi,

the post several minutes ago (not the year old one) had accidentally corrupted
testcase.

------------------------------------------------------------------------------

reposting with no changes, the last post is a year old one, it had been already
approved by Tom that time so I will check it in in some time.
	[patch] Fix SIGTERM signal safety (PR gdb/15358)
	https://sourceware.org/ml/gdb-patches/2013-07/msg00094.html
	Message-ID: <20130702200010.GA23478@host2.jankratochvil.net>

The statement about PR remote/15297 dependency is IMO false, this patch works
on its own.  The PR remote/15297 patch remains unfinished/pending, but that is
off-topic here.

------------------------------------------------------------------------------

gdb deadlock due to gdb calling calloc() in signal handler
http://sourceware.org/bugzilla/show_bug.cgi?id=15358

Patch depends in its functionality on:
	[patchv2 2/2] Fix CTRL-C for remote.c (PR remote/15297)
	http://sourceware.org/ml/gdb-patches/2013-06/msg00943.html
	Message-ID: <20130630181110.GB29548@host2.jankratochvil.net>

async mode seems easy.

The sync mode is a bit difficult - assuming it is safe to call quit_force from
any place of QUIT;.  OTOH the patch above assumes it can do:
	  if (check_quit_flag ())
	    send_interrupt_sequence ();
which clears quit_flag but we used set_quit_flag () to call quit_force, not
just to throw the quit exception.  This is why QUIT now checks also for
SYNC_QUIT_FORCE_RUN.

The change in linux-nat.c comes from testing i386-linux-nat.c (therefore
32-bit host GDB).  i386_linux_resume there calls QUIT; via target_read ().
This is a bug on its own, filed as:
	http://sourceware.org/bugzilla/show_bug.cgi?id=15713

But I have seen another bug in linux-nat.c, it was depending on PTRACE_KILL
but at least Linux kernel ptrace expert Oleg Nesterov considers PTRACE_KILL
superseded by kill(SIGKILL).  Therefore I used there (also) more safe SIGKILL 
so that the possibly inconsistent state of inferior from i386_linux_resume
does not matter.


No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and in
gdbserver mode.


Thanks,
Jan


gdb/
2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* defs.h (sync_quit_force_run): New declaration.
	(QUIT): Check also SYNC_QUIT_FORCE_RUN.
	* event-top.c (async_sigterm_handler): New declaration.
	(async_sigterm_token): New variable.
	(async_init_signals): Create also async_sigterm_token.
	(async_sigterm_handler): New function.
	(sync_quit_force_run): New variable.
	(handle_sigterm): Replace quit_force call by other calls.
	* linux-nat.c (linux_nat_kill): Use kill_callback first.
	Extend the comment for stop_callback.
	* utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.

gdb/testsuite/
2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* gdb.base/gdb-sigterm.c: New file.
	* gdb.base/gdb-sigterm.exp: New file.

diff --git a/gdb/defs.h b/gdb/defs.h
index 480133d..47da43a 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -171,6 +171,9 @@ extern int check_quit_flag (void);
 /* * Set the quit flag.  */
 extern void set_quit_flag (void);
 
+/* Flag that function quit should call quit_force.  */
+extern volatile int sync_quit_force_run;
+
 extern int immediate_quit;
 
 extern void quit (void);
@@ -183,7 +186,7 @@ extern void quit (void);
    needed.  */
 
 #define QUIT { \
-  if (check_quit_flag ()) quit (); \
+  if (check_quit_flag () || sync_quit_force_run) quit (); \
   if (deprecated_interactive_hook) deprecated_interactive_hook (); \
 }
 
diff --git a/gdb/event-top.c b/gdb/event-top.c
index fbe89bd..af1562c 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -72,6 +72,7 @@ static void async_float_handler (gdb_client_data);
 #ifdef STOP_SIGNAL
 static void async_stop_sig (gdb_client_data);
 #endif
+static void async_sigterm_handler (gdb_client_data arg);
 
 /* Readline offers an alternate interface, via callback
    functions.  These are all included in the file callback.c in the
@@ -135,6 +136,7 @@ static struct async_signal_handler *sigfpe_token;
 #ifdef STOP_SIGNAL
 static struct async_signal_handler *sigtstp_token;
 #endif
+static struct async_signal_handler *async_sigterm_token;
 
 /* Structure to save a partially entered command.  This is used when
    the user types '\' at the end of a command line.  This is necessary
@@ -770,6 +772,8 @@ async_init_signals (void)
     create_async_signal_handler (async_stop_sig, NULL);
 #endif
 
+  async_sigterm_token =
+    create_async_signal_handler (async_sigterm_handler, NULL);
 }
 
 /* Tell the event loop what to do if SIGINT is received.
@@ -797,13 +801,31 @@ handle_sigint (int sig)
   gdb_call_async_signal_handler (sigint_token, immediate_quit);
 }
 
+/* Handle GDB exit upon receiving SIGTERM if target_can_async_p ().  */
+
+static void
+async_sigterm_handler (gdb_client_data arg)
+{
+  quit_force (NULL, stdin == instream);
+}
+
+/* See defs.h.  */
+volatile int sync_quit_force_run;
+
 /* Quit GDB if SIGTERM is received.
    GDB would quit anyway, but this way it will clean up properly.  */
 void
 handle_sigterm (int sig)
 {
   signal (sig, handle_sigterm);
-  quit_force ((char *) 0, stdin == instream);
+
+  if (target_can_async_p ())
+    mark_async_signal_handler (async_sigterm_token);
+  else
+    {
+      sync_quit_force_run = 1;
+      set_quit_flag ();
+    }
 }
 
 /* Do the quit.  All the checks have been done by the caller.  */
diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
index b615423..ec84188 100644
--- a/gdb/linux-nat.c
+++ b/gdb/linux-nat.c
@@ -3777,8 +3777,15 @@ linux_nat_kill (struct target_ops *ops)
     {
       ptid_t ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
 
+      /* Kill all LWP's before trying to stop them.  In rare cases the
+	 lwp_info state may not match the inferior and
+	 stop_wait_callback could lock up.  */
+      iterate_over_lwps (ptid, kill_callback, NULL);
+
       /* Stop all threads before killing them, since ptrace requires
-	 that the thread is stopped to sucessfully PTRACE_KILL.  */
+	 that the thread is stopped to sucessfully PTRACE_KILL.
+	 kill_callback normally already turned the inferior into a zombie
+	 except for old Linux kernels 2.4.x.  */
       iterate_over_lwps (ptid, stop_callback, NULL);
       /* ... and wait until all of them have reported back that
 	 they're no longer running.  */
diff --git a/gdb/utils.c b/gdb/utils.c
index 9d068c5..364470c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -999,6 +999,12 @@ print_sys_errmsg (const char *string, int errcode)
 void
 quit (void)
 {
+  if (sync_quit_force_run)
+    {
+      sync_quit_force_run = 0;
+      quit_force (NULL, stdin == instream);
+    }
+
 #ifdef __MSDOS__
   /* No steenking SIGINT will ever be coming our way when the
      program is resumed.  Don't lie.  */
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.c b/gdb/testsuite/gdb.base/gdb-sigterm.c
new file mode 100644
index 0000000..ffa09e4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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 <unistd.h>
+
+int
+main (void)
+{
+  alarm (60);
+
+  for (;;); /* loop-line */
+}
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
new file mode 100644
index 0000000..8baeb96
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
@@ -0,0 +1,94 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 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/>.
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
+    return -1
+}
+
+proc do_test { pass } {
+    global testfile gdb_prompt binfile pf_prefix
+
+    if ![runto_main] {
+	return -1
+    }
+
+    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
+		   temporary
+
+    # gdb_continue_to_breakpoint would print a pass message.
+    gdb_test "continue" "Temporary breakpoint .* loop-line .*" ""
+
+    gdb_test_no_output "set range-stepping off" ""
+    gdb_test_no_output "set debug infrun 1" ""
+    gdb_test_no_output "set debug lin-lwp 1" ""
+
+    set test "run a bit #$pass"
+    set abort 1
+    gdb_test_multiple "step" $test {
+	-re "infrun: stepping inside range" {
+	    # Suppress pass $test
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+    }
+    if $abort {
+	return
+    }
+
+    set gdb_pid [exp_pid -i [board_info host fileid]]
+    remote_exec host "kill -TERM ${gdb_pid}"
+
+    set test "expect eof #$pass"
+    set abort 1
+    set stepping 0
+    gdb_test_multiple "" $test {
+	eof {
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+	-re "infrun: stepping inside range" {
+	    incr stepping
+	    if { $stepping > 200 } {
+		fail "$test (stepping inside range $stepping times)"
+	    } else {
+		exp_continue
+	    }
+	}
+    }
+    if $abort {
+	return
+    }
+}
+
+
+for {set pass 0} {$pass < 50} {incr pass} {
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async off" ""
+    with_test_prefix "sync" {
+        do_test $pass
+    }
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async on" ""
+    with_test_prefix "async" {
+        do_test $pass
+    }
+}
+pass "$pass SIGTERM passes"

Re: [patchv2] Fix SIGTERM signal safety (PR gdb/15358) [refresh]

[Doug Evans]

Jan Kratochvil writes:
 > [...]

Hi.
I have some comments and requests for this patch.

A high level comment is that I guess this patch means it's
now more important to fix the spots of gdb where QUITs aren't
called for long times (I was working on a common one, debug info
reading, but had to defer that to work on something else).

 > The sync mode is a bit difficult - assuming it is safe to call quit_force from
 > any place of QUIT;.  OTOH the patch above assumes it can do:
 > 	  if (check_quit_flag ())
 > 	    send_interrupt_sequence ();
 > which clears quit_flag but we used set_quit_flag () to call quit_force, not
 > just to throw the quit exception.  This is why QUIT now checks also for
 > SYNC_QUIT_FORCE_RUN.
 > 
 > The change in linux-nat.c comes from testing i386-linux-nat.c (therefore
 > 32-bit host GDB).  i386_linux_resume there calls QUIT; via target_read ().
 > This is a bug on its own, filed as:
 > 	http://sourceware.org/bugzilla/show_bug.cgi?id=15713
 > 
 > But I have seen another bug in linux-nat.c, it was depending on PTRACE_KILL
 > but at least Linux kernel ptrace expert Oleg Nesterov considers PTRACE_KILL
 > superseded by kill(SIGKILL).  Therefore I used there (also) more safe SIGKILL 
 > so that the possibly inconsistent state of inferior from i386_linux_resume
 > does not matter.
 > 
 > 
 > No regressions on {x86_64,x86_64-m32,i686}-fedora19pre-linux-gnu and in
 > gdbserver mode.
 > 
 > 
 > Thanks,
 > Jan
 > 
 > 
 > gdb/
 > 2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
 > 
 > 	PR gdb/15358
 > 	* defs.h (sync_quit_force_run): New declaration.
 > 	(QUIT): Check also SYNC_QUIT_FORCE_RUN.
 > 	* event-top.c (async_sigterm_handler): New declaration.
 > 	(async_sigterm_token): New variable.
 > 	(async_init_signals): Create also async_sigterm_token.
 > 	(async_sigterm_handler): New function.
 > 	(sync_quit_force_run): New variable.
 > 	(handle_sigterm): Replace quit_force call by other calls.
 > 	* linux-nat.c (linux_nat_kill): Use kill_callback first.
 > 	Extend the comment for stop_callback.
 > 	* utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.
 > 
 > gdb/testsuite/
 > 2013-07-02  Jan Kratochvil  <jan.kratochvil@redhat.com>
 > 
 > 	PR gdb/15358
 > 	* gdb.base/gdb-sigterm.c: New file.
 > 	* gdb.base/gdb-sigterm.exp: New file.
 > 
 > diff --git a/gdb/defs.h b/gdb/defs.h
 > index 480133d..47da43a 100644
 > --- a/gdb/defs.h
 > +++ b/gdb/defs.h
 > @@ -171,6 +171,9 @@ extern int check_quit_flag (void);
 >  /* * Set the quit flag.  */
 >  extern void set_quit_flag (void);
 >  
 > +/* Flag that function quit should call quit_force.  */
 > +extern volatile int sync_quit_force_run;
 > +
 >  extern int immediate_quit;
 >  
 >  extern void quit (void);
 > @@ -183,7 +186,7 @@ extern void quit (void);
 >     needed.  */
 >  
 >  #define QUIT { \
 > -  if (check_quit_flag ()) quit (); \
 > +  if (check_quit_flag () || sync_quit_force_run) quit (); \
 >    if (deprecated_interactive_hook) deprecated_interactive_hook (); \
 >  }
 >  
 > diff --git a/gdb/event-top.c b/gdb/event-top.c
 > index fbe89bd..af1562c 100644
 > --- a/gdb/event-top.c
 > +++ b/gdb/event-top.c
 > @@ -72,6 +72,7 @@ static void async_float_handler (gdb_client_data);
 >  #ifdef STOP_SIGNAL
 >  static void async_stop_sig (gdb_client_data);
 >  #endif
 > +static void async_sigterm_handler (gdb_client_data arg);
 >  
 >  /* Readline offers an alternate interface, via callback
 >     functions.  These are all included in the file callback.c in the
 > @@ -135,6 +136,7 @@ static struct async_signal_handler *sigfpe_token;
 >  #ifdef STOP_SIGNAL
 >  static struct async_signal_handler *sigtstp_token;
 >  #endif
 > +static struct async_signal_handler *async_sigterm_token;
 >  
 >  /* Structure to save a partially entered command.  This is used when
 >     the user types '\' at the end of a command line.  This is necessary
 > @@ -770,6 +772,8 @@ async_init_signals (void)
 >      create_async_signal_handler (async_stop_sig, NULL);
 >  #endif
 >  
 > +  async_sigterm_token =
 > +    create_async_signal_handler (async_sigterm_handler, NULL);
 >  }

I realize SIGTERM isn't handled exactly like SIGINT, but IWBN
to keep all the SIGTERM bits together in this function.
How about moving the above snippet to after this:

  signal (SIGINT, handle_sigint);
  sigint_token =
    create_async_signal_handler (async_request_quit, NULL);
  signal (SIGTERM, handle_sigterm);

btw, the rule has been effectively changed to require = being
put on the next line.
While I certainly wouldn't ask for whitespace change in this patch,
[IOW I'm not asking for the existing code to be fixed]
I'm curious what the rule is for handling this here.
For the patch at hand, should it be written as this:

  async_sigterm_token =
    create_async_signal_handler (async_sigterm_handler, NULL);

or this:

  async_sigterm_token
    = create_async_signal_handler (async_sigterm_handler, NULL);

?
I honestly don't know.
It's not important of course, just wondering.

 >  
 >  /* Tell the event loop what to do if SIGINT is received.
 > @@ -797,13 +801,31 @@ handle_sigint (int sig)
 >    gdb_call_async_signal_handler (sigint_token, immediate_quit);
 >  }
 >  
 > +/* Handle GDB exit upon receiving SIGTERM if target_can_async_p ().  */
 > +
 > +static void
 > +async_sigterm_handler (gdb_client_data arg)
 > +{
 > +  quit_force (NULL, stdin == instream);
 > +}
 > +
 > +/* See defs.h.  */
 > +volatile int sync_quit_force_run;
 > +
 >  /* Quit GDB if SIGTERM is received.
 >     GDB would quit anyway, but this way it will clean up properly.  */
 >  void
 >  handle_sigterm (int sig)
 >  {
 >    signal (sig, handle_sigterm);
 > -  quit_force ((char *) 0, stdin == instream);

I think we need a comment here explaining *why* we can't
just call quit_force here.

 > +
 > +  if (target_can_async_p ())
 > +    mark_async_signal_handler (async_sigterm_token);
 > +  else
 > +    {
 > +      sync_quit_force_run = 1;
 > +      set_quit_flag ();
 > +    }
 >  }
 >  
 >  /* Do the quit.  All the checks have been done by the caller.  */
 > diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c
 > index b615423..ec84188 100644
 > --- a/gdb/linux-nat.c
 > +++ b/gdb/linux-nat.c
 > @@ -3777,8 +3777,15 @@ linux_nat_kill (struct target_ops *ops)
 >      {
 >        ptid_t ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
 >  
 > +      /* Kill all LWP's before trying to stop them.  In rare cases the
 > +	 lwp_info state may not match the inferior and
 > +	 stop_wait_callback could lock up.  */
 > +      iterate_over_lwps (ptid, kill_callback, NULL);
 > +
 >        /* Stop all threads before killing them, since ptrace requires
 > -	 that the thread is stopped to sucessfully PTRACE_KILL.  */
 > +	 that the thread is stopped to sucessfully PTRACE_KILL.
 > +	 kill_callback normally already turned the inferior into a zombie
 > +	 except for old Linux kernels 2.4.x.  */
 >        iterate_over_lwps (ptid, stop_callback, NULL);
 >        /* ... and wait until all of them have reported back that
 >  	 they're no longer running.  */

This part feels sufficiently outside the scope of this patch
that IWBN if this were a separate patch.

 > diff --git a/gdb/utils.c b/gdb/utils.c
 > index 9d068c5..364470c 100644
 > --- a/gdb/utils.c
 > +++ b/gdb/utils.c
 > @@ -999,6 +999,12 @@ print_sys_errmsg (const char *string, int errcode)
 >  void
 >  quit (void)
 >  {
 > +  if (sync_quit_force_run)
 > +    {
 > +      sync_quit_force_run = 0;
 > +      quit_force (NULL, stdin == instream);
 > +    }
 > +

Bleah.  Not your problem, and obviously I'm not suggesting
fixing the names in this patch,  but "quit" is overloaded.
quit is really quit_current_command (or some such),
and quit_force is really quit_and_exit (or some such).

I raise the issue because the patch is conflating them,
and I'm wondering if that's a good idea.

 > [...]
 >
 > diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
 > new file mode 100644
 > index 0000000..8baeb96
 > --- /dev/null
 > +++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
 > @@ -0,0 +1,94 @@
 > +# This testcase is part of GDB, the GNU debugger.
 > +#
 > +# Copyright 2013 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/>.
 > +
 > +standard_testfile
 > +
 > +if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
 > +    return -1
 > +}
 > +
 > +proc do_test { pass } {
 > +    global testfile gdb_prompt binfile pf_prefix
 > +
 > +    if ![runto_main] {
 > +	return -1
 > +    }
 > +
 > +    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
 > +		   temporary
 > +
 > +    # gdb_continue_to_breakpoint would print a pass message.
 > +    gdb_test "continue" "Temporary breakpoint .* loop-line .*" ""
 > +
 > +    gdb_test_no_output "set range-stepping off" ""
 > +    gdb_test_no_output "set debug infrun 1" ""
 > +    gdb_test_no_output "set debug lin-lwp 1" ""
 > +
 > +    set test "run a bit #$pass"
 > +    set abort 1
 > +    gdb_test_multiple "step" $test {
 > +	-re "infrun: stepping inside range" {
 > +	    # Suppress pass $test
 > +	    verbose -log "$pf_prefix $test"
 > +	    set abort 0
 > +	}
 > +    }
 > +    if $abort {
 > +	return
 > +    }
 > +
 > +    set gdb_pid [exp_pid -i [board_info host fileid]]
 > +    remote_exec host "kill -TERM ${gdb_pid}"
 > +
 > +    set test "expect eof #$pass"
 > +    set abort 1
 > +    set stepping 0
 > +    gdb_test_multiple "" $test {
 > +	eof {
 > +	    verbose -log "$pf_prefix $test"
 > +	    set abort 0
 > +	}
 > +	-re "infrun: stepping inside range" {
 > +	    incr stepping
 > +	    if { $stepping > 200 } {
 > +		fail "$test (stepping inside range $stepping times)"
 > +	    } else {
 > +		exp_continue
 > +	    }
 > +	}
 > +    }
 > +    if $abort {
 > +	return
 > +    }
 > +}
 > +
 > +
 > +for {set pass 0} {$pass < 50} {incr pass} {

How come 50 passes?
At the least a comment is required explaining why.

 > +
 > +    clean_restart ${testfile}
 > +    gdb_test_no_output "set target-async off" ""
 > +    with_test_prefix "sync" {
 > +        do_test $pass
 > +    }
 > +
 > +    clean_restart ${testfile}
 > +    gdb_test_no_output "set target-async on" ""
 > +    with_test_prefix "async" {
 > +        do_test $pass
 > +    }
 > +}
 > +pass "$pass SIGTERM passes"

-- 
/dje

[patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Jan Kratochvil]

On Sat, 15 Mar 2014 20:44:03 +0100, Doug Evans wrote:
> A high level comment is that I guess this patch means it's
> now more important to fix the spots of gdb where QUITs aren't
> called for long times

I do not see it so much related.  The problem is if GDB locks up and never
terminates like in this PR.


> Jan Kratochvil writes:
>  > +  async_sigterm_token =
>  > +    create_async_signal_handler (async_sigterm_handler, NULL);
>  >  }
> 
> I realize SIGTERM isn't handled exactly like SIGINT, but IWBN
> to keep all the SIGTERM bits together in this function.
> How about moving the above snippet to after this:

I agree, done.


> btw, the rule has been effectively changed to require = being
> put on the next line.
> While I certainly wouldn't ask for whitespace change in this patch,
> [IOW I'm not asking for the existing code to be fixed]

This is wrong.  If there should be some rule benefiting some unification of
the code base then the codebase has to be reformatted to such rule first.


> I'm curious what the rule is for handling this here.
> For the patch at hand, should it be written as this:
> 
>   async_sigterm_token =
>     create_async_signal_handler (async_sigterm_handler, NULL);
> 
> or this:
> 
>   async_sigterm_token
>     = create_async_signal_handler (async_sigterm_handler, NULL);
> 
> ?
> I honestly don't know.

This is one of the problems of GDB.

I have changed my patch but I do not know according to which rule and it no
longer matches the existing code around.


[...]
>  >  void
>  >  handle_sigterm (int sig)
>  >  {
>  >    signal (sig, handle_sigterm);
>  > -  quit_force ((char *) 0, stdin == instream);
> 
> I think we need a comment here explaining *why* we can't
> just call quit_force here.

Added there:
  /* 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 ();
>  > +    }
>  >  }


>  > --- a/gdb/linux-nat.c
>  > +++ b/gdb/linux-nat.c
>  > @@ -3777,8 +3777,15 @@ linux_nat_kill (struct target_ops *ops)
>  >      {
>  >        ptid_t ptid = pid_to_ptid (ptid_get_pid (inferior_ptid));
>  >  
>  > +      /* Kill all LWP's before trying to stop them.  In rare cases the
>  > +	 lwp_info state may not match the inferior and
>  > +	 stop_wait_callback could lock up.  */
>  > +      iterate_over_lwps (ptid, kill_callback, NULL);
>  > +
>  >        /* Stop all threads before killing them, since ptrace requires
>  > -	 that the thread is stopped to sucessfully PTRACE_KILL.  */
>  > +	 that the thread is stopped to sucessfully PTRACE_KILL.
>  > +	 kill_callback normally already turned the inferior into a zombie
>  > +	 except for old Linux kernels 2.4.x.  */
>  >        iterate_over_lwps (ptid, stop_callback, NULL);
>  >        /* ... and wait until all of them have reported back that
>  >  	 they're no longer running.  */
> 
> This part feels sufficiently outside the scope of this patch
> that IWBN if this were a separate patch.

OK, moved this one to a different patch.


[...]
>  > +for {set pass 0} {$pass < 50} {incr pass} {
> 
> How come 50 passes?
> At the least a comment is required explaining why.

Added:

# Testcase was FAILing approx. on 10th pass with unpatched GDB.
# 50 runs should be approx. a safe number to be sure it is fixed now.


Thanks,
Jan


gdb/
2014-03-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* defs.h (sync_quit_force_run): New declaration.
	(QUIT): Check also SYNC_QUIT_FORCE_RUN.
	* event-top.c (async_sigterm_handler): New declaration.
	(async_sigterm_token): New variable.
	(async_init_signals): Create also async_sigterm_token.
	(async_sigterm_handler): New function.
	(sync_quit_force_run): New variable.
	(handle_sigterm): Replace quit_force call by other calls.
	* utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.

gdb/testsuite/
2014-03-16  Jan Kratochvil  <jan.kratochvil@redhat.com>

	PR gdb/15358
	* gdb.base/gdb-sigterm.c: New file.
	* gdb.base/gdb-sigterm.exp: New file.

diff --git a/gdb/defs.h b/gdb/defs.h
index 480133d..47da43a 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -171,6 +171,9 @@ extern int check_quit_flag (void);
 /* * Set the quit flag.  */
 extern void set_quit_flag (void);
 
+/* Flag that function quit should call quit_force.  */
+extern volatile int sync_quit_force_run;
+
 extern int immediate_quit;
 
 extern void quit (void);
@@ -183,7 +186,7 @@ extern void quit (void);
    needed.  */
 
 #define QUIT { \
-  if (check_quit_flag ()) quit (); \
+  if (check_quit_flag () || sync_quit_force_run) quit (); \
   if (deprecated_interactive_hook) deprecated_interactive_hook (); \
 }
 
diff --git a/gdb/event-top.c b/gdb/event-top.c
index fbe89bd..c353f48 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -72,6 +72,7 @@ static void async_float_handler (gdb_client_data);
 #ifdef STOP_SIGNAL
 static void async_stop_sig (gdb_client_data);
 #endif
+static void async_sigterm_handler (gdb_client_data arg);
 
 /* Readline offers an alternate interface, via callback
    functions.  These are all included in the file callback.c in the
@@ -135,6 +136,7 @@ static struct async_signal_handler *sigfpe_token;
 #ifdef STOP_SIGNAL
 static struct async_signal_handler *sigtstp_token;
 #endif
+static struct async_signal_handler *async_sigterm_token;
 
 /* Structure to save a partially entered command.  This is used when
    the user types '\' at the end of a command line.  This is necessary
@@ -733,6 +735,8 @@ async_init_signals (void)
   sigint_token =
     create_async_signal_handler (async_request_quit, NULL);
   signal (SIGTERM, handle_sigterm);
+  async_sigterm_token
+    = create_async_signal_handler (async_sigterm_handler, NULL);
 
   /* If SIGTRAP was set to SIG_IGN, then the SIG_IGN will get passed
      to the inferior and breakpoints will be ignored.  */
@@ -769,7 +773,6 @@ async_init_signals (void)
   sigtstp_token =
     create_async_signal_handler (async_stop_sig, NULL);
 #endif
-
 }
 
 /* Tell the event loop what to do if SIGINT is received.
@@ -797,13 +800,33 @@ handle_sigint (int sig)
   gdb_call_async_signal_handler (sigint_token, immediate_quit);
 }
 
+/* Handle GDB exit upon receiving SIGTERM if target_can_async_p ().  */
+
+static void
+async_sigterm_handler (gdb_client_data arg)
+{
+  quit_force (NULL, stdin == instream);
+}
+
+/* See defs.h.  */
+volatile int sync_quit_force_run;
+
 /* Quit GDB if SIGTERM is received.
    GDB would quit anyway, but this way it will clean up properly.  */
 void
 handle_sigterm (int sig)
 {
   signal (sig, handle_sigterm);
-  quit_force ((char *) 0, stdin == instream);
+
+  /* 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 ();
+    }
 }
 
 /* Do the quit.  All the checks have been done by the caller.  */
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.c b/gdb/testsuite/gdb.base/gdb-sigterm.c
new file mode 100644
index 0000000..ffa09e4
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.c
@@ -0,0 +1,26 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2013 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 <unistd.h>
+
+int
+main (void)
+{
+  alarm (60);
+
+  for (;;); /* loop-line */
+}
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
new file mode 100644
index 0000000..b57680d
--- /dev/null
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
@@ -0,0 +1,96 @@
+# This testcase is part of GDB, the GNU debugger.
+#
+# Copyright 2013 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/>.
+
+standard_testfile
+
+if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
+    return -1
+}
+
+proc do_test { pass } {
+    global testfile gdb_prompt binfile pf_prefix
+
+    if ![runto_main] {
+	return -1
+    }
+
+    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
+		   temporary
+
+    # gdb_continue_to_breakpoint would print a pass message.
+    gdb_test "continue" "Temporary breakpoint .* loop-line .*" ""
+
+    gdb_test_no_output "set range-stepping off" ""
+    gdb_test_no_output "set debug infrun 1" ""
+    gdb_test_no_output "set debug lin-lwp 1" ""
+
+    set test "run a bit #$pass"
+    set abort 1
+    gdb_test_multiple "step" $test {
+	-re "infrun: stepping inside range" {
+	    # Suppress pass $test
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+    }
+    if $abort {
+	return
+    }
+
+    set gdb_pid [exp_pid -i [board_info host fileid]]
+    remote_exec host "kill -TERM ${gdb_pid}"
+
+    set test "expect eof #$pass"
+    set abort 1
+    set stepping 0
+    gdb_test_multiple "" $test {
+	eof {
+	    verbose -log "$pf_prefix $test"
+	    set abort 0
+	}
+	-re "infrun: stepping inside range" {
+	    incr stepping
+	    if { $stepping > 200 } {
+		fail "$test (stepping inside range $stepping times)"
+	    } else {
+		exp_continue
+	    }
+	}
+    }
+    if $abort {
+	return
+    }
+}
+
+# Testcase was FAILing approx. on 10th pass with unpatched GDB.
+# 50 runs should be approx. a safe number to be sure it is fixed now.
+
+for {set pass 0} {$pass < 50} {incr pass} {
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async off" ""
+    with_test_prefix "sync" {
+        do_test $pass
+    }
+
+    clean_restart ${testfile}
+    gdb_test_no_output "set target-async on" ""
+    with_test_prefix "async" {
+        do_test $pass
+    }
+}
+pass "$pass SIGTERM passes"
diff --git a/gdb/utils.c b/gdb/utils.c
index 9d068c5..364470c 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -999,6 +999,12 @@ print_sys_errmsg (const char *string, int errcode)
 void
 quit (void)
 {
+  if (sync_quit_force_run)
+    {
+      sync_quit_force_run = 0;
+      quit_force (NULL, stdin == instream);
+    }
+
 #ifdef __MSDOS__
   /* No steenking SIGINT will ever be coming our way when the
      program is resumed.  Don't lie.  */

Re: [patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Doug Evans]

On Sun, Mar 16, 2014 at 6:53 AM, Jan Kratochvil
<jan.kratochvil@redhat.com> wrote:
> gdb/
> 2014-03-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>         PR gdb/15358
>         * defs.h (sync_quit_force_run): New declaration.
>         (QUIT): Check also SYNC_QUIT_FORCE_RUN.
>         * event-top.c (async_sigterm_handler): New declaration.
>         (async_sigterm_token): New variable.
>         (async_init_signals): Create also async_sigterm_token.
>         (async_sigterm_handler): New function.
>         (sync_quit_force_run): New variable.
>         (handle_sigterm): Replace quit_force call by other calls.
>         * utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.
>
> gdb/testsuite/
> 2014-03-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
>
>         PR gdb/15358
>         * gdb.base/gdb-sigterm.c: New file.
>         * gdb.base/gdb-sigterm.exp: New file.

Fine by me.
Thanks.

[commit] [patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Jan Kratochvil]

On Mon, 17 Mar 2014 23:35:21 +0100, Doug Evans wrote:
> On Sun, Mar 16, 2014 at 6:53 AM, Jan Kratochvil
> <jan.kratochvil@redhat.com> wrote:
> > gdb/
> > 2014-03-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> >
> >         PR gdb/15358
> >         * defs.h (sync_quit_force_run): New declaration.
> >         (QUIT): Check also SYNC_QUIT_FORCE_RUN.
> >         * event-top.c (async_sigterm_handler): New declaration.
> >         (async_sigterm_token): New variable.
> >         (async_init_signals): Create also async_sigterm_token.
> >         (async_sigterm_handler): New function.
> >         (sync_quit_force_run): New variable.
> >         (handle_sigterm): Replace quit_force call by other calls.
> >         * utils.c (quit): Call quit_force if SYNC_QUIT_FORCE_RUN.
> >
> > gdb/testsuite/
> > 2014-03-16  Jan Kratochvil  <jan.kratochvil@redhat.com>
> >
> >         PR gdb/15358
> >         * gdb.base/gdb-sigterm.c: New file.
> >         * gdb.base/gdb-sigterm.exp: New file.
> 
> Fine by me.

Checked in:
	06c868a8dc5ef46ab7dd6601c8bc2f417e415af9


Thanks,
Jan

Re: [patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Yufeng Zhang]

On 03/16/14 13:53, Jan Kratochvil wrote:
[snip]
> diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
> new file mode 100644
> index 0000000..b57680d
> --- /dev/null
> +++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
> @@ -0,0 +1,96 @@
> +# This testcase is part of GDB, the GNU debugger.
> +#
> +# Copyright 2013 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/>.
> +
> +standard_testfile
> +
> +if { [build_executable ${testfile}.exp ${testfile}] == -1 } {
> +    return -1
> +}
> +
> +proc do_test { pass } {
> +    global testfile gdb_prompt binfile pf_prefix
> +
> +    if ![runto_main] {
> +       return -1
> +    }
> +
> +    gdb_breakpoint "${testfile}.c:[gdb_get_line_number "loop-line" ${testfile}.c]" \
> +                  temporary
> +
> +    # gdb_continue_to_breakpoint would print a pass message.
> +    gdb_test "continue" "Temporary breakpoint .* loop-line .*" ""
> +
> +    gdb_test_no_output "set range-stepping off" ""
> +    gdb_test_no_output "set debug infrun 1" ""
> +    gdb_test_no_output "set debug lin-lwp 1" ""

The test fails on aarch64-none-elf.  Should the test only run on Linux 
targets?

Thanks,
Yufeng

Re: [patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]

On Sun, 23 Mar 2014 16:19:08 +0100, Yufeng Zhang wrote:
> On 03/16/14 13:53, Jan Kratochvil wrote:
> >+    gdb_test_no_output "set range-stepping off" ""
> >+    gdb_test_no_output "set debug infrun 1" ""
> >+    gdb_test_no_output "set debug lin-lwp 1" ""
> 
> The test fails on aarch64-none-elf.  Should the test only run on
> Linux targets?

I see that lin-lwp probably does not exist on such GDB.

That "set debug lin-lwp 1" command even is not needed for the functionality of
this testcase.

Does this patch fix it on aarch64-none-elf or should it be really disabled?
Could you post me off-list gdb.log?


Thanks,
Jan

[-- Attachment #2: 1 --]
[-- Type: text/plain, Size: 600 bytes --]

gdb/testsuite/
2014-03-23  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/gdb-sigterm.exp (do_test): Remove "set debug lin-lwp 1".

diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
index b57680d..f52517c 100644
--- a/gdb/testsuite/gdb.base/gdb-sigterm.exp
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
@@ -36,7 +36,6 @@ proc do_test { pass } {
 
     gdb_test_no_output "set range-stepping off" ""
     gdb_test_no_output "set debug infrun 1" ""
-    gdb_test_no_output "set debug lin-lwp 1" ""
 
     set test "run a bit #$pass"
     set abort 1

Re: [patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Yufeng Zhang]

Hi Jan,

On 03/23/14 16:57, Jan Kratochvil wrote:
> On Sun, 23 Mar 2014 16:19:08 +0100, Yufeng Zhang wrote:
>> On 03/16/14 13:53, Jan Kratochvil wrote:
>>> +    gdb_test_no_output "set range-stepping off" ""
>>> +    gdb_test_no_output "set debug infrun 1" ""
>>> +    gdb_test_no_output "set debug lin-lwp 1" ""
>>
>> The test fails on aarch64-none-elf.  Should the test only run on
>> Linux targets?
>
> I see that lin-lwp probably does not exist on such GDB.
>
> That "set debug lin-lwp 1" command even is not needed for the functionality of
> this testcase.
>
> Does this patch fix it on aarch64-none-elf or should it be really disabled?

The patch does fix the failure.  Thanks for your help!

Thanks,
Yufeng

[obv commit] [patchv3] Fix SIGTERM signal safety (PR gdb/15358)

[Jan Kratochvil]

[-- Attachment #1: Type: text/plain, Size: 193 bytes --]

On Mon, 24 Mar 2014 17:33:01 +0100, Yufeng Zhang wrote:
> The patch does fix the failure.  Thanks for your help!

Checked in as obvious:
	d23487918b315cccd04c2b60a7cf7775920caecb


Thanks,
Jan

[-- Attachment #2: Type: message/rfc822, Size: 1586 bytes --]

From: Jan Kratochvil <jan.kratochvil@redhat.com>
Subject: [PATCH] testsuite: Remove needless linux-nat requirement in gdb-sigterm.exp.
Date: Mon, 24 Mar 2014 17:37:32 +0100

That "set debug lin-lwp 1" command even is not needed for the functionality of
this testcase.

This patch does fix a testcase error on aarch64-none-elf.

gdb/testsuite/
2014-03-24  Jan Kratochvil  <jan.kratochvil@redhat.com>

	* gdb.base/gdb-sigterm.exp (do_test): Remove "set debug lin-lwp 1".

Message-ID: <20140323165745.GA23830@host2.jankratochvil.net>
---
 gdb/testsuite/ChangeLog                | 4 ++++
 gdb/testsuite/gdb.base/gdb-sigterm.exp | 1 -
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index 3581132..88ba2a3 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,7 @@
+2014-03-24  Jan Kratochvil  <jan.kratochvil@redhat.com>
+
+	* gdb.base/gdb-sigterm.exp (do_test): Remove "set debug lin-lwp 1".
+
 2014-03-22  Doug Evans  <xdje42@gmail.com>
 
 	* gdb.python/python.exp (python not supported): Verify multi-line
diff --git a/gdb/testsuite/gdb.base/gdb-sigterm.exp b/gdb/testsuite/gdb.base/gdb-sigterm.exp
index b57680d..f52517c 100644
--- a/gdb/testsuite/gdb.base/gdb-sigterm.exp
+++ b/gdb/testsuite/gdb.base/gdb-sigterm.exp
@@ -36,7 +36,6 @@ proc do_test { pass } {
 
     gdb_test_no_output "set range-stepping off" ""
     gdb_test_no_output "set debug infrun 1" ""
-    gdb_test_no_output "set debug lin-lwp 1" ""
 
     set test "run a bit #$pass"
     set abort 1
-- 
1.8.5.3