Re: [PATCH/RFC] mingw: implement PTHREAD_MUTEX_INITIALIZER

From: Atsushi Nakagawa <atnak@chejz.com>
Date: Oct 26, 2011, 03:05 UTC
Message-ID: <16520370.401.1319598319120.JavaMail.geo-discussion-forums@prms22>
In-Reply-To: <1319554509-6532-1-git-send-email-kusmabite@gmail.com>

On Oct 25, 11:55 pm, Erik Faye-Lund <kusmab...@gmail.com> wrote:

> [...]
> +int pthread_mutex_init(pthread_mutex_t *mutex, const pthread_mutexattr_t

*attr)

Show 10 quoted lines

> +{
> +       InitializeCriticalSection(&mutex->cs);
> +       mutex->autoinit = 0;
> +       return 0;
> +}
> +
> +int pthread_mutex_lock(pthread_mutex_t *mutex)
> +{
> +       if (mutex->autoinit) {
> +               if (InterlockedCompareExchange(&mutex->autoinit, -1, 1)

!= -1) {

I'm making the assumption that mutex->autoinit starts off as 1 before things get multi-threaded..

I've only looked at what's in the patch so I could be missing vital context.. Anyways, is there a reason why you made this "InterlockedCompareExchange(..., -1, 1) != -1" and not "InterlockedCompareExchange(..., -1, 1) == 1"?

It looks to me the former adds a race condition after "if (mutex->autoinit) {". e.g. A second thread could reinitialize mutex->cs after the first thread has already entered EnterCriticalSection(...).

Show 11 quoted lines

> +                       pthread_mutex_init(mutex, NULL);
> +                       mutex->autoinit = 0;
> +               } else
> +                       while (mutex->autoinit != 0)
> +                               ; /* wait for other thread */
> +       }
> +
> +       EnterCriticalSection(&mutex->cs);
> +       return 0;
> +}
> [...]

-- 
Atsushi Nakagawa