apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: Event and atomics, round II
Date Tue, 07 Jan 2014 17:38:23 GMT
Hi,

I cc to APR's dev list so to propose a patch regarding
apr_atomic_dec32()platform dependent return
values.

On Mon, Jan 6, 2014 at 5:27 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:

>
> On Mon, Dec 16, 2013 at 4:25 PM, Jim Jagielski <jim@jagunet.com> wrote:
>
>> Now that 2.4.7 has been out for awhile, I would have assumed
>> that if people were hitting the "atomics not working as
>> expected" error (using unsigned as signed), we would have
>> started hearing about it... But, afaik, we haven't.
>>
>
> Since this is reproductable on any platform using "atomic/unix/ia32.c" or
> "atomic/os390/atomic.c" (no gcc/solaris atomic builtins available), I think
> it should be fixed somehow.
>
>
>> So this leads me to the following discussion: should we stay
>> with the "workaround" started in
>>
>>         http://svn.apache.org/viewvc?view=revision&revision=1545286
>>
>> where we use an zero-point offset, or go back to the old method,
>> or do something else?
>>
>
> apr_atomic_dec32() is documented (and acts) as returning 0 (when reached)
> or non-0 (otherwise), and in the case where the implementation returns the
> new (decremented) value as non-0, that's with a conversion for unsigned32
> to signed32 (which is undefined AFAIK)
> .
>
>
> IMHO, apr_atomic_dec32() should be fixed to return a boolean value on all
> platforms (as documented, no undefined conversion), and httpd use
> apr_atomic_add32(&foo, -1) instead when it wants the previous value (or
> previous - 1 for the new one).
>

The patch below is for apr_atomic_dec32() to always return a boolean value
(as documented, 0 => hit zero, !0 => otherwise), on any platform.

It avoids unsigned to signed conversion, but the caller can't expect
anymore the returned value to be the new atomic value (that was platform
dependent behaviour anyway).

The caller should instead use apr_atomic_add32(&foo, -1) to decrement and
get the previous value atomically (or all that minus 1 for the new value).

Since the API is not really symetric now and requires the caller to do
things like that, maybe a new API would be welcome, with symetric
apr_atomic32_read/set/inc/dec/sub/add/cas() functions working on signed
integers, and with the equivalent 64bits (where available) and pointer
versions...

Regards,
Yann.


Index: atomic/win32/apr_atomic.c
===================================================================
--- atomic/win32/apr_atomic.c    (revision 1556264)
+++ atomic/win32/apr_atomic.c    (working copy)
@@ -75,7 +75,7 @@ APR_DECLARE(apr_uint32_t) apr_atomic_inc32(volatil
 #if (defined(_M_IA64) || defined(_M_AMD64)) && !defined(RC_INVOKED)
     return InterlockedIncrement(mem) - 1;
 #elif defined(__MINGW32__)
-    return InterlockedIncrement((long *)mem) - 1;
+    return InterlockedIncrement((long *)mem) - 1L;
 #else
     return ((apr_atomic_win32_ptr_fn)InterlockedIncrement)(mem) - 1;
 #endif
@@ -84,11 +84,11 @@ APR_DECLARE(apr_uint32_t) apr_atomic_inc32(volatil
 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
 #if (defined(_M_IA64) || defined(_M_AMD64)) && !defined(RC_INVOKED)
-    return InterlockedDecrement(mem);
+    return InterlockedDecrement(mem) != 0;
 #elif defined(__MINGW32__)
-    return InterlockedDecrement((long *)mem);
+    return InterlockedDecrement((long *)mem) != 0L;
 #else
-    return ((apr_atomic_win32_ptr_fn)InterlockedDecrement)(mem);
+    return ((apr_atomic_win32_ptr_fn)InterlockedDecrement)(mem) != 0;
 #endif
 }

Index: atomic/unix/ia32.c
===================================================================
--- atomic/unix/ia32.c    (revision 1556264)
+++ atomic/unix/ia32.c    (working copy)
@@ -57,14 +57,14 @@ APR_DECLARE(apr_uint32_t) apr_atomic_inc32(volatil

 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
-    unsigned char prev;
+    unsigned char flag;

     asm volatile ("lock; decl %0; setnz %1"
-                  : "=m" (*mem), "=qm" (prev)
+                  : "=m" (*mem), "=qm" (flag)
                   : "m" (*mem)
                   : "memory");

-    return prev;
+    return flag != 0;
 }

 APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
Index: atomic/unix/s390.c
===================================================================
--- atomic/unix/s390.c    (revision 1556264)
+++ atomic/unix/s390.c    (working copy)
@@ -82,7 +82,7 @@ APR_DECLARE(void) apr_atomic_sub32(volatile apr_ui

 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
-    return atomic_sub(mem, 1);
+    return atomic_sub(mem, 1) != 0;
 }

 APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
Index: atomic/unix/mutex.c
===================================================================
--- atomic/unix/mutex.c    (revision 1556264)
+++ atomic/unix/mutex.c    (working copy)
@@ -142,7 +142,7 @@ APR_DECLARE(int) apr_atomic_dec32(volatile apr_uin

     MUTEX_UNLOCK(mutex);

-    return new;
+    return new != 0;
 }

 APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
Index: atomic/unix/ppc.c
===================================================================
--- atomic/unix/ppc.c    (revision 1556264)
+++ atomic/unix/ppc.c    (working copy)
@@ -91,7 +91,7 @@ APR_DECLARE(apr_uint32_t) apr_atomic_inc32(volatil

 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
-    apr_uint32_t prev;
+    apr_uint32_t new;

     asm volatile ("1:\n"                       /* lost reservation     */
                   "    lwarx   %0,0,%2\n"      /* load and reserve     */
@@ -99,11 +99,11 @@ APR_DECLARE(int) apr_atomic_dec32(volatile apr_uin
                   PPC405_ERR77_SYNC            /* ppc405 Erratum 77    */
                   "    stwcx.  %0,0,%2\n"      /* store new value      */
                   "    bne-    1b\n"           /* loop if lost         */
-                  : "=&b" (prev), "=m" (*mem)
+                  : "=&b" (new), "=m" (*mem)
                   : "b" (mem), "m" (*mem)
                   : "cc", "memory");

-    return prev;
+    return new != 0;
 }

 APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
Index: atomic/unix/builtins.c
===================================================================
--- atomic/unix/builtins.c    (revision 1556264)
+++ atomic/unix/builtins.c    (working copy)
@@ -50,7 +50,7 @@ APR_DECLARE(apr_uint32_t) apr_atomic_inc32(volatil

 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
-    return __sync_sub_and_fetch(mem, 1);
+    return __sync_sub_and_fetch(mem, 1) != 0;
 }

 APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
Index: atomic/unix/solaris.c
===================================================================
--- atomic/unix/solaris.c    (revision 1556264)
+++ atomic/unix/solaris.c    (working copy)
@@ -52,7 +52,7 @@ APR_DECLARE(apr_uint32_t) apr_atomic_inc32(volatil

 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
-    return atomic_dec_32_nv(mem);
+    return atomic_dec_32_nv(mem) != 0;
 }

 APR_DECLARE(apr_uint32_t) apr_atomic_cas32(volatile apr_uint32_t *mem,
apr_uint32_t with,
Index: atomic/netware/apr_atomic.c
===================================================================
--- atomic/netware/apr_atomic.c    (revision 1556264)
+++ atomic/netware/apr_atomic.c    (working copy)
@@ -61,7 +61,7 @@ APR_DECLARE(apr_uint32_t) apr_atomic_xchg32(volati

 APR_DECLARE(int) apr_atomic_dec32(volatile apr_uint32_t *mem)
 {
-    return (atomic_xchgadd((unsigned long *)mem, 0xFFFFFFFF) - 1);
+    return atomic_xchgadd((unsigned long *)mem, 0xFFFFFFFF) != 1UL;
 }

 APR_DECLARE(void *) apr_atomic_casptr(volatile void **mem, void *with,
const void *cmp)
[EOS]

Mime
View raw message