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). Currently (in 2.4.7), ap_queue_info_try_get_idler() and the "atomics not working as expected" check are broken on some platforms, they both use apr_atomic_dec32()'s return value the wrong way. Moreover, apr_atomic_add32(&foo, -1) is used for a while in ap_queue_info_wait_for_idler(), and seems to work on all platforms as a safe replacement... So I think your commits (1545286, 1545292, 1545325, 1545364, 1545408, 1545411, 1545412) worth being backported. Regards, Yann.
