diff options
| -rw-r--r-- | ChangeLog | 124 | ||||
| -rw-r--r-- | create.c | 48 | ||||
| -rw-r--r-- | implement.h | 15 | ||||
| -rw-r--r-- | pthread.h | 26 | ||||
| -rw-r--r-- | pthread_barrier_wait.c | 20 | ||||
| -rw-r--r-- | pthread_join.c | 163 | ||||
| -rw-r--r-- | pthread_once.c | 12 | ||||
| -rw-r--r-- | tests/ChangeLog | 4 | ||||
| -rw-r--r-- | tests/GNUmakefile | 3 | ||||
| -rw-r--r-- | tests/Makefile | 3 |
10 files changed, 267 insertions, 151 deletions
@@ -1,59 +1,103 @@ -2004-07-01 Anuj Goyal <anuj.goyal at gmail.com> +2004-09-09 Tristan Savatier <tristan at mpegtv.com>
+ * pthread.h (struct pthread_once_t_): Qualify the 'done' element + as 'volatile'. + * pthread_once.c: Concerned about possible race condition, + specifically on MPU systems re concurrent access to multibyte types. + [Maintainer's note: the race condition is harmless on SPU systems + and only a problem on MPU systems if concurrent access results in an + exception (presumably generated by a hardware interrupt). There are + other instances of similar harmless race conditions that have not been + identified as issues.] - * builddmc.bat: New; Windows bat file to build the library. - * config.h (__DMC__): Support for Digital Mars compiler. - * create.c (__DMC__): Likewise. - * pthread_exit.c (__DMC__): Likewise. - * pthread_join.c (__DMC__): Likewise. - * ptw32_threadDestroy.c (__DMC__): Likewise. - * ptw32_threadStart.c (__DMC__): Likewise. - * ptw32_throw.c (__DMC__): Likewise. +2004-09-09 Ross Johnson <rpj at callisto.canberra.edu.au>
-2004-06-29 Anuj Goyal <anuj.goyal at gmail.com> + * pthread.h: Declare additional types as volatile. - * pthread.h (__DMC__): Initial support for Digital Mars compiler. +2004-08-27 Ross Johnson <rpj at callisto.canberra.edu.au>
-2004-06-29 Will Bryant <will.bryant at ecosm.com> + * pthread_barrier_wait.c (pthread_barrier_wait): Remove excessive code + by substituting the internal non-cancelable version of sem_wait + (ptw32_semwait). - * README.Borland: New; description of Borland changes. - * Bmakefile: New makefile for the Borland make utility. - * ptw32_InterlockedCompareExchange.c: - Add Borland compatible asm code. +2004-08-25 Ross Johnson <rpj at callisto.canberra.edu.au>
-2004-06-26 Jason Bard <BardJA at Npt.NUWC.Navy.Mil> + * pthread_join.c (pthread_join): Rewrite and re-order the conditional
+ tests in an attempt to improve efficiency and remove a race
+ condition.
- * pthread.h (HAVE_STRUCT_TIMESPEC): If undefined, define it - to avoid timespec struct redefined errors elsewhere in an - application. +2004-08-23 Ross Johnson <rpj at callisto.canberra.edu.au>
-2004-06-21 Ross Johnson <rpj at callisto.canberra.edu.au> + * create.c (pthread_create): Don't create a thread if the thread
+ id pointer location (first arg) is inaccessible. A memory
+ protection fault will result if the thread id arg isn't an accessible
+ location. This is consistent with GNU/Linux but different to
+ Solaris or MKS (and possibly others), which accept NULL as meaning
+ 'don't return the created thread's ID'. Applications that run
+ using pthreads-win32 will run on all other POSIX threads
+ implementations, at least w.r.t. this feature.
- * pthread.h (PTHREAD_RECURSIVE_MUTEX_INITIALIZER): Mutex - initialiser added for compatibility with Linux threads and - others; currently not included in SUSV3. - * pthread.h (PTHREAD_ERRORCHECK_MUTEX_INITIALIZER): Likewise. - * pthread.h (PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP): Likewise. - * pthread.h (PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP): Likewise. + It was decided not to copy the Solaris et al behaviour because,
+ although it would have simplified some application porting (but only
+ from Solaris to Windows), the feature is not technically necessary,
+ and the alternative segfault behaviour helps avoid buggy application
+ code.
- * ptw32_mutex_check_need_init.c (ptw32_mutex_check_need_init): - Add new initialisers. +2004-07-01 Anuj Goyal <anuj.goyal at gmail.com>
- * pthread_mutex_lock.c (pthread_mutex_lock): Check for new - initialisers. - * pthread_mutex_trylock.c (pthread_mutex_trylock): Likewise. - * pthread_mutex_timedlock.c (pthread_mutex_timedlock): Likewise. - * pthread_mutex_unlock.c (pthread_mutex_unlock): Likewise. - * pthread_mutex_destroy.c (pthread_mutex_destroy): Likewise. + * builddmc.bat: New; Windows bat file to build the library.
+ * config.h (__DMC__): Support for Digital Mars compiler.
+ * create.c (__DMC__): Likewise.
+ * pthread_exit.c (__DMC__): Likewise.
+ * pthread_join.c (__DMC__): Likewise.
+ * ptw32_threadDestroy.c (__DMC__): Likewise.
+ * ptw32_threadStart.c (__DMC__): Likewise.
+ * ptw32_throw.c (__DMC__): Likewise.
-2004-05-20 Ross Johnson <rpj at callisto.canberra.edu.au> +2004-06-29 Anuj Goyal <anuj.goyal at gmail.com>
- * README.NONPORTABLE: Document pthread_win32_test_features_np(). - * FAQ: Update various answers. + * pthread.h (__DMC__): Initial support for Digital Mars compiler.
+ +2004-06-29 Will Bryant <will.bryant at ecosm.com>
+ + * README.Borland: New; description of Borland changes.
+ * Bmakefile: New makefile for the Borland make utility.
+ * ptw32_InterlockedCompareExchange.c:
+ Add Borland compatible asm code.
+ +2004-06-26 Jason Bard <BardJA at Npt.NUWC.Navy.Mil>
+ + * pthread.h (HAVE_STRUCT_TIMESPEC): If undefined, define it
+ to avoid timespec struct redefined errors elsewhere in an
+ application.
+ +2004-06-21 Ross Johnson <rpj at callisto.canberra.edu.au>
+ + * pthread.h (PTHREAD_RECURSIVE_MUTEX_INITIALIZER): Mutex
+ initialiser added for compatibility with Linux threads and
+ others; currently not included in SUSV3.
+ * pthread.h (PTHREAD_ERRORCHECK_MUTEX_INITIALIZER): Likewise.
+ * pthread.h (PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP): Likewise.
+ * pthread.h (PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP): Likewise.
+ + * ptw32_mutex_check_need_init.c (ptw32_mutex_check_need_init):
+ Add new initialisers.
+ + * pthread_mutex_lock.c (pthread_mutex_lock): Check for new
+ initialisers.
+ * pthread_mutex_trylock.c (pthread_mutex_trylock): Likewise.
+ * pthread_mutex_timedlock.c (pthread_mutex_timedlock): Likewise.
+ * pthread_mutex_unlock.c (pthread_mutex_unlock): Likewise.
+ * pthread_mutex_destroy.c (pthread_mutex_destroy): Likewise.
+ +2004-05-20 Ross Johnson <rpj at callisto.canberra.edu.au>
+ + * README.NONPORTABLE: Document pthread_win32_test_features_np().
+ * FAQ: Update various answers.
2004-05-19 Ross Johnson <rpj at callisto.canberra.edu.au>
- * Makefile: Don't define _WIN32_WINNT on compiler command line. - * GNUmakefile: Likewise. + * Makefile: Don't define _WIN32_WINNT on compiler command line.
+ * GNUmakefile: Likewise.
2004-05-16 Ross Johnson <rpj at callisto.canberra.edu.au>
@@ -49,7 +49,10 @@ pthread_create (pthread_t * tid, * ------------------------------------------------------ * DOCPUBLIC * This function creates a thread running the start function, - * passing it the parameter value, 'arg'. + * passing it the parameter value, 'arg'. The 'attr' + * argument specifies optional creation attributes. + * The identity of the new thread is returned + * via 'tid', which should not be NULL. * * PARAMETERS * tid @@ -69,8 +72,8 @@ pthread_create (pthread_t * tid, * This function creates a thread running the start function, * passing it the parameter value, 'arg'. The 'attr' * argument specifies optional creation attributes. - * The thread is identity of the new thread is returned - * as 'tid' + * The identity of the new thread is returned + * via 'tid', which should not be the NULL pointer. * * RESULTS * 0 successfully created thread, @@ -81,6 +84,7 @@ pthread_create (pthread_t * tid, */ { pthread_t thread; + register pthread_attr_t a; HANDLE threadH = 0; int result = EAGAIN; int run = PTW32_TRUE; @@ -88,6 +92,23 @@ pthread_create (pthread_t * tid, long stackSize; int priority; + /* + * Before doing anything, check that tid can be stored through + * without invoking a memory protection error (segfault). + * Make sure that the assignment below can't be optimised out by the compiler. + * This is assured by conditionally assigning *tid again at the end. + */ + *tid = NULL; + + if (attr != NULL) + { + a = *attr; + } + else + { + a = NULL; + } + if ((thread = ptw32_new ()) == NULL) { goto FAIL0; @@ -104,15 +125,15 @@ pthread_create (pthread_t * tid, parms->start = start; parms->arg = arg; - if (attr != NULL && *attr != NULL) + if (a != NULL) { - stackSize = (*attr)->stacksize; - thread->detachState = (*attr)->detachstate; - priority = (*attr)->param.sched_priority; + stackSize = a->stacksize; + thread->detachState = a->detachstate; + priority = a->param.sched_priority; #if HAVE_SIGSET_T - thread->sigmask = (*attr)->sigmask; + thread->sigmask = a->sigmask; #endif /* HAVE_SIGSET_T */ @@ -133,7 +154,7 @@ pthread_create (pthread_t * tid, * an 'attr' arg to pthread_create() is equivalent to defaulting to * PTHREAD_EXPLICIT_SCHED and priority THREAD_PRIORITY_NORMAL. */ - if (PTHREAD_INHERIT_SCHED == (*attr)->inheritsched) + if (PTHREAD_INHERIT_SCHED == a->inheritsched) { /* * If the thread that called pthread_create() is a Win32 thread @@ -181,7 +202,7 @@ pthread_create (pthread_t * tid, if (threadH != 0) { - if (attr != NULL && *attr != NULL) + if (a != NULL) { (void) ptw32_setthreadpriority (thread, SCHED_OTHER, priority); } @@ -222,7 +243,7 @@ pthread_create (pthread_t * tid, SuspendThread (threadH); } - if (attr != NULL && *attr != NULL) + if (a != NULL) { (void) ptw32_setthreadpriority (thread, SCHED_OTHER, priority); } @@ -256,7 +277,10 @@ FAIL0: free (parms); } } - *tid = thread; + else + { + *tid = thread; + } #ifdef _UWIN if (result == 0) diff --git a/implement.h b/implement.h index 4ce91cd..95308f5 100644 --- a/implement.h +++ b/implement.h @@ -107,20 +107,6 @@ typedef enum PThreadState; -typedef enum -{ - /* - * This enumeration represents the reason why a thread has - * terminated/is terminating. - */ - PThreadDemisePeaceful = 0, /* Death due natural causes */ - PThreadDemiseCancelled, /* Death due to user cancel */ - PThreadDemiseException, /* Death due to unhandled */ - /* exception */ - PThreadDemiseNotDead /* I'm not dead! */ -} -PThreadDemise; - struct pthread_t_ { #ifdef _UWIN @@ -130,7 +116,6 @@ struct pthread_t_ HANDLE threadH; /* POSIX thread is invalid if threadH == 0 */ pthread_t prevReuse; /* Links threads on reuse stack */ PThreadState state; - PThreadDemise demise; void *exitStatus; void *parms; int ptErrno; @@ -519,20 +519,20 @@ extern "C" #if defined(_UWIN) && PTW32_LEVEL >= PTW32_LEVEL_MAX # include <sys/types.h> #else -typedef struct pthread_t_ *pthread_t; -typedef struct pthread_attr_t_ *pthread_attr_t; +typedef struct pthread_t_ * volatile pthread_t; +typedef struct pthread_attr_t_ * volatile pthread_attr_t; typedef struct pthread_once_t_ pthread_once_t; -typedef struct pthread_key_t_ *pthread_key_t; -typedef struct pthread_mutex_t_ *pthread_mutex_t; -typedef struct pthread_mutexattr_t_ *pthread_mutexattr_t; -typedef struct pthread_cond_t_ *pthread_cond_t; -typedef struct pthread_condattr_t_ *pthread_condattr_t; +typedef struct pthread_key_t_ * volatile pthread_key_t; +typedef struct pthread_mutex_t_ * volatile pthread_mutex_t; +typedef struct pthread_mutexattr_t_ * volatile pthread_mutexattr_t; +typedef struct pthread_cond_t_ * volatile pthread_cond_t; +typedef struct pthread_condattr_t_ * volatile pthread_condattr_t; #endif -typedef struct pthread_rwlock_t_ *pthread_rwlock_t; -typedef struct pthread_rwlockattr_t_ *pthread_rwlockattr_t; -typedef struct pthread_spinlock_t_ *pthread_spinlock_t; -typedef struct pthread_barrier_t_ *pthread_barrier_t; -typedef struct pthread_barrierattr_t_ *pthread_barrierattr_t; +typedef struct pthread_rwlock_t_ * volatile pthread_rwlock_t; +typedef struct pthread_rwlockattr_t_ * volatile pthread_rwlockattr_t; +typedef struct pthread_spinlock_t_ * volatile pthread_spinlock_t; +typedef struct pthread_barrier_t_ * volatile pthread_barrier_t; +typedef struct pthread_barrierattr_t_ * volatile pthread_barrierattr_t; /* * ==================== @@ -607,7 +607,7 @@ enum { struct pthread_once_t_ { - int done; /* indicates if user function executed */ + volatile int done; /* indicates if user function executed */ long started; /* First thread to increment this value */ /* to zero executes the user function */ }; diff --git a/pthread_barrier_wait.c b/pthread_barrier_wait.c index b067d66..200a02d 100644 --- a/pthread_barrier_wait.c +++ b/pthread_barrier_wait.c @@ -73,26 +73,10 @@ pthread_barrier_wait (pthread_barrier_t * barrier) } else { - BOOL switchCancelState; - int oldCancelState; - pthread_t self = pthread_self (); - /* - * This routine is not a cancelation point, so temporarily - * prevent sem_wait() from being one. - * PTHREAD_CANCEL_ASYNCHRONOUS threads can still be canceled. + * Use the non-cancelable version of sem_wait(). */ - switchCancelState = (self->cancelType == PTHREAD_CANCEL_DEFERRED && - 0 == - pthread_setcancelstate (PTHREAD_CANCEL_DISABLE, - &oldCancelState)); - - result = sem_wait (&(b->semBarrierBreeched[step])); - - if (switchCancelState) - { - (void) pthread_setcancelstate (oldCancelState, NULL); - } + result = ptw32_semwait (&(b->semBarrierBreeched[step])); } /* diff --git a/pthread_join.c b/pthread_join.c index 97cbd51..89ee1a5 100644 --- a/pthread_join.c +++ b/pthread_join.c @@ -83,22 +83,62 @@ pthread_join (pthread_t thread, void **value_ptr) int result; pthread_t self; - /* This is the proper way to test for a valid thread ID */ - result = pthread_kill (thread, 0); - if (0 != result) + /* + * Possibilities for the target thread on entry to pthread_join(): + * + * 1) the target thread is detached, in which case it could be destroyed and + * it's thread id (struct) reused before we get the reuse lock; + * 2) the target thread is joinable, in which case it will not be destroyed + * until it has been joined, and it's thread id cannot be reused before we + * get access to it. + * + * Only (1) is a potential race condition. + * While (1) is possibly an application programming error, pthread_join is + * required to handle it with an error. + * If an application attempts to join a detached thread that exits before we + * get the reuse lock, it's thread struct could be reused for a new joinable + * thread before we get the reuse_lock and we will then be joining the wrong + * thread. + * + * To fix this properly would require a reuse count as part of the thread id + * so that we could confirm that we are working with the same thread. This + * option will require a major change to the API, forcing recompilation of + * applications that use the library. + * + * Cheaper alternatives that do not change the API are: + * - separate reuse stacks could be used for detached and joinable + * threads. Threads which detach themselves will not present a race condition + * because they will be placed on a different stack to that which provides + * reusable structs to new joinable threads. + * The problem with this solution is that, an application that creates + * joinable threads which then detach themselves can cause the detach reuse + * stack to grow indefinitely. + * + * - maintain a sufficiently large reuse pool and manage it as a FIFO + * queue. This would prevent the immediate reuse of thread structs, but this + * only provides a statistical safeguard, not a deterministic one. + * + * - include an entry point array at the start of each thread struct so that + * each reuse (modulo N) returns a different thread id (pointer to the thread + * struct). Offset 0 from this pointer will contain a validity field and an + * offset value field (subtracted from the pointer to obtain the address of + * thread struct). This option is similar to including a reuse counter with + * the thread id but maintains the thread id as a simple pointer. + * + * As at 27/08/2004, none of the above have been implemented. + */ + + EnterCriticalSection (&ptw32_thread_reuse_lock); + /* + * The first test is the same as pthread_kill(thread, 0), duplicated here + * so that we can use the reuse_lock to ensure the thread isn't destroyed + * and reused before we've finished with the POSIX thread struct. + */ + if (NULL == thread + || NULL == thread->threadH + || THREAD_PRIORITY_ERROR_RETURN == GetThreadPriority (thread->threadH)) { - return result; - } - - self = pthread_self (); - if (NULL == self) - { - return ENOENT; - } - - if (0 != pthread_equal (self, thread)) - { - result = EDEADLK; + result = ESRCH; } else if (PTHREAD_CREATE_DETACHED == thread->detachState) { @@ -106,60 +146,81 @@ pthread_join (pthread_t thread, void **value_ptr) } else { - /* - * Pthread_join is a cancelation point. - * If we are canceled then our target thread must not be - * detached (destroyed). This is guarranteed because - * pthreadCancelableWait will not return if we - * are canceled. + /* + * The target thread is joinable and can't be reused before we join it. */ - result = pthreadCancelableWait (thread->threadH); - if (result == 0) + LeaveCriticalSection (&ptw32_thread_reuse_lock); + + if (NULL == (self = pthread_self ())) + { + result = ENOENT; + } + else if (0 != pthread_equal (self, thread)) + { + result = EDEADLK; + } + else { + /* + * Pthread_join is a cancelation point. + * If we are canceled then our target thread must not be + * detached (destroyed). This is guarranteed because + * pthreadCancelableWait will not return if we + * are canceled. + */ + result = pthreadCancelableWait (thread->threadH); + + if (0 == result) + { #if ! defined (__MINGW32__) || defined (__MSVCRT__) || defined (__DMC__) - if (value_ptr != NULL - && !GetExitCodeThread (thread->threadH, (LPDWORD) value_ptr)) - { - result = ESRCH; - } - else - { + if (value_ptr != NULL + && !GetExitCodeThread (thread->threadH, (LPDWORD) value_ptr)) + { + result = ESRCH; + } + else + { + /* + * The result of making multiple simultaneous calls to + * pthread_join() specifying the same target is undefined. + */ + ptw32_threadDestroy (thread); + } + +#else /* __MINGW32__ && ! __MSVCRT__ */ + + /* + * If using CRTDLL, the thread may have exited, and endthread + * will have closed the handle. + */ + if (value_ptr != NULL) + { + *value_ptr = thread->exitStatus; + } + /* * The result of making multiple simultaneous calls to * pthread_join() specifying the same target is undefined. */ ptw32_threadDestroy (thread); - } -#else /* __MINGW32__ && ! __MSVCRT__ */ +#endif /* __MINGW32__ && ! __MSVCRT__ */ - /* - * If using CRTDLL, the thread may have exited, and endthread - * will have closed the handle. - */ - if (value_ptr != NULL) + } + else { - *value_ptr = thread->exitStatus; + result = ESRCH; } - - /* - * The result of making multiple simultaneous calls to - * pthread_join() specifying the same target is undefined. - */ - ptw32_threadDestroy (thread); - -#endif /* __MINGW32__ && ! __MSVCRT__ */ - - } - else - { - result = ESRCH; } + + goto FAIL0; } + LeaveCriticalSection (&ptw32_thread_reuse_lock); +FAIL0: return (result); } /* pthread_join */ diff --git a/pthread_once.c b/pthread_once.c index d37ee3d..34dacea 100644 --- a/pthread_once.c +++ b/pthread_once.c @@ -86,6 +86,18 @@ pthread_once (pthread_once_t * once_control, void (*init_routine) (void)) result = 0; } + /* + * The race condition involving once_control->done is harmless. + * The problem experienced in MPU environments with multibyte variables + * is also not a problem because the value (initially zero i.e. PTW32_FALSE) + * is only ever tested for non-zero. In the event of a race occuring, the + * worst result is that up to N-1 threads (N = number of CPUs) may enter the + * while loop and yield their run state unnecessarily, and this can only + * ever occur at most once. + * + * The alternatives are to use a condition variable (overkill?), or + * InterlockedCompareExchange() to test/set once_control->done. + */ if (!once_control->done) { if (InterlockedIncrement (&(once_control->started)) == 0) diff --git a/tests/ChangeLog b/tests/ChangeLog index 0a408b8..2d0c570 100644 --- a/tests/ChangeLog +++ b/tests/ChangeLog @@ -6,6 +6,10 @@ * exit4.c (main): Likewise. * exit5.c (main): Likewise. +2004-08-26 Ross Johnson <rpj@callisto.canberra.edu.au>
+ + * create3.c: New test. + 2004-06-21 Ross Johnson <rpj@callisto.canberra.edu.au>
* mutex2r.c: New test.
diff --git a/tests/GNUmakefile b/tests/GNUmakefile index 323f24b..04f9943 100644 --- a/tests/GNUmakefile +++ b/tests/GNUmakefile @@ -94,7 +94,7 @@ TESTS = sizes loadfree \ spin1 spin2 spin3 spin4 \ barrier1 barrier2 barrier3 barrier4 barrier5 \ exception1 exception2 exception3 \ - cancel9 + cancel9 create3 BENCHTESTS = \ benchtest1 benchtest2 benchtest3 benchtest4 benchtest5 @@ -180,6 +180,7 @@ context1.pass: cancel2.pass count1.pass: join1.pass create1.pass: mutex2.pass create2.pass: create1.pass +create3.pass: delay1.pass: cancel2.pass delay2.pass: delay1.pass equal1.pass: create1.pass diff --git a/tests/Makefile b/tests/Makefile index 6aefab7..31979a4 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -110,7 +110,7 @@ PASSES= sizes.pass loadfree.pass \ spin1.pass spin2.pass spin3.pass spin4.pass \
barrier1.pass barrier2.pass barrier3.pass barrier4.pass barrier5.pass \
exception1.pass exception2.pass exception3.pass \
- cancel9
+ cancel9 create3
BENCHRESULTS = \
benchtest1.bench benchtest2.bench benchtest3.bench benchtest4.bench benchtest5.bench
@@ -264,6 +264,7 @@ context1.pass: cancel2.pass count1.pass: join1.pass
create1.pass: mutex2.pass
create2.pass: create1.pass
+create3.pass:
delay1.pass:
delay2.pass: delay1.pass
equal1.pass: create1.pass
|