* DevNotes: Add entry cgf-000019.

* dcrt0.cc (do_exit): Just set exit_state to ES_EVENTS_TERMINATE and nuke call
to events_terminate which just set a superfluous flag.
* sigproc.cc (signal_exit_code): New variable.
(setup_signal_exit): Define new function.
(_cygtls::signal_exit): Remove accommodations for closing the signal pipe
handle.
(exit_thread): Just sleep if we're exiting.
(wait_sig): If signal_exit_code is set, just handle bookkeeping signals and
exit ReadFile loop if there is nothing more to process.  Call signal_exit at
end if signal_exit_code is non-zero.
* sigproc.h (setup_signal_exit): Declare new function.
* exceptions.cc (sigpacket::process): Use setup_signal_exit to control exiting
due to a signal.
(exception::handle): Ditto.  Query exit_state rather than defunct exit_already
to determine if we are exiting.
* globals.cc (ES_SIGNAL_EXIT): New enum.
* sync.h (lock_process::release): New function for explicitly unlocking muto.
(lock_process::~lock_process): Use release method.

8 files changed, 117 insertions, 43 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 4cfece717..96471a06d 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,3 +1,26 @@
+2012-12-28  Christopher Faylor  <me.cygwin2012@cgf.cx>
+
+	* DevNotes: Add entry cgf-000019.
+	* dcrt0.cc (do_exit): Just set exit_state to ES_EVENTS_TERMINATE and
+	nuke call to events_terminate which just set a superfluous flag.
+	* sigproc.cc (signal_exit_code): New variable.
+	(setup_signal_exit): Define new function.
+	(_cygtls::signal_exit): Remove accommodations for closing the signal
+	pipe handle.
+	(exit_thread): Just sleep if we're exiting.
+	(wait_sig): If signal_exit_code is set, just handle bookkeeping signals
+	and exit ReadFile loop if there is nothing more to process.  Call
+	signal_exit at end if signal_exit_code is non-zero.
+	* sigproc.h (setup_signal_exit): Declare new function.
+	* exceptions.cc (sigpacket::process): Use setup_signal_exit to control
+	exiting due to a signal.
+	(exception::handle): Ditto.  Query exit_state rather than defunct
+	exit_already to determine if we are exiting.
+	* globals.cc (ES_SIGNAL_EXIT): New enum.
+	* sync.h (lock_process::release): New function for explicitly unlocking
+	muto.
+	(lock_process::~lock_process): Use release method.
+
 2012-12-27  Christopher Faylor  <me.cygwin2012@cgf.cx>
 
 	* fork.cc (child_info::prefork): Fix error message formatting.
diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes
index 04abd99cc..01293bc50 100644
--- a/winsup/cygwin/DevNotes
+++ b/winsup/cygwin/DevNotes
@@ -1,3 +1,32 @@
+2012-12-28  cgf-000019
+
+(I forgot to mention that cgf-000018 was reverted.  Although I never saw
+a hang from this, I couldn't convince myself that one wasn't possible.)
+
+This fix attempts to correct a deadlock where, when a true Windows
+signal arrives, Windows creates a thread which "does stuff" and attempts
+to exit.  In the process of exiting Cygwin grabs the process lock.  If
+the signal thread has seen the signal and wants to exit, it can't
+because the newly-created thread now holds it.  But, since the new
+thread is relying on the signal thread to release its process lock,
+it exits and the process lock is never released.
+
+To fix this, I removed calls to _cygtls::signal_exit in favor of
+flagging that we were exiting by setting signal_exit_code (almost forgot
+to mark that NO_COPY: that would have been fun).  The new function
+setup_signal_exit() now handles setting things up so that ReadFile loop
+in wait_sig will do the right thing when it terminates.  This function
+may just Sleep indefinitely if a signal is being sent from a thread
+other than the signal thread.  wait_sig() was changed so that it will
+essentially drop into asychronous-read-mode when a signal which exits
+has been detected.  The ReadFile loop is exited when we know that the
+process is supposed to be exiting and there is nothing else in the
+signal queue.
+
+Although I never actually saw this happen, exit_thread() was also
+changed to release the process lock and just sleep indefintely if it is
+detected that we are exiting.
+
 2012-12-21  cgf-000018
 
 Re: cgf-000017
diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index 0a2584239..b55dd9069 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -1098,10 +1098,7 @@ do_exit (int status)
   lock_process until_exit (true);
 
   if (exit_state < ES_EVENTS_TERMINATE)
