Re: [Bug 269] Many compiler warnings with gcc 4.0
bugzilla-daemon@dump.segv.org wrote:
------- Additional Comments From nbk@sitadelle.com 2005-08-24 09:58 ------- Many thanks for second patch! I've added it to the CVS branch 1.0, so your fixes will be in release 1.0.5.
There're still problems of incompatible pointer types, but I need Alan's opinion about bringing so many casts in the source. (personally I don't like the idea)
Neither do I. They make the code harder to read & maintain. Alan DeKok.
On Thu, Aug 25, 2005 at 05:24:05PM -0400, Alan DeKok wrote:
bugzilla-daemon@dump.segv.org wrote:
------- Additional Comments From nbk@sitadelle.com 2005-08-24 09:58 ------- Many thanks for second patch! I've added it to the CVS branch 1.0, so your fixes will be in release 1.0.5.
There're still problems of incompatible pointer types, but I need Alan's opinion about bringing so many casts in the source. (personally I don't like the idea)
Neither do I. They make the code harder to read & maintain.
Alan DeKok.
A brief glance through the patch in the bugreport indicates a lot of the casts were for value_pair's strvalue member from uint8_t* to char*... What might solve this better is if we can somehow get char to default to unsigned when building, and all these will go away. Also, hidden amongst the signedness errors are occasional "comparison always <blah> due to limited range". One such (that I've looked at before and given up on) is src/lib/radius.c:1439. This comes about because the definition of TAG_VALID_ZERO contains two tests, one of which is >=0 which is called against a uint8_t* here. The other two uses of TAG_VALID_ZERO are in valuepair.c, and are made with an explicitly signed char coming out of strtol (which is defined here as returning a long int...! Is this a big nasty bug????) and which could maybe be replaced with strtoul, since a return <0 is discarded. However, strtoul appears to discard any negatiion, rather than truncating at 0, so this last may not be much use. Possibly large swathes of the server could be cleaned up to use uint8_t instead of char, and such warnings could be eliminated, but we will still get signedness problems from the libc calls which are defined with char parameters, and char is considered signed. I agree that the cast is ugly, and in fact a few prototypes (such as rad_pwdecode) could be changed to take a uint8_t instead of a char to remove some similar casts. (Speculation based on the block directly above src/lib/radius.c:1439) And I then suggest that the above is more like HEAD work than RELEASE_1_0 work. ^_^ -- ----------------------------------------------------------- Paul "TBBle" Hampson, MCSE 8th year CompSci/Asian Studies student, ANU The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361) Paul.Hampson@Pobox.Com Of course Pacman didn't influence us as kids. If it did, we'd be running around in darkened rooms, popping pills and listening to repetitive music. -- Kristian Wilson, Nintendo, Inc, 1989 License: http://creativecommons.org/licenses/by/2.1/au/ -----------------------------------------------------------
Paul TBBle Hampson <Paul.Hampson@Pobox.com> wrote:
A brief glance through the patch in the bugreport indicates a lot of the casts were for value_pair's strvalue member from uint8_t* to char*...
What might solve this better is if we can somehow get char to default to unsigned when building, and all these will go away.
Sure.
Also, hidden amongst the signedness errors are occasional "comparison always <blah> due to limited range". One such (that I've looked at before and given up on) is src/lib/radius.c:1439. This comes about because the definition of TAG_VALID_ZERO contains two tests, one of which is >=3D0 which is called against a uint8_t* here.
Yeah, it's annoying. I'm looking at that code now, and will see if I can fix it. I don't think it has any bad side-effects, but it should be fixed.
Possibly large swathes of the server could be cleaned up to use uint8_t instead of char, and such warnings could be eliminated, but we will still get signedness problems from the libc calls which are defined with char parameters, and char is considered signed.
Yes.
I agree that the cast is ugly, and in fact a few prototypes (such as rad_pwdecode) could be changed to take a uint8_t instead of a char to remove some similar casts. (Speculation based on the block directly above src/lib/radius.c:1439)
Yes.
And I then suggest that the above is more like HEAD work than RELEASE_1_0 work.
Yes. Alan DeKok.
I tend to think this is the wrong approach. By convention, C strings are signed and Pascal strings (I know, nobody uses them anymore) are unsigned. If char defaults to unsigned, it could cause more problems than it solves. We want the compiler to tell us if we're mixing C- strings and data buffers. I agree with the other list members who don't like the casts. My question is, why is the strvalue field unsigned? Does it ever contain data that's not a C-string? Steve On Aug 30, 2005, at 12:56 PM, Alan DeKok wrote:
What might solve this better is if we can somehow get char to default to unsigned when building, and all these will go away.
Sure.
On Tue, Aug 30, 2005 at 01:15:45PM -0700, Steven Simon wrote:
I tend to think this is the wrong approach. By convention, C strings are signed and Pascal strings (I know, nobody uses them anymore) are unsigned. If char defaults to unsigned, it could cause more problems than it solves. We want the compiler to tell us if we're mixing C- strings and data buffers. I agree with the other list members who don't like the casts. My question is, why is the strvalue field unsigned? Does it ever contain data that's not a C-string?
Chars are unsigned on Linux PowerPC. I don't believe there's anything that depends on chars being signed that doesn't explicitly declare such, as I ran FreeRADIUS on LinuxPPC without issues for a few years. I think all signed-char assumptions were therefore shaken out a fair while ago. So we could add -funsigned-char to CFLAGS and see what happens... ^_^ I thought gcc4 was moving to unsigned-char by default, but I guess not... (C-strings are null-terminated. Pascal strings start with their length. ^_^) What's actually happening here is not that we're confusing data buffers and strings, but that we're using string functions (which actually operate on null-terminated data buffers) on null-terminated data buffers. So if anyone's confusing the two, it's glibc. ^_^ -- ----------------------------------------------------------- Paul "TBBle" Hampson, MCSE 8th year CompSci/Asian Studies student, ANU The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361) Paul.Hampson@Pobox.Com Of course Pacman didn't influence us as kids. If it did, we'd be running around in darkened rooms, popping pills and listening to repetitive music. -- Kristian Wilson, Nintendo, Inc, 1989 License: http://creativecommons.org/licenses/by/2.1/au/ -----------------------------------------------------------
On August 31, 2005 4:47:16 PM +1000 Paul TBBle Hampson <Paul.Hampson@Pobox.com> wrote:
On Tue, Aug 30, 2005 at 01:15:45PM -0700, Steven Simon wrote:
I tend to think this is the wrong approach. By convention, C strings are signed and Pascal strings (I know, nobody uses them anymore) are unsigned. If char defaults to unsigned, it could cause more problems than it solves. We want the compiler to tell us if we're mixing C- strings and data buffers.
Absolutely, but this only comes into play when promoting char to int, as is done with varargs functions, e.g. sprintf(). (There is no such thing as a char argument to a varargs function; all chars are promoted to int, and unsigned chars to unsigned ints.) One common place this happens is for debug output, when printing the contents of buffers. <http://www.freshsources.com/1994017B.HTM> is a good page describing the difference between signed and unsigned char.
Chars are unsigned on Linux PowerPC. I don't believe there's anything that depends on chars being signed that doesn't explicitly declare such, as I ran FreeRADIUS on LinuxPPC without issues for a few years. I think all signed-char assumptions were therefore shaken out a fair while ago. ...
To say that chars are unsigned on Linux PPC is meaningless. Whether chars are signed or unsigned is up to the compiler. Now it may be the case that gcc on Linux PPC does treat char as unsigned, but I highly doubt it. Linux PPC itself may have lots of code that explicitly has unsigned char data, but that doesn't affect application code at all. Even whether or not glibc uses explicit unsigned char internally doesn't affect applications since behavior must still conform to the various standards. Wherever char *'s are used, promotion isn't done so signed vs. unsigned doesn't matter. (And current standards call for void * instead of char * almost everywhere.) My point is that FR running successfully on Linux PPC, which may use unsigned char everywhere internally (I'll take your word on it) does not imply anything about how char might be treated in application code. Forcing unsigned char would be a mistake, IMHO. I personally think casts are good. One man's ugly notation is another man's inline documentation. -frank
Frank Cusack <fcusack@fcusack.com> wrote:
Forcing unsigned char would be a mistake, IMHO. I personally think casts are good. One man's ugly notation is another man's inline documentation.
The casts hilight API incompatibilities, and potential security issues. Maybe since we're doing massive re-writes anyhow, we could have: VALUE_PAIR { union { char strvalue[]; uint8_t octets[]; ... } data; } And then use the "right" data member to access the data. "uint8_t" shouldn't be passed to "strcpy", especially if we're using "uint8_t" as "opaque data". Alan DeKok.
looks like you beat me to it ;) Steve On Aug 31, 2005, at 1:09 PM, Alan DeKok wrote:
Frank Cusack <fcusack@fcusack.com> wrote:
Forcing unsigned char would be a mistake, IMHO. I personally think casts are good. One man's ugly notation is another man's inline documentation.
The casts hilight API incompatibilities, and potential security issues.
Maybe since we're doing massive re-writes anyhow, we could have:
VALUE_PAIR { union { char strvalue[]; uint8_t octets[]; ... } data; }
And then use the "right" data member to access the data. "uint8_t" shouldn't be passed to "strcpy", especially if we're using "uint8_t" as "opaque data".
Alan DeKok.
- List info/subscribe/unsubscribe? See http://www.freeradius.org/list/ devel.html
On August 31, 2005 4:09:49 PM -0400 Alan DeKok <aland@ox.org> wrote:
Frank Cusack <fcusack@fcusack.com> wrote:
Forcing unsigned char would be a mistake, IMHO. I personally think casts are good. One man's ugly notation is another man's inline documentation.
The casts hilight API incompatibilities, and potential security issues.
Maybe since we're doing massive re-writes anyhow, we could have:
VALUE_PAIR { union { char strvalue[]; uint8_t octets[]; ... } data; }
And then use the "right" data member to access the data. "uint8_t" shouldn't be passed to "strcpy", especially if we're using "uint8_t" as "opaque data".
I personally dislike that, however I have seen other pieces of code that did similar things to avoid casts. Myself, I find it quite hard to read. Not sure why you'd use uint8_t instead of unsigned char though. -frank
Frank Cusack <fcusack@fcusack.com> wrote:
I personally dislike that, however I have seen other pieces of code that did similar things to avoid casts. Myself, I find it quite hard to read.
I agree. That's why som eheader files do: #define my_foo bar.baz.y.z.foo It breaks some things, but it's easy to read.
Not sure why you'd use uint8_t instead of unsigned char though.
Less typing? Personally, I don't like "unsigned char", as that says to me "unsigned ASCII character". Instead "uint8_t" says "unsigned 8 bits of data". Alan DeKok.
On Wed, Aug 31, 2005 at 04:09:49PM -0400, Alan DeKok wrote:
Maybe since we're doing massive re-writes anyhow, we could have:
VALUE_PAIR { union { char strvalue[]; uint8_t octets[]; ... }} data;
Can we throw a couple of #defines in there too? #define vp_strval data.strvalue #define vp_octets data.octets ? That way vp->vp_strval is a direct substitute for vp->strvalue. It carries a risk of namespace pollution though. (I'm not too fussed, I'm as happy as not with unions, but I know some people dislike the extra component.) Otherwise, an anonymous union might do it, if I recall correctly? -- ----------------------------------------------------------- Paul "TBBle" Hampson, MCSE 8th year CompSci/Asian Studies student, ANU The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361) Paul.Hampson@Pobox.Com Of course Pacman didn't influence us as kids. If it did, we'd be running around in darkened rooms, popping pills and listening to repetitive music. -- Kristian Wilson, Nintendo, Inc, 1989 License: http://creativecommons.org/licenses/by/2.1/au/ -----------------------------------------------------------
Paul TBBle Hampson <Paul.Hampson@Pobox.com> wrote:
Can we throw a couple of #defines in there too? #define vp_strval data.strvalue #define vp_octets data.octets ?
Yes.
That way vp->vp_strval is a direct substitute for vp->strvalue. It carries a risk of namespace pollution though.
Try using "st_mtime" in your code. The headers for the stat() routine do the same thing.
(I'm not too fussed, I'm as happy as not with unions, but I know some people dislike the extra component.)
Otherwise, an anonymous union might do it, if I recall correctly?
I'm not sure how portable that is. In any case vp->vp_strvalue looks fine to me. It also lets us do vp->vp_ipv6addr and other things, which would be very good. Alan DeKok.
The casts are a mixed bag. They can be distracting, but they document expectations, as Frank pointed out. I'll present another option, also a bit ugly, and see how it goes. We could use a union that has accessors for each value type. The code would have to use the appropriate one for the attribute. union { char str[MAX_STRING_LEN]; // or sstr for signed uint8_t data[MAX_STRING_LEN]; // or ustr for unsigned } value; Steve On Aug 31, 2005, at 12:38 PM, Frank Cusack wrote:
On August 31, 2005 4:47:16 PM +1000 Paul TBBle Hampson <Paul.Hampson@Pobox.com> wrote:
On Tue, Aug 30, 2005 at 01:15:45PM -0700, Steven Simon wrote:
I tend to think this is the wrong approach. By convention, C strings are signed and Pascal strings (I know, nobody uses them anymore) are unsigned. If char defaults to unsigned, it could cause more problems than it solves. We want the compiler to tell us if we're mixing C- strings and data buffers.
Absolutely, but this only comes into play when promoting char to int, as is done with varargs functions, e.g. sprintf(). (There is no such thing as a char argument to a varargs function; all chars are promoted to int, and unsigned chars to unsigned ints.)
One common place this happens is for debug output, when printing the contents of buffers.
<http://www.freshsources.com/1994017B.HTM> is a good page describing the difference between signed and unsigned char.
Chars are unsigned on Linux PowerPC. I don't believe there's anything that depends on chars being signed that doesn't explicitly declare such, as I ran FreeRADIUS on LinuxPPC without issues for a few years. I think all signed-char assumptions were therefore shaken out a fair while ago. ...
To say that chars are unsigned on Linux PPC is meaningless. Whether chars are signed or unsigned is up to the compiler. Now it may be the case that gcc on Linux PPC does treat char as unsigned, but I highly doubt it.
Linux PPC itself may have lots of code that explicitly has unsigned char data, but that doesn't affect application code at all. Even whether or not glibc uses explicit unsigned char internally doesn't affect applications since behavior must still conform to the various standards. Wherever char *'s are used, promotion isn't done so signed vs. unsigned doesn't matter. (And current standards call for void * instead of char * almost everywhere.)
My point is that FR running successfully on Linux PPC, which may use unsigned char everywhere internally (I'll take your word on it) does not imply anything about how char might be treated in application code.
Forcing unsigned char would be a mistake, IMHO. I personally think casts are good. One man's ugly notation is another man's inline documentation.
-frank - List info/subscribe/unsubscribe? See http://www.freeradius.org/ list/devel.html
On Wed, Aug 31, 2005 at 12:38:03PM -0700, Frank Cusack wrote:
On August 31, 2005 4:47:16 PM +1000 Paul TBBle Hampson <Paul.Hampson@Pobox.com> wrote:
Chars are unsigned on Linux PowerPC. I don't believe there's anything that depends on chars being signed that doesn't explicitly declare such, as I ran FreeRADIUS on LinuxPPC without issues for a few years. I think all signed-char assumptions were therefore shaken out a fair while ago. ...
To say that chars are unsigned on Linux PPC is meaningless. Whether chars are signed or unsigned is up to the compiler. Now it may be the case that gcc on Linux PPC does treat char as unsigned, but I highly doubt it.
OK, let me rephrase. gcc and glibc on Linux PPC (specifically tested on Debian) have unsigned chars by default. This affects applications that do not explicitly state their signedness requirements, which luckily FreeRADIUS does, as far as I can tell. (The first thing I did when I started running FreeRADIUS on PowerPC is look for "comparison is always <blah> due to limited range of type" warnings from compiler output.) -- ----------------------------------------------------------- Paul "TBBle" Hampson, MCSE 8th year CompSci/Asian Studies student, ANU The Boss, Bubblesworth Pty Ltd (ABN: 51 095 284 361) Paul.Hampson@Pobox.Com Of course Pacman didn't influence us as kids. If it did, we'd be running around in darkened rooms, popping pills and listening to repetitive music. -- Kristian Wilson, Nintendo, Inc, 1989 License: http://creativecommons.org/licenses/by/2.1/au/ -----------------------------------------------------------
participants (4)
-
Alan DeKok -
Frank Cusack -
Paul TBBle Hampson -
Steven Simon