Re: [PATCH][PR gdb/8527] Interrupt not functional in Eclipse/CDT on Solaris

[Rainer Orth <ro at CeBiTec dot Uni-Bielefeld dot DE>]

Hi Joel, Brian,

>> 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  <phantall@gmail.com>
>> 
>> 	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.

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University


2018-08-07  Brian Vandenberg  <phantall@gmail.com>

	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/signal-no-ctty.c, gdb.base/signal-no-ctty.exp: New
	test.

# HG changeset patch
# Parent  ba9899dff4a02b0de8cd73acc977a524b3921581
Interrupt not functional in Eclipse/CDT on Solaris (PR gdb/8527)

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/signal-no-ctty.c b/gdb/testsuite/gdb.base/signal-no-ctty.c
new file mode 100644
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signal-no-ctty.c
@@ -0,0 +1,31 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2018 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 <http://www.gnu.org/licenses/>.  */
+
+#include <unistd.h>
+
+int main()
+{
+  /* Detach from controlling terminal.  */
+  if (setpgrp () == (pid_t) -1)
+    return 1;
+
+  /* Don't let the test case run forever.  */
+  alarm (60);
+
+  for (;;)
+    ;
+}
diff --git a/gdb/testsuite/gdb.base/signal-no-ctty.exp b/gdb/testsuite/gdb.base/signal-no-ctty.exp
new file mode 100644
--- /dev/null
+++ b/gdb/testsuite/gdb.base/signal-no-ctty.exp
@@ -0,0 +1,71 @@
+# Copyright 2013-2018 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 <http://www.gnu.org/licenses/>.
+
+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 {[prepare_for_testing "failed to prepare" $testfile $srcfile debug]} {
+    return -1
+}
+
+# Wait a bit and stop the target with ctrl-c.
+proc do_test {} {
+    global binfile
+
+    # For this to work we must be sure to consume the "Continuing."
+    # message first, or GDB's signal handler may not be in place.
+    after 500 {send_gdb "\003"}
+    gdb_test "" "(Program|Thread .*) received signal SIGINT.*" "stop with control-c"
+}
+
+# With native debugging and "run" (with job control), the ctrl-c always
+# reaches the inferior, not gdb.  With remote debugging, the ctrl-c reaches
+# GDB first.
+with_test_prefix "run" {
+    clean_restart $binfile
+
+    gdb_run_cmd
+
+    do_test
+}
+
+# With "attach" however, even with native debugging, the ctrl-c always
+# reaches GDB first.  Test that as well.
+with_test_prefix "attach" {
+    if {[can_spawn_for_attach]} {
+	clean_restart $binfile
+
+	set test_spawn_id [spawn_wait_for_attach $binfile]
+	set testpid [spawn_id_get_pid $test_spawn_id]
+
+	gdb_test "attach $testpid" "Attaching to.*process $testpid.*libc.*" "attach"
+
+	send_gdb "continue\n"
+
+	do_test
+
+	kill_wait_spawned_process $test_spawn_id
+    }
+}