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 -- ----------------------------------------------------------------------------- Rainer Orth, Center for Biotechnology, Bielefeld University 2018-08-07 Brian Vandenberg Rainer Orth 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.