From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19067 invoked by alias); 5 Mar 2008 17:24:46 -0000 Received: (qmail 19057 invoked by uid 22791); 5 Mar 2008 17:24:44 -0000 X-Spam-Check-By: sourceware.org Received: from NaN.false.org (HELO nan.false.org) (208.75.86.248) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 05 Mar 2008 17:24:17 +0000 Received: from nan.false.org (localhost [127.0.0.1]) by nan.false.org (Postfix) with ESMTP id 7ADA598119 for ; Wed, 5 Mar 2008 17:24:15 +0000 (GMT) Received: from caradoc.them.org (22.svnf5.xdsl.nauticom.net [209.195.183.55]) by nan.false.org (Postfix) with ESMTP id 0EF5D9802B for ; Wed, 5 Mar 2008 17:24:15 +0000 (GMT) Received: from drow by caradoc.them.org with local (Exim 4.69) (envelope-from ) id 1JWxLi-0003oT-CW for gdb-patches@sourceware.org; Wed, 05 Mar 2008 12:24:14 -0500 Date: Wed, 05 Mar 2008 17:24:00 -0000 From: Daniel Jacobowitz To: gdb-patches@sourceware.org Subject: Re: [RFC] Avoid some crashes at SIGINT Message-ID: <20080305172414.GA14641@caradoc.them.org> Mail-Followup-To: gdb-patches@sourceware.org References: <20080227025444.GA22935@caradoc.them.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20080227025444.GA22935@caradoc.them.org> User-Agent: Mutt/1.5.17 (2007-12-11) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-03/txt/msg00034.txt.bz2 On Tue, Feb 26, 2008 at 09:54:44PM -0500, Daniel Jacobowitz wrote: > 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. I also needed to call gdb_call_async_signal_handler for things that didn't require immediate action, because if we don't then we hang around in gdb_select; on Unix a signal handler running will cause select to return EINTR. I've tested and committed the attached patch. As before, it's a big ugly, but the ugliness is localized to Windows-specific files. -- Daniel Jacobowitz CodeSourcery 2008-03-05 Daniel Jacobowitz * 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. (mark_async_signal_handler): Add comments. * event-top.c (handle_sigint): Use gdb_call_async_signal_handler. * mingw-hdep.c (sigint_event, sigint_handler): New. (gdb_select): Use them. Wait for the readline signal handler to finish. (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 5 Mar 2008 15:13:26 -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) $(readline_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 5 Mar 2008 15:13:26 -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 5 Mar 2008 15:13:26 -0000 @@ -86,9 +86,30 @@ extern void start_event_loop (void); extern int gdb_do_one_event (void *data); extern void delete_file_handler (int fd); extern void add_file_handler (int fd, handler_func * proc, gdb_client_data client_data); -extern void mark_async_signal_handler (struct async_signal_handler *async_handler_ptr); extern struct async_signal_handler * create_async_signal_handler (sig_handler_func * proc, gdb_client_data client_data); 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 HANDLER 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, with IMMEDIATE_P == 1. */ +void call_async_signal_handler (struct async_signal_handler *handler); + +/* Call the handler from HANDLER the next time through the event loop. + Do not call this directly; use gdb_call_async_signal_handler, + below, with IMMEDIATE_P == 0. */ +void mark_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. If IMMEDIATE_P is set, this triggers either + immediately (for POSIX platforms), or from gdb_select (for + MinGW). If IMMEDIATE_P is clear, the handler will run the next + time we return to the event loop and any current select calls + will be interrupted. */ + +void gdb_call_async_signal_handler (struct async_signal_handler *handler, + int immediate_p); 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 5 Mar 2008 15:13:26 -0000 @@ -975,13 +975,9 @@ handle_sigint (int sig) immediate_quit is set. If we didn't, SIGINT would be really processed only the next time through the event loop. To get to that point, though, the command that we want to interrupt needs to - finish first, which is unacceptable. */ - if (immediate_quit) - async_request_quit (0); - else - /* If immediate quit is not set, we process SIGINT the next time - through the loop, which is fine. */ - mark_async_signal_handler_wrapper (sigint_token); + finish first, which is unacceptable. If immediate quit is not set, + we process SIGINT the next time through the loop, which is fine. */ + gdb_call_async_signal_handler (sigint_token, immediate_quit); } /* Quit GDB if SIGTERM is received. 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 5 Mar 2008 15:13:26 -0000 @@ -19,13 +19,23 @@ #include "defs.h" #include "serial.h" +#include "event-loop.h" #include "gdb_assert.h" #include "gdb_select.h" #include "gdb_string.h" +#include "readline/readline.h" #include +/* 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 +151,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 +208,51 @@ gdb_select (int n, fd_set *readfds, fd_s } } + /* With multi-threaded SIGINT handling, there is a race between the + readline signal handler and GDB. It may still be in + rl_prep_terminal in another thread. Do not return until it is + done; we can check the state here because we never longjmp from + signal handlers on Windows. */ + while (RL_ISSTATE (RL_STATE_SIGHANDLER)) + Sleep (1); + + if (h == sigint_event + || WaitForSingleObject (sigint_event, 0) == WAIT_OBJECT_0) + { + if (sigint_handler != NULL) + call_async_signal_handler (sigint_handler); + + if (num_ready == 0) + { + errno = EINTR; + return -1; + } + } + 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, + int immediate_p) +{ + if (immediate_p) + sigint_handler = handler; + else + { + mark_async_signal_handler (handler); + sigint_handler = NULL; + } + 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 5 Mar 2008 15:13:26 -0000 @@ -18,6 +18,7 @@ along with this program. If not, see . */ #include "defs.h" +#include "event-loop.h" #include "gdb_string.h" @@ -50,3 +51,16 @@ 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, + int immediate_p) +{ + if (immediate_p) + call_async_signal_handler (handler); + else + mark_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 5 Mar 2008 15:13:26 -0000 @@ -28,6 +28,7 @@ #include "gdb_stat.h" #include "exceptions.h" #include "remote-fileio.h" +#include "event-loop.h" #include #include @@ -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, 1); 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 5 Mar 2008 15:13:27 -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, 1); } /* 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, 1); 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