[vlc-devel] [PATCH 3/5] threads: VLC_CANCEL_ADDR_SET/CLEAR always use an atomic_int internally

Rémi Denis-Courmont remi at remlab.net
Fri Feb 7 16:44:06 CET 2020


P.S.: I'm aware that atomic ops on signed integers use 2 complements. What concerns me is when the value of the atomic variable is manipulated non-atomically - typically in a compare-exchange loop.

Le 7 février 2020 17:38:02 GMT+02:00, "Rémi Denis-Courmont" <remi at remlab.net> a écrit :
>Hi,
>
>Where do you get that from? The Windows backend can handle any suitably
>aligned 32-bits blob. And unsigned poses less concerns with wrapping.
>
>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);
>>-- 
>>2.17.1
>>
>>_______________________________________________
>>vlc-devel mailing list
>>To unsubscribe or modify your subscription options:
>>https://mailman.videolan.org/listinfo/vlc-devel
>
>-- 
>Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser
>ma brièveté.

-- 
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20200207/8c7fbe57/attachment.html>


More information about the vlc-devel mailing list