Re: WINDOWS32 "Signal 127" due to a path in quotes bug

[Eli Zaretskii]

> Date: Mon, 01 Aug 2005 00:09:54 +0100
> From: "J. Grant" <address@hidden>
> CC: address@hidden
> 
> 1) Testing on Debian GNU/Linux
> 
> $ make -f makefile_sig.mak -j8
> <snip>
> 15299
> 15300
> make: *** [temp1] Interrupt
> make: *** [temp2] Interrupt

Yep, I see the same on a Debian box, except that I don't see those
15XXX numbers.  Where are they from? did you run a Makefile that's
different from what you sent? was this with a version of Make other
than 3.81beta3?

> MSYS, with MSVC build:
> 
> make -f makefile_sig.mak -j8
> 
> 15299
> 15300
> 
> rxvt.exe
>     sh.exe
>       make.exe
>          main.exe
>          main.exe
> 
> Pressing <ctrl> + C kills make.exe.  So main.exe and main.exe continue
> on their own, sh and rxvt are broken, and have to be killed and restarted.

I cannot reproduce this, but I don't have MSYS and rxvt installed.  In
particular, I don't see make.exe killed, but its children still
running.  Can you see what happens when you invoke the same command,
but outside rxvt/sh?  I.e., if you invoke Make from the stock Windows
Command Prompt window running cmd.exe?

> would it be expected that the <ctrl>+c signal would not be passed to the
> launched process?

In general, Ctrl-C is documented on Windows to be passed to all the
programs that are attached to the console which has the focus when
Ctrl-C is hit.  For the precise Microsoft language and some pointers
to related functions, see:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/base/getexitcodeprocess.asp

> 2) Using todays CVS source on an MSCV build I experienced this:
> 
> C:\msys\1.0\home\now3d>make -f makefile_sig.mak -j2
> main
> main
> counting:
> counting:
> make: *** [temp1] Error -1073741510make:
> *** [temp2] Error -1073741510
> 
> 
> Is the odd exit value just because my simple C test program does not
> handle <ctrl>+C ?

No, it's a bug in Make's w32-specific code.  See the patches below.
In a nutshell, this special value is returned when the subordinate
program was interrupted by Ctrl-C; sub_proc.c should note that and set
pproc->signal accordingly.

I also found another serious problem with the current w32 code.  It
doesn't take into consideration the fact that (as the above URL spells
out), when Ctrl-C is hit, Windows creates a new thread in the
application to handle the signal.  This causes Make to suddenly become
multithreaded, unlike on Posix platforms.  The two threads each call
the function reap_children in a loop, which create ``interesting''
race conditions.  You can see this even without parallelism: just
invoke Make without -j8, but with --debug=v,j and watch the strange
reports about "Live child" even after the interrupt was already
received and the losing child reaped and removed from the chain.  I
fixed that (in the patches below) by manually suspending the main
thread when the signal handler is invoked, because that's what happens
on Posix platforms.

After applying the patches below, I see the same output on both Debian
GNU/Linux and Windows XP.

The next problem to attack would be the other signals: Ctrl-BREAK
first and foremost, then SIGSEGV and SIGFPE.  When I have time...

Here are the patches I suggest to fix the problems with SIGINT.  I
tested them only in the MinGW build running from cmd.exe, so testing
in other environment and with other compilers is welcome.

2005-08-06  Eli Zaretskii  <address@hidden>

        * w32/subproc/sub_proc.c: Include signal.h.
        (process_pipe_io, process_file_io): Pass a pointer to a local
        DWORD variable to GetExitCodeProcess.  If the exit code is
        CONTROL_C_EXIT, put SIGINT into pproc->signal.

        * job.c [WINDOWS32]: Include windows.h.
        (main_thread) [WINDOWS32]: New global variable.
        (reap_children) [WINDOWS32]: Get the handle for the main thread
        and store it in main_thread.

        * commands.c [WINDOWS32]: Include windows.h and w32err.h.
        (fatal_error_signal) [WINDOWS32]: Suspend the main thread before
        doing anything else.  When we are done, close the main thread
        handle and exit with status 130.


--- commands.c~1        2005-03-04 16:52:32.000000000 +0200
+++ commands.c  2005-08-06 15:59:15.770000000 +0300
@@ -23,6 +23,10 @@ Boston, MA 02111-1307, USA.  */
 #include "variable.h"
 #include "job.h"
 #include "commands.h"
+#ifdef WINDOWS32
+#include <windows.h>
+#include "w32err.h"
+#endif
 
 #if VMS
 # define FILE_LIST_SEPARATOR ','
@@ -420,6 +424,27 @@ fatal_error_signal (int sig)
 
   exit (10);
 #else /* not Amiga */