-    {
-      exit_state = ES_EVENTS_TERMINATE;
-      events_terminate ();
-    }
+    exit_state = ES_EVENTS_TERMINATE;
 
   if (exit_state < ES_SIGNAL)
     {
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index 590fcd9b5..f1f0a43f8 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -41,8 +41,6 @@ static BOOL WINAPI ctrl_c_handler (DWORD);
 
 /* This is set to indicate that we have already exited.  */
 
-static NO_COPY int exit_already = 0;
-
 NO_COPY static struct
 {
   unsigned int code;
@@ -481,9 +479,9 @@ exception::handle (EXCEPTION_RECORD *e, exception_list *frame, CONTEXT *in, void
       return 0;
     }
 
-  /* If we've already exited, don't do anything here.  Returning 1
+  /* If we're exiting, don't do anything here.  Returning 1
      tells Windows to keep looking for an exception handler.  */
-  if (exit_already || e->ExceptionFlags)
+  if (exit_state || e->ExceptionFlags)
     return 1;
 
   siginfo_t si = {0};
@@ -673,8 +671,7 @@ exception::handle (EXCEPTION_RECORD *e, exception_list *frame, CONTEXT *in, void
 			  error_code);
 	}
 
-      /* Flag signal + core dump */
-      me.signal_exit ((cygheap->rlim_core > 0UL ? 0x80 : 0) | si.si_signo);
+      setup_signal_exit ((cygheap->rlim_core > 0UL ? 0x80 : 0) | si.si_signo);
     }
 
   si.si_addr =  (si.si_signo == SIGSEGV || si.si_signo == SIGBUS
@@ -1249,13 +1246,9 @@ done:
   return rc;
 
 exit_sig:
-  tls->signal_exit (si.si_signo);	/* never returns */
-}
-
-void
-events_terminate ()
-{
-  exit_already = 1;
+  sigproc_printf ("setting up for exit with signal %d", si.si_signo);
+  setup_signal_exit (si.si_signo);
+  return rc;
 }
 
 int
diff --git a/winsup/cygwin/globals.cc b/winsup/cygwin/globals.cc
index 387515851..e0fa3eed4 100644
--- a/winsup/cygwin/globals.cc
+++ b/winsup/cygwin/globals.cc
@@ -34,6 +34,7 @@ UINT system_wow64_directory_length;
 enum exit_states
 {
   ES_NOT_EXITING = 0,
+  ES_SIGNAL_EXIT,
   ES_EXIT_STARTING,
   ES_PROCESS_LOCKED,
   ES_EVENTS_TERMINATE,
diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
index 33fbb3a00..32bc9a8f2 100644
--- a/winsup/cygwin/sigproc.cc
+++ b/winsup/cygwin/sigproc.cc
@@ -61,6 +61,7 @@ _cygtls NO_COPY *_sig_tls;
 
 Static HANDLE my_sendsig;
 Static HANDLE my_readsig;
+Static int signal_exit_code;
 
 /* Function declarations */
 static int __stdcall checkstate (waitq *) __attribute__ ((regparm (1)));
@@ -361,32 +362,12 @@ close_my_readsig ()
     ForceCloseHandle1 (h, my_readsig);
 }
 
-/* Cover function to `do_exit' to handle exiting even in presence of more
-   exceptions.  We used to call exit, but a SIGSEGV shouldn't cause atexit
-   routines to run.  */
+/* Exit due to a signal, even in presence of more exceptions.  We used to just
+   call exit, but a SIGSEGV shouldn't cause atexit routines to run.
+   Should only be called from the signal thread.  */
 void
 _cygtls::signal_exit (int rc)
 {
-  HANDLE myss = my_sendsig;
-  my_sendsig = NULL;		 /* Make no_signals_allowed return true */
-
-  /* This code used to try to always close my_readsig but it ended up
-     blocking for reasons that people in google think make sense.
-     It's possible that it was blocking because ReadFile was still active
-     but it isn't clear why this only caused random hangs rather than
-     consistent hangs.  So, for now at least, avoid closing my_readsig
-     unless this is the signal thread.  */
-  if (&_my_tls == _sig_tls)
-    close_my_readsig ();	/* Stop any currently executing sig_sends */
-  else
-    {
-      sigpacket sp = {};
-      sp.si.si_signo = __SIGEXIT;
-      DWORD len;
-      /* Write a packet to the wait_sig thread which tells it to exit and
-	 close my_readsig.  */
-      WriteFile (myss, &sp, sizeof (sp), &len, NULL);
-    }
   signal_debugger (rc & 0x7f);
 
   if (rc == SIGQUIT || rc == SIGABRT)
@@ -553,6 +534,27 @@ sigproc_terminate (exit_states es)
     }
 }
 
