Re: svn commit: r726113 - /httpd/httpd/trunk/server/mpm/worker/fdqueue.c

[Basant Kumar kukreja <Ba...@Sun.COM>]

Yes, unless until we clearly understand that volatile is not useful here, we
should keep it.

Regards,
Basant.

On Fri, Dec 12, 2008 at 02:57:03PM -0800, Chris Darroch wrote:
> Ruediger Pluem wrote:
>
>> Not quite sure if this is really correct because apr_atomic_casptr
>> wants to have a (volatile void**) as first parameter.
>
>   That's what made me less than sure ... but my gcc 4.1.2 -Wall
> definitely doesn't like that void** ("dereferencing type-punned pointer ...").
>
>   I think void* should be OK, because the APR function still
> gets a pointer-sized parameter, and casts it to (volatile void**)
> locally, which is presumably what really matters: that within the CAS
> function the optimizer understands that the pointed-to value (another
> pointer, in this case) can be changed by something else it doesn't
> know about, and so won't optimize away any accesses.
>
>   But ... I'm no expert on these kinds of compiler guts.  Revert if
> it seems wrong and better to spew some warnings.
>
> Chris.
>
> -- 
> GPG Key ID: 366A375B
> GPG Key Fingerprint: 485E 5041 17E1 E2BB C263  E4DE C8E3 FA36 366A 375B
>

Re: svn commit: r726113 - /httpd/httpd/trunk/server/mpm/worker/fdqueue.c

[Chris Darroch <ch...@pearsoncmg.com>]

Hi --

   I wrote:

>    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.

   That all said, I did some additional reading on void** casts
and the meaning of the type-punning warning gcc generates, and
it would seem that the underlying reason for the warning here has to
do exclusively with casting a pointer to data (struct recycled_pool*)
to a void*.  See http://c-faq.com/ptrs/genericpp.html
for what seems like a pretty decent explanation.

   Based on that, it would appear to me that the issue with casting
the arguments passed to apr_atomic_casptr() is that should httpd
be compiled on some unusual platform where a void* is, say, larger
than a struct recycled_pool*, things might go awry.

   Now, there are a number of APR functions which take void**
arguments, and I haven't thought about any of these except for the
two apr_atomic_*() ones.

   Does anyone know if we should be concerned about any modern
platforms where void* is a different size than some other pointers?
(E.g., pointers to data and pointers to functions might be different
sizes, and void* the larger of the two?)  In such a case, we might
need to refactor some code, including these apr_atomic_casptr()
calls.  If not, then casting to void* to silence the warnings ought
to be tolerable, I think.

Chris.

-- 
GPG Key ID: 366A375B
GPG Key Fingerprint: 485E 5041 17E1 E2BB C263  E4DE C8E3 FA36 366A 375B

Re: svn commit: r726113 - /httpd/httpd/trunk/server/mpm/worker/fdqueue.c

[Chris Darroch <ch...@pearsoncmg.com>]

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