Message ID | CAEJ-0i-Tw17AQDm9ZvXkh73g1zvYDf8tLcQyC28bEDJL_+KizA@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 66386 invoked by alias); 16 Dec 2015 22:17:34 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 66370 invoked by uid 89); 16 Dec 2015 22:17:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 spammy=differs X-HELO: mail-qk0-f175.google.com Received: from mail-qk0-f175.google.com (HELO mail-qk0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 16 Dec 2015 22:17:33 +0000 Received: by mail-qk0-f175.google.com with SMTP id p187so88531153qkd.1 for <gdb-patches@sourceware.org>; Wed, 16 Dec 2015 14:17:32 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.13.217.131 with SMTP id b125mr29047747ywe.222.1450304250765; Wed, 16 Dec 2015 14:17:30 -0800 (PST) Received: by 10.37.118.88 with HTTP; Wed, 16 Dec 2015 14:17:30 -0800 (PST) Date: Wed, 16 Dec 2015 15:17:30 -0700 Message-ID: <CAEJ-0i-Tw17AQDm9ZvXkh73g1zvYDf8tLcQyC28bEDJL_+KizA@mail.gmail.com> Subject: [RFC][PATCH][PR gdb/8527] Fix interrupt (^C) in Solaris From: Brian Vandenberg <phantall@gmail.com> To: gdb-patches@sourceware.org Content-Type: multipart/mixed; boundary=001a114fb4fcaf027c05270b4561 |
Commit Message
Brian Vandenberg
Dec. 16, 2015, 10:17 p.m. UTC
This patch addresses an issue in Solaris, bug 8527. The problem: If gdb attaches to a process that either has no controlling terminal, or the controlling terminal differs from the one gdb is running under, hitting ^C doesn't interrupt the debugged process. The change makes it possible to interrupt/resume without any apparent problems, but I don't know the ramifications of these changes. I ran across this issue and a fix that was proposed 12 years ago. Is this a reasonable change? -brian gdb/Changelog: 2015-12-16 Brian Vandenberg <phantall@gmail.com> PR gdb/8527 * gdb/procfs.c (proc_wait_for_stop): Added set_sigint_trap() & clear_sigint_trap()
Comments
> This patch addresses an issue in Solaris, bug 8527. > > The problem: > > If gdb attaches to a process that either has no controlling > terminal, or the controlling terminal differs from the one > gdb is running under, hitting ^C doesn't interrupt the > debugged process. > > The change makes it possible to interrupt/resume > without any apparent problems, but I don't know the > ramifications of these changes. I ran across this > issue and a fix that was proposed 12 years ago. > > Is this a reasonable change? > > -brian > gdb/Changelog: > 2015-12-16 Brian Vandenberg <phantall@gmail.com> > > PR gdb/8527 > * gdb/procfs.c (proc_wait_for_stop): Added set_sigint_trap() & clear_sigint_trap() FWIW, we have had this change in AdaCore's version since 2007 at least (probably even earlier, but digging deeper is more difficult), and we haven't heard of problems because of it. Given that procfs.c now appears to be used by Solaris targets only (x86, x86_64, sparc and sparc64), I propose that we integrate that patch. In terms of the patch's submission, I have a small number of small comments (everything small :-)). First, in the ChangeLog is almost perfect. - two spaces after your name, before the email address. - Remove the "gdb/" in "gdb/procfs.c", since the name of the file is relative to the ChangeLog, which is in gdb/. - Also, we limit the length of each line to 79-80 characters. - small nitpick, I think "and" is better than "&" in this case, as "&" has a mathematical connotation for me; - we say "call to set_sigint_trap" rather than "set_sigint_trap()". - and we end sentences with periods (GNU Coding Style). Therefore: gdb/Changelog: 2015-12-16 Brian Vandenberg <phantall@gmail.com> PR gdb/8527 * procfs.c (proc_wait_for_stop): Added call to set_sigint_trap and clear_sigint_trap. > diff --git a/gdb/procfs.c b/gdb/procfs.c > index 7b7ff45..7cd870c 100644 > --- a/gdb/procfs.c > +++ b/gdb/procfs.c > @@ -1487,7 +1487,12 @@ proc_wait_for_stop (procinfo *pi) > { > procfs_ctl_t cmd = PCWSTOP; > > + set_sigint_trap(); I would add a comment before the call to set_sigint_trap, to confirm to the reader that we are doing this in order to resolve an issue. Something close to what you wrote in your email seems to fit the bill... > 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; > } And finally; this patch is fairly tiny, so there should be no copyright issue preventing us from incorporating it. But I also see that you have several contributions pending, and so, for us to continue accepting patches from you, it would be better if you had a copyright assignment on file. Has anyone asked you about that? Eventually, when the paperwork is done, the only requirement for getting "write after approval" access to the repo is to submit one good patch, which I think you have met, or will meet very shortly. Thanks,
diff --git a/gdb/procfs.c b/gdb/procfs.c index 7b7ff45..7cd870c 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -1487,7 +1487,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; }