+/* Set up stuff so that the signal thread will know that we are
+   exiting due to a signal.  */
+void
+setup_signal_exit (int sig)
+{
+  signal_exit_code = sig;	/* Tell wait_sig() that we are exiting. */
+  exit_state = ES_SIGNAL_EXIT;	/* Tell the rest of the world that we are exiting. */
+
+  if (&_my_tls != _sig_tls)
+    {
+      sigpacket sp = {};
+      sp.si.si_signo = __SIGEXIT;
+      DWORD len;
+      /* Write a packet to the wait_sig thread.  It will eventuall cause
+	 the process to exit too.  So just wait for that to happen after
+	 sending the packet. */
+      WriteFile (my_sendsig, &sp, sizeof (sp), &len, NULL);
+      Sleep (INFINITE);
+    }
+}
+
 /* Exit the current thread very carefully.
    See cgf-000017 in DevNotes for more details on why this is
    necessary.  */
@@ -576,10 +578,16 @@ exit_thread (DWORD res)
   siginfo_t si = {__SIGTHREADEXIT, SI_KERNEL};
   si.si_value.sival_ptr = h;
   lock_process for_now;		/* May block indefinitely if we're exiting. */
+  if (exit_state)
+    {
+      for_now.release ();
+      Sleep (INFINITE);
+    }
+
   /* Tell wait_sig to wait for this thread to exit.  It can then release
      the lock below and close the above-opened handle. */
   sig_send (myself_nowait, si, &_my_tls);
-  ExitThread (0);		/* Should never hit this */
+  ExitThread (0);
 }
 
 int __stdcall
@@ -1368,6 +1376,7 @@ pending_signals::next ()
 static void WINAPI
 wait_sig (VOID *)
 {
+  extern int signal_exit_code;
   _sig_tls = &_my_tls;
   sig_hold = CreateEvent (&sec_none_nih, FALSE, FALSE, NULL);
 
@@ -1380,7 +1389,14 @@ wait_sig (VOID *)
     {
       if (pack.si.si_signo == __SIGHOLD)
 	WaitForSingleObject (sig_hold, INFINITE);
+
       DWORD nb;
+      /* If signal_exit_code is set then we are shutting down due to a signal.
+	 We'll exit this loop iff there is nothing more in the signal queue.  */
+      if (signal_exit_code
+	  && (!PeekNamedPipe (my_readsig, NULL, 0, NULL, &nb, NULL) || !nb))
+	break;
+
       pack.sigtls = NULL;
       if (!ReadFile (my_readsig, &pack, sizeof (pack), &nb, NULL))
 	break;
@@ -1400,6 +1416,9 @@ wait_sig (VOID *)
 	  continue;
 	}
 
+      if (signal_exit_code && pack.si.si_signo > 0)
+	continue;		/* No more real signals allowed */
+
       sigset_t dummy_mask;
       if (!pack.mask)
 	{
@@ -1505,8 +1524,14 @@ wait_sig (VOID *)
 	break;
     }
 
-  close_my_readsig ();
   sigproc_printf ("signal thread exiting");
+
+  my_sendsig = NULL;		/* Make no_signals_allowed return true */
+  close_my_readsig ();		/* Cause any sig_send's to stop */
+
+  if (signal_exit_code)
+    _my_tls.signal_exit (signal_exit_code);
+
   /* Just wait for the process to go away.  Otherwise, this thread's
      exit value could be interpreted as the process exit value.
      See cgf-000017 in DevNotes for more details.  */
diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h
index 371641b40..26332935e 100644
--- a/winsup/cygwin/sigproc.h
+++ b/winsup/cygwin/sigproc.h
@@ -89,6 +89,7 @@ void __stdcall sigalloc ();
 int kill_pgrp (pid_t, siginfo_t&);
 int killsys (pid_t, int);
 void exit_thread (DWORD) __attribute__ ((regparm (1), noreturn));
+void setup_signal_exit (int) __attribute__ ((regparm (1)));
 
 extern "C" void sigdelayed ();
 
diff --git a/winsup/cygwin/sync.h b/winsup/cygwin/sync.h
index d424d39ab..5c6bc4b27 100644
--- a/winsup/cygwin/sync.h
+++ b/winsup/cygwin/sync.h
@@ -55,10 +55,15 @@ public:
     if (exiting && exit_state < ES_PROCESS_LOCKED)
       exit_state = ES_PROCESS_LOCKED;
   }
+  void release ()
+  {
+    locker.release ();
+    skip_unlock = true;
+  }
   ~lock_process ()
   {
     if (!skip_unlock)
-      locker.release ();
+      release ();
   }
   static void force_release (_cygtls *tid) {locker.release (tid);}
   friend class dtable;