From patchwork Sun Mar 16 13:53:34 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Kratochvil X-Patchwork-Id: 99 Return-Path: X-Original-To: siddhesh@wilcox.dreamhost.com Delivered-To: siddhesh@wilcox.dreamhost.com Received: from homiemail-mx21.g.dreamhost.com (caibbdcaaahb.dreamhost.com [208.113.200.71]) by wilcox.dreamhost.com (Postfix) with ESMTP id 2FF093601A8 for ; Sun, 16 Mar 2014 06:53:50 -0700 (PDT) Received: by homiemail-mx21.g.dreamhost.com (Postfix, from userid 14314964) id A38921B32499; Sun, 16 Mar 2014 06:53:49 -0700 (PDT) X-Original-To: gdb@patchwork.siddhesh.in Delivered-To: x14314964@homiemail-mx21.g.dreamhost.com Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by homiemail-mx21.g.dreamhost.com (Postfix) with ESMTPS id 74DC11B3CB3B for ; Sun, 16 Mar 2014 06:53:49 -0700 (PDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; q=dns; s=default; b=HA9S Cq6wfpgPz5UkkzNeU1mf2+r5SfeFTcTjM0DjFLm5PMYWPFkiQK1/0Fp7gB20jmEL THqPe3bQfV6jiDgUJfyIwQF3j1tPTG+j9VO4AGkg2mITa3Dm96bK89e10xJHaJsE UC6xPpjc2XSuUMH+ywslW2BueqTmJ4A357KQwts= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:date:from:to:cc:subject:message-id:references :mime-version:content-type:in-reply-to; s=default; bh=OKA1J6IsHg pZNOEaYuDau1tK7YY=; b=GFkYaRNdWL8AHRxa631/XHBy6uUs+w8a7RdkMQwD6W 2sMmbdcZpIIW813z/y1Fwd6liwX65WNI4lm2R1cLJvkFGjXJWKCe4V3Cd3pDZOS7 b7kWPk4gBwlSJnMnMFPa69AOBH0JTEHV/nhQjmsOpIbeiEAta4nQrAJ8Zwv08KE6 g= Received: (qmail 2543 invoked by alias); 16 Mar 2014 13:53:47 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 2525 invoked by uid 89); 16 Mar 2014 13:53:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-4.1 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 16 Mar 2014 13:53:42 +0000 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s2GDrdDl018879 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 16 Mar 2014 09:53:39 -0400 Received: from host2.jankratochvil.net (ovpn-116-86.ams2.redhat.com [10.36.116.86]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id s2GDrZu2031016 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NO); Sun, 16 Mar 2014 09:53:37 -0400 Date: Sun, 16 Mar 2014 14:53:34 +0100 From: Jan Kratochvil To: Doug Evans Cc: gdb-patches@sourceware.org Subject: [patchv3] Fix SIGTERM signal safety (PR gdb/15358) Message-ID: <20140316135334.GA30698@host2.jankratochvil.net> References: <20140314183709.GA28050@host2.jankratochvil.net> <20140314184535.GA30853@host2.jankratochvil.net> <21284.44419.745786.47756@ruffy.mtv.corp.google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <21284.44419.745786.47756@ruffy.mtv.corp.google.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-DH-Original-To: gdb@patchwork.siddhesh.in 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 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 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 . */ + +#include + +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 . + +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. */