* 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.
This commit is contained in:
		@@ -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.
 | 
			
		||||
 
 | 
			
		||||
@@ -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
 | 
			
		||||
 
 | 
			
		||||
@@ -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)
 | 
			
		||||
    {
 | 
			
		||||
 
 | 
			
		||||
@@ -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
 | 
			
		||||
 
 | 
			
		||||
@@ -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,
 | 
			
		||||
 
 | 
			
		||||
@@ -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.  */
 | 
			
		||||
 
 | 
			
		||||
@@ -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 ();
 | 
			
		||||
 | 
			
		||||
 
 | 
			
		||||
@@ -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;
 | 
			
		||||
 
 | 
			
		||||
		Reference in New Issue
	
	Block a user