From 614aff88a0cf6c0ec5ec5ba063b003549dedc9db Mon Sep 17 00:00:00 2001 From: Christopher Faylor Date: Fri, 21 Dec 2012 18:52:00 +0000 Subject: [PATCH] * DevNotes: Add entry cgf-000017. * _cygtls.cc (_cygtls::call2): Use new exit_thread function in place of ExitThread. * miscfuncs.cc (thread_wrapper): Ditto. * thread.cc (pthread::exit): Ditto. (pthread_mutex::unlock): Set tid to NULL rather than 0. (pthread_spinlock::unlock): Ditto. * pinfo.cc (commune_process): Actually call lock_process constructor. * sigproc.cc (exit_thread): New function. (wait_sig): Handle __SIGTHREADEXIT case. Don't just block rather than returning from this function. * sigproc.h (__SIGTHREADEXIT): New enum. (exit_thread): Declare. * sync.cc (muto::release): Accept a tls command-line argument. * sync.h (muto::release): Accept a tls command-line parameter. Default to &_my_tls. --- winsup/cygwin/ChangeLog | 19 +++++++++++++++ winsup/cygwin/DevNotes | 24 +++++++++++++++++++ winsup/cygwin/cygtls.cc | 2 +- winsup/cygwin/miscfuncs.cc | 3 ++- winsup/cygwin/pinfo.cc | 2 +- winsup/cygwin/sigproc.cc | 49 +++++++++++++++++++++++++++++++++++++- winsup/cygwin/sigproc.h | 4 +++- winsup/cygwin/sync.cc | 7 +++--- winsup/cygwin/sync.h | 3 ++- winsup/cygwin/thread.cc | 6 ++--- 10 files changed, 106 insertions(+), 13 deletions(-) diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index f1c444daf..76c6929ba 100644 --- a/winsup/cygwin/ChangeLog +++ b/winsup/cygwin/ChangeLog @@ -1,3 +1,22 @@ +2012-12-21 Christopher Faylor + + * DevNotes: Add entry cgf-000017. + * _cygtls.cc (_cygtls::call2): Use new exit_thread function in place of + ExitThread. + * miscfuncs.cc (thread_wrapper): Ditto. + * thread.cc (pthread::exit): Ditto. + (pthread_mutex::unlock): Set tid to NULL rather than 0. + (pthread_spinlock::unlock): Ditto. + * pinfo.cc (commune_process): Actually call lock_process constructor. + * sigproc.cc (exit_thread): New function. + (wait_sig): Handle __SIGTHREADEXIT case. Don't just block rather than + returning from this function. + * sigproc.h (__SIGTHREADEXIT): New enum. + (exit_thread): Declare. + * sync.cc (muto::release): Accept a tls command-line argument. + * sync.h (muto::release): Accept a tls command-line parameter. Default + to &_my_tls. + 2012-12-20 Corinna Vinschen * dcrt0.cc (build_argv): Allow quoted filenames in @ expression. diff --git a/winsup/cygwin/DevNotes b/winsup/cygwin/DevNotes index aeca33076..efa338afd 100644 --- a/winsup/cygwin/DevNotes +++ b/winsup/cygwin/DevNotes @@ -1,3 +1,27 @@ +2012-12-21 cgf-000017 + +The changes in this set are to work around the issue noted here: + +http://cygwin.com/ml/cygwin/2012-12/threads.html#00140 + +The problem is, apparently, that the return value of an ExitThread() +will take precedence over the return value of TerminateProcess/ExitProcess +if the thread is the last one exiting. That's rather amazing... + +For the fix, I replaced all calls to ExitThread with exit_thread(). The +exit_thread function, creates a handle to the current thread and sends +it to a packet via sig_send(__SIGTHREADEXIT). Then it acquires the +process lock and calls ExitThread. + +wait_sig will then wait for the handle, indicating that the thread has +exited, and, when that has happened, remove the process lock on behalf +of the now-defunct thread. wait_sig will now also avoid actually +exiting since it could trigger the same problem. + +Holding process_lock should prevent threads from exiting while a Cygwin +process is shutting down. They will just block forever in that case - +just like wait_sig. + 2012-08-17 cgf-000016 While debugging another problem I finally noticed that diff --git a/winsup/cygwin/cygtls.cc b/winsup/cygwin/cygtls.cc index 2ff22ed98..a94f333a2 100644 --- a/winsup/cygwin/cygtls.cc +++ b/winsup/cygwin/cygtls.cc @@ -102,7 +102,7 @@ _cygtls::call2 (DWORD (*func) (void *, void *), void *arg, void *buf) dynamically loaded. */ if ((void *) func != (void *) dll_crt0_1 && (void *) func != (void *) dll_dllcrt0_1) - ExitThread (res); + exit_thread (res); } void diff --git a/winsup/cygwin/miscfuncs.cc b/winsup/cygwin/miscfuncs.cc index 5f0625447..3d76ed823 100644 --- a/winsup/cygwin/miscfuncs.cc +++ b/winsup/cygwin/miscfuncs.cc @@ -27,6 +27,7 @@ details. */ #include "dtable.h" #include "cygheap.h" #include "pinfo.h" +#include "sigproc.h" #include "exception.h" long tls_ix = -1; @@ -546,7 +547,7 @@ thread_wrapper (VOID *arg) : : [WRAPPER_ARG] "r" (&wrapper_arg), [CYGTLS] "i" (CYGTLS_PADSIZE)); /* Never return from here. */ - ExitThread (0); + exit_thread (0); } HANDLE WINAPI diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc index 7280bce54..6bd05131b 100644 --- a/winsup/cygwin/pinfo.cc +++ b/winsup/cygwin/pinfo.cc @@ -515,7 +515,7 @@ commune_process (void *arg) if (process_sync) // FIXME: this test shouldn't be necessary ProtectHandle (process_sync); - lock_process now (); + lock_process now; if (si._si_commune._si_code & PICOM_EXTRASTR) si._si_commune._si_str = (char *) (&si + 1); diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc index a08a55ed3..be89d8685 100644 --- a/winsup/cygwin/sigproc.cc +++ b/winsup/cygwin/sigproc.cc @@ -553,6 +553,33 @@ sigproc_terminate (exit_states es) } } +/* Exit the current thread very carefully. + See cgf-000017 in DevNotes for more details on why this is + necessary. */ +void +exit_thread (DWORD res) +{ + HANDLE h; + + if (!DuplicateHandle (GetCurrentProcess (), GetCurrentThread (), + GetCurrentProcess (), &h, + 0, FALSE, DUPLICATE_SAME_ACCESS)) + { +#ifdef DEBUGGING + system_printf ("couldn't duplicate the current thread, %E"); +#endif + ExitThread (res); + } + ProtectHandle1 (h, exit_thread); + siginfo_t si = {__SIGTHREADEXIT, SI_KERNEL}; + si.si_value.sival_ptr = h; + /* 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); + lock_process for_now; + ExitThread (0); /* Should never hit this */ +} + int __stdcall sig_send (_pinfo *p, int sig, _cygtls *tid) { @@ -1419,6 +1446,23 @@ wait_sig (VOID *) case __SIGSETPGRP: init_console_handler (true); break; + case __SIGTHREADEXIT: + { + /* Serialize thread exit as the thread exit code can be interpreted + as the process exit code in some cases when racing with + ExitProcess/TerminateProcess. + So, wait for the thread which sent this signal to exit, then + release the process lock which it held and close it's handle. + See cgf-000017 in DevNotes for more details. + */ + HANDLE h = (HANDLE) pack.si.si_value.sival_ptr; + DWORD res = WaitForSingleObject (h, 5000); + lock_process::force_release (pack.sigtls); + ForceCloseHandle1 (h, exit_thread); + if (res != WAIT_OBJECT_0) + system_printf ("WaitForSingleObject(%p) for thread exit returned %u", h, res); + } + break; default: if (pack.si.si_signo < 0) sig_clear (-pack.si.si_signo); @@ -1461,5 +1505,8 @@ wait_sig (VOID *) close_my_readsig (); sigproc_printf ("signal thread exiting"); - ExitThread (0); + /* 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. */ + Sleep (INFINITE); } diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h index a5a2e04c5..27f690dc1 100644 --- a/winsup/cygwin/sigproc.h +++ b/winsup/cygwin/sigproc.h @@ -25,7 +25,8 @@ enum __SIGHOLD = -(NSIG + 7), __SIGNOHOLD = -(NSIG + 8), __SIGEXIT = -(NSIG + 9), - __SIGSETPGRP = -(NSIG + 10) + __SIGSETPGRP = -(NSIG + 10), + __SIGTHREADEXIT = -(NSIG + 11) }; #endif @@ -87,6 +88,7 @@ void __stdcall sigalloc (); int kill_pgrp (pid_t, siginfo_t&); int killsys (pid_t, int); +void exit_thread (DWORD) __attribute__ ((regparm(1), noreturn)); extern "C" void sigdelayed (); diff --git a/winsup/cygwin/sync.cc b/winsup/cygwin/sync.cc index f3796272f..d8b3d8f54 100644 --- a/winsup/cygwin/sync.cc +++ b/winsup/cygwin/sync.cc @@ -4,7 +4,8 @@ which is intended to operate similarly to a mutex but attempts to avoid making expensive calls to the kernel. - Copyright 2000, 2001, 2002, 2003, 2004, 2005, 2008, 2009, 2010 Red Hat, Inc. + Copyright 2000, 2001, 2002, 2003, 2004, 2005, 2008, 2009, 2010, 2011, 2012 + Red Hat, Inc. This file is part of Cygwin. @@ -109,10 +110,8 @@ muto::acquired () /* Return the muto lock. Needs to be called once per every acquire. */ int -muto::release () +muto::release (_cygtls *this_tls) { - void *this_tls = &_my_tls; - if (tls != this_tls || !visits) { SetLastError (ERROR_NOT_OWNER); /* Didn't have the lock. */ diff --git a/winsup/cygwin/sync.h b/winsup/cygwin/sync.h index c9c5fb595..d424d39ab 100644 --- a/winsup/cygwin/sync.h +++ b/winsup/cygwin/sync.h @@ -33,7 +33,7 @@ public: ~muto () #endif int acquire (DWORD ms = INFINITE) __attribute__ ((regparm (2))); /* Acquire the lock. */ - int release () __attribute__ ((regparm (1))); /* Release the lock. */ + int release (_cygtls * = &_my_tls) __attribute__ ((regparm (2))); /* Release the lock. */ bool acquired () __attribute__ ((regparm (1))); void upforgrabs () {tls = this;} // just set to an invalid address @@ -60,6 +60,7 @@ public: if (!skip_unlock) locker.release (); } + static void force_release (_cygtls *tid) {locker.release (tid);} friend class dtable; friend class fhandler_fifo; }; diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc index eacf26741..415ad4af7 100644 --- a/winsup/cygwin/thread.cc +++ b/winsup/cygwin/thread.cc @@ -532,7 +532,7 @@ pthread::exit (void *value_ptr) _main_tls = dummy; _main_tls->initialized = false; } - ExitThread (0); + exit_thread (0); } } @@ -1778,7 +1778,7 @@ pthread_mutex::unlock () { owner = (pthread_t) _unlocked_mutex; #ifdef DEBUGGING - tid = 0; + tid = NULL; #endif if (InterlockedDecrement ((long *) &lock_counter)) ::SetEvent (win32_obj_id); // Another thread is waiting @@ -1905,7 +1905,7 @@ pthread_spinlock::unlock () { owner = (pthread_t) _unlocked_mutex; #ifdef DEBUGGING - tid = 0; + tid = NULL; #endif InterlockedExchange ((long *) &lock_counter, 0); ::SetEvent (win32_obj_id);