[PR,gdb/8527] Interrupt not functional in Eclipse/CDT on Solaris

  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.
  

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