[PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Patrick Palka]

Currently the top-level driver handles SIGINT by immediately killing
itself even when the driver has subprocesses (e.g. cc1, as) running.  I
don't think this is a good idea because

  1. if the top-level driver is waiting on a hanging subprocess,
  pressing ^C will kill the driver but it may not necessarily kill the
  subprocess; an unresponsive, perhaps busy-looping subprocess may be
  running in the background yet the compiler will seem to have to
  terminated successfully.

  2. when debugging gcc with "gcc -wrapper gdb,--args" we are unable to
  use ^C from within the GDB subprocess because pressing ^C immediately
  kills the driver and we lose our terminal.  This makes debugging more
  inconvenient.

This patch fixes these two issues by having the driver ignore SIGINT
while a subprocess is running.  The driver instead will have to wait for
the subprocess to exit before it terminates, like usual.

I tested this change by running "gcc -wrapper gdb", "gcc -wrapper
valgrind" and plain old "gcc" in various ways (-pipe, -flto, -c, etc)
and pressing ^C during compilation.  I noticed no differences in
behavior or promptness other than finally being able to use ^C inside
GDB.

Does this change look OK for trunk after a successful bootstrap +
regtest on x86_64-unknown-linux-gnu?
---
 gcc/gcc.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/gcc/gcc.c b/gcc/gcc.c
index 653ca8d..0802f41 100644
--- a/gcc/gcc.c
+++ b/gcc/gcc.c
@@ -2653,7 +2653,11 @@ add_sysrooted_prefix (struct path_prefix *pprefix, const char *prefix,
   add_prefix (pprefix, prefix, component, priority,
 	      require_machine_suffix, os_multilib);
 }
