* DevNotes: Add entry cgf-000016.

* cygtls.h (_cygtls::push): Inline.
(_cygtls::interrupt_now): Change signal number argument to siginfo_t argument.
(_cygtls::interrupt_setup): Ditto.
(_cygtls::set_siginfo): Delete declaration.
(_cygtls::reset_signal_arrived): Don't reset signal_arrived signal.  Just reset
flag.
* exceptions.cc (_cygtls::interrupt_now): Reflect argument changes.  Pass si to
interrupt_setup.
(_cygtls::interrupt_setup): Reflect argument changes.  Fill out tls infodata
here using passed-in si.  Use si.si_signo instead of sig.
(sigpacket::setup_handler): Move this function into sigpacket class.  Use si
field from the class as appropriate.
(sigpacket::process): Don't call tls->set_siginfo here since setup_handler
could fail.  Eliminate now-unneeded sig argument.
* sigproc.h (sigpacket::setup_handler): Move setup_handler to this class.

6 files changed, 66 insertions, 38 deletions

diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog
index 937ca36b9..2b67a163c 100644
--- a/winsup/cygwin/ChangeLog
+++ b/winsup/cygwin/ChangeLog
@@ -1,5 +1,26 @@
 2012-08-17  Christopher Faylor  <me.cygwin2012@cgf.cx>
 
+	* DevNotes: Add entry cgf-000016.
+	* cygtls.h (_cygtls::push): Inline.
+	(_cygtls::interrupt_now): Change signal number argument to siginfo_t
+	argument.
+	(_cygtls::interrupt_setup): Ditto.
+	(_cygtls::set_siginfo): Delete declaration.
+	(_cygtls::reset_signal_arrived): Don't reset signal_arrived signal.
+	Just reset flag.
+	* exceptions.cc (_cygtls::interrupt_now): Reflect argument changes.
+	Pass si to interrupt_setup.
+	(_cygtls::interrupt_setup): Reflect argument changes.  Fill out tls
+	infodata here using passed-in si.  Use si.si_signo instead of sig.
+	(sigpacket::setup_handler): Move this function into sigpacket class.
+	Use si field from the class as appropriate.
+	(sigpacket::process): Don't call tls->set_siginfo here since
+	setup_handler could fail.  Eliminate now-unneeded sig argument.
+	* sigproc.h (sigpacket::setup_handler): Move setup_handler to this
+	class.
+
+2012-08-17  Christopher Faylor  <me.cygwin2012@cgf.cx>
+
 	* exceptions.cc (sig_handle_tty_stop): Clear tls sig field.
 	(sigpacket::process): When continuing, set tls->sig before arming
 	signal_arrived.
diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes
index 0e86fe516..aeca33076 100644
--- a/winsup/cygwin/DevNotes
+++ b/winsup/cygwin/DevNotes
@@ -1,3 +1,29 @@
+2012-08-17  cgf-000016
+
+While debugging another problem I finally noticed that
+sigpacket::process was unconditionally calling tls->set_siginfo prior to
+calling setup_handler even though setup_handler could fail.  In the
+event of two successive signals, that would cause the second signal's
+info to overwrite the first even though the signal handler for the first
+would eventually be called.  Doh.
+
+Fixing this required passing the sigpacket si field into setup_handler.
+Making setup_handler part of the sigpacket class seemed to make a lot of
+sense so that's what I did.  Then I passed the si element into
+interrupt_setup so that the infodata structure could be filled out prior
+to arming the signal.
+
+The other changes checked in here eliminate the ResetEvent for
+signal_arrived since previous changes to cygwait should handle the
+case of spurious signal_arrived detection.  Since signal_arrived is
+not a manual-reset event, we really should just let the appropriate
+WFMO handle it.  Otherwise, there is a race where a signal comes in
+a "split second" after WFMO responds to some other event.  Resetting
+the signal_arrived would cause any subsequent WFMO to never be
+triggered.  My current theory is that this is what is causing:
+
+http://cygwin.com/ml/cygwin/2012-08/msg00310.html
+
 2012-08-15  cgf-000015
 
 RIP cancelable_wait.  Yay.
diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc
index eca0ac9a0..2ff22ed98 100644
--- a/winsup/cygwin/cygtls.cc
+++ b/winsup/cygwin/cygtls.cc
@@ -196,15 +196,3 @@ _cygtls::remove (DWORD wait)
   cygheap->remove_tls (this, wait);
   remove_wq (wait);
 }
-
-void
-_cygtls::push (__stack_t addr)
-{
-  *stackptr++ = (__stack_t) addr;
-}
-
-void
-_cygtls::set_siginfo (sigpacket *pack)
-{
-  infodata = pack->si;
-}
diff --git a/winsup/cygwin/cygtls.h b/winsup/cygwin/cygtls.h
index d009da169..4f4f17c6b 100644
--- a/winsup/cygwin/cygtls.h
+++ b/winsup/cygwin/cygtls.h
@@ -207,17 +207,16 @@ public:
   void init_thread (void *, DWORD (*) (void *, void *));
   static void call (DWORD (*) (void *, void *), void *);
   void remove (DWORD);
-  void push (__stack_t) __attribute__ ((regparm (2)));
+  void push (__stack_t addr) {*stackptr++ = (__stack_t) addr;}
   __stack_t pop () __attribute__ ((regparm (1)));
   __stack_t retaddr () {return stackptr[-1];}
   bool isinitialized () const
   {
     return initialized == CYGTLS_INITIALIZED;
   }
-  bool interrupt_now (CONTEXT *, int, void *, struct sigaction&)
+  bool interrupt_now (CONTEXT *, siginfo_t&, void *, struct sigaction&)
     __attribute__((regparm(3)));
-  void __stdcall interrupt_setup (int sig, void *handler,
-				  struct sigaction& siga)
+  void __stdcall interrupt_setup (siginfo_t&, void *, struct sigaction&)
     __attribute__((regparm(3)));
 
   bool inside_kernel (CONTEXT *);
@@ -228,7 +227,6 @@ public:
 #ifdef CYGTLS_HANDLE
   operator HANDLE () const {return tid ? tid->win32_obj_id : NULL;}
 #endif
-  void set_siginfo (struct sigpacket *) __attribute__ ((regparm (3)));
   int call_signal_handler () __attribute__ ((regparm (1)));
   void remove_wq (DWORD) __attribute__ ((regparm (1)));
   void fixup_after_fork () __attribute__ ((regparm (1)));
@@ -255,12 +253,7 @@ public:
 	signal_waiting = true;
       }
   }
-  void reset_signal_arrived ()
-  {
-    if (signal_arrived)
-      ResetEvent (signal_arrived);
-    signal_waiting = false;
-  }
+  void reset_signal_arrived () { signal_waiting = false; }
 private:
   void call2 (DWORD (*) (void *, void *), void *, void *) __attribute__ ((regparm (3)));
   /*gentls_offsets*/
diff --git a/winsup/cygwin/exceptions.cc b/winsup/cygwin/exceptions.cc
index fe5010ee1..aa3032816 100644
--- a/winsup/cygwin/exceptions.cc
+++ b/winsup/cygwin/exceptions.cc
@@ -757,7 +757,7 @@ sig_handle_tty_stop (int sig)
 } /* end extern "C" */
 
 bool
