[vlc-devel] [PATCH] win32: in debug builds make sure the non-recursive locks are not reentrant

[Rémi Denis-Courmont]

Le 2016-06-05 14:35, Steve Lhomme a écrit :
> Use a semaphore with a single token for non recursive locks
> Use the same code as non-debug builds with recursive locks
>
> --
> hopefully fixes the make check header issue in
> https://patches.videolan.org/patch/13618/
> ---
>  include/vlc_threads.h |  4 ++++
>  src/win32/thread.c    | 49 
> +++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>
> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
> index 33b9b53..08b10f2 100644
> --- a/include/vlc_threads.h
> +++ b/include/vlc_threads.h
> @@ -66,6 +66,10 @@ typedef struct
>              unsigned long contention;
>          };
>          CRITICAL_SECTION mutex;
> +
> +        /* non-recursive semaphore */
> +        HANDLE hMutex;
> +        unsigned long lock_thread_id;
>      };
>  } vlc_mutex_t;
>  #define VLC_STATIC_MUTEX { false, { { false, 0 } } }
> diff --git a/src/win32/thread.c b/src/win32/thread.c
> index 963a8d7..0fab5ad 100644
> --- a/src/win32/thread.c
> +++ b/src/win32/thread.c
> @@ -43,6 +43,12 @@
>  static vlc_mutex_t super_mutex;
>  static vlc_cond_t  super_variable;
>
> +#ifndef NDEBUG
> +#define MUTEX_ALWAYS_RECURSIVE 0
> +#else
> +#define MUTEX_ALWAYS_RECURSIVE 1
> +#endif
> +
>  #define IS_INTERRUPTIBLE (!VLC_WINSTORE_APP || _WIN32_WINNT >= 
> 0x0A00)
>
>  /*** Threads ***/
> @@ -71,13 +77,20 @@ void vlc_mutex_init( vlc_mutex_t *p_mutex )
>  {
>      /* This creates a recursive mutex. This is OK as fast mutexes 
> have
>       * no defined behavior in case of recursive locking. */
> +#if MUTEX_ALWAYS_RECURSIVE
>      InitializeCriticalSection (&p_mutex->mutex);
> +#else
> +    p_mutex->hMutex = CreateSemaphore( NULL, 1, 1, NULL );
> +#endif

Isn't a mutex a lot slower than a critical section, as it's a kernel 
handle? If you want to rule out recursive locking, you just need a 
boolean on top of the critical section, AFAICT.

>      p_mutex->dynamic = true;
>  }
>
>  void vlc_mutex_init_recursive( vlc_mutex_t *p_mutex )
>  {
>      InitializeCriticalSection( &p_mutex->mutex );
> +#if !MUTEX_ALWAYS_RECURSIVE
> +    p_mutex->hMutex = INVALID_HANDLE_VALUE;
> +#endif
>      p_mutex->dynamic = true;
>  }
>
> @@ -85,6 +98,11 @@ void vlc_mutex_init_recursive( vlc_mutex_t 
> *p_mutex )
>  void vlc_mutex_destroy (vlc_mutex_t *p_mutex)
>  {
>      assert (p_mutex->dynamic);
> +#if !MUTEX_ALWAYS_RECURSIVE
> +    if ( p_mutex->hMutex != INVALID_HANDLE_VALUE )
> +        CloseHandle( p_mutex->hMutex );
> +    else
> +#endif
>      DeleteCriticalSection (&p_mutex->mutex);
>  }
>
> @@ -108,6 +126,15 @@ void vlc_mutex_lock (vlc_mutex_t *p_mutex)
>          return;
>      }
>
> +#if !MUTEX_ALWAYS_RECURSIVE
> +    if ( p_mutex->hMutex != INVALID_HANDLE_VALUE )
> +    {
> +        assert(p_mutex->lock_thread_id != vlc_thread_id());

What prevents another thread from writing lock_thread_id concurrently?

> +        WaitForSingleObject( p_mutex->hMutex, INFINITE );
> +        p_mutex->lock_thread_id = vlc_thread_id();
> +    }
> +    else
> +#endif
>      EnterCriticalSection (&p_mutex->mutex);
>  }
>
> @@ -128,6 +155,20 @@ int vlc_mutex_trylock (vlc_mutex_t *p_mutex)
>          return ret;
>      }
>
> +#if !MUTEX_ALWAYS_RECURSIVE
> +    if ( p_mutex->hMutex != INVALID_HANDLE_VALUE )
> +    {
> +        assert(p_mutex->lock_thread_id != vlc_thread_id());
> +        if ( WaitForSingleObject( p_mutex->hMutex, 0 ) == 
> WAIT_OBJECT_0 )
> +        {
> +            p_mutex->lock_thread_id = vlc_thread_id();
> +            return 0;
> +        }
> +        else
> +            return EBUSY;
> +    }
> +    else
> +#endif
>      return TryEnterCriticalSection (&p_mutex->mutex) ? 0 : EBUSY;
>  }
>
> @@ -146,6 +187,14 @@ void vlc_mutex_unlock (vlc_mutex_t *p_mutex)
>          return;
>      }
>
> +#if !MUTEX_ALWAYS_RECURSIVE
> +    if ( p_mutex->hMutex != INVALID_HANDLE_VALUE )
> +    {
> +        p_mutex->lock_thread_id = 0;
> +        ReleaseSemaphore( p_mutex->hMutex, 1, NULL );
> +    }
> +    else
> +#endif
>      LeaveCriticalSection (&p_mutex->mutex);
>  }

-- 
Rémi Denis-Courmont
http://www.remlab.net/