From patchwork Tue Feb 26 15:14:42 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Rainer Orth X-Patchwork-Id: 31597 Received: (qmail 82914 invoked by alias); 26 Feb 2019 15:14:57 -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 82903 invoked by uid 89); 26 Feb 2019 15:14:57 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_SHORT autolearn=ham version=3.3.2 spammy=pressed, brian, Brian, rough X-HELO: smtp.CeBiTec.Uni-Bielefeld.DE Received: from smtp.CeBiTec.Uni-Bielefeld.DE (HELO smtp.CeBiTec.Uni-Bielefeld.DE) (129.70.160.84) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Feb 2019 15:14:53 +0000 Received: from localhost (localhost.CeBiTec.Uni-Bielefeld.DE [127.0.0.1]) by smtp.CeBiTec.Uni-Bielefeld.DE (Postfix) with ESMTP id 0B207FE9; Tue, 26 Feb 2019 16:14:51 +0100 (CET) Received: from smtp.CeBiTec.Uni-Bielefeld.DE ([127.0.0.1]) by localhost (malfoy.CeBiTec.Uni-Bielefeld.DE [127.0.0.1]) (amavisd-new, port 10024) with LMTP id Z2xUKcs5dFj2; Tue, 26 Feb 2019 16:14:43 +0100 (CET) Received: from piggy (piggy.CeBiTec.Uni-Bielefeld.DE [129.70.160.213]) by smtp.CeBiTec.Uni-Bielefeld.DE (Postfix) with SMTP id 7BCE4FE7; Tue, 26 Feb 2019 16:14:42 +0100 (CET) Received: by piggy (sSMTP sendmail emulation); Tue, 26 Feb 2019 16:14:42 +0100 From: "Rainer Orth" To: Joel Brobecker Cc: Brian Vandenberg , gdb-patches@sourceware.org Subject: Re: [PATCH][PR gdb/8527] Interrupt not functional in Eclipse/CDT on Solaris References: <20181101211949.GB2705@adacore.com> Date: Tue, 26 Feb 2019 16:14:42 +0100 In-Reply-To: (Rainer Orth's message of "Thu, 22 Nov 2018 11:19:17 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 X-IsSubscribed: yes Hi Joel, >>> In Solaris: >>> >>> If gdb attaches to a process that either has no controlling terminal, >>> or the controlling terminal differs from the one gdb is running under, >>> break/^C doesn't interrupt the debugged process. >>> >>> This is a fix that was proposed for this problem quite awhile ago but >>> never implemented; it's been in the Adacore GDB branch for quite >>> awhile. >>> >>> Without going into unnecessary details I cannot easily run the test >>> suite against this change right now. If this patch gets rejected >>> based on that, when I have time I'll see about getting IllumOS >>> installed in a VM and test it there, but the problem was originally >>> found in sparc Solaris. >>> >>> ---- >>> >>> note: this patch was tested against 8.1.1. It looks like it probably >>> still applies on the 8.2 branch, but I won't be able to test it until >>> 8.2 is released. >>> >>> -brian >>> >>> ps, my assignment/release forms were completed/received 10/30/2017 >> >>> gdb/Changelog: >>> 2018-08-07 Brian Vandenberg >>> >>> PR gdb/8527 >>> * procfs.c (proc_wait_for_stop): calls to set_sigint_trap and >>> clear_sigint_trap. >> >> I'm not sure anyone took the time to answer this message; if not, >> I apologize. > > I'm sorry it took me so long, too, first to get well again and then to > get to this. > >> I can testify that, for as long as we got the patch in the AdaCore >> sources, we never noticed any ill effect. We never got to validate >> it against the official testsuite, unfortunately, because for some >> reason, when I did so, our Solaris machines would badly crash. Did > > This is weird: the only time I saw something like this was with a few > Solaris 12/11.4 Beta builds. > >> you run the testsuite before and after the patch, by any chance? >> >> Rainer (in Cc:) is our maintainer for Solaris, so he may have an opinion. > > I've now regtested the patch on both amd64-pc-solaris2.11 and > sparcv9-sun-solaris2.11. Initially, gdb.base/attach.exp seemed to > regress on 32-bit Solaris/SPARC, but that turned out to be due to > flakyness of parts of the testsuite on Solaris. > >> In the meantime, I have a trivial coding style comment: >> >>> >>> diff --git a/gdb/procfs.c b/gdb/procfs.c >>> index 7b7ff45..7cd870c 100644 >>> --- a/gdb/procfs.c >>> +++ b/gdb/procfs.c >>> @@ -913,7 +913,12 @@ proc_wait_for_stop (procinfo *pi) >>> >>> procfs_ctl_t cmd = PCWSTOP; >>> >>> + /* PR gdb/8527 >>> + * Was not correctly interrupting the inferior process >>> + * when ^C was pressed in the debug terminal. >>> + */ >> >> For multiline comments like the above, we do not repeat the '*' >> at the beginning of each line. >> >> /* PR gdb/8527: Was not correctly interrupting the inferior process >> when ^C was pressed in the debug terminal. */ >> >> And if I may, reading this sentence, it's a bit hard to understand >> what the comment is trying to explain. The following might be >> a little more precise: >> >> /* PR gdb/8527: Call set_sigint_trap to make sure that a ctrl-c >> pressed in the debugger terminal gets passed down to the >> inferior, thus causing it to be interrupted. */ > > TBH, I'd just leave off the comments: this is just what > set_sigint_trap/clear_sigint_trap are supposed to do. No other use > comments on this, and the PR reference can easily be found in the commit > message and the ChangeLog. > >>> + set_sigint_trap(); > > Another nit: blank before (), here and below. > >>> win = (write (pi->ctl_fd, (char *) &cmd, sizeof (cmd)) == sizeof (cmd)); >>> + >>> + /* PR gdb/8527 */ >>> + clear_sigint_trap(); >>> + >>> /* We been runnin' and we stopped -- need to update status. */ >>> pi->status_valid = 0; > > When I noticed that we don't have any test for this issue, I started to > develop one. This turned out to be a rough trip and my first foray into > the gdb testsuite, so please bear with me. > > Looking for possible testcases to modify, I first came > gdb.base/interrupt-daemon.exp. However, there turned out to be two > issues: I'd needed the pid of the grandchild process to attach to, and > this wasn't emitted to gdb.log when printed. > > Besides, when I checked the test as is, it already FAILs on Solaris. > This seems to happen because set follow-fork-mode child isn't > implemented, but fails silently and the list of targets supporting it is > is either incomplete or completely missing in the tests that use it. > > Next, I started with a copy of gdb.base/random-signal.c, adding a call > to setpgrp to detach it from its controling terminal. > > Initially, that test FAILed, too, because it expected a > > Program received signal SIGINT.* > > message while on Solaris I get > > Thread N received signal SIGINT.* > > I suppose that's because starting with Solaris 10, every process is > multithreaded. Once I allowed for that form, too, the test PASSed on > Solaris, both for the run and attach cases. I'll go through the > testsuite and allow for both cases there, too, probably causing several > tests to magically PASS now ;-) > > As expected, with unmodified gdb, I get the expected > > FAIL: gdb.base/signal-no-ctty.exp: attach: stop with control-c (timeout) > > However, when I tested the testcase on Linux/x86_64, it FAILs: > > attach 113292 > Attaching to program: > /vol/gcc/obj/gdb/gdb/dist/gdb/testsuite/outputs/gdb.base/signal-no-ctty/signal-no-ctty, > process 113292 > warning: process 113292 is a zombie - the process has already terminated > ptrace: Operation not permitted. > (gdb) FAIL: gdb.base/signal-no-ctty.exp: attach: attach > > The weird thing is that both with the setpgrp call and when run like > > $ ./signal-no-ctty < /dev/null >& /dev/null & > > ps still shows a controlling terminal for the process. Don't yet know > what's going on here. > > Current patch attached for reference. I never got a reply to this one, but I think I just figured out the testcase part myself. Tested on amd64-pc-solaris2.11 and sparcv9-sun-solaris2.11 (test fails before with FAIL: gdb.base/signal-no-ctty.exp: child: stop with control-c (timeout) and passes after) and x86_64-pc-linux-gnu. Ok for master? Rainer # HG changeset patch # Parent 14950de898700dec7316fb3a5691413d651ddf42 Can't interrupt process without controlling terminal on Solaris (PR gdb/8527) 2018-08-07 Brian Vandenberg gdb: PR gdb/8527 * procfs.c (proc_wait_for_stop): Wrap write of PCWSTOP in set_sigint_trap, clear_sigint_trap. gdb/testsuite: PR gdb/8527 * gdb.base/sigint-no-ctty.c, gdb.base/sigint-no-ctty.exp: New test. diff --git a/gdb/procfs.c b/gdb/procfs.c --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -909,7 +909,12 @@ proc_wait_for_stop (procinfo *pi) procfs_ctl_t cmd = PCWSTOP; + set_sigint_trap (); + win = (write (pi->ctl_fd, (char *) &cmd, sizeof (cmd)) == sizeof (cmd)); + + clear_sigint_trap (); + /* We been runnin' and we stopped -- need to update status. */ pi->status_valid = 0; diff --git a/gdb/testsuite/gdb.base/sigint-no-ctty.c b/gdb/testsuite/gdb.base/sigint-no-ctty.c new file mode 100644 --- /dev/null +++ b/gdb/testsuite/gdb.base/sigint-no-ctty.c @@ -0,0 +1,61 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2017-2019 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 +#include + +/* GDB reads this to figure out the child's PID. */ +pid_t child_pid; + +void +marker (void) +{ +} + +int +main () +{ + /* Don't let the test case run forever. */ + alarm (60); + + child_pid = fork (); + + switch (child_pid) + { + case -1: + return 1; + + case 0: + break; + + default: + while (1) + { + marker (); + usleep (1000); + } + } + + /* Detach from controlling terminal. */ + if (setsid () == (pid_t) -1) + return 1; + + for (;;) + ; + + return 0; +} diff --git a/gdb/testsuite/gdb.base/sigint-no-ctty.exp b/gdb/testsuite/gdb.base/sigint-no-ctty.exp new file mode 100644 --- /dev/null +++ b/gdb/testsuite/gdb.base/sigint-no-ctty.exp @@ -0,0 +1,87 @@ +# Copyright 2019 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 . */ + +if [target_info exists gdb,nosignals] { + verbose "Skipping signal-no-ctty.exp because of nosignals." + continue +} + +# This test requires sending ^C to interrupt the running target. +if [target_info exists gdb,nointerrupts] { + verbose "Skipping signal-no-ctty.exp because of nointerrupts." + return +} + +standard_testfile + +if { [build_executable ${testfile}.exp ${testfile} $srcfile {debug}] == -1 } { + return -1 +} + +proc do_test {} { + global binfile + global decimal + + set test_spawn_id [spawn_wait_for_attach $binfile] + set parent_pid [spawn_id_get_pid $test_spawn_id] + + # Attach to the parent, run it to a known point, extract the + # child's PID, and detach. + with_test_prefix "parent" { + clean_restart ${binfile} + + gdb_test "attach $parent_pid" \ + "Attaching to program.*, process $parent_pid.*" \ + "attach" + + gdb_breakpoint "marker" + gdb_continue_to_breakpoint "marker" + + set child_pid [get_integer_valueof "child_pid" -1] + if {$child_pid == -1} { + return + } + + gdb_test "detach" \ + "Detaching from program: .*process $parent_pid\r\n\\\[Inferior $decimal \\(.*\\) detached\\\]" + + remote_exec host "kill -9 $parent_pid" + } + + # Start over, and attach to the child this time. + with_test_prefix "child" { + global gdb_prompt + + clean_restart $binfile + + gdb_test "attach $child_pid" \ + "Attaching to program.*, process $child_pid.*" \ + "attach" + + gdb_test_multiple "continue" "continue" { + -re "Continuing" { + pass "continue" + } + } + + after 500 {send_gdb "\003"} + gdb_test "" "(Program|Thread .*) received signal SIGINT.*" \ + "stop with control-c" + + remote_exec host "kill -9 $child_pid" + } +} + +do_test