+#ifdef WINDOWS32
+  extern HANDLE main_thread;
+
+  /* Windows creates a sperate thread for handling Ctrl+C, so we need
+     to suspend the main thread, or else we will have race conditions
+     when both threads call reap_children.  */
+  if (main_thread)
+    {
+      DWORD susp_count = SuspendThread (main_thread);
+
+      if (susp_count != 0)
+       fprintf (stderr, "SuspendThread: suspend count = %ld\n", susp_count);
+      else if (susp_count == (DWORD)-1)
+       {
+         DWORD ierr = GetLastError ();
+
+         fprintf (stderr, "SuspendThread: error %ld: %s\n",
+                  ierr, map_windows32_error_to_string (ierr));
+       }
+    }
+#endif
   handling_fatal_signal = 1;
 
   /* Set the handling for this signal to the default.
@@ -482,8 +507,11 @@ fatal_error_signal (int sig)
 #endif
 
 #ifdef WINDOWS32
-  /* Cannot call W32_kill with a pid (it needs a handle) */
-  exit (EXIT_FAILURE);
+  if (main_thread)
+    CloseHandle (main_thread);
+  /* Cannot call W32_kill with a pid (it needs a handle).  The exit
+     status of 130 emulates what happens in Bash.  */
+  exit (130);
 #else
   /* Signal the same code; this time it will really be fatal.  The signal
      will be unblocked when we return and arrive then to kill us.  */

--- job.c~1     2005-07-30 16:57:16.755000000 +0300
+++ job.c       2005-08-06 17:14:42.691875000 +0300
@@ -33,10 +33,12 @@ Boston, MA 02111-1307, USA.  */
 
 /* Default shell to use.  */
 #ifdef WINDOWS32
+#include <windows.h>
 
 char *default_shell = "sh.exe";
 int no_default_sh_exe = 1;
 int batch_mode_shell = 1;
+HANDLE main_thread;
 
 #elif defined (_AMIGA)
 
@@ -611,10 +613,30 @@ reap_children (int block, int err)
           {
             HANDLE hPID;
             int err;
+            HANDLE hcTID, hcPID;
             exit_code = 0;
             exit_sig = 0;
             coredump = 0;
 
+            /* Record the thread ID of the main process, so that we
+               could suspend it in the signal handler.  */
+            if (!main_thread)
+              {
+                hcTID = GetCurrentThread ();
+                hcPID = GetCurrentProcess ();
+                if (!DuplicateHandle (hcPID, hcTID, hcPID, &main_thread, 0,
+                                      FALSE, DUPLICATE_SAME_ACCESS))
+                  {
+                    DWORD e = GetLastError ();
+                    fprintf(stderr,
+                            "Determine main thread ID (Error %ld: %s)\n",
+                            e, map_windows32_error_to_string(e));
+                  }
+                else
+                  DB (DB_VERBOSE, ("Main thread handle = 0x%08lx\n",
+                                   (unsigned long)main_thread));
+              }
+
             /* wait for anything to finish */
             hPID = process_wait_for_any();
             if (hPID)

--- w32/subproc/sub_proc.c~     2005-06-27 21:40:56.000000000 +0300
+++ w32/subproc/sub_proc.c      2005-08-06 16:45:22.941875000 +0300
@@ -1,6 +1,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <process.h>  /* for msvc _beginthreadex, _endthreadex */
+#include <signal.h>
 #include <windows.h>
 
 #include "sub_proc.h"
@@ -762,7 +764,13 @@ process_pipe_io(
 
                } else if (ready_hand == childhand) {
 
-                       GetExitCodeResult = GetExitCodeProcess(childhand, 
(DWORD*)&pproc->exit_code);
+                       DWORD ierr;
+                       GetExitCodeResult = GetExitCodeProcess(childhand, 
&ierr);
+                       if (ierr == CONTROL_C_EXIT) {
+                               pproc->signal = SIGINT;
+                       } else {
+                               pproc->exit_code = ierr;
+                       }
                        if (GetExitCodeResult == FALSE) {
                                pproc->last_err = GetLastError();
                                pproc->lerrno = E_SCALL;
@@ -811,6 +819,7 @@ process_file_io(
        HANDLE childhand;
        DWORD wait_return;
        BOOL GetExitCodeResult;
+       DWORD ierr;
 
        if (proc == NULL)
                pproc = process_wait_for_any_private();
@@ -854,7 +863,12 @@ process_file_io(
                goto done2;
        }
 
-       GetExitCodeResult = GetExitCodeProcess(childhand, 
(DWORD*)&pproc->exit_code);
+       GetExitCodeResult = GetExitCodeProcess(childhand, &ierr);
+       if (ierr == CONTROL_C_EXIT) {
+               pproc->signal = SIGINT;
+       } else {
+               pproc->exit_code = ierr;
+       }
        if (GetExitCodeResult == FALSE) {
                pproc->last_err = GetLastError();
                pproc->lerrno = E_SCALL;