Re: egcs -O breaks ping.c:in_cksum()
On Tue, Nov 16, 1999 at 03:17:43PM +1100, Bruce Evans wrote:
On Mon, 15 Nov 1999, Pierre Beyssac wrote:
- volatile u_short answer = 0;
+ union {
+ u_int16_t us;
+ u_int8_t uc[2];
+ } answer;
This has indentation bugs.
Uh, which one(s) do you mean exactly? The 4-space indented union
(I just followed style(9)) or the double space before uc[2] (it
was just to align us and uc vertically)?
ping.c still assumes that u_short is u_int16_t everywhere else.
But in_cksum() is more or less self-contained. Probably it's more
consistent (even withing in_cksum which uses u_short elsewhere) to
change back the union to u_short and u_char, though.
This `answer' variable has nothing to do with the final `answer' variable.
The latter should not be a union. The original code apparently reuses
`answer' to do manual register allocation for ancient compilers.
Agreed.
Perhaps the above should be written as:
sum += ntohs(*(u_char *)w 8);
to avoid the undefined union access (answer.us).
Uh... I'm not sure I don't prefer the union, actually :-)
--
Pierre Beyssac [EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Tue, 16 Nov 1999 09:45:36 +0100, Pierre Beyssac wrote:
- volatile u_short answer = 0;
+ union {
+ u_int16_t us;
+ u_int8_t uc[2];
+ } answer;
Uh, which one(s) do you mean exactly? The 4-space indented union
(I just followed style(9))
The word ``union'' doesn't appear in style(9) and a 1 tab indent is used
consistently in the examples of structs. Use 1 tab.
or the double space before uc[2] (it was just to align us and uc
vertically)?
Use tabs for that as well.
Look at the rest of ping.c, and you'll see that 4-space indents aren't
used except to prevent line-wrap in one weird case of a switch block and
for run-over lines.
Ciao,
Sheldon.
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Tue, Nov 16, 1999 at 10:58:06AM +0200, Sheldon Hearn wrote: The word ``union'' doesn't appear in style(9) and a 1 tab indent is used consistently in the examples of structs. Use 1 tab. Right, I reread style(9) and I apparently misunderstood the following part which only applies to code (mainly inside a statement): Indentation is an 8 character tab. Second level indents are four spaces. -- Pierre Beyssac [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Tue, 16 Nov 1999, Pierre Beyssac wrote:
On Tue, Nov 16, 1999 at 03:17:43PM +1100, Bruce Evans wrote:
On Mon, 15 Nov 1999, Pierre Beyssac wrote:
- volatile u_short answer = 0;
+ union {
+ u_int16_t us;
+ u_int8_t uc[2];
+ } answer;
This has indentation bugs.
Uh, which one(s) do you mean exactly? The 4-space indented union
(I just followed style(9)) or the double space before uc[2] (it
was just to align us and uc vertically)?
See Sheldon's reply. u_int16_t and u_int8_t are too wide for the
normal indentation rules to apply. Various inconsistent formattings
are used for them. E.g., in Lite2, ufs/ufs/dir.h uses an extra space
in one struct and an extra tab in the others. This is another reason
to use u_short :-).
ping.c still assumes that u_short is u_int16_t everywhere else.
But in_cksum() is more or less self-contained. Probably it's more
consistent (even withing in_cksum which uses u_short elsewhere) to
change back the union to u_short and u_char, though.
There are better examples to copy in the kernel. They still use too
many shorts, ints and union hacks, however. The alpha version is
most interesting.
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Mon, Nov 15, 1999 at 05:59:23PM -0800, Kris Kennaway wrote: On Tue, 16 Nov 1999, Pierre Beyssac wrote: I've checked, the answer is no: apparently, in_cksum() in routed/rdisc.c is only called in two places, both with an even size. Can it hurt to pre-emptively fix it anyway in case some future change pulls the rug out from underneath? We could, but since the danger is purely theoretical for now (and probably will stay that way forever), I don't see any advantage in cluttering up the code. Since routed is sometimes sync'ed from external sources, it would only make life harder for the people doing the merges. Plus, everyone steals in_cksum from ping, not from routed (at least, that's what I do :-) Since in_cksum is used in several places (there's another optimized copy in libstand), a cleaner solution would be to put it in some library. -- Pierre Beyssac [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
In message [EMAIL PROTECTED], Pierre Beyssac writes: Since in_cksum is used in several places (there's another optimized copy in libstand), a cleaner solution would be to put it in some library. Isn't there one in libalias already ? -- Poul-Henning Kamp FreeBSD coreteam member [EMAIL PROTECTED] "Real hackers run -current on their laptop." FreeBSD -- It will take a long time before progress goes too far! To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Tue, Nov 16, 1999 at 07:29:35PM +0100, Poul-Henning Kamp wrote: In message [EMAIL PROTECTED], Pierre Beyssac writes: Since in_cksum is used in several places (there's another optimized Isn't there one in libalias already ? Right. I missed it because it's called PacketAliasInternetChecksum()... -- Pierre Beyssac [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
egcs -O breaks ping.c:in_cksum()
I've discovered the following problem, either due to egcs or the
source code for in_cksum in ping, I'm not sure.
The symptom is that in_cksum() returns an invalid result on an
odd-size buffer, when compile optimization is on.
You can check this by trying "ping -s 65 localhost" and seeing that
no reply is ever received. The kernel ICMP bad checksum count just
increases.
The problem is apparently due to the following code fragment:
register u_short answer = 0;
[...]
/* mop up an odd byte, if necessary */
if (nleft == 1) {
*(u_char *)(answer) = *(u_char *)w ;
sum += answer;
}
Removing the "register" declaration for 'answer' doesn't help.
OTOH, adding "volatile" does help and seems to fix the problem.
Only I'm not sure that's the correct fix because I'm unsure about
the exact semantics of "volatile"; it might well be an egcs bug.
Attached is a test program if anyone wishes to experiment.
Try to compile with and without -O and see the difference.
The correct output is "cksum=f9f6", the wrong output is "cksum=f5f6".
--
Pierre Beyssac [EMAIL PROTECTED]
#include sys/types.h
#include stdio.h
/*
* in_cksum --
* Checksum routine for Internet Protocol family headers (C Version)
*/
u_short
in_cksum(addr, len)
u_short *addr;
int len;
{
register int nleft = len;
register u_short *w = addr;
int sum = 0;
volatile u_short answer = 0;
/*
* Our algorithm is simple, using a 32 bit accumulator (sum), we add
* sequential 16 bit words to it, and at the end, fold back all the
* carry bits from the top 16 bits into the lower 16 bits.
*/
while (nleft 1) {
sum += *w++;
nleft -= 2;
}
/* mop up an odd byte, if necessary */
if (nleft == 1) {
*(u_char *)(answer) = *(u_char *)w ;
sum += answer;
}
/* add back carry outs from top 16 bits to low 16 bits */
sum = (sum 16) + (sum 0x); /* add hi 16 to low 16 */
sum += (sum 16); /* add carry */
answer = ~sum; /* truncate to 16 bits */
return(answer);
}
int main()
{
unsigned char tb[] = { 1, 2, 3, 4, 5 };
printf("cksum=%04x\n", in_cksum(tb, sizeof tb));
}
Re: egcs -O breaks ping.c:in_cksum()
On Mon, 15 Nov 1999 17:48:31 +0100, Pierre Beyssac wrote: I've discovered the following problem, either due to egcs or the source code for in_cksum in ping, I'm not sure. Alas, you're not in virgin territory, Columbus. :-) See PR 13292. Ciao, Sheldon. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Mon, Nov 15, 1999 at 06:52:23PM +0200, Sheldon Hearn wrote: On Mon, 15 Nov 1999 17:48:31 +0100, Pierre Beyssac wrote: I've discovered the following problem, either due to egcs or the source code for in_cksum in ping, I'm not sure. See PR 13292. Wow, Thanks! August 21th, it's not really new... Maybe I can at least commit the addition of "volatile" to the source code. That will work around that particular bug until egcs is fixed... That doesn't say how many occurences of similar code there are in the rest of the system, of course. -- Pierre Beyssac [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Mon, 15 Nov 1999 18:01:45 +0100, Pierre Beyssac wrote: Maybe I can at least commit the addition of "volatile" to the source code. That will work around that particular bug until egcs is fixed... FWIW, the newly committed gcc-2.95.2 doesn't "fix" the problem. Ciao, Sheldon. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
Maybe I can at least commit the addition of "volatile" to the source code. That will work around that particular bug until egcs is fixed... FWIW, the newly committed gcc-2.95.2 doesn't "fix" the problem. Are you sure? GCC-2.95.2 seems OK here. Dima To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Mon, 15 Nov 1999 18:01:45 +0100, Pierre Beyssac [EMAIL PROTECTED] said: Maybe I can at least commit the addition of "volatile" to the source code. That will work around that particular bug until egcs is fixed... It's not a compiler bug, it's a source code bug. The C Language specifies that pointers to distinct types can be assumed, under certain conditions, never to alias one another. (This might actually be a C99 feature as opposed to C89.) Recent values of GCC make use of this obscure language feature to improve optimization. Essentially, the optimizer can assume that stores through a pointer of type `foo *' will never modify any local variable which is not of type `foo' (and thus those values can remain cached in registers). If, rather than casting pointers, the code used a union (containing one u_int16_t and one array[2] of u_int8_t), the compiler would have enough information to know about the aliases. -GAWollman -- Garrett A. Wollman | O Siem / We are all family / O Siem / We're all the same [EMAIL PROTECTED] | O Siem / The fires of freedom Opinions not those of| Dance in the burning flame MIT, LCS, CRS, or NSA| - Susan Aglukark and Chad Irschick To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Mon, Nov 15, 1999 at 01:35:15PM -0500, Garrett Wollman wrote:
If, rather than casting pointers, the code used a union (containing
one u_int16_t and one array[2] of u_int8_t), the compiler would have
enough information to know about the aliases.
You're right, this seems to work even with optimization turned on.
If nobody objects, I'll commit it.
--- ck.c.oldMon Nov 15 19:41:35 1999
+++ ck.cMon Nov 15 19:39:43 1999
@@ -13,7 +13,10 @@
register int nleft = len;
register u_short *w = addr;
int sum = 0;
- volatile u_short answer = 0;
+ union {
+ u_int16_t us;
+ u_int8_t uc[2];
+ } answer;
/*
* Our algorithm is simple, using a 32 bit accumulator (sum), we add
@@ -27,15 +30,16 @@
/* mop up an odd byte, if necessary */
if (nleft == 1) {
- *(u_char *)(answer) = *(u_char *)w ;
- sum += answer;
+ answer.uc[0] = *(u_char *)w ;
+ answer.uc[1] = 0;
+ sum += answer.us;
}
/* add back carry outs from top 16 bits to low 16 bits */
sum = (sum 16) + (sum 0x); /* add hi 16 to low 16 */
sum += (sum 16); /* add carry */
- answer = ~sum; /* truncate to 16 bits */
- return(answer);
+ answer.us = ~sum; /* truncate to 16 bits */
+ return(answer.us);
}
int main()
--
Pierre Beyssac [EMAIL PROTECTED]
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
:It's not a compiler bug, it's a source code bug. : :The C Language specifies that pointers to distinct types can be :assumed, under certain conditions, never to alias one another. (This :... :Recent values of GCC make use of this obscure language feature to :improve optimization. Essentially, the optimizer can assume that :... :-GAWollman : :-- :Garrett A. Wollman | O Siem / We are all family / O Siem / We're all the same Someone try the patch below and tell me if it fixes the problem. If it does then I'll commit it. If someone else wants to commit a 'better' fix, be my guest! (but inform the list that you've done so). Otherwise this is the one that will go in. -Matt Index: ping.c === RCS file: /home/ncvs/src/sbin/ping/ping.c,v retrieving revision 1.45 diff -u -r1.45 ping.c --- ping.c 1999/08/28 00:13:59 1.45 +++ ping.c 1999/11/15 19:26:23 @@ -920,6 +920,9 @@ /* * in_cksum -- * Checksum routine for Internet Protocol family headers (C Version) + * + * note: volatilization of 'answer' is a bad hack to work around an + * aliasing problem. */ u_short in_cksum(addr, len) @@ -929,7 +932,7 @@ register int nleft = len; register u_short *w = addr; register int sum = 0; - u_short answer = 0; + volatile u_short answer = 0; /* * Our algorithm is simple, using a 32 bit accumulator (sum), we add To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
[in_cksum bugs] Fix committed for ping. There's another bug in sbin/routed/rdisc.c:in_cksum() on odd packet sizes, albeit I'm not sure it's ever triggered (does routed ever generate odd-size packets?). It's a portability bug (works only on little-endian machines). I'll commit the same fix if there's no objection. -- Pierre Beyssac [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
[in_cksum bugs] There's another bug in sbin/routed/rdisc.c:in_cksum() on odd packet sizes, albeit I'm not sure it's ever triggered (does routed ever generate odd-size packets?). I've checked, the answer is no: apparently, in_cksum() in routed/rdisc.c is only called in two places, both with an even size. -- Pierre Beyssac[EMAIL PROTECTED] [EMAIL PROTECTED] BSD : il y a moins bien, mais c'est coté en bourse Free domains: http://www.eu.org/ or mail [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Tue, 16 Nov 1999, Pierre Beyssac wrote: [in_cksum bugs] There's another bug in sbin/routed/rdisc.c:in_cksum() on odd packet sizes, albeit I'm not sure it's ever triggered (does routed ever generate odd-size packets?). I've checked, the answer is no: apparently, in_cksum() in routed/rdisc.c is only called in two places, both with an even size. Can it hurt to pre-emptively fix it anyway in case some future change pulls the rug out from underneath? Kris Cthulhu for President! For when you're tired of choosing the _lesser_ of two evils.. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: egcs -O breaks ping.c:in_cksum()
On Mon, 15 Nov 1999, Pierre Beyssac wrote:
On Mon, Nov 15, 1999 at 01:35:15PM -0500, Garrett Wollman wrote:
If, rather than casting pointers, the code used a union (containing
one u_int16_t and one array[2] of u_int8_t), the compiler would have
enough information to know about the aliases.
You're right, this seems to work even with optimization turned on.
If nobody objects, I'll commit it.
--- ck.c.old Mon Nov 15 19:41:35 1999
+++ ck.c Mon Nov 15 19:39:43 1999
@@ -13,7 +13,10 @@
register int nleft = len;
register u_short *w = addr;
int sum = 0;
- volatile u_short answer = 0;
+ union {
+ u_int16_t us;
+ u_int8_t uc[2];
+ } answer;
This has indentation bugs.
ping.c still assumes that u_short is u_int16_t everywhere else.
/*
* Our algorithm is simple, using a 32 bit accumulator (sum), we add
@@ -27,15 +30,16 @@
/* mop up an odd byte, if necessary */
if (nleft == 1) {
- *(u_char *)(answer) = *(u_char *)w ;
- sum += answer;
+ answer.uc[0] = *(u_char *)w ;
+ answer.uc[1] = 0;
+ sum += answer.us;
This `answer' variable has nothing to do with the final `answer' variable.
The latter should not be a union. The original code apparently reuses
`answer' to do manual register allocation for ancient compilers.
Perhaps the above should be written as:
sum += ntohs(*(u_char *)w 8);
to avoid the undefined union access (answer.us). I think this works
on all systems, but it is a pessimisation on some little-endian systems
including i386's (on i386's, ntohs() is inline, but it is inline asm
so the compiler can't see that it just reverses the shift).
Bruce
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
