[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:55, Steve Lhomme a écrit :
> 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.

The assert is undefined if another thread writes the value, either to 
lock or unlock the mutex. And I don't see how you ensure that is not 
happening 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/
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel

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