[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
>>