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]