apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@gmail.com>
Subject Re: Event and atomics, round II
Date Tue, 07 Jan 2014 20:15:21 GMT
On Tue, Jan 7, 2014 at 12:38 PM, Yann Ylavic <ylavic.dev@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.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).
>

+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/

Mime
View raw message