Message ID | CAEJ-0i_YWq+__iD079Z=RNws+pa8QQD4uBqqbLb7jc4QvCdCQQ@mail.gmail.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 111193 invoked by alias); 7 Aug 2018 19:13:45 -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 111063 invoked by uid 89); 7 Aug 2018 19:13:44 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=cdt, CDT, Was, eclipse X-HELO: mail-yb0-f169.google.com Received: from mail-yb0-f169.google.com (HELO mail-yb0-f169.google.com) (209.85.213.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 07 Aug 2018 19:13:43 +0000 Received: by mail-yb0-f169.google.com with SMTP id d18-v6so3103032ybq.5 for <gdb-patches@sourceware.org>; Tue, 07 Aug 2018 12:13:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:from:date:message-id:subject:to; bh=AsVa3aek6yzxCSVPg9T7vCuNXdOc5EAQ1XyNM4jkAQU=; b=nlc8Nn/2PoGlaR+6+D3+3Wuv23NH12KPOLEWZZfxhDkhlk/Lom7UUOoI7Lav2+4OSu MfEhH784X40IVmbUV3YV8XbTcbIgg8Ll/UzlPQM+IzMP6AocQE/WAbWRbZKSrzF4bG4N Jfy1NSLnyzNf8cmVjtpxS+dBRVFn0g9zbcLjNfB0TTU39/J3GVH9zPByaL9xenLNhzX0 rZKAaaAzjl0JRLZK5bj0jf3I1jKOz0fnFYXTIpfbd3THlXiANcyg5dkDYeiQXHHPRTYP rkx0QvEHPjk2RZmMQjhqdU+asqUDcJW9Qqj6UfmMyaeLtVhlL4WNctQL6gvo3x0Y0cS+ NXCw== MIME-Version: 1.0 Received: by 2002:a25:57c2:0:0:0:0:0 with HTTP; Tue, 7 Aug 2018 12:13:40 -0700 (PDT) From: Brian Vandenberg <phantall@gmail.com> Date: Tue, 7 Aug 2018 13:13:40 -0600 Message-ID: <CAEJ-0i_YWq+__iD079Z=RNws+pa8QQD4uBqqbLb7jc4QvCdCQQ@mail.gmail.com> Subject: [PATCH][PR gdb/8527] Interrupt not functional in Eclipse/CDT on Solaris To: gdb-patches@sourceware.org Content-Type: multipart/mixed; boundary="0000000000001db2fa0572dd316e" |
Commit Message
Brian Vandenberg
Aug. 7, 2018, 7:13 p.m. UTC
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.
Comments
Hi 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 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 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. 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. */ > + set_sigint_trap(); > + > 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;
Greetings, Did you run the testsuite before and after the patch, by any chance? Nope. In my work environment I don't have much flexibility on getting/installing software. If I run the test suite I would probably have to setup an IllumOS VM at home to run it, but that'd be x86 not SPARC. 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. */ I've no qualms with those changes. Thanks for your feedback. -brian On Thu, Nov 1, 2018 at 3:19 PM Joel Brobecker <brobecker@adacore.com> wrote: > Hi 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 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 > 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. > > 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. */ > > > + set_sigint_trap(); > > + > > 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; > > > -- > Joel >
On 2018-11-01 5:45 p.m., Brian Vandenberg wrote: > Greetings, > > Did you run the testsuite before and after the patch, by any chance? > > > Nope. In my work environment I don't have much flexibility on > getting/installing software. If I run the test suite I would probably have > to setup an IllumOS VM at home to run it, but that'd be x86 not SPARC. > > > 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. */ > > > I've no qualms with those changes. Thanks for your feedback. Asking because it's ambiguous... do you plan on sending an updated patch? As for the patch content and its testing, perhaps Rainer can give some feedback. Simon
Hi Simon, > On 2018-11-01 5:45 p.m., Brian Vandenberg wrote: >> Greetings, >> >> Did you run the testsuite before and after the patch, by any chance? >> >> >> Nope. In my work environment I don't have much flexibility on >> getting/installing software. If I run the test suite I would probably have >> to setup an IllumOS VM at home to run it, but that'd be x86 not SPARC. >> >> >> 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. */ >> >> >> I've no qualms with those changes. Thanks for your feedback. > > Asking because it's ambiguous... do you plan on sending an updated patch? I don't think this is necessary: I'm going to take care of that. > As for the patch content and its testing, perhaps Rainer can give some feedback. Sorry for the delay in replying: I've both been very busy with end-of-stage1 gcc stuff and unwell lately. I hope to get to this soon. Rainer
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. + */ + set_sigint_trap(); + 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;