[vlc-devel] [PATCH 3/5] threads: VLC_CANCEL_ADDR_SET/CLEAR always use an atomic_int internally
Steve Lhomme
robux4 at ycbcr.xyz
Mon Feb 10 09:36:37 CET 2020
On 2020-02-07 16:38, Rémi Denis-Courmont wrote:
> Hi,
>
> Where do you get that from?
From
https://code.videolan.org/videolan/vlc/blob/master/src/win32/thread.c#L68
It's not clear for Android though
https://code.videolan.org/videolan/vlc/blob/master/src/android/thread.c#L142
But in the end that's also what is used:
https://code.videolan.org/videolan/vlc/blob/master/src/android/thread.c#L287
> The Windows backend can handle any suitably
> aligned 32-bits blob. And unsigned poses less concerns with wrapping.
I think it's better to have this in the signature, even if it involves a
conscious cast when the types don't exactly match.
There's currently no check on the caller (ultimately the caller is
vlc_cancel_addr_prepare). Resulting in passing a plain unsigned pointer
where an atomic 32 bits pointer (or can it be another size on other
platforms ?) is expected.
The only check there is is that the condition value one can fit in the
type that is assumed on all the backends. And even then if it was an
atomic char it would probably pass the static assert but fail at runtime.
I'm OK with using atomic_uint instead of atomic_int for this patch, and
update the backends accordingly. We only set the value to 1.
> Le 7 février 2020 17:02:08 GMT+02:00, Steve Lhomme <robux4 at ycbcr.xyz> a
> écrit :
>
> Not a atomic_uint.
> ------------------------------------------------------------------------
> include/vlc_threads.h | 4 ++--
> src/android/thread.c | 4 ++--
> src/misc/threads.c | 26 +++++++++++++-------------
> src/win32/thread.c | 4 ++--
> 4 files changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/include/vlc_threads.h b/include/vlc_threads.h
> index 8a39c610870..7ae25931b72 100644
> --- a/include/vlc_threads.h
> +++ b/include/vlc_threads.h
> @@ -1028,8 +1028,8 @@ enum
> {
> VLC_CLEANUP_PUSH,
> VLC_CLEANUP_POP,
> - VLC_CANCEL_ADDR_SET,
> - VLC_CANCEL_ADDR_CLEAR,
> + VLC_CANCEL_ADDR_SET, /* atomic_int* */
> + VLC_CANCEL_ADDR_CLEAR, /* atomic_int* */
> };
>
> #if defined (LIBVLC_USE_PTHREAD_CLEANUP)
> diff --git a/src/android/thread.c b/src/android/thread.c
> index ca715418d41..aec539b4f03 100644
> --- a/src/android/thread.c
> +++ b/src/android/thread.c
> @@ -341,7 +341,7 @@ void vlc_control_cancel(int cmd, ...)
> {
> case VLC_CANCEL_ADDR_SET:
> {
> - void *addr = va_arg(ap, void *);
> + void *addr = va_arg(ap, atomic_int *);
>
> vlc_mutex_lock(&th->wait.lock);
> assert(th->wait.addr == NULL);
> @@ -352,7 +352,7 @@ void vlc_control_cancel(int cmd, ...)
>
> case VLC_CANCEL_ADDR_CLEAR:
> {
> - void *addr = va_arg(ap, void *);
> + void *addr = va_arg(ap, atomic_int *);
>
> vlc_mutex_lock(&th->wait.lock);
> assert(th->wait.addr == addr);
> diff --git a/src/misc/threads.c b/src/misc/threads.c
> index 501a17ac9b3..87c3da03d9e 100644
> --- a/src/misc/threads.c
> +++ b/src/misc/threads.c
> @@ -149,7 +149,7 @@ bool vlc_mutex_marked(const vlc_mutex_t *mutex)
> #if defined(LIBVLC_NEED_SLEEP) || defined(LIBVLC_NEED_CONDVAR)
> #include <stdatomic.h>
>
> -static void vlc_cancel_addr_prepare(void *addr)
> +static void vlc_cancel_addr_prepare(atomic_int *addr)
> {
> /* Let thread subsystem on address to broadcast for cancellation */
> vlc_cancel_addr_set(addr);
> @@ -159,7 +159,7 @@ static void vlc_cancel_addr_prepare(void *addr)
> vlc_cleanup_pop();
> }
>
> -static void vlc_cancel_addr_finish(void *addr)
> +static void vlc_cancel_addr_finish(atomic_int *addr)
> {
> vlc_cancel_addr_clear(addr);
> /* Act on cancellation as potential wake-up source */
> @@ -193,14 +193,14 @@ void (vlc_tick_sleep)(vlc_tick_t delay)
> #ifdef LIBVLC_NEED_CONDVAR
> #include <stdalign.h>
>
> -static inline atomic_uint *vlc_cond_value(vlc_cond_t *cond)
> +static inline atomic_int *vlc_cond_value(vlc_cond_t *cond)
> {
> /* XXX: ugly but avoids including stdatomic.h in vlc_threads.h */
> - static_assert (sizeof (cond->value) <= sizeof (atomic_uint),
> + static_assert (sizeof (cond->value) <= sizeof (atomic_int),
> "Size mismatch!");
> - static_assert ((alignof (cond->value) % alignof (atomic_uint)) == 0,
> + static_assert ((alignof (cond->value) % alignof (atomic_int)) == 0,
> "Alignment mismatch");
> - return (atomic_uint *)&cond->value;
> + return (atomic_int *)&cond->value;
> }
>
> void vlc_cond_init(vlc_cond_t *cond)
> @@ -247,7 +247,7 @@ void vlc_cond_broadcast(vlc_cond_t *cond)
>
> void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex)
> {
> - unsigned value = atomic_load_explicit(vlc_cond_value(cond),
> + int value = atomic_load_explicit(vlc_cond_value(cond),
> memory_order_relaxed);
> while (value & 1)
> {
> @@ -258,20 +258,20 @@ void vlc_cond_wait(vlc_cond_t *cond, vlc_mutex_t *mutex)
> value++;
> }
>
> - vlc_cancel_addr_prepare(&cond->value);
> + vlc_cancel_addr_prepare(vlc_cond_value(cond));
> vlc_mutex_unlock(mutex);
>
> vlc_atomic_wait(&cond->value, value);
>
> vlc_mutex_lock(mutex);
> - vlc_cancel_addr_finish(&cond->value);
> + vlc_cancel_addr_finish(vlc_cond_value(cond));
> }
>
> static int vlc_cond_wait_delay(vlc_cond_t *cond, vlc_mutex_t *mutex,
> vlc_tick_t delay)
> {
> - unsigned value = atomic_load_explicit(vlc_cond_value(cond),
> - memory_order_relaxed);
> + int value = atomic_load_explicit(vlc_cond_value(cond),
> + memory_order_relaxed);
> while (value & 1)
> {
> if (atomic_compare_exchange_weak_explicit(vlc_cond_value(cond), &value,
> @@ -281,7 +281,7 @@ static int vlc_cond_wait_delay(vlc_cond_t *cond, vlc_mutex_t *mutex,
> value++;
> }
>
> - vlc_cancel_addr_prepare(&cond->value);
> + vlc_cancel_addr_prepare(vlc_cond_value(cond));
> vlc_mutex_unlock(mutex);
>
> if (delay > 0)
> @@ -290,7 +290,7 @@ static int vlc_cond_wait_delay(vlc_cond_t *cond, vlc_mutex_t *mutex,
> value = 0;
>
> vlc_mutex_lock(mutex);
> - vlc_cancel_addr_finish(&cond->value);
> + vlc_cancel_addr_finish(vlc_cond_value(cond));
>
> return value ? 0 : ETIMEDOUT;
> }
> diff --git a/src/win32/thread.c b/src/win32/thread.c
> index 6af2eb83852..10c36e9ed82 100644
> --- a/src/win32/thread.c
> +++ b/src/win32/thread.c
> @@ -669,7 +669,7 @@ void vlc_control_cancel (int cmd, ...)
>
> case VLC_CANCEL_ADDR_SET:
> {
> - void *addr = va_arg(ap, void *);
> + void *addr = va_arg(ap, atomic_int *);
>
> EnterCriticalSection(&th->wait.lock);
> assert(th->wait.addr == NULL);
> @@ -680,7 +680,7 @@ void vlc_control_cancel (int cmd, ...)
>
> case VLC_CANCEL_ADDR_CLEAR:
> {
> - void *addr = va_arg(ap, void *);
> + void *addr = va_arg(ap, atomic_int *);
>
> EnterCriticalSection(&th->wait.lock);
> assert(th->wait.addr == addr);
>
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser
> ma brièveté.
>
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>
More information about the vlc-devel
mailing list