-_cygtls::interrupt_now (CONTEXT *cx, int sig, void *handler,
+_cygtls::interrupt_now (CONTEXT *cx, siginfo_t& si, void *handler,
 			struct sigaction& siga)
 {
   bool interrupted;
@@ -771,7 +771,7 @@ _cygtls::interrupt_now (CONTEXT *cx, int sig, void *handler,
   else
     {
       push ((__stack_t) cx->Eip);
-      interrupt_setup (sig, handler, siga);
+      interrupt_setup (si, handler, siga);
       cx->Eip = pop ();
       SetThreadContext (*this, cx); /* Restart the thread in a new location */
       interrupted = true;
@@ -780,7 +780,7 @@ _cygtls::interrupt_now (CONTEXT *cx, int sig, void *handler,
 }
 
 void __stdcall
-_cygtls::interrupt_setup (int sig, void *handler, struct sigaction& siga)
+_cygtls::interrupt_setup (siginfo_t& si, void *handler, struct sigaction& siga)
 {
   push ((__stack_t) sigdelayed);
   deltamask = siga.sa_mask & ~SIG_NONMASKABLE;
@@ -795,7 +795,8 @@ _cygtls::interrupt_setup (int sig, void *handler, struct sigaction& siga)
       myself->process_state |= PID_STOPPED;
     }
 
-  this->sig = sig;			// Should always be last thing set to avoid a race
+  infodata = si;
+  this->sig = si.si_signo;		// Should always be last thing set to avoid a race
 
   if (incyg)
     {
@@ -805,7 +806,7 @@ _cygtls::interrupt_setup (int sig, void *handler, struct sigaction& siga)
     }
 
   proc_subproc (PROC_CLEARWAIT, 1);
-  sigproc_printf ("armed signal_arrived %p, signal %d", signal_arrived, sig);
+  sigproc_printf ("armed signal_arrived %p, signal %d", signal_arrived, si.si_signo);
 }
 
 extern "C" void __stdcall
@@ -815,10 +816,8 @@ set_sig_errno (int e)
   _my_tls.saved_errno = e;
 }
 
-static int setup_handler (int, void *, struct sigaction&, _cygtls *tls)
-  __attribute__((regparm(3)));
-static int
-setup_handler (int sig, void *handler, struct sigaction& siga, _cygtls *tls)
+int
+sigpacket::setup_handler (void *handler, struct sigaction& siga, _cygtls *tls)
 {
   CONTEXT cx;
   bool interrupted = false;
@@ -826,7 +825,7 @@ setup_handler (int sig, void *handler, struct sigaction& siga, _cygtls *tls)
   if (tls->sig)
     {
       sigproc_printf ("trying to send signal %d but signal %d already armed",
-		      sig, tls->sig);
+		      si.si_signo, tls->sig);
       goto out;
     }
 
@@ -839,7 +838,7 @@ setup_handler (int sig, void *handler, struct sigaction& siga, _cygtls *tls)
 	    {
 	      sigproc_printf ("controlled interrupt. stackptr %p, stack %p, stackptr[-1] %p",
 			      tls->stackptr, tls->stack, tls->stackptr[-1]);
-	      tls->interrupt_setup (sig, handler, siga);
+	      tls->interrupt_setup (si, handler, siga);
 	      interrupted = true;
 	      tls->unlock ();
 	      goto out;
@@ -868,7 +867,7 @@ setup_handler (int sig, void *handler, struct sigaction& siga, _cygtls *tls)
 	      if (!GetThreadContext (hth, &cx))
 		sigproc_printf ("couldn't get context of thread, %E");
 	      else
-		interrupted = tls->interrupt_now (&cx, sig, handler, siga);
+		interrupted = tls->interrupt_now (&cx, si, handler, siga);
 
 	      tls->unlock ();
 	      ResumeThread (hth);
@@ -885,7 +884,7 @@ setup_handler (int sig, void *handler, struct sigaction& siga, _cygtls *tls)
     }
 
 out:
-  sigproc_printf ("signal %d %sdelivered", sig, interrupted ? "" : "not ");
+  sigproc_printf ("signal %d %sdelivered", si.si_signo, interrupted ? "" : "not ");
   return interrupted;
 }
 
@@ -1234,10 +1233,9 @@ dosig:
       rc = -1;		/* No signals delivered if stopped */
   else
     {
-      tls->set_siginfo (this);
       /* Dispatch to the appropriate function. */
       sigproc_printf ("signal %d, signal handler %p", si.si_signo, handler);
-      rc = setup_handler (si.si_signo, handler, thissig, tls);
+      rc = setup_handler (handler, thissig, tls);
       continue_now = false;
     }
 
diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h
index 6e6f3f46f..d442232e9 100644
--- a/winsup/cygwin/sigproc.h
+++ b/winsup/cygwin/sigproc.h
@@ -56,6 +56,8 @@ struct sigpacket
     struct sigpacket *next;
   };
   int __stdcall process () __attribute__ ((regparm (1)));
+  int setup_handler (void *handler, struct sigaction& siga, _cygtls *tls)
+    __attribute__ ((regparm (3)));
 };
 
 void __stdcall sig_dispatch_pending (bool fast = false)