Basant Kumar kukreja wrote:
Yes, unless until we clearly understand that volatile is not useful here, we
should keep it.
The thing that makes me reasonably sure this is OK (and this is for
trunk/2.4 only) is that there are other apr_atomic_*() functions used
in fdqueue.c without any volatile casting, and I see no reason why
casting the arguments to some but not all of these calls as volatile
would make any difference.
What we had before was:
if (apr_atomic_casptr((volatile void**)&(queue_info->recycled_pools),
new_recycle, next) == next) {
but also:
if (apr_atomic_cas32(&(queue_info->idlers), prev_idlers + 1,
prev_idlers) == prev_idlers) {
I see no reason why a pointer to an integer doesn't need a
volatile cast, while a pointer to a pointer does. I'm also not
aware of any problems reported caused by the lack of a cast on these
other apr_atomic_*() calls.
Consider too that all the apr_atomic_*() functions explicitly cast
their arguments as pointers to volatile data, so the compiler should
perform no optimization on data accesses within the function
(which is presumably where it counts). Putting a volatile cast on
the argument in the call could only mean something, it seems to me,
if the APR functions didn't explicitly do this already.
Further, the (volatile void**) casts for the recycled_pools pointer
were only added in the first place, according to the commit log, to
avoid compiler warnings (see r101236) -- which they no longer do for
gcc anyway, even if they ever did for some platform and compiler in
the past.
If we choose to revert this and restore the (volatile void**)
casts and live with the compiler warnings for the sake of extra
caution, then I think we must also stick a (volatile apr_uint32_t*) cast
on the other apr_atomic_*() calls, otherwise it makes no sense to me
to cast some but not others.
I'm pretty sure the casts on the recycled_pools pointer were only
added in the first place not because of anything to do with
the volatile keyword, but rather, because void** arguments seem to
have a habit of causing compilers to throw warnings. The volatile
keyword was, I strongly suspect, just stuck into the cast because
that's what it looks like in the APR function signatures.
Chris.
--
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263 E4DE C8E3 FA36 366A 375B