-\f
+
+/* True if the SIGINT signal should be ignored by the driver's
+   signal handler.  */
+static bool ignore_sigint_p;
+
 /* Execute the command specified by the arguments on the current line of spec.
    When using pipes, this includes several piped-together commands
    with `|' between them.
@@ -2839,6 +2843,10 @@ execute (void)
   if (pex == NULL)
     fatal_error ("pex_init failed: %m");
 
+  /* A SIGINT raised while subprocesses are running should not kill the
+     driver.  */
+  ignore_sigint_p = true;
+
   for (i = 0; i < n_commands; i++)
     {
       const char *errmsg;
@@ -2878,6 +2886,8 @@ execute (void)
     if (!pex_get_status (pex, n_commands, statuses))
       fatal_error ("failed to get exit status: %m");
 
+    ignore_sigint_p = false;
+
     if (report_times || report_times_to_file)
       {
 	times = (struct pex_time *) alloca (n_commands * sizeof (struct pex_time));
@@ -2893,6 +2903,9 @@ execute (void)
 
 	if (WIFSIGNALED (status))
 	  {
+	    if (WTERMSIG (status) == SIGINT)
+	      raise (SIGINT);
+	    else
 #ifdef SIGPIPE
 	    /* SIGPIPE is a special case.  It happens in -pipe mode
 	       when the compiler dies before the preprocessor is done,
@@ -6710,6 +6723,12 @@ set_input (const char *filename)
 static void
 fatal_signal (int signum)
 {
+  if (signum == SIGINT && ignore_sigint_p)
+    {
+      signal (signum, fatal_signal);
+      return;
+    }
+
   signal (signum, SIG_DFL);
   delete_failure_queue ();
   delete_temp_files ();
-- 
2.2.0.rc1.23.gf570943

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Patrick Palka]

On Sat, 15 Nov 2014, Patrick Palka wrote:

> Currently the top-level driver handles SIGINT by immediately killing
> itself even when the driver has subprocesses (e.g. cc1, as) running.  I
> don't think this is a good idea because
>
>  1. if the top-level driver is waiting on a hanging subprocess,
>  pressing ^C will kill the driver but it may not necessarily kill the
>  subprocess; an unresponsive, perhaps busy-looping subprocess may be
>  running in the background yet the compiler will seem to have to
>  terminated successfully.
>
>  2. when debugging gcc with "gcc -wrapper gdb,--args" we are unable to
>  use ^C from within the GDB subprocess because pressing ^C immediately
>  kills the driver and we lose our terminal.  This makes debugging more
>  inconvenient.
>
> This patch fixes these two issues by having the driver ignore SIGINT
> while a subprocess is running.  The driver instead will have to wait for
> the subprocess to exit before it terminates, like usual.
>
> I tested this change by running "gcc -wrapper gdb", "gcc -wrapper
> valgrind" and plain old "gcc" in various ways (-pipe, -flto, -c, etc)
> and pressing ^C during compilation.  I noticed no differences in
> behavior or promptness other than finally being able to use ^C inside
> GDB.
>
> Does this change look OK for trunk after a successful bootstrap +
> regtest on x86_64-unknown-linux-gnu?

I forgot a ChangeLog entry:

 	* gcc.c (ignore_sigint_p): New static variable.
 	(execute): Use it.
 	(fatal_signal): Ignore SIGINT if ignore_sigint_p is true.

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Richard Biener]

On Sun, Nov 16, 2014 at 2:01 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
> Currently the top-level driver handles SIGINT by immediately killing
> itself even when the driver has subprocesses (e.g. cc1, as) running.  I
> don't think this is a good idea because
>
>   1. if the top-level driver is waiting on a hanging subprocess,
>   pressing ^C will kill the driver but it may not necessarily kill the
>   subprocess; an unresponsive, perhaps busy-looping subprocess may be
>   running in the background yet the compiler will seem to have to
>   terminated successfully.
>
>   2. when debugging gcc with "gcc -wrapper gdb,--args" we are unable to
>   use ^C from within the GDB subprocess because pressing ^C immediately
>   kills the driver and we lose our terminal.  This makes debugging more
>   inconvenient.
>
> This patch fixes these two issues by having the driver ignore SIGINT
> while a subprocess is running.  The driver instead will have to wait for
> the subprocess to exit before it terminates, like usual.
>
> I tested this change by running "gcc -wrapper gdb", "gcc -wrapper
> valgrind" and plain old "gcc" in various ways (-pipe, -flto, -c, etc)
> and pressing ^C during compilation.  I noticed no differences in
> behavior or promptness other than finally being able to use ^C inside
> GDB.
>
> Does this change look OK for trunk after a successful bootstrap +
> regtest on x86_64-unknown-linux-gnu?

This means I can no longer interrupt a compile that is running too long?
That sounds wrong.

You should instead debug the actual compiler, not the driver.

Richard.

> ---
>  gcc/gcc.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/gcc.c b/gcc/gcc.c
> index 653ca8d..0802f41 100644
> --- a/gcc/gcc.c
> +++ b/gcc/gcc.c
> @@ -2653,7 +2653,11 @@ add_sysrooted_prefix (struct path_prefix *pprefix, const char *prefix,
>    add_prefix (pprefix, prefix, component, priority,
>               require_machine_suffix, os_multilib);
>  }
> -
> +
> +/* True if the SIGINT signal should be ignored by the driver's
> +   signal handler.  */
> +static bool ignore_sigint_p;
> +
>  /* Execute the command specified by the arguments on the current line of spec.
>     When using pipes, this includes several piped-together commands
>     with `|' between them.
> @@ -2839,6 +2843,10 @@ execute (void)
>    if (pex == NULL)
>      fatal_error ("pex_init failed: %m");
>
> +  /* A SIGINT raised while subprocesses are running should not kill the
> +     driver.  */
> +  ignore_sigint_p = true;
> +
>    for (i = 0; i < n_commands; i++)
>      {
>        const char *errmsg;
> @@ -2878,6 +2886,8 @@ execute (void)
>      if (!pex_get_status (pex, n_commands, statuses))
>        fatal_error ("failed to get exit status: %m");
>
> +    ignore_sigint_p = false;
> +
>      if (report_times || report_times_to_file)
>        {
>         times = (struct pex_time *) alloca (n_commands * sizeof (struct pex_time));
> @@ -2893,6 +2903,9 @@ execute (void)
>
>         if (WIFSIGNALED (status))
>           {
> +           if (WTERMSIG (status) == SIGINT)
> +             raise (SIGINT);
> +           else
>  #ifdef SIGPIPE
>             /* SIGPIPE is a special case.  It happens in -pipe mode
>                when the compiler dies before the preprocessor is done,
> @@ -6710,6 +6723,12 @@ set_input (const char *filename)
>  static void
>  fatal_signal (int signum)
>  {
> +  if (signum == SIGINT && ignore_sigint_p)
> +    {
> +      signal (signum, fatal_signal);
> +      return;
> +    }
> +
>    signal (signum, SIG_DFL);
>    delete_failure_queue ();
>    delete_temp_files ();
> --
> 2.2.0.rc1.23.gf570943
>

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Andreas Schwab]

Richard Biener <richard.guenther@gmail.com> writes:

> This means I can no longer interrupt a compile that is running too long?

The compiler will receive your interrupt as well, and die eventually,
unless it is blocked for some reason, in which case you will now notice
instead of leaving behind a lingering process.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Mike Stump]

On Nov 17, 2014, at 2:23 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> On Sun, Nov 16, 2014 at 2:01 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> Currently the top-level driver handles SIGINT by immediately killing
>> itself even when the driver has subprocesses (e.g. cc1, as) running.  I
>> don't think this is a good idea because
>> 
>>  1. if the top-level driver is waiting on a hanging subprocess,
>>  pressing ^C will kill the driver but it may not necessarily kill the
>>  subprocess; an unresponsive, perhaps busy-looping subprocess may be
>>  running in the background yet the compiler will seem to have to
>>  terminated successfully.

You’d need to describe what unresponsive is and how it got that way.  SIGINT is a request that the job stop.  Everything we do from the driver should come down reliably and pretty quickly with a SIGINT.  If something doesn’t, tell us what, and why, and we can see about fixing that.

>>  2. when debugging gcc with "gcc -wrapper gdb,--args" we are unable to
>>  use ^C from within the GDB subprocess because pressing ^C immediately
>>  kills the driver and we lose our terminal.  This makes debugging more
>>  inconvenient.

If valgrind doesn’t stop with SIGINT, and you want it to, you should file a bug report with them.

> This means I can no longer interrupt a compile that is running too long?
> That sounds wrong.

Yeah, I don’t think I favor it as well.

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Patrick Palka]

On Mon, Nov 17, 2014 at 5:23 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sun, Nov 16, 2014 at 2:01 AM, Patrick Palka <patrick@parcs.ath.cx> wrote:
>> Currently the top-level driver handles SIGINT by immediately killing
>> itself even when the driver has subprocesses (e.g. cc1, as) running.  I
>> don't think this is a good idea because
>>
>>   1. if the top-level driver is waiting on a hanging subprocess,
>>   pressing ^C will kill the driver but it may not necessarily kill the
>>   subprocess; an unresponsive, perhaps busy-looping subprocess may be
>>   running in the background yet the compiler will seem to have to
>>   terminated successfully.
>>
>>   2. when debugging gcc with "gcc -wrapper gdb,--args" we are unable to
>>   use ^C from within the GDB subprocess because pressing ^C immediately
>>   kills the driver and we lose our terminal.  This makes debugging more
>>   inconvenient.
>>
>> This patch fixes these two issues by having the driver ignore SIGINT
>> while a subprocess is running.  The driver instead will have to wait for
>> the subprocess to exit before it terminates, like usual.
>>
>> I tested this change by running "gcc -wrapper gdb", "gcc -wrapper
>> valgrind" and plain old "gcc" in various ways (-pipe, -flto, -c, etc)
>> and pressing ^C during compilation.  I noticed no differences in
>> behavior or promptness other than finally being able to use ^C inside
>> GDB.
>>
>> Does this change look OK for trunk after a successful bootstrap +
>> regtest on x86_64-unknown-linux-gnu?
>
> This means I can no longer interrupt a compile that is running too long?
> That sounds wrong.

The mechanism relied on to kill the subprocesses isn't changed (that
is, a SIGINT sent to all processes in the process group when ^C is
pressed).  What is changed is that the driver now waits for this
mechanism to succeed in killing the subprocesses before killing
itself.

>
> You should instead debug the actual compiler, not the driver.

Yeah, only recently I learned that people actually do this. I thought
everyone used -wrapper and so would have shared my grief with not
being able to use ^C inside gdb :) Since one should not be using
-wrapper gdb in the first place, I'm not particularly wanting about
this patch anymore.

>
> Richard.
>
>> ---
>>  gcc/gcc.c | 21 ++++++++++++++++++++-
>>  1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/gcc.c b/gcc/gcc.c
>> index 653ca8d..0802f41 100644
>> --- a/gcc/gcc.c
>> +++ b/gcc/gcc.c
>> @@ -2653,7 +2653,11 @@ add_sysrooted_prefix (struct path_prefix *pprefix, const char *prefix,
>>    add_prefix (pprefix, prefix, component, priority,
>>               require_machine_suffix, os_multilib);
>>  }
>> -
>> +
>> +/* True if the SIGINT signal should be ignored by the driver's
>> +   signal handler.  */
>> +static bool ignore_sigint_p;
>> +
>>  /* Execute the command specified by the arguments on the current line of spec.
>>     When using pipes, this includes several piped-together commands
>>     with `|' between them.
>> @@ -2839,6 +2843,10 @@ execute (void)
>>    if (pex == NULL)
>>      fatal_error ("pex_init failed: %m");
>>
>> +  /* A SIGINT raised while subprocesses are running should not kill the
>> +     driver.  */
>> +  ignore_sigint_p = true;
>> +
>>    for (i = 0; i < n_commands; i++)
>>      {
>>        const char *errmsg;
>> @@ -2878,6 +2886,8 @@ execute (void)
>>      if (!pex_get_status (pex, n_commands, statuses))
>>        fatal_error ("failed to get exit status: %m");
>>
>> +    ignore_sigint_p = false;
>> +
>>      if (report_times || report_times_to_file)
>>        {
>>         times = (struct pex_time *) alloca (n_commands * sizeof (struct pex_time));
>> @@ -2893,6 +2903,9 @@ execute (void)
>>
>>         if (WIFSIGNALED (status))
>>           {
>> +           if (WTERMSIG (status) == SIGINT)
>> +             raise (SIGINT);
>> +           else
>>  #ifdef SIGPIPE
>>             /* SIGPIPE is a special case.  It happens in -pipe mode
>>                when the compiler dies before the preprocessor is done,
>> @@ -6710,6 +6723,12 @@ set_input (const char *filename)
>>  static void
>>  fatal_signal (int signum)
>>  {
>> +  if (signum == SIGINT && ignore_sigint_p)
>> +    {
>> +      signal (signum, fatal_signal);
>> +      return;
>> +    }
>> +
>>    signal (signum, SIG_DFL);
>>    delete_failure_queue ();
>>    delete_temp_files ();
>> --
>> 2.2.0.rc1.23.gf570943
>>

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Michael Matz]

Hi,

On Mon, 17 Nov 2014, Richard Biener wrote:

> This means I can no longer interrupt a compile that is running too long? 

No, that's not what it means, cc1 will also get the SIGINT.

> You should instead debug the actual compiler, not the driver.

-wrapper is specifically also for invoking cc1 with gdb from the driver 
(that's the usecase documented with -wrapper), so it better should work as 
intended.  I don't know what problems Patrick had with that, though.  For 
me gcc -wrapper gdb,--args works as expected (as in ^C interrupts cc1 
returning to gdb).


Ciao,
Michael.

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Patrick Palka]

On Tue, Nov 18, 2014 at 11:14 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Mon, 17 Nov 2014, Richard Biener wrote:
>
>> This means I can no longer interrupt a compile that is running too long?
>
> No, that's not what it means, cc1 will also get the SIGINT.
>
>> You should instead debug the actual compiler, not the driver.
>
> -wrapper is specifically also for invoking cc1 with gdb from the driver
> (that's the usecase documented with -wrapper), so it better should work as
> intended.  I don't know what problems Patrick had with that, though.  For
> me gcc -wrapper gdb,--args works as expected (as in ^C interrupts cc1
> returning to gdb).

Yes it does for me too. But pressing ^C in gdb while cc1 is not
running (by accident or with intention, e.g. pressing ^C to quickly
clear the command prompt) will kill the driver and gdb after it. It's
not a huge problem but it does cause some inconvenience for users of
-wrapper gdb.

>
>
> Ciao,
> Michael.

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Michael Matz]

Hi,

On Thu, 20 Nov 2014, Patrick Palka wrote:

> > -wrapper is specifically also for invoking cc1 with gdb from the 
> > driver (that's the usecase documented with -wrapper), so it better 
> > should work as intended.  I don't know what problems Patrick had with 
> > that, though.  For me gcc -wrapper gdb,--args works as expected (as in 
> > ^C interrupts cc1 returning to gdb).
> 
> Yes it does for me too. But pressing ^C in gdb while cc1 is not running 
> (by accident or with intention, e.g. pressing ^C to quickly clear the 
> command prompt) will kill the driver and gdb after it. It's not a huge 
> problem but it does cause some inconvenience for users of -wrapper gdb.

Aha!  Indeed that's quite ugly.  I think fixing this would be appropriate.


Ciao,
Michael.

Re: [PATCH] driver: ignore SIGINT while waiting on subprocesses to finish

[Joseph Myers]

On Sat, 15 Nov 2014, Patrick Palka wrote:

>   1. if the top-level driver is waiting on a hanging subprocess,
>   pressing ^C will kill the driver but it may not necessarily kill the
>   subprocess; an unresponsive, perhaps busy-looping subprocess may be
>   running in the background yet the compiler will seem to have to
>   terminated successfully.

The subprocess should be in the same process group as the driver, so ^C 
should send SIGINT to both processes.  Is your concern about ^C being 
pressed at exactly the time when collect2 or lto-wrapper has just called 
signal (SIGINT, SIG_IGN) in order to find out whether SIGINT was already 
set to SIG_IGN?  If so, the right way to avoid that short period of time 
when signals are ignored but shouldn't be would I think be to use 
sigaction to query the existing handler without changing it, when 
sigaction is available.

-- 
Joseph S. Myers
joseph@codesourcery.com