On Feb 23 13:59, Corinna Vinschen wrote: > On Feb 23 13:14, Corinna Vinschen wrote: > > On Feb 22 22:54, Lasse Collin wrote: > > > It seems that a signal can cause pthread_join to incorrectly return > > > EINVAL. I debugged it only a little but hopefully someone finds this > > > useful: > > > > > > In the file thread.cc, function pthread::join, the call to cygwait may > > > return WAIT_SIGNALED if a signal is sent to the process. The switch > > > statement handling the return value assumes that only WAIT_OBJECT_0 and > > > WAIT_CANCELED are possible. The default section of the switch statement > > > has a comment "should never happen" and it returns EINVAL. It might be > > > that the problem occurs only when SA_RESTART isn't used. > [...] > I looked into the Linux man page for pthread_join(1). It doesn't mention > signals and EINTR at all. Then I looked into the SUSv4 pages(2) and it > only has this to say: > > The pthread_join() function shall not return an error code of [EINTR]. > > Searching further on this I found this(3): > > The wait in pthread_join is not broken by a signal. If a thread > waiting in pthread_join receives a signal that is not masked, if will > execute the signal handler, and then return to waiting in > pthread_join. > > Taking that at face value, the following patch should do the right > thing, doesn't it?
On second thought, this is not the right way to handle this. The WAIT_SIGNALED is returned because we're in the main thread and SA_RESTART is not set, as you assumed above. This leads to the question why this scenario isn't handled directly in cygwait. So what I did now is to apply the below patch to CVS. It adds a flag cw_sig_restart to cygwait, which also restarts in the main thread if SA_RESTART is not set, as it's supposed to be for pthread_join. I uploaded a new developer snapshot to https://cygwin.com/snapshots/ Can you please test if it works as desired? Thanks, Corinna * cygwait.h (enum cw_wait_mask): Add cw_sig_restart. Add comments to explain the meaning of the possible values. * cygwait.cc (is_cw_sig_restart): Define. (is_cw_sig_handle): Check for cw_sig_restart as well. (cygwait): Restart always if cw_sig_restart is set. * thread.cc (pthread::join): Call cygwait with cw_sig_restart flag to avoid having to handle signals at all. Index: cygwait.h =================================================================== RCS file: /cvs/src/src/winsup/cygwin/cygwait.h,v retrieving revision 1.10 diff -u -p -r1.10 cygwait.h --- cygwait.h 9 Apr 2013 01:01:19 -0000 1.10 +++ cygwait.h 23 Feb 2015 13:54:48 -0000 @@ -1,7 +1,7 @@ /* cygwait.h - Copyright 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013 - Red Hat, Inc. + Copyright 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012, 2013, + 2015 Red Hat, Inc. This file is part of Cygwin. @@ -16,11 +16,12 @@ enum cw_wait_mask { - cw_cancel = 0x0001, - cw_cancel_self = 0x0002, - cw_sig = 0x0004, - cw_sig_eintr = 0x0008, - cw_sig_cont = 0x0010 + cw_cancel = 0x0001, /* Cancellation point. Return to caller. */ + cw_cancel_self = 0x0002, /* Cancellation point. Cancel self. */ + cw_sig = 0x0004, /* Handle signals. */ + cw_sig_eintr = 0x0008, /* Caller handles signals. */ + cw_sig_cont = 0x0010, /* Caller handles SIGCONT. */ + cw_sig_restart = 0x0020 /* Restart even if SA_RESTART isn't set. */ }; extern LARGE_INTEGER cw_nowait_storage; Index: cygwait.cc =================================================================== RCS file: /cvs/src/src/winsup/cygwin/cygwait.cc,v retrieving revision 1.12 diff -u -p -r1.12 cygwait.cc --- cygwait.cc 23 Feb 2015 13:32:16 -0000 1.12 +++ cygwait.cc 23 Feb 2015 13:54:48 -0000 @@ -18,8 +18,10 @@ #define is_cw_sig (mask & cw_sig) #define is_cw_sig_eintr (mask & cw_sig_eintr) #define is_cw_sig_cont (mask & cw_sig_cont) +#define is_cw_sig_restart (mask & cw_sig_restart) -#define is_cw_sig_handle (mask & (cw_sig | cw_sig_eintr | cw_sig_cont)) +#define is_cw_sig_handle (mask & (cw_sig | cw_sig_eintr \ + | cw_sig_cont | cw_sig_restart)) LARGE_INTEGER cw_nowait_storage; @@ -88,7 +90,7 @@ cygwait (HANDLE object, PLARGE_INTEGER t continue; if (is_cw_sig_eintr || (is_cw_sig_cont && sig == SIGCONT)) ; - else if (_my_tls.call_signal_handler ()) + else if (_my_tls.call_signal_handler () || is_cw_sig_restart) continue; res = WAIT_SIGNALED; /* caller will deal with signals */ } Index: thread.cc =================================================================== RCS file: /cvs/src/src/winsup/cygwin/thread.cc,v retrieving revision 1.296 diff -u -p -r1.296 thread.cc --- thread.cc 28 Nov 2014 20:46:13 -0000 1.296 +++ thread.cc 23 Feb 2015 13:54:48 -0000 @@ -1,7 +1,7 @@ /* thread.cc: Locking and threading module functions Copyright 1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, - 2009, 2010, 2011, 2012, 2013, 2014 Red Hat, Inc. + 2009, 2010, 2011, 2012, 2013, 2014, 2015 Red Hat, Inc. This file is part of Cygwin. @@ -2399,7 +2399,8 @@ pthread::join (pthread_t *thread, void * (*thread)->attr.joinable = PTHREAD_CREATE_DETACHED; (*thread)->mutex.unlock (); - switch (cygwait ((*thread)->win32_obj_id, cw_infinite, cw_sig | cw_cancel)) + switch (cygwait ((*thread)->win32_obj_id, cw_infinite, + cw_sig | cw_sig_restart | cw_cancel)) { case WAIT_OBJECT_0: if (return_val) -- Corinna Vinschen Please, send mails regarding Cygwin to Cygwin Maintainer cygwin AT cygwin DOT com Red Hat
pgp5Z1mg2OXOO.pgp
Description: PGP signature
