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

[Steve Lhomme]

On Sun, Jun 5, 2016 at 2:41 PM, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 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.

Yes, that might work.

>>      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?

The only way the assert will fail is if the lock is actually locked by
the same thread. If it doesn't assert, it may still produce a deadlock
that can still be debugged.

>> +        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/
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel