You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2005/10/03 15:33:55 UTC
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
William A. Rowe, Jr. wrote:
>
> Joe Orton wrote:
> > On Mon, Oct 03, 2005 at 08:11:44AM -0400, Jim Jagielski wrote:
> >
> >>Joe Orton wrote:
> >>
> >>>On Mon, Oct 03, 2005 at 07:50:39AM -0400, Jim Jagielski wrote:
> >>>
> >>>>Just some lines that caught my eye:
> >>>>... Whenever I see conditionals cast to (long) I get
> >>>>suspicious.
> >>>
> >>>These are all cases where an integer is stored in a pointer; it's safe
> >>>to assume that a long will fit in a pointer on all platforms which httpd
> >>>runs on as a practical consideration, and using a cast to long rather
> >>>than a cast to int will avoid compiler warnings on LP64 platforms.
> >>
> >>... Certainly a union could be
> >>used to avoid this.
> >
> > I usually end up deciding these issues are so marginal
> > that there's some better way to spend time fixing real bugs than to try
> > and break code which works perfectly well ;)
>
> Joe's correct that this code change works on ILP32, ILP64 and LP64,
> platforms - but I concur with Jim that for the casual developer, the
> purpose is hard to glean...
>
> ...perhaps we need an apr_intptr_t type which is a best-fit int for any
> arbitrary void* storage class?
>
ANSI C specifically allows for pointers to be converted to
a "large enough" integral type. For safety, I would suggest unsigned
long, but whenever we have impl dependent code, we should make
sure it's well documented, abstract it out if possible, or, at
least, confirm during configuration that our assumptions are
correct.
--
=======================================================================
Jim Jagielski [|] jim@jaguNET.com [|] http://www.jaguNET.com/
"If you can dodge a wrench, you can dodge a ball."
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 5, 2005, at 8:49 AM, Brian Candler wrote:
>
> Now, Apache is arguably abusing void * by using it as a data type for
> holding arbitrary integers.
A valid argument... Instead we should have an opaque container
which is large enough to hold a long as well as a void *.
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Posted by Brian Candler <B....@pobox.com>.
On Tue, Oct 04, 2005 at 10:04:14PM +0200, Rdiger Plm wrote:
> >> P.S. The reverse case is sillier, given that the value is moving to a
> >> *larger* type and therefore no data loss can occur:
> >>
> >> short a;
> >> long b = a; // (7) no warning
> >>
> >> short a;
> >> void *b = a; // (8) warns "pointer from integer without a
> >> cast" (ok)
> >> void *c = (void *)a; // (9) warns "cast to pointer from integer of
> >> different size"
> >> void *d = (void *)(long)a; // (10) even more stupid!!!
> >>
> >> Note that having a 'large enough' integral type in case (10) is not good
> >> enough. We need to cast 'a' to an integral type which is *exactly* the
> >> same
> >> size as a void *, to silence this particular warning.
> >
> >
> > I agree with your assertion that the code example above IS a gcc bug.
> > This is expected in C++, but the waning (9) above is completely bogus,
>
> I do not think so. While "a" does fit in "c" from the storage point of view
> converting "c" to a different pointer type e.g. (char *) and dereferencing it
> afterwards most likely leads to SIGBUS or SIGSEGV. So I think a warning is
> justified here.
I disagree. For example, if you take a (char *) and cast it to a (void *),
and later to an (int *), you will very likely get your SIGBUS error, but the
compiler won't warn you about it. That's because (void *) is explicitly
meant to be "a pointer to anything you choose". Once you've cast anything
through a void *, you lose any type checking of the thing pointed to.
If you're casting an integer to a pointer, as Apache does: then almost
certainly the pointer isn't going to be usable for deferencing (unless the
value you're casting from happened to hold an integer representation of
another pointer). Furthermore, we're casting to a void *, and you can't
dereference through a void * anyway.
Now, Apache is arguably abusing void * by using it as a data type for
holding arbitrary integers. One assumption is that those integers (like
pid_t) are all no larger than a void *. That's a reasonable assumption on
most platforms (OK, TurboC excepted :-)
Once we agree on that assumption, then there is no need to see a warning
when casting a small integer type into a larger void *, or a void * back
into a smaller integer type. So I say it's the warning that's the problem,
not the code.
> What's your though on AP_PTRINT(n) / AP_INTPTR(p)?
Well, I suppose we're always going to have to have a cast if moving an int
into a void * or vice versa. So:
pid = AP_PTRINT(ap_hash_get(....))
works as well as anything. I would vote having the second level of cast
though: that is, not
pid = (pid_t)AP_INTPTR(ap_hash_get(....))
If we're using AP_PTRINT then I think we have to assume the compiler won't
give you a warning if assigning a 64-bit integral value into a 32-bit
variable. That seems to work for gcc, right now. But if it doesn't in
future, we would then need macros like
AP_PTR_PID_T(n)
AP_PID_T_PTR(p)
Ergh.
IMO this still boils down to: gcc is giving a code warning which is
inappropriate/irrelevant for the coding style of the Apache project. Hell,
just *document* that this warning is generated all over the place, and that
anyone building the code should ignore it. Then the code can say
pid = (pid_t)ap_hash_get(...)
which is what was meant all along.
Cheers,
Brian.
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Posted by Rüdiger Plüm <r....@gmx.de>.
On 10/04/2005 09:32 PM, William A. Rowe, Jr. wrote:
> Brian Candler wrote:
>
[..cut..]
>
>> P.S. The reverse case is sillier, given that the value is moving to a
>> *larger* type and therefore no data loss can occur:
>>
>> short a;
>> long b = a; // (7) no warning
>>
>> short a;
>> void *b = a; // (8) warns "pointer from integer without a
>> cast" (ok)
>> void *c = (void *)a; // (9) warns "cast to pointer from integer of
>> different size"
>> void *d = (void *)(long)a; // (10) even more stupid!!!
>>
>> Note that having a 'large enough' integral type in case (10) is not good
>> enough. We need to cast 'a' to an integral type which is *exactly* the
>> same
>> size as a void *, to silence this particular warning.
>
>
> I agree with your assertion that the code example above IS a gcc bug.
> This is expected in C++, but the waning (9) above is completely bogus,
I do not think so. While "a" does fit in "c" from the storage point of view
converting "c" to a different pointer type e.g. (char *) and dereferencing it
afterwards most likely leads to SIGBUS or SIGSEGV. So I think a warning is
justified here.
> as you illustrate with line (7) above.
>
This is different as no harm can be expected here like in (9).
[..cut..]
Regards
Rüdiger
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Brian Candler wrote:
>
> Hmm. Given the example
>
> pid = (pid_t)((long)apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id)));
>
> then it's no good declaring variable 'pid' to be of type apr_intptr_t rather
> than pid_t, because somewhere down the line it will be passed to a function
> which expects a pid_t argument, and you'll get a warning there instead.
pid = (pid_t)apr_hash_get(script_hash, &cgid_req.conn_id,
sizeof(cgid_req.conn_id));
would be correct in -this- case. We have to make a small concession
to the platform architects, that there's no sense in more pid's than
you could count with a void*, so no matter if pid_t is an int or long
(or a handle) this simply works. Now if the line above produces our
compile warning, of truncation, there's no issue I suppose with
pid = (pid_t)((ap_ptrint_t)apr_hash_get(script_hash,
&cgid_req.conn_id,
sizeof(cgid_req.conn_id)));
or perhaps we simplify, with AP_INTPTR(n) and AP_PTRINT(p) macros?
pid = (pid_t)AP_PTRINT(apr_hash_get(script_hash,
&cgid_req.conn_id,
sizeof(cgid_req.conn_id));
e.g.
#define AP_INTPTR(n) ((void*)((ap_ptrint_t)(n)))
#define AP_PTRINT(n) ((ap_ptrint_t)(n))
I suppose I understand the truncation warning but...
> Now, Apache has lots of cases where (void *)'s are cast to integral types
> (possibly smaller than a void *) and back again. Since this is intentional,
> I think the real solution is to turn off the compiler warning for this case,
> not to bastardise the code with lots of double-casts. The more unnecessary
> casts you put into your code, the more likely you are to make type errors
> which the compiler can't check.
> IMO, if gcc can't disable this warning, then we need to file a bug report.
> Or else just pipe gcc stderr through
>
> egrep -v "warning: cast (to|from) pointer (to|from) integer of different size"
>
> and forget about it.
Now that's not a healthy situation, -1 on the gcc pipe :)
> P.S. The reverse case is sillier, given that the value is moving to a
> *larger* type and therefore no data loss can occur:
>
> short a;
> long b = a; // (7) no warning
>
> short a;
> void *b = a; // (8) warns "pointer from integer without a cast" (ok)
> void *c = (void *)a; // (9) warns "cast to pointer from integer of different size"
> void *d = (void *)(long)a; // (10) even more stupid!!!
>
> Note that having a 'large enough' integral type in case (10) is not good
> enough. We need to cast 'a' to an integral type which is *exactly* the same
> size as a void *, to silence this particular warning.
I agree with your assertion that the code example above IS a gcc bug.
This is expected in C++, but the waning (9) above is completely bogus,
as you illustrate with line (7) above.
What's your though on AP_PTRINT(n) / AP_INTPTR(p)?
Bill
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Posted by Brian Candler <B....@pobox.com>.
On Tue, Oct 04, 2005 at 12:12:14PM -0400, Jim Jagielski wrote:
> Brian Candler wrote:
> >
> > Hmm. Given the example
> >
> > pid = (pid_t)((long)apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id)));
> >
> > then it's no good declaring variable 'pid' to be of type apr_intptr_t rather
> > than pid_t, because somewhere down the line it will be passed to a function
> > which expects a pid_t argument, and you'll get a warning there instead. Are
> > you saying it should be written as
> >
>
> The above line just confuses me, but I haven't taken the time
> to try to understand the rationale for why it's written the way it is.
That's what I was hoping the little example with shorts, longs and void *'s
would explain :-)
In the above code:
- apr_hash_get returns void *
- if you assign it directly to 'pid', you get a compiler warning saying
"assigning integer from pointer without a cast". That's fair enough.
- however, if you cast it to pid_t, you get a compiler warning saying
"cast from pointer to integer of different size", if pid_t is not
actually the same size as a void *
So stupidly you have to cast it to an integer of the same size as a pointer,
*then* cast it to pid_t or whatever you wanted in the first place, to
silence the warnings.
I think this is the compiler grumbling about something which it shouldn't,
at least for Apache's way of working: the code explicitly casts void * to a
smaller integral type, because it was being used to hold a value of a
smaller integral type in the first place. IMO the cast is the programmer's
way of saying "yes, I know this is a pointer, and yes I want you treat it as
a pid_t (or whatever)". I guess the warning is for the benefit of people who
use an integral type for temporarily holding a pointer, rather than a
pointer type for temporarily holding an integer.
I believe the solution is to silence the compiler warnings, rather than
corrupt the code with superfluous casts. A union might be cleaner, in which
case you'd write something like
pid = apr_hash_get(...).num;
I would hope that most modern compilers are able to cope with passing and
returning unions by value (especially ones which fit within a machine word),
but maybe that doesn't apply to all the platforms Apache is supposed to
compile on.
Regards,
Brian.
// Proof of concept
#include <stdio.h>
typedef union {
long long_v;
void *void_p;
} ap_union;
ap_union foo(ap_union bar)
{
return bar;
}
int main(void)
{
ap_union x, y;
x.void_p = "hello";
y = foo(x);
printf("%s\n", (char *)y.void_p);
printf("%08lx\n", y.long_v);
return 0;
}
Re: svn commit: r293305 - in /httpd/httpd/branches/2.2.x/modules:
Posted by Brian Candler <B....@pobox.com>.
On Mon, Oct 03, 2005 at 09:33:55AM -0400, Jim Jagielski wrote:
> > Joe's correct that this code change works on ILP32, ILP64 and LP64,
> > platforms - but I concur with Jim that for the casual developer, the
> > purpose is hard to glean...
> >
> > ...perhaps we need an apr_intptr_t type which is a best-fit int for any
> > arbitrary void* storage class?
> >
>
> ANSI C specifically allows for pointers to be converted to
> a "large enough" integral type. For safety, I would suggest unsigned
> long, but whenever we have impl dependent code, we should make
> sure it's well documented, abstract it out if possible, or, at
> least, confirm during configuration that our assumptions are
> correct.
Hmm. Given the example
pid = (pid_t)((long)apr_hash_get(script_hash, &cgid_req.conn_id, sizeof(cgid_req.conn_id)));
then it's no good declaring variable 'pid' to be of type apr_intptr_t rather
than pid_t, because somewhere down the line it will be passed to a function
which expects a pid_t argument, and you'll get a warning there instead. Are
you saying it should be written as
pid = (pid_t)(apr_intptr_t)apr_hash_get(...)
instead? Or just as
pid = (apr_intptr_t)apr_hash_get(...)
and rely on the compiler not moaning if pid_t is smaller than apr_intptr_t?
ISTM the fundamental problem is illustrated here:
long a;
short b = a; // (1) a warning would be useful here
short c = (short)a; // (2) no warning
void *a;
short b = a; // (3) warns "integer from pointer without a cast" (ok)
short c = (short)a; // (4) warns "cast from pointer to integer of different size"
so you have to write
short d = (short)(long)a; // (5) stupid!!!
which is just ridiculous, or possibly
short e = (long)a; // (6) ??
which looks like the programmer might have made a mistake. It doesn't seem
to issue a warning at present (as tested with gcc 3.4.2), but I expect it
might in the future, by analogy with case (1).
As I see it, if I explicitly cast something to a (foo) then I'm contracting
with the compiler that I'm happy to have the value truncated to fit, as in
case (2), so I don't see that the compiler should warn in case (4).
Now, Apache has lots of cases where (void *)'s are cast to integral types
(possibly smaller than a void *) and back again. Since this is intentional,
I think the real solution is to turn off the compiler warning for this case,
not to bastardise the code with lots of double-casts. The more unnecessary
casts you put into your code, the more likely you are to make type errors
which the compiler can't check.
IMO, if gcc can't disable this warning, then we need to file a bug report.
Or else just pipe gcc stderr through
egrep -v "warning: cast (to|from) pointer (to|from) integer of different size"
and forget about it.
Regards,
Brian.
P.S. The reverse case is sillier, given that the value is moving to a
*larger* type and therefore no data loss can occur:
short a;
long b = a; // (7) no warning
short a;
void *b = a; // (8) warns "pointer from integer without a cast" (ok)
void *c = (void *)a; // (9) warns "cast to pointer from integer of different size"
void *d = (void *)(long)a; // (10) even more stupid!!!
Note that having a 'large enough' integral type in case (10) is not good
enough. We need to cast 'a' to an integral type which is *exactly* the same
size as a void *, to silence this particular warning.