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

[Brian Vandenberg <phantall at gmail dot com>]

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
>