Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 24 Sep 2010 17:11:02 -0400
From:      Jung-uk Kim <jkim@FreeBSD.org>
To:        John Baldwin <jhb@freebsd.org>
Cc:        Daniel Eischen <deischen@freebsd.org>, freebsd-threads@freebsd.org
Subject:   Re: threads/150889: PTHREAD_MUTEX_INITIALIZER +	pthread_mutex_destroy () == EINVAL
Message-ID:  <201009241711.16129.jkim@FreeBSD.org>
In-Reply-To: <201009241137.56764.jkim@FreeBSD.org>
References:  <201009232220.o8NMK3fX011639@freefall.freebsd.org> <201009240926.12958.jhb@freebsd.org> <201009241137.56764.jkim@FreeBSD.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--Boundary-00=_0PRnME9TC8tgtzT
Content-Type: text/plain;
  charset="iso-8859-1"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

On Friday 24 September 2010 11:37 am, Jung-uk Kim wrote:
> On Friday 24 September 2010 09:26 am, John Baldwin wrote:
> > On Thursday, September 23, 2010 11:48:40 pm Jung-uk Kim wrote:
> > > On Thursday 23 September 2010 06:44 pm, Daniel Eischen wrote:
> > > > You shouldn't have to call pthread_mutex_init() on a mutex
> > > > initialized with PTHREAD_MUTEX_INITIALIZER.  Our
> > > > implementation should auto initialize the mutex when it is
> > > > first used; if it doesn't, I think that is a bug.
> > >
> > > Ah, I see.  I verified that libthr does it correctly.  However,
> > > that's a hack and it is far from real static allocation
> > > although it should work pretty well in reality, IMHO.  More
> > > over, it will have a side-effect, i.e., any destroyed mutex may
> > > be resurrected if it is used again.  POSIX seems to say it
> > > should return EINVAL when it happens. :-(
> >
> > I think the fix there is that we should put a different value
> > ((void *)1 for example) into "destroyed" mutex objects than 0 so
> > that destroyed mutexes can be differentiated from statically
> > initialized mutexes.  This would also allow us to properly return
> > EBUSY, etc.
>
> It would be nice if we had "uninitialized" as (void *)0 and "static
> initializer" as (void *)1.  IMHO, it looks more natural, i.e.,
> "uninitialized" or "destroyed" one gets zero, and "dynamically
> initialized" or "statically initialized" one gets non-zero.  Can we
> break the ABI for 9, maybe? ;-)

--------------------------------
WARNING!!! WARNING!!! WARNING!!!
--------------------------------
This patch is a proof of concept!
This patch massively breaks pthread ABI!
This patch may crash and burn your system!

I couldn't resist trying. ;-)

Jung-uk Kim

--Boundary-00=_0PRnME9TC8tgtzT
Content-Type: text/plain;
  charset="iso-8859-1";
  name="pthread.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
	filename="pthread.diff"

--- include/pthread.h	2009-03-14 16:10:14.000000000 -0400
+++ include/pthread.h	2010-09-24 16:34:45.000000000 -0400
@@ -97,10 +97,10 @@
 /*
  * Static initialization values. 
  */
-#define PTHREAD_MUTEX_INITIALIZER	NULL
-#define PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP	NULL
-#define PTHREAD_COND_INITIALIZER	NULL
-#define PTHREAD_RWLOCK_INITIALIZER	NULL
+#define	PTHREAD_MUTEX_INITIALIZER	((void *)1)
+#define	PTHREAD_ADAPTIVE_MUTEX_INITIALIZER_NP	((void *)1)
+#define	PTHREAD_COND_INITIALIZER	((void *)1)
+#define	PTHREAD_RWLOCK_INITIALIZER	((void *)1)
 
 /*
  * Default attribute arguments (draft 4, deprecated).
--- lib/libthr/thread/thr_cond.c	2010-09-01 12:13:31.000000000 -0400
+++ lib/libthr/thread/thr_cond.c	2010-09-24 15:36:56.000000000 -0400
@@ -94,7 +94,7 @@ init_static(struct pthread *thread, pthr
 
 	THR_LOCK_ACQUIRE(thread, &_cond_static_lock);
 
-	if (*cond == NULL)
+	if (*cond == STATIC_INITIALIZER)
 		ret = cond_init(cond, NULL);
 	else
 		ret = 0;
@@ -121,6 +121,8 @@ _pthread_cond_destroy(pthread_cond_t *co
 
 	if (*cond == NULL)
 		rval = EINVAL;
+	else if (*cond == STATIC_INITIALIZER)
+		*cond = NULL;
 	else {
 		cv = *cond;
 		THR_UMUTEX_LOCK(curthread, &cv->c_lock);
@@ -184,7 +186,7 @@ cond_wait_common(pthread_cond_t *cond, p
 	 * If the condition variable is statically initialized,
 	 * perform the dynamic initialization:
 	 */
-	if (__predict_false(*cond == NULL &&
+	if (__predict_false(*cond == STATIC_INITIALIZER &&
 	    (ret = init_static(curthread, cond)) != 0))
 		return (ret);
 
@@ -273,7 +275,7 @@ cond_signal_common(pthread_cond_t *cond,
 	 * If the condition variable is statically initialized, perform dynamic
 	 * initialization.
 	 */
-	if (__predict_false(*cond == NULL &&
+	if (__predict_false(*cond == STATIC_INITIALIZER &&
 	    (ret = init_static(curthread, cond)) != 0))
 		return (ret);
 
--- lib/libthr/thread/thr_mutex.c	2010-09-01 12:13:32.000000000 -0400
+++ lib/libthr/thread/thr_mutex.c	2010-09-24 16:32:07.000000000 -0400
@@ -184,7 +184,7 @@ init_static(struct pthread *thread, pthr
 
 	THR_LOCK_ACQUIRE(thread, &_mutex_static_lock);
 
-	if (*mutex == NULL)
+	if (*mutex == STATIC_INITIALIZER)
 		ret = mutex_init(mutex, NULL, calloc);
 	else
 		ret = 0;
@@ -263,6 +263,8 @@ _pthread_mutex_destroy(pthread_mutex_t *
 
 	if (__predict_false(*mutex == NULL))
 		ret = EINVAL;
+	else if (*mutex == STATIC_INITIALIZER)
+		*mutex = NULL;
 	else {
 		id = TID(curthread);
 
@@ -346,7 +348,7 @@ __pthread_mutex_trylock(pthread_mutex_t 
 	 * If the mutex is statically initialized, perform the dynamic
 	 * initialization:
 	 */
-	if (__predict_false(*mutex == NULL)) {
+	if (__predict_false(*mutex == STATIC_INITIALIZER)) {
 		ret = init_static(curthread, mutex);
 		if (__predict_false(ret))
 			return (ret);
@@ -454,7 +456,7 @@ __pthread_mutex_lock(pthread_mutex_t *mu
 	 * If the mutex is statically initialized, perform the dynamic
 	 * initialization:
 	 */
-	if (__predict_false((m = *mutex) == NULL)) {
+	if (__predict_false((m = *mutex) == STATIC_INITIALIZER)) {
 		ret = init_static(curthread, mutex);
 		if (__predict_false(ret))
 			return (ret);
@@ -479,7 +481,7 @@ __pthread_mutex_timedlock(pthread_mutex_
 	 * If the mutex is statically initialized, perform the dynamic
 	 * initialization:
 	 */
-	if (__predict_false((m = *mutex) == NULL)) {
+	if (__predict_false((m = *mutex) == STATIC_INITIALIZER)) {
 		ret = init_static(curthread, mutex);
 		if (__predict_false(ret))
 			return (ret);
@@ -614,6 +616,9 @@ mutex_unlock_common(pthread_mutex_t *mut
 	if (__predict_false((m = *mutex) == NULL))
 		return (EINVAL);
 
+	if (*mutex == STATIC_INITIALIZER)
+		return (EPERM);
+
 	/*
 	 * Check if the running thread is not the owner of the mutex.
 	 */
@@ -749,7 +754,7 @@ __pthread_mutex_setspinloops_np(pthread_
 	struct pthread *curthread = _get_curthread();
 	int ret;
 
-	if (__predict_false(*mutex == NULL)) {
+	if (__predict_false(*mutex == STATIC_INITIALIZER)) {
 		ret = init_static(curthread, mutex);
 		if (__predict_false(ret))
 			return (ret);
@@ -773,7 +778,7 @@ __pthread_mutex_setyieldloops_np(pthread
 	struct pthread *curthread = _get_curthread();
 	int ret;
 
-	if (__predict_false(*mutex == NULL)) {
+	if (__predict_false(*mutex == STATIC_INITIALIZER)) {
 		ret = init_static(curthread, mutex);
 		if (__predict_false(ret))
 			return (ret);
@@ -788,7 +793,7 @@ _pthread_mutex_isowned_np(pthread_mutex_
 	struct pthread *curthread = _get_curthread();
 	int ret;
 
-	if (__predict_false(*mutex == NULL)) {
+	if (__predict_false(*mutex == STATIC_INITIALIZER)) {
 		ret = init_static(curthread, mutex);
 		if (__predict_false(ret))
 			return (ret);
--- lib/libthr/thread/thr_private.h	2010-09-24 12:01:02.000000000 -0400
+++ lib/libthr/thread/thr_private.h	2010-09-24 14:43:35.000000000 -0400
@@ -125,6 +125,8 @@ TAILQ_HEAD(mutex_queue, pthread_mutex);
 		}						\
 	} while (0)
 
+#define	STATIC_INITIALIZER	((void *)1)
+
 struct pthread_mutex {
 	/*
 	 * Lock for accesses to this structure.
--- lib/libthr/thread/thr_rwlock.c	2009-07-06 05:31:04.000000000 -0400
+++ lib/libthr/thread/thr_rwlock.c	2010-09-24 16:45:37.000000000 -0400
@@ -64,10 +64,12 @@ rwlock_init(pthread_rwlock_t *rwlock, co
 int
 _pthread_rwlock_destroy (pthread_rwlock_t *rwlock)
 {
-	int ret;
+	int ret = 0;
 
 	if (rwlock == NULL)
 		ret = EINVAL;
+	else if (*rwlock == STATIC_INITIALIZER)
+		*rwlock = NULL;
 	else {
 		pthread_rwlock_t prwlock;
 
@@ -75,7 +77,6 @@ _pthread_rwlock_destroy (pthread_rwlock_
 		*rwlock = NULL;
 
 		free(prwlock);
-		ret = 0;
 	}
 	return (ret);
 }
@@ -87,7 +88,7 @@ init_static(struct pthread *thread, pthr
 
 	THR_LOCK_ACQUIRE(thread, &_rwlock_static_lock);
 
-	if (*rwlock == NULL)
+	if (*rwlock == STATIC_INITIALIZER)
 		ret = rwlock_init(rwlock, NULL);
 	else
 		ret = 0;
@@ -119,7 +120,7 @@ rwlock_rdlock_common(pthread_rwlock_t *r
 	prwlock = *rwlock;
 
 	/* check for static initialization */
-	if (__predict_false(prwlock == NULL)) {
+	if (__predict_false(prwlock == STATIC_INITIALIZER)) {
 		if ((ret = init_static(curthread, rwlock)) != 0)
 			return (ret);
 
@@ -212,7 +213,7 @@ _pthread_rwlock_tryrdlock (pthread_rwloc
 	prwlock = *rwlock;
 
 	/* check for static initialization */
-	if (__predict_false(prwlock == NULL)) {
+	if (__predict_false(prwlock == STATIC_INITIALIZER)) {
 		if ((ret = init_static(curthread, rwlock)) != 0)
 			return (ret);
 
@@ -256,7 +257,7 @@ _pthread_rwlock_trywrlock (pthread_rwloc
 	prwlock = *rwlock;
 
 	/* check for static initialization */
-	if (__predict_false(prwlock == NULL)) {
+	if (__predict_false(prwlock == STATIC_INITIALIZER)) {
 		if ((ret = init_static(curthread, rwlock)) != 0)
 			return (ret);
 
@@ -283,7 +284,7 @@ rwlock_wrlock_common (pthread_rwlock_t *
 	prwlock = *rwlock;
 
 	/* check for static initialization */
-	if (__predict_false(prwlock == NULL)) {
+	if (__predict_false(prwlock == STATIC_INITIALIZER)) {
 		if ((ret = init_static(curthread, rwlock)) != 0)
 			return (ret);
 
@@ -364,6 +365,9 @@ _pthread_rwlock_unlock (pthread_rwlock_t
 	if (__predict_false(prwlock == NULL))
 		return (EINVAL);
 
+	if (prwlock == STATIC_INITIALIZER)
+		return (EPERM);
+
 	state = prwlock->lock.rw_state;
 	if (state & URWLOCK_WRITE_OWNER) {
 		if (__predict_false(prwlock->owner != curthread))

--Boundary-00=_0PRnME9TC8tgtzT--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201009241711.16129.jkim>