On Tue, Jan 7, 2014 at 12:38 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
> 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....@gmail.com> wrote: > >> >> On Mon, Dec 16, 2013 at 4:25 PM, Jim Jagielski <j...@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). > +1 for APR trunk, +0.9 for future APR 1.6.x, -0.9 for APR 1.5.x... alternate opinions? > > 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] > -- Born in Roswell... married an alien... http://emptyhammock.com/
