apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "William A. Rowe, Jr." <wr...@rowe-clan.net>
Subject Re: Showstopper ... was: Tagged the tree
Date Wed, 08 Jan 2003 20:49:15 GMT
{Trimming this to @apr for this bit of discussion.}
At 10:16 AM 1/8/2003, Brian Pane wrote:
>On Wed, 2003-01-08 at 07:15, William A. Rowe, Jr. wrote:
>
>> Just an observation reviewing the apr/includes/ changes... I don't like the
>> look of this code;
>> 
>> +#define apr_atomic_casptr(mem,with,cmp) (void*)atomic_cmpxchg((unsigned long *)(mem),(unsigned
long)(cmp),(unsigned long)(with))
>> 
>> Very simply, this is a very easily broken macro... it is too easy to silently 
>> pass the wrong pointer arg to apr_atomic_casptr and explode.
>> 
>> We need some type safety here, and this macro (I believe) destroys
>> all hope of that.  It seems this needs to become a function.  Macro to
>> function should be a safe change, since code compiled to an earlier
>> apr will continue to use this unsafe but otherwise usable macro.
>
>There's a good reason why the atomic CAS can't be a function.
>Its most common use is in spinlocks, where the probability of
>having to retry the CAS increases with every clock cycle you
>spend in the "if (cas failed) {retry;}" loop.  Adding a function
>call around the CAS would double the length of the code path
>between the lookup of the old value and the compare-and-swap
>of the new value, which in turn doubles the probability that
>the CAS will fail and require a retry.
>
>As for type safety, keep in mind that the role of
>apr_atomic_casptr is to atomically swap any pointer to
>an object.  If we were to turn it into a function, its first
>argument would have to be a void**.

In that case... what about a trick (I believe) Ben Laurie taught us?
Using a typedef for clarity:

typedef void*(*apr_atomic_casptr_fn_t)(unsigned long* mem, unsigned long cmp, unsigned long
with);

#define apr_atomic_casptr ((apr_atomic_casptr_fn_t)(atomic_cmpxchg))

In this way, the arguments to apr_atomic_casptr will be evaluated in terms
of the apr_atomic_casptr_fn_t declaration.

This presumes the arguments exactly match the atomic_cmpxchg function, 
with the exception of twos-compliment signedness.

Make sense?




Mime
View raw message