From 39d01715004ef9a79036965d83192339ec923cac Mon Sep 17 00:00:00 2001 From: Christopher Faylor Date: Fri, 17 Aug 2012 17:29:21 +0000 Subject: * 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. --- winsup/cygwin/ChangeLog | 21 +++++++++++++++++++++ winsup/cygwin/DevNotes | 26 ++++++++++++++++++++++++++ winsup/cygwin/cygtls.cc | 12 ------------ winsup/cygwin/cygtls.h | 15 ++++----------- winsup/cygwin/exceptions.cc | 28 +++++++++++++--------------- winsup/cygwin/sigproc.h | 2 ++ 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,3 +1,24 @@ +2012-08-17 Christopher Faylor + + * 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 * exceptions.cc (sig_handle_tty_stop): Clear tls sig field. 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) -- cgit v1.2.3