[RFC] Avoid some crashes at SIGINT

[Daniel Jacobowitz <drow at false dot org>]

GDB has several SIGINT handlers which call longjmp.  This is
problematic for at least two reasons.  One is that we could be in the
middle of something unwise to longjmp out of, for instance malloc.  In
practice, this never happens because we're usually waiting for I/O
when one of the relevant handlers is invoked, but there are a number
of places where it could definitely happen.

The other, more immediately for me, is Windows (sorry...).  Windows
has a unique implementation of SIGINT handlers, which makes sense for
GUI apps but not so much for ported console apps: it starts a new
thread and runs the handler in that thread.  Obviously, if you longjmp
from one thread to a jmp_buf saved in another (still running!) thread,
badness ensues.

My goals in fixing this were to hide the Windows ugliness, and to fit
in nicely with GDB's asynchronous event loop.  Since we do not return
to the primary event loop during target actions (for the current,
non-async GDB), I couldn't rely on the event loop entirely.  But I
could use the same token mechanism and thus share the bodies of
handlers for async mode with the Windows case.

The new interface is gdb_call_async_signal_handler.  SIGINT handlers,
when running in non-async mode or otherwise wanting an "immediate"
reaction, can call this and pass it the async handler's token instead
of duplicating the async handler's code.  Then for POSIX platforms
the function is called immediately and for Windows it is deferred
to gdb_select.  The event loop always uses gdb_select and so do
the remote targets which had SIGINT handlers.

Some day, we may want to use the same mechanism that I've added here
for POSIX platforms too.  That will close the race conditions of where
we are when the signal arrives.  That requires async GDB; Vladimir has
been looking at Nick's and Apple's patches for this again.  For now,
I left the POSIX platform behavior unchanged.

I've tested this on Windows and Linux.  Saying yes at the "Give up
(and stop debugging it)? (y or n)" prompt no longer crashes GDB.

Anyone have comments on this approach?

-- 
Daniel Jacobowitz
CodeSourcery

2008-02-26  Daniel Jacobowitz  <dan@codesourcery.com>

	* Makefile.in (mingw-hdep.o, posix-hdep.o, remote-fileio.o): Update.
	* event-loop.c (call_async_signal_handler): New.
	* event-loop.h (call_async_signal_handler)
	(gdb_call_async_signal_handler): Declare.
	* event-top.c (handle_sigint): Use gdb_call_async_signal_handler.
	* mingw-hdep.c (sigint_event, sigint_handler): New.
	(gdb_select): Use them.
	(gdb_call_async_signal_handler, _initialize_mingw_hdep): New functions.
	* posix-hdep.c (gdb_call_async_signal_handler): New function.
	* remote-fileio.c (sigint_fileio_token, async_remote_fileio_interrupt):
	New.
	(remote_fileio_ctrl_c_signal_handler): Use
	gdb_call_async_signal_handler.
	(initialize_remote_fileio): Initialize sigint_fileio_token.
	* remote.c (initialize_sigint_signal_handler, handle_remote_sigint): Do
	not initialize tokens here.
	(handle_remote_sigint_twice): Likewise.  Reinstall
	handle_remote_sigint.
	(async_remote_interrupt_twice): Just call interrupt_query.
	(cleanup_sigint_signal_handler): Do not delete tokens.
	(remote_interrupt, remote_interrupt_twice): Use
	gdb_call_async_signal_handler.
	(interrupt_query): Reinstall the default signal handler.
	(_initialize_remote): Initialize tokens here.

Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.984
diff -u -p -r1.984 Makefile.in
--- Makefile.in	20 Feb 2008 15:45:20 -0000	1.984
+++ Makefile.in	27 Feb 2008 00:30:39 -0000
@@ -2455,8 +2455,8 @@ mep-tdep.o: $(defs_h) $(frame_h) $(frame
 	$(floatformat_h) $(sim_regno_h) $(disasm_h) $(trad_frame_h) \
 	$(reggroups_h) $(elf_bfd_h) $(elf_mep_h) $(gdb_assert_h) \
 	$(mep_desc_h) $(mep_opc_h) $(prologue_value_h) $(infcall_h)
-mingw-hdep.o: mingw-hdep.c $(defs_h) $(serial_h) $(gdb_assert_h) \
-	$(gdb_select_h) $(gdb_string_h)
+mingw-hdep.o: mingw-hdep.c $(defs_h) $(serial_h) $(event_loop_h) \
+	$(gdb_assert_h) $(gdb_select_h) $(gdb_string_h)
 minsyms.o: minsyms.c $(defs_h) $(gdb_string_h) $(symtab_h) $(bfd_h) \
 	$(symfile_h) $(objfiles_h) $(demangle_h) $(value_h) $(cp_abi_h)
 mips64obsd-nat.o: mips64obsd-nat.c $(defs_h) $(inferior_h) $(regcache_h) \
@@ -2553,7 +2553,8 @@ p-exp.o: p-exp.c $(defs_h) $(gdb_string_
 p-lang.o: p-lang.c $(defs_h) $(gdb_string_h) $(symtab_h) $(gdbtypes_h) \
 	$(expression_h) $(parser_defs_h) $(language_h) $(p_lang_h) \
 	$(valprint_h) $(value_h)
-posix-hdep.o: posix-hdep.c $(defs_h) $(gdb_string_h) $(gdb_select_h)
+posix-hdep.o: posix-hdep.c $(defs_h) $(event_loop_h) $(gdb_string_h) \
+	$(gdb_select_h)
 ppcbug-rom.o: ppcbug-rom.c $(defs_h) $(gdbcore_h) $(target_h) $(monitor_h) \
 	$(serial_h) $(regcache_h)
 ppc-linux-nat.o: ppc-linux-nat.c $(defs_h) $(gdb_string_h) $(frame_h) \
@@ -2624,7 +2625,7 @@ remote.o: remote.c $(defs_h) $(gdb_strin
 	$(target_descriptions_h) $(gdb_fileio_h)
 remote-fileio.o: remote-fileio.c $(defs_h) $(gdb_string_h) $(gdbcmd_h) \
 	$(remote_h) $(gdb_fileio_h) $(gdb_wait_h) $(gdb_stat_h) \
-	$(exceptions_h) $(remote_fileio_h)
+	$(exceptions_h) $(remote_fileio_h) $(event_loop_h)
 remote-m32r-sdi.o: remote-m32r-sdi.c $(defs_h) $(gdbcmd_h) $(gdbcore_h) \
 	$(inferior_h) $(target_h) $(regcache_h) $(gdb_string_h) $(serial_h)
 remote-mips.o: remote-mips.c $(defs_h) $(inferior_h) $(bfd_h) $(symfile_h) \
Index: event-loop.c
===================================================================
RCS file: /cvs/src/src/gdb/event-loop.c,v
retrieving revision 1.34
diff -u -p -r1.34 event-loop.c
--- event-loop.c	1 Jan 2008 22:53:09 -0000	1.34
+++ event-loop.c	27 Feb 2008 00:30:39 -0000
@@ -891,6 +891,15 @@ create_async_signal_handler (sig_handler
   return async_handler_ptr;
 }
 
+/* Call the handler from HANDLER immediately.  This function runs
+   signal handlers when returning to the event loop would be too
+   slow.  */
+void
+call_async_signal_handler (struct async_signal_handler *handler)
+{
+  (*handler->proc) (handler->client_data);
+}
+
 /* Mark the handler (ASYNC_HANDLER_PTR) as ready. This information will
    be used when the handlers are invoked, after we have waited for
    some event.  The caller of this function is the interrupt handler
Index: event-loop.h
===================================================================
RCS file: /cvs/src/src/gdb/event-loop.h,v
retrieving revision 1.8
diff -u -p -r1.8 event-loop.h
--- event-loop.h	1 Jan 2008 22:53:09 -0000	1.8
+++ event-loop.h	27 Feb 2008 00:30:39 -0000
@@ -92,3 +92,16 @@ extern struct async_signal_handler *
 extern void delete_async_signal_handler (struct async_signal_handler **async_handler_ptr);
 extern int create_timer (int milliseconds, timer_handler_func * proc, gdb_client_data client_data);
 extern void delete_timer (int id);
+
+/* Call the handler from ASYNC_HANDLER_PTR immediately.  This function
+   runs signal handlers when returning to the event loop would be too
+   slow.  Do not call this directly; use gdb_call_async_signal_handler,
+   below.  */
+void call_async_signal_handler (struct async_signal_handler *handler);
+
+/* Wrapper for the body of signal handlers.  Call this function from
+   any SIGINT handler which needs to access GDB data structures or
+   escape via longjmp.  This triggers either immediately (for POSIX
+   platforms), or from gdb_select (for MinGW).  */
+
+void gdb_call_async_signal_handler (struct async_signal_handler *handler);
Index: event-top.c
===================================================================
RCS file: /cvs/src/src/gdb/event-top.c,v
retrieving revision 1.56
diff -u -p -r1.56 event-top.c
--- event-top.c	1 Jan 2008 22:53:09 -0000	1.56
+++ event-top.c	27 Feb 2008 00:30:40 -0000
@@ -977,7 +977,7 @@ handle_sigint (int sig)
      that point, though, the command that we want to interrupt needs to
      finish first, which is unacceptable. */
   if (immediate_quit)
-    async_request_quit (0);
+    gdb_call_async_signal_handler (sigint_token);
   else
     /* If immediate quit is not set, we process SIGINT the next time
        through the loop, which is fine. */
Index: mingw-hdep.c
===================================================================
RCS file: /cvs/src/src/gdb/mingw-hdep.c,v
retrieving revision 1.8
diff -u -p -r1.8 mingw-hdep.c
--- mingw-hdep.c	1 Jan 2008 22:53:12 -0000	1.8
+++ mingw-hdep.c	27 Feb 2008 00:30:40 -0000
@@ -19,6 +19,7 @@
 
 #include "defs.h"
 #include "serial.h"
+#include "event-loop.h"
 
 #include "gdb_assert.h"
 #include "gdb_select.h"
@@ -26,6 +27,14 @@
 
 #include <windows.h>
 
+/* This event is signalled whenever an asynchronous SIGINT handler
+   needs to perform an action in the main thread.  */
+static HANDLE sigint_event;
+
+/* When SIGINT_EVENT is signalled, gdb_select will call this
+   function.  */
+struct async_signal_handler *sigint_handler;
+
 /* The strerror() function can return NULL for errno values that are
    out of range.  Provide a "safe" version that always returns a
    printable string.
@@ -141,14 +150,9 @@ gdb_select (int n, fd_set *readfds, fd_s
 	  handles[num_handles++] = except;
 	}
     }
-  /* If we don't need to wait for any handles, we are done.  */
-  if (!num_handles)
-    {
-      if (timeout)
-	Sleep (timeout->tv_sec * 1000 + timeout->tv_usec / 1000);
 
-      return 0;
-    }
+  gdb_assert (num_handles < MAXIMUM_WAIT_OBJECTS);
+  handles[num_handles++] = sigint_event;
 
   event = WaitForMultipleObjects (num_handles,
 				  handles,
@@ -203,5 +207,32 @@ gdb_select (int n, fd_set *readfds, fd_s
 	}
     }
 
+  if (h == sigint_event
+      || WaitForSingleObject (sigint_event, 0) == WAIT_OBJECT_0)
+    {
+      call_async_signal_handler (sigint_handler);
+
+      if (num_ready == 0)
+	errno = EINTR;
+    }
+
   return num_ready;
 }
+
+/* Wrapper for the body of signal handlers.  On Windows systems, a
+   SIGINT handler runs in its own thread.  We can't longjmp from
+   there, and we shouldn't even prompt the user.  Delay HANDLER
+   until the main thread is next in gdb_select.  */
+
+void
+gdb_call_async_signal_handler (struct async_signal_handler *handler)
+{
+  sigint_handler = handler;
+  SetEvent (sigint_event);
+}
+
+void
+_initialize_mingw_hdep (void)
+{
+  sigint_event = CreateEvent (0, FALSE, FALSE, 0);
+}
Index: posix-hdep.c
===================================================================
RCS file: /cvs/src/src/gdb/posix-hdep.c,v
retrieving revision 1.5
diff -u -p -r1.5 posix-hdep.c
--- posix-hdep.c	1 Jan 2008 22:53:12 -0000	1.5
+++ posix-hdep.c	27 Feb 2008 00:30:40 -0000
@@ -18,6 +18,7 @@
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include "event-loop.h"
 
 #include "gdb_string.h"
 
@@ -50,3 +51,12 @@ gdb_select (int n, fd_set *readfds, fd_s
 {
   return select (n, readfds, writefds, exceptfds, timeout);
 }
+
+/* Wrapper for the body of signal handlers.  Nothing special needed on
+   POSIX platforms.  */
+
+void
+gdb_call_async_signal_handler (struct async_signal_handler *handler)
+{
+  call_async_signal_handler (handler);
+}
Index: remote-fileio.c
===================================================================
RCS file: /cvs/src/src/gdb/remote-fileio.c,v
retrieving revision 1.27
diff -u -p -r1.27 remote-fileio.c
--- remote-fileio.c	1 Jan 2008 22:53:12 -0000	1.27
+++ remote-fileio.c	27 Feb 2008 00:30:40 -0000
@@ -28,6 +28,7 @@
 #include "gdb_stat.h"
 #include "exceptions.h"
 #include "remote-fileio.h"
+#include "event-loop.h"
 
 #include <fcntl.h>
 #include <sys/time.h>
@@ -47,6 +48,8 @@ static struct {
 
 static int remote_fio_system_call_allowed = 0;
 
+static struct async_signal_handler *sigint_fileio_token;
+
 static int
 remote_fileio_init_fd_map (void)
 {
@@ -504,12 +507,18 @@ remote_fileio_sig_exit (void)
 }
 
 static void
+async_remote_fileio_interrupt (gdb_client_data arg)
+{
+  deprecated_throw_reason (RETURN_QUIT);
+}
+
+static void
 remote_fileio_ctrl_c_signal_handler (int signo)
 {
   remote_fileio_sig_set (SIG_IGN);
   remote_fio_ctrl_c_flag = 1;
   if (!remote_fio_no_longjmp)
-    deprecated_throw_reason (RETURN_QUIT);
+    gdb_call_async_signal_handler (sigint_fileio_token);
   remote_fileio_sig_set (remote_fileio_ctrl_c_signal_handler);
 }
 
@@ -1451,6 +1460,9 @@ void
 initialize_remote_fileio (struct cmd_list_element *remote_set_cmdlist,
 			  struct cmd_list_element *remote_show_cmdlist)
 {
+  sigint_fileio_token =
+    create_async_signal_handler (async_remote_fileio_interrupt, NULL);
+
   add_cmd ("system-call-allowed", no_class,
 	   set_system_call_allowed,
 	   _("Set if the host system(3) call is allowed for the target."),
Index: remote.c
===================================================================
RCS file: /cvs/src/src/gdb/remote.c,v
retrieving revision 1.280
diff -u -p -r1.280 remote.c
--- remote.c	25 Feb 2008 09:59:06 -0000	1.280
+++ remote.c	27 Feb 2008 00:30:40 -0000
@@ -3158,8 +3158,6 @@ remote_async_resume (ptid_t ptid, int st
 static void
 initialize_sigint_signal_handler (void)
 {
-  sigint_remote_token =
-    create_async_signal_handler (async_remote_interrupt, NULL);
   signal (SIGINT, handle_remote_sigint);
 }
 
@@ -3168,8 +3166,6 @@ static void
 handle_remote_sigint (int sig)
 {
   signal (sig, handle_remote_sigint_twice);
-  sigint_remote_twice_token =
-    create_async_signal_handler (async_remote_interrupt_twice, NULL);
   mark_async_signal_handler_wrapper (sigint_remote_token);
 }
 
@@ -3179,9 +3175,7 @@ handle_remote_sigint (int sig)
 static void
 handle_remote_sigint_twice (int sig)
 {
-  signal (sig, handle_sigint);
-  sigint_remote_twice_token =
-    create_async_signal_handler (inferior_event_handler_wrapper, NULL);
+  signal (sig, handle_remote_sigint);
   mark_async_signal_handler_wrapper (sigint_remote_twice_token);
 }
 
@@ -3203,13 +3197,8 @@ async_remote_interrupt_twice (gdb_client
 {
   if (remote_debug)
     fprintf_unfiltered (gdb_stdlog, "remote_interrupt_twice called\n");
-  /* Do something only if the target was not killed by the previous
-     cntl-C.  */
-  if (target_executing)
-    {
-      interrupt_query ();
-      signal (SIGINT, handle_remote_sigint);
-    }
+
+  interrupt_query ();
 }
 
 /* Reinstall the usual SIGINT handlers, after the target has
@@ -3218,10 +3207,6 @@ static void
 cleanup_sigint_signal_handler (void *dummy)
 {
   signal (SIGINT, handle_sigint);
-  if (sigint_remote_twice_token)
-    delete_async_signal_handler (&sigint_remote_twice_token);
-  if (sigint_remote_token)
-    delete_async_signal_handler (&sigint_remote_token);
 }
 
 /* Send ^C to target to halt it.  Target will respond, and send us a
@@ -3239,10 +3224,7 @@ remote_interrupt (int signo)
   /* If this doesn't work, try more severe steps.  */
   signal (signo, remote_interrupt_twice);
 
-  if (remote_debug)
-    fprintf_unfiltered (gdb_stdlog, "remote_interrupt called\n");
-
-  target_stop ();
+  gdb_call_async_signal_handler (sigint_remote_token);
 }
 
 /* The user typed ^C twice.  */
@@ -3251,7 +3233,7 @@ static void
 remote_interrupt_twice (int signo)
 {
   signal (signo, ofunc);
-  interrupt_query ();
+  gdb_call_async_signal_handler (sigint_remote_twice_token);
   signal (signo, remote_interrupt);
 }
 
@@ -3282,6 +3264,7 @@ interrupt_query (void)
 Give up (and stop debugging it)? "))
     {
       target_mourn_inferior ();
+      signal (SIGINT, handle_sigint);
       deprecated_throw_reason (RETURN_QUIT);
     }
 
@@ -7499,6 +7482,12 @@ _initialize_remote (void)
   /* Hook into new objfile notification.  */
   observer_attach_new_objfile (remote_new_objfile);
 
+  /* Set up signal handlers.  */
+  sigint_remote_token =
+    create_async_signal_handler (async_remote_interrupt, NULL);
+  sigint_remote_twice_token =
+    create_async_signal_handler (inferior_event_handler_wrapper, NULL);
+
 #if 0
   init_remote_threadtests ();
 #endif