Re: Patch to remove undefined C code
> Yes. In practice the usual question is whether the compiler will > evaluate the operands from left to right or from right to left, > but the compiler is within its rights to evaluate the operands in > any order it wants. For ia64, it would be good to evaluate the operands in parallel. One could do the same with naked Transmeta hardware and other VLIW processors. Tera's machine has cheap hardware threads that could be used. Compaq's 21364 or 21464 may allow this too. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
([EMAIL PROTECTED]) nobly encouraged empiricism thusly... > Just to encourage empiricism, I usually check stuff like that with > > % cat > foo.c > main(){ > int i; > i = 1 , 2; > printf("%d\n",i); > } > % gcc foo.c > % ./a.out > 1 > > or similar if I'm confused. Indeed. Further curiosity is sometimes promptly slaked in a manner such as this... :; cLIeNUX0 /dev/tty4 17:43:31 / :;cc1 main(){ int i; i = 1 , 2; printf("%d\n",i); } :; cLIeNUX0 /dev/tty4 17:43:31 / :; The output of cc1 is not shown in the above. You'll have to play that little ode to the joy of empiricism yourself. Rick Hohensee - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
([EMAIL PROTECTED]) nobly encouraged empiricism thusly... Just to encourage empiricism, I usually check stuff like that with % cat foo.c main(){ int i; i = 1 , 2; printf("%d\n",i); } % gcc foo.c % ./a.out 1 or similar if I'm confused. Indeed. Further curiosity is sometimes promptly slaked in a manner such as this... :; cLIeNUX0 /dev/tty4 17:43:31 / :;cc1 main(){ int i; i = 1 , 2; printf("%d\n",i); } :; cLIeNUX0 /dev/tty4 17:43:31 / :; The output of cc1 is not shown in the above. You'll have to play that little ode to the joy of empiricism yourself. Rick Hohensee - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Yes. In practice the usual question is whether the compiler will evaluate the operands from left to right or from right to left, but the compiler is within its rights to evaluate the operands in any order it wants. For ia64, it would be good to evaluate the operands in parallel. One could do the same with naked Transmeta hardware and other VLIW processors. Tera's machine has cheap hardware threads that could be used. Compaq's 21364 or 21464 may allow this too. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
[CC trimmed] Bill Wendling wrote: > > Also sprach Tom Leete: > } > } You are correct that in C the rightmost argument is always > } at the open end of the stack, and that varargs require that. > } The opposite is called the Pascal convention. > } > Where in the standard does it say this? It's probably done most of the > time in this fashion for convenience, but I don't believe it's in the > standard. > The standard is careful to avoid implementation/ABI issues. It doesn't specify the use of a stack at all. Nonetheless, there exists a C convention for stack based archs, which is as I described. Pascal convention can accomodate varargs by indirection, but that is OT. My point was that stack order of values is independent of the order of evaluation. gcc is neither dumb nor psychic. It will not honor a sequence point which exists only in the programmer's head. Tom - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, Oct 17, 2000 at 07:42:31AM +, Peter Samuelson wrote: > So, if you choose left-to-right, how do you implement varargs? Keep in > mind that prototypes are optional. I bet anything your solution is > slower and more complex than right-to-left, as all known compilers do. Calling a varargs function with no prototype in scope is undefined behavior. For instance, when I designed a C calling convention in a class long ago (for some mips-li[kt]e instruction set), I decided that the first few arguments of normal functions would be passed in registers, but in varargs functions all arguments would be passed on the stack. This is a perfectly legal under the C standard, even though calling printf without a declaration visible would break. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
> [Matti Aarnio] > > > That depends mainly on question: Does your stack grow up or down ? Actually a combination -- which direction do arguments grow in relation to stack growth direction, and do you have a stack push instruction. > [Ben Pfaff] > > No it doesn't. It depends mainly on whether the ABI for the machine > > says that arguments are pushed onto the stack in left-to-right or > > right-to-left order. You could do either on x86, for instance, and > > it has nothing to do with whether the x86 stack grows up or down. No, that's only true if your target supports stack pushes. For instance, gcc on x86/linux will evaluate arguments right-to-left, since it hopes to push arguments to the stack in that order. But gcc on sparc will evaluate arguments mostly left-to-right, even though the arguments are stored on the stack in the same layout as x86. And actually it's more complicated than that, since I believe that on most RISC we'll evaluate the arguments that wind up on the stack first, left-to-right, then the arguments that wind up in registers, left-to-right. On Tue, Oct 17, 2000 at 07:42:31AM +, Peter Samuelson wrote: > So, if you choose left-to-right, how do you implement varargs? Keep in > mind that prototypes are optional. I bet anything your solution is > slower and more complex than right-to-left, as all known compilers do. Speak ye little of "all known compilers". It is possible to implement varargs however you like. Up, down, or sideways, every direction is used by some target somewhere. r~ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, Oct 17, 2000 at 10:40:12AM +0100, Bernd Schmidt wrote: > > I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate > > the assembler code. The old code is 17 instructions long and the new code > > is 11 instructions. As well as being shorter, simple timing test indicate > > that the new code is significantly quicker. > > This is something the compiler ought to know about. Ought to, but we mostly only rewrite addresses like that inside loops. You might be able to convince CSE to massage that sort of thing, but I imagine it would be ugly. It would be cool though -- get everything into direct offset form as much as possible might even make it easier to transform it all back to autoinc if the target supports that sort of thing. r~ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bill Wendling <[EMAIL PROTECTED]> writes: > Also sprach Bernd Schmidt: > } > Looking at the above code, I noticed that there are a lot of ++ > } > operations. I rewrote the code as: > } > > } > setup_from[0] = setup_from[1] = eaddrs[0]; > } > setup_from[2] = setup_from[3] = eaddrs[1]; > } > setup_from[4] = setup_from[5] = eaddrs[2]; > } > setup_from += 6; > } > > } > I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" > } > to generate the assembler code. The old code is 17 instructions > } > long and the new code is 11 instructions. As well as being > } > shorter, simple timing test indicate that the new code is > } > significantly quicker. > } > } This is something the compiler ought to know about. > > A compiler can only guess so much. Don't look to a compiler to fix poorly > written code... While I agree with this sentiment, I'll probably go with what Bernd Schmidt says in this case. He is, after all, the expert on that part of GCC (CSE, loop opts, flow, etc.). - Hari -- Raja R Harinath -- [EMAIL PROTECTED] "When all else fails, read the instructions." -- Cahn's Axiom "Our policy is, when in doubt, do the right thing." -- Roy L Ash - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, 17 Oct 2000, Horst von Brand wrote: > Richard Guenther <[EMAIL PROTECTED]> said: > > The following one is wrong, tho - should be rather > > str[i] = dn[i]; i++; > > > > diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c linux-2.4-fixe > > d/drivers/isdn/sc/debug.c > > > --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 > > > +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 > > > @@ -70,6 +70,6 @@ > > > int i = 0; > > > > > > while(dn[i] != ',') > > > - str[i] = dn[i++]; > > > + str[i] = dn[i], i++; > > > str[i] = 0x0; > > What is wrong with plain and simple: > >for(i = 0; dn[i] != ','; i++) > str[i] = dn[i]; >str[i] = '\0'; Nothing. Feel free to submit a better patch. I was just trying to correct the code with the minimum set of modifications so as to avoid breaking anything by accident. Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Tom Leete: } } You are correct that in C the rightmost argument is always } at the open end of the stack, and that varargs require that. } The opposite is called the Pascal convention. } Where in the standard does it say this? It's probably done most of the time in this fashion for convenience, but I don't believe it's in the standard. -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Bernd Schmidt: } > Looking at the above code, I noticed that there are a lot of ++ } > operations. I rewrote the code as: } > } > setup_from[0] = setup_from[1] = eaddrs[0]; } > setup_from[2] = setup_from[3] = eaddrs[1]; } > setup_from[4] = setup_from[5] = eaddrs[2]; } > setup_from += 6; } > } > I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate } > the assembler code. The old code is 17 instructions long and the new code } > is 11 instructions. As well as being shorter, simple timing test indicate } > that the new code is significantly quicker. } } This is something the compiler ought to know about. } A compiler can only guess so much. Don't look to a compiler to fix poorly written code... -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Mark Montague: } } dn[i], i++ evaluates to i, but str[i] = dn[i], i++ sets str[i] to } dn[i] first, then increments i returning its previous value, which is } discarded. Which was probably specified this way so } } for(i=1,j=2; something; something else) } } works as expected, rather than setting i and j to 2, and discarding } the 1. } Um...it sets i to 1 and j to 2. } Just to encourage empiricism, I usually check stuff like that with } } % cat > foo.c } main(){ } int i; } i = 1 , 2; } printf("%d\n",i); } } } % gcc foo.c } % ./a.out } 1 } } or similar if I'm confused. } This isn't a good test of what's proper according to the standard. If you are testing some undefined part of the standard, a compile may get it "right" in one version but is perfectly within its rights to do something else in the next version. The only way to solve these things is to look at what the standard says and follow it. All else is hopes and wishes. The above code is specified as working in the standard (evaluating the assign and then evaluating the 2). -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt <[EMAIL PROTECTED]> said: > On Tue, 17 Oct 2000, Richard Guenther wrote: > > On Tue, 17 Oct 2000, Bernd Schmidt wrote: > > > On Tue, 17 Oct 2000, Richard Guenther wrote: > > > > The following one is wrong, tho - should be rather > > > > str[i] = dn[i]; i++; > > > > > > Nope. (Well, at least you need to add extra braces.) The comma is a > > > sequence point. > > Umm, I thought, dn[i], i++ evaluates to i and dn[i++] evaluates > > to dn[i], so it should be either i++, dn[i-1] or the one I showed > > above? > According to operator precedence rules, > str[i] = dn[i], i++; > is equivalent to > (str[i] = dn[i]), i++; > Comma has the lowest precedence of all C operators. If knowledgeable C users have doubts on the code's meaning, *rewrite it*. This is just misunderstandings (and bugs) waiting for the unwary. -- Dr. Horst H. von Brand mailto:[EMAIL PROTECTED] Departamento de Informatica Fono: +56 32 654431 Universidad Tecnica Federico Santa Maria +56 32 654239 Casilla 110-V, Valparaiso, ChileFax: +56 32 797513 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Richard Guenther <[EMAIL PROTECTED]> said: > On Mon, 16 Oct 2000, Bernd Schmidt wrote: > The following one is wrong, tho - should be rather > str[i] = dn[i]; i++; > > diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c linux-2.4-fixe > d/drivers/isdn/sc/debug.c > > --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 > > +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 > > @@ -70,6 +70,6 @@ > > int i = 0; > > > > while(dn[i] != ',') > > - str[i] = dn[i++]; > > + str[i] = dn[i], i++; > > str[i] = 0x0; What is wrong with plain and simple: for(i = 0; dn[i] != ','; i++) str[i] = dn[i]; str[i] = '\0'; This one even has a chance of being understood... if you want fancy fooling around, even this is more readable than the original: char *p = dn, *q = str; while((*q++ = *p++) != ',') ; *q = '\0'; Even so, all 3 _will_ break when a ','-less dn comes along... -- Dr. Horst H. von Brand mailto:[EMAIL PROTECTED] Departamento de Informatica Fono: +56 32 654431 Universidad Tecnica Federico Santa Maria +56 32 654239 Casilla 110-V, Valparaiso, ChileFax: +56 32 797513 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Peter Samuelson wrote: > > [Matti Aarnio] > > > That depends mainly on question: Does your stack grow up or down ? > > [Ben Pfaff] > > No it doesn't. It depends mainly on whether the ABI for the machine > > says that arguments are pushed onto the stack in left-to-right or > > right-to-left order. You could do either on x86, for instance, and > > it has nothing to do with whether the x86 stack grows up or down. > > So, if you choose left-to-right, how do you implement varargs? Keep in > mind that prototypes are optional. I bet anything your solution is > slower and more complex than right-to-left, as all known compilers do. > > Peter You are correct that in C the rightmost argument is always at the open end of the stack, and that varargs require that. The opposite is called the Pascal convention. This thread has incorrectly identified the order of expression evaluation with the physical stack layout of the resulting values. The two properties are orthogonal. The compiler is free to evaluate arguments in any order, and knows which stack slot gets each result. For instance, evaluation reordering might be done to optimize cache coherency. It is conceivable that a SMP-aware compiler would evaluate heavyweight arguments in parallel (though currently Linux would not support that). Tom - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Richard Guenther <[EMAIL PROTECTED]> writes: > On Tue, 17 Oct 2000, Bernd Schmidt wrote: > > > On Tue, 17 Oct 2000, Richard Guenther wrote: > > > > > On Mon, 16 Oct 2000, Bernd Schmidt wrote: > > > > > > > I've been playing with some gcc patches to detect code with undefined > > > > behaviour of the i = i++ variety. The patch below fixes all places in > > > > the kernel that I could find. Note that in some cases, it wasn't > > > > entirely clear what the code intended, so I had to guess. > > > > > > > > I haven't tested this patch at all other than to make sure it compiles. > > > > > > The following one is wrong, tho - should be rather > > > str[i] = dn[i]; i++; > > > > Nope. (Well, at least you need to add extra braces.) The comma is a > > sequence point. > > Umm, I thought, dn[i], i++ evaluates to i and dn[i++] evaluates > to dn[i], so it should be either i++, dn[i-1] or the one I showed > above? dn[i], i++ evaluates to i, but str[i] = dn[i], i++ sets str[i] to dn[i] first, then increments i returning its previous value, which is discarded. Which was probably specified this way so for(i=1,j=2; something; something else) works as expected, rather than setting i and j to 2, and discarding the 1. Just to encourage empiricism, I usually check stuff like that with % cat > foo.c main(){ int i; i = 1 , 2; printf("%d\n",i); } % gcc foo.c % ./a.out 1 or similar if I'm confused. - M -- Mark "Monty" Montague | [EMAIL PROTECTED] | I don't do Windows(tm) I'm dubious about any company whose assets can be destroyed by rm -rf http://www.gg.caltech.edu/~monty/monty.shtml> X-PGP-Fingerprint: E4 EA 6D B1 82 46 DB A1 B0 FF 60 B9 F9 5D 5C F7 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, 17 Oct 2000, Richard Guenther wrote: > On Tue, 17 Oct 2000, Bernd Schmidt wrote: > > On Tue, 17 Oct 2000, Richard Guenther wrote: > > > The following one is wrong, tho - should be rather > > > str[i] = dn[i]; i++; > > > > Nope. (Well, at least you need to add extra braces.) The comma is a > > sequence point. > > Umm, I thought, dn[i], i++ evaluates to i and dn[i++] evaluates > to dn[i], so it should be either i++, dn[i-1] or the one I showed > above? According to operator precedence rules, str[i] = dn[i], i++; is equivalent to (str[i] = dn[i]), i++; Comma has the lowest precedence of all C operators. Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, 17 Oct 2000, Bernd Schmidt wrote: > On Tue, 17 Oct 2000, Richard Guenther wrote: > > > On Mon, 16 Oct 2000, Bernd Schmidt wrote: > > > > > I've been playing with some gcc patches to detect code with undefined > > > behaviour of the i = i++ variety. The patch below fixes all places in > > > the kernel that I could find. Note that in some cases, it wasn't > > > entirely clear what the code intended, so I had to guess. > > > > > > I haven't tested this patch at all other than to make sure it compiles. > > > > The following one is wrong, tho - should be rather > > str[i] = dn[i]; i++; > > Nope. (Well, at least you need to add extra braces.) The comma is a > sequence point. Umm, I thought, dn[i], i++ evaluates to i and dn[i++] evaluates to dn[i], so it should be either i++, dn[i-1] or the one I showed above? Richard. > > > diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c >linux-2.4-fixed/drivers/isdn/sc/debug.c > > > --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 > > > +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 > > > @@ -70,6 +70,6 @@ > > > int i = 0; > > > > > > while(dn[i] != ',') > > > - str[i] = dn[i++]; > > > + str[i] = dn[i], i++; > > > str[i] = 0x0; > > > } > > > Bernd > -- Richard Guenther <[EMAIL PROTECTED]> WWW: http://www.anatom.uni-tuebingen.de/~richi/ The GLAME Project: http://www.glame.de/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
> Looking at the above code, I noticed that there are a lot of ++ > operations. I rewrote the code as: > > setup_from[0] = setup_from[1] = eaddrs[0]; > setup_from[2] = setup_from[3] = eaddrs[1]; > setup_from[4] = setup_from[5] = eaddrs[2]; > setup_from += 6; > > I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate > the assembler code. The old code is 17 instructions long and the new code > is 11 instructions. As well as being shorter, simple timing test indicate > that the new code is significantly quicker. This is something the compiler ought to know about. Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, 17 Oct 2000, Helge Hafting wrote: > Bernd Schmidt wrote: > > > > I've been playing with some gcc patches to detect code with undefined > > behaviour of the i = i++ variety. The patch below fixes all places in > > the kernel that I could find. Note that in some cases, it wasn't > > entirely clear what the code intended, so I had to guess. > > Please don't guess. Look at the generated assembly, then make the > unambigous code do whatever the old code did. Chances are high it > worked ok by luck, as misbehaving code tend to get noticed as > it fails. That does seem to be true for all parts except the tulip code. I did look at the assembly where I was in doubt. Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, 17 Oct 2000, Richard Guenther wrote: > On Mon, 16 Oct 2000, Bernd Schmidt wrote: > > > I've been playing with some gcc patches to detect code with undefined > > behaviour of the i = i++ variety. The patch below fixes all places in > > the kernel that I could find. Note that in some cases, it wasn't > > entirely clear what the code intended, so I had to guess. > > > > I haven't tested this patch at all other than to make sure it compiles. > > The following one is wrong, tho - should be rather > str[i] = dn[i]; i++; Nope. (Well, at least you need to add extra braces.) The comma is a sequence point. > > diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c >linux-2.4-fixed/drivers/isdn/sc/debug.c > > --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 > > +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 > > @@ -70,6 +70,6 @@ > > int i = 0; > > > > while(dn[i] != ',') > > - str[i] = dn[i++]; > > + str[i] = dn[i], i++; > > str[i] = 0x0; > > } Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, 16 Oct 2000, Bernd Schmidt wrote: > I've been playing with some gcc patches to detect code with undefined > behaviour of the i = i++ variety. The patch below fixes all places in > the kernel that I could find. Note that in some cases, it wasn't > entirely clear what the code intended, so I had to guess. > > I haven't tested this patch at all other than to make sure it compiles. The following one is wrong, tho - should be rather str[i] = dn[i]; i++; > diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c >linux-2.4-fixed/drivers/isdn/sc/debug.c > --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 > +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 > @@ -70,6 +70,6 @@ > int i = 0; > > while(dn[i] != ',') > - str[i] = dn[i++]; > + str[i] = dn[i], i++; > str[i] = 0x0; > } -- Richard Guenther <[EMAIL PROTECTED]> WWW: http://www.anatom.uni-tuebingen.de/~richi/ The GLAME Project: http://www.glame.de/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt wrote: > > I've been playing with some gcc patches to detect code with undefined > behaviour of the i = i++ variety. The patch below fixes all places in > the kernel that I could find. Note that in some cases, it wasn't > entirely clear what the code intended, so I had to guess. Please don't guess. Look at the generated assembly, then make the unambigous code do whatever the old code did. Chances are high it worked ok by luck, as misbehaving code tend to get noticed as it fails. Helge Hafting - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
[Matti Aarnio] > > That depends mainly on question: Does your stack grow up or down ? [Ben Pfaff] > No it doesn't. It depends mainly on whether the ABI for the machine > says that arguments are pushed onto the stack in left-to-right or > right-to-left order. You could do either on x86, for instance, and > it has nothing to do with whether the x86 stack grows up or down. So, if you choose left-to-right, how do you implement varargs? Keep in mind that prototypes are optional. I bet anything your solution is slower and more complex than right-to-left, as all known compilers do. Peter - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Peter Samuelson wrote: [Matti Aarnio] That depends mainly on question: Does your stack grow up or down ? [Ben Pfaff] No it doesn't. It depends mainly on whether the ABI for the machine says that arguments are pushed onto the stack in left-to-right or right-to-left order. You could do either on x86, for instance, and it has nothing to do with whether the x86 stack grows up or down. So, if you choose left-to-right, how do you implement varargs? Keep in mind that prototypes are optional. I bet anything your solution is slower and more complex than right-to-left, as all known compilers do. Peter You are correct that in C the rightmost argument is always at the open end of the stack, and that varargs require that. The opposite is called the Pascal convention. This thread has incorrectly identified the order of expression evaluation with the physical stack layout of the resulting values. The two properties are orthogonal. The compiler is free to evaluate arguments in any order, and knows which stack slot gets each result. For instance, evaluation reordering might be done to optimize cache coherency. It is conceivable that a SMP-aware compiler would evaluate heavyweight arguments in parallel (though currently Linux would not support that). Tom - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Richard Guenther [EMAIL PROTECTED] said: On Mon, 16 Oct 2000, Bernd Schmidt wrote: The following one is wrong, tho - should be rather str[i] = dn[i]; i++; diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c linux-2.4-fixe d/drivers/isdn/sc/debug.c --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 @@ -70,6 +70,6 @@ int i = 0; while(dn[i] != ',') - str[i] = dn[i++]; + str[i] = dn[i], i++; str[i] = 0x0; What is wrong with plain and simple: for(i = 0; dn[i] != ','; i++) str[i] = dn[i]; str[i] = '\0'; This one even has a chance of being understood... if you want fancy fooling around, even this is more readable than the original: char *p = dn, *q = str; while((*q++ = *p++) != ',') ; *q = '\0'; Even so, all 3 _will_ break when a ','-less dn comes along... -- Dr. Horst H. von Brand mailto:[EMAIL PROTECTED] Departamento de Informatica Fono: +56 32 654431 Universidad Tecnica Federico Santa Maria +56 32 654239 Casilla 110-V, Valparaiso, ChileFax: +56 32 797513 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt [EMAIL PROTECTED] said: On Tue, 17 Oct 2000, Richard Guenther wrote: On Tue, 17 Oct 2000, Bernd Schmidt wrote: On Tue, 17 Oct 2000, Richard Guenther wrote: The following one is wrong, tho - should be rather str[i] = dn[i]; i++; Nope. (Well, at least you need to add extra braces.) The comma is a sequence point. Umm, I thought, dn[i], i++ evaluates to i and dn[i++] evaluates to dn[i], so it should be either i++, dn[i-1] or the one I showed above? According to operator precedence rules, str[i] = dn[i], i++; is equivalent to (str[i] = dn[i]), i++; Comma has the lowest precedence of all C operators. If knowledgeable C users have doubts on the code's meaning, *rewrite it*. This is just misunderstandings (and bugs) waiting for the unwary. -- Dr. Horst H. von Brand mailto:[EMAIL PROTECTED] Departamento de Informatica Fono: +56 32 654431 Universidad Tecnica Federico Santa Maria +56 32 654239 Casilla 110-V, Valparaiso, ChileFax: +56 32 797513 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Mark Montague: } } dn[i], i++ evaluates to i, but str[i] = dn[i], i++ sets str[i] to } dn[i] first, then increments i returning its previous value, which is } discarded. Which was probably specified this way so } } for(i=1,j=2; something; something else) } } works as expected, rather than setting i and j to 2, and discarding } the 1. } Um...it sets i to 1 and j to 2. } Just to encourage empiricism, I usually check stuff like that with } } % cat foo.c } main(){ } int i; } i = 1 , 2; } printf("%d\n",i); } } } % gcc foo.c } % ./a.out } 1 } } or similar if I'm confused. } This isn't a good test of what's proper according to the standard. If you are testing some undefined part of the standard, a compile may get it "right" in one version but is perfectly within its rights to do something else in the next version. The only way to solve these things is to look at what the standard says and follow it. All else is hopes and wishes. The above code is specified as working in the standard (evaluating the assign and then evaluating the 2). -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Bernd Schmidt: } Looking at the above code, I noticed that there are a lot of ++ } operations. I rewrote the code as: } } setup_from[0] = setup_from[1] = eaddrs[0]; } setup_from[2] = setup_from[3] = eaddrs[1]; } setup_from[4] = setup_from[5] = eaddrs[2]; } setup_from += 6; } } I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate } the assembler code. The old code is 17 instructions long and the new code } is 11 instructions. As well as being shorter, simple timing test indicate } that the new code is significantly quicker. } } This is something the compiler ought to know about. } A compiler can only guess so much. Don't look to a compiler to fix poorly written code... -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Tom Leete: } } You are correct that in C the rightmost argument is always } at the open end of the stack, and that varargs require that. } The opposite is called the Pascal convention. } Where in the standard does it say this? It's probably done most of the time in this fashion for convenience, but I don't believe it's in the standard. -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, 17 Oct 2000, Horst von Brand wrote: Richard Guenther [EMAIL PROTECTED] said: The following one is wrong, tho - should be rather str[i] = dn[i]; i++; diff -x log.build -x .* -dru linux-2.4/drivers/isdn/sc/debug.c linux-2.4-fixe d/drivers/isdn/sc/debug.c --- linux-2.4/drivers/isdn/sc/debug.c Thu Apr 2 01:21:04 1998 +++ linux-2.4-fixed/drivers/isdn/sc/debug.c Mon Oct 16 14:53:49 2000 @@ -70,6 +70,6 @@ int i = 0; while(dn[i] != ',') - str[i] = dn[i++]; + str[i] = dn[i], i++; str[i] = 0x0; What is wrong with plain and simple: for(i = 0; dn[i] != ','; i++) str[i] = dn[i]; str[i] = '\0'; Nothing. Feel free to submit a better patch. I was just trying to correct the code with the minimum set of modifications so as to avoid breaking anything by accident. Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bill Wendling [EMAIL PROTECTED] writes: Also sprach Bernd Schmidt: } Looking at the above code, I noticed that there are a lot of ++ } operations. I rewrote the code as: } } setup_from[0] = setup_from[1] = eaddrs[0]; } setup_from[2] = setup_from[3] = eaddrs[1]; } setup_from[4] = setup_from[5] = eaddrs[2]; } setup_from += 6; } } I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" } to generate the assembler code. The old code is 17 instructions } long and the new code is 11 instructions. As well as being } shorter, simple timing test indicate that the new code is } significantly quicker. } } This is something the compiler ought to know about. A compiler can only guess so much. Don't look to a compiler to fix poorly written code... While I agree with this sentiment, I'll probably go with what Bernd Schmidt says in this case. He is, after all, the expert on that part of GCC (CSE, loop opts, flow, etc.). - Hari -- Raja R Harinath -- [EMAIL PROTECTED] "When all else fails, read the instructions." -- Cahn's Axiom "Our policy is, when in doubt, do the right thing." -- Roy L Ash - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, Oct 17, 2000 at 10:40:12AM +0100, Bernd Schmidt wrote: I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate the assembler code. The old code is 17 instructions long and the new code is 11 instructions. As well as being shorter, simple timing test indicate that the new code is significantly quicker. This is something the compiler ought to know about. Ought to, but we mostly only rewrite addresses like that inside loops. You might be able to convince CSE to massage that sort of thing, but I imagine it would be ugly. It would be cool though -- get everything into direct offset form as much as possible might even make it easier to transform it all back to autoinc if the target supports that sort of thing. r~ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Tue, Oct 17, 2000 at 07:42:31AM +, Peter Samuelson wrote: So, if you choose left-to-right, how do you implement varargs? Keep in mind that prototypes are optional. I bet anything your solution is slower and more complex than right-to-left, as all known compilers do. Calling a varargs function with no prototype in scope is undefined behavior. For instance, when I designed a C calling convention in a class long ago (for some mips-li[kt]e instruction set), I decided that the first few arguments of normal functions would be passed in registers, but in varargs functions all arguments would be passed on the stack. This is a perfectly legal under the C standard, even though calling printf without a declaration visible would break. Jeff - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
[CC trimmed] Bill Wendling wrote: Also sprach Tom Leete: } } You are correct that in C the rightmost argument is always } at the open end of the stack, and that varargs require that. } The opposite is called the Pascal convention. } Where in the standard does it say this? It's probably done most of the time in this fashion for convenience, but I don't believe it's in the standard. The standard is careful to avoid implementation/ABI issues. It doesn't specify the use of a stack at all. Nonetheless, there exists a C convention for stack based archs, which is as I described. Pascal convention can accomodate varargs by indirection, but that is OT. My point was that stack order of values is independent of the order of evaluation. gcc is neither dumb nor psychic. It will not honor a sequence point which exists only in the programmer's head. Tom - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Matti Aarnio <[EMAIL PROTECTED]> writes: > On Mon, Oct 16, 2000 at 04:55:08PM -0400, Ben Pfaff wrote: > > Yes. In practice the usual question is whether the compiler will > > evaluate the operands from left to right or from right to left, > > but the compiler is within its rights to evaluate the operands in > > any order it wants. > > That depends mainly on question: Does your stack grow up or down ? No it doesn't. It depends mainly on whether the ABI for the machine says that arguments are pushed onto the stack in left-to-right or right-to-left order. You could do either on x86, for instance, and it has nothing to do with whether the x86 stack grows up or down. > > > Does this imply that even the evaluation of a function pointer > > > is itself undefined in terms of ordering? > > > > Yes. > >For things like lone pointer referral with pre/post inc/dec: > *p++ >it is well defined, but for things like: > *p++ + *p >it is not. (Will the second *p be evaluated before or after *p++ >post-increment ?) Non sequitur: I see no function pointers being evaluated here. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, Oct 16, 2000 at 04:55:08PM -0400, Ben Pfaff wrote: > > > $ The order of evaluation of the function designator, the > > > $ arguments, and subexpressions within the arguments is > > > $ unspecified, ... > > > > I sit surprised and corrected. With every version of every C compiler on > > every OS I have ever used (over 100 combinations from AmigaDOS 1.1 using > > Manx C to E1 using Kai C++) the behavior has been the same for parameter > > lists as for the comma operator in this respect. > > Yes. In practice the usual question is whether the compiler will > evaluate the operands from left to right or from right to left, > but the compiler is within its rights to evaluate the operands in > any order it wants. That depends mainly on question: Does your stack grow up or down ? Machines where stack grows up are a bit rare, but they do exist. The CISC archetype, IBM S/360/370/390 has simple instruction to add a small positive offset value into pointer register, but not to substract it (nor have negative offsets). Thus most stackfull languages there have the stack growing up. Doing downwards-growing stack setup requires 1 extra register use, and one extra instruction per frame entry, plus considerably more for parameter push. (But it was a great beast to run Fortran-IV which didn't need stack, just subfunction invocation record -- at a static location unique for each subfunction --> no recursion supported.) > > Does this imply that even the evaluation of a function pointer > > is itself undefined in terms of ordering? > > Yes. For things like lone pointer referral with pre/post inc/dec: *p++ it is well defined, but for things like: *p++ + *p it is not. (Will the second *p be evaluated before or after *p++ post-increment ?) /Matti Aarnio - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, 16 Oct 2000, Mike Castle wrote: > On Mon, Oct 16, 2000 at 04:47:09PM -0400, Alexander Viro wrote: > > tmp = *p++; > > *q = f(tmp, *p++); > > return p; > > > > is equivalent to more idiomatic > > > > *q = f(p[0], p[1]); > > return p+2; > > > Which gets better assembler out of various versions of gcc? On which platform? If it would be VAX - sure, autoincrement rocks, but for something like x86 I would expect the second form to do better. Besides, it's more readable and is harder to fsck up when you are modifying the code. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, Oct 16, 2000 at 04:47:09PM -0400, Alexander Viro wrote: > tmp = *p++; > *q = f(tmp, *p++); > return p; > > is equivalent to more idiomatic > > *q = f(p[0], p[1]); > return p+2; Which gets better assembler out of various versions of gcc? mrc -- Mike Castle Life is like a clock: You can work constantly [EMAIL PROTECTED] and be right all the time, or not work at all www.netcom.com/~dalgoda/ and be right at least twice a day. -- mrc We are all of us living in the shadow of Manhattan. -- Watchmen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
RE: Patch to remove undefined C code
>-Original Message- >From: Alexander Viro [mailto:[EMAIL PROTECTED]] [snip] > >No arguments here, but proposed fixes were remarkably ugly. Example: > >tmp = *p++; >*q = f(tmp, *p++); >return p; > >is equivalent to more idiomatic > >*q = f(p[0], p[1]); >return p+2; > >And example with copying the string up to the comma... Yuck. Legal C != >decent C. Strongly agree. If the changes were an elegant solution to the ambiguities I never would have cared. In fact the whole reason I bothered to read the patch in the first place was that I feel strongly that something like that _was_ needed, but the solutions really seemed inferior to the original code in terms of elegance of expression. (Well some of the original code was pretty ugly too :-) --Jonathan-- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Jonathan George <[EMAIL PROTECTED]> writes: > > $ The order of evaluation of the function designator, the > > $ arguments, and subexpressions within the arguments is > > $ unspecified, ... > >-- > > I sit surprised and corrected. With every version of every C compiler on > every OS I have ever used (over 100 combinations from AmigaDOS 1.1 using > Manx C to E1 using Kai C++) the behavior has been the same for parameter > lists as for the comma operator in this respect. Yes. In practice the usual question is whether the compiler will evaluate the operands from left to right or from right to left, but the compiler is within its rights to evaluate the operands in any order it wants. > Does this imply that even the evaluation of a function pointer > is itself undefined in terms of ordering? Yes. > >"Premature optimization is the root of all evil." > >--D. E. Knuth, "Structured Programming with go to Statements" > > Absolutely true. Good sigmonster, have $TREAT. > I would be really interested to see how well the kernel compiler does at > optimizing out all of these new tmp variables in terms of the resulting > register utilization and performance. At least an assembler code size > comparison is in order for each of these individual cases. I agree with you and Al Viro that the proposed fixes were less than desirable. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
RE: Patch to remove undefined C code
-Original Message- From: Ben Pfaff [mailto:[EMAIL PROTECTED]] >Jonathan George <[EMAIL PROTECTED]> writes: > >> This patch has many bogus corrections where new variables were created, but >> the order of evaluation is already unambiguous. >> >> For example each comma separated clause in an expression is guaranteed to be >> completely evaluated before the next comma separated clause Including >> Assignments. > >No, that is not true in general. When the comma in question is >the comma operator, it is true. But when the comma separates >arguments to a function, it is not: the order of evaluation of >function arguments is implementation dependent. See C89 section >6.3.2.2 "Function calls": > > $ The order of evaluation of the function designator, the > $ arguments, and subexpressions within the arguments is > $ unspecified, ... >-- I sit surprised and corrected. With every version of every C compiler on every OS I have ever used (over 100 combinations from AmigaDOS 1.1 using Manx C to E1 using Kai C++) the behavior has been the same for parameter lists as for the comma operator in this respect. Does this imply that even the evaluation of a function pointer is itself undefined in terms of ordering? >"Premature optimization is the root of all evil." >--D. E. Knuth, "Structured Programming with go to Statements" Absolutely true. However, optimization of well tested, and algorithmically validated kernel code in a time critical section does not constitute premature optimization as I'm sure Donald would agree. I would be really interested to see how well the kernel compiler does at optimizing out all of these new tmp variables in terms of the resulting register utilization and performance. At least an assembler code size comparison is in order for each of these individual cases. That is my only real concern (other than all of that syntactically ambiguous code I have written over the last 15 years ;-) is that critical performance not be compromised in the kernel. --Jonathan-- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On 16 Oct 2000, Ben Pfaff wrote: > Jonathan George <[EMAIL PROTECTED]> writes: > > > This patch has many bogus corrections where new variables were created, but > > the order of evaluation is already unambiguous. > > > > For example each comma separated clause in an expression is guaranteed to be > > completely evaluated before the next comma separated clause Including > > Assignments. > > No, that is not true in general. When the comma in question is > the comma operator, it is true. But when the comma separates > arguments to a function, it is not: the order of evaluation of > function arguments is implementation dependent. See C89 section > 6.3.2.2 "Function calls": [snip] No arguments here, but proposed fixes were remarkably ugly. Example: tmp = *p++; *q = f(tmp, *p++); return p; is equivalent to more idiomatic *q = f(p[0], p[1]); return p+2; And example with copying the string up to the comma... Yuck. Legal C != decent C. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Jonathan George <[EMAIL PROTECTED]> writes: > This patch has many bogus corrections where new variables were created, but > the order of evaluation is already unambiguous. > > For example each comma separated clause in an expression is guaranteed to be > completely evaluated before the next comma separated clause Including > Assignments. No, that is not true in general. When the comma in question is the comma operator, it is true. But when the comma separates arguments to a function, it is not: the order of evaluation of function arguments is implementation dependent. See C89 section 6.3.2.2 "Function calls": $ The order of evaluation of the function designator, the $ arguments, and subexpressions within the arguments is $ unspecified, ... -- "Premature optimization is the root of all evil." --D. E. Knuth, "Structured Programming with go to Statements" - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Abramo Bagnara: } } Isn't this more efficient? } n = (x>>32) | (x<<32); } n = ((n & 0xLL)<<16) | (n & 0xLL)>>16; } n = ((n & 0x00ff00ff00ff00ffLL)<<8) | (n & 0xff00ff00ff00ff00LL)>>8; } } 6 shift } 4 and } 3 or } Plus 3 assigns...but they may get optimized out. :) } instead of } } 8 shift } 8 and } 7 or } -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
At 01:21 PM 10/16/00, Jeff Garzik wrote: >Bernd Schmidt wrote: > > diff -x log.build -x .* -dru linux-2.4/drivers/net/tulip/tulip_core.c > linux-2.4-fixed/drivers/net/tulip/tulip_core.c > > --- linux-2.4/drivers/net/tulip/tulip_core.cMon Oct 16 13:51:23 2000 > > +++ linux-2.4-fixed/drivers/net/tulip/tulip_core.c Mon Oct 16 > 15:40:12 2000 >[...] > > @@ -944,9 +946,9 @@ > > > > /* Fill the final entry with our physical address. */ > > eaddrs = (u16 *)dev->dev_addr; > > - *setup_frm++ = *setup_frm++ = eaddrs[0]; > > - *setup_frm++ = *setup_frm++ = eaddrs[1]; > > - *setup_frm++ = *setup_frm++ = eaddrs[2]; > > + *setup_frm++ = eaddrs[0]; *setup_frm++ = eaddrs[0]; > > + *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1]; > > + *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2]; > > > > spin_lock_irqsave(>lock, flags); > > Looking at the above code, I noticed that there are a lot of ++ operations. I rewrote the code as: setup_from[0] = setup_from[1] = eaddrs[0]; setup_from[2] = setup_from[3] = eaddrs[1]; setup_from[4] = setup_from[5] = eaddrs[2]; setup_from += 6; I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate the assembler code. The old code is 17 instructions long and the new code is 11 instructions. As well as being shorter, simple timing test indicate that the new code is significantly quicker. David David Relson Osage Software Systems, Inc. [EMAIL PROTECTED] 514 W. Keech Ave. www.osagesoftware.com Ann Arbor, MI 48103 voice: 734.821.8800fax: 734.821.8800 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt wrote: > > + > +#define ___swab64(x) \ > +({ \ > + __u64 __x = (x); \ > + ((__u64)( \ > + (__u64)(((__u64)(__x) & (__u64)0x00ffULL) << 56) | \ > + (__u64)(((__u64)(__x) & (__u64)0xff00ULL) << 40) | \ > + (__u64)(((__u64)(__x) & (__u64)0x00ffULL) << 24) | \ > + (__u64)(((__u64)(__x) & (__u64)0xff00ULL) << 8) | \ > + (__u64)(((__u64)(__x) & (__u64)0x00ffULL) >> 8) | \ > + (__u64)(((__u64)(__x) & (__u64)0xff00ULL) >> 24) | \ > + (__u64)(((__u64)(__x) & (__u64)0x00ffULL) >> 40) | \ > + (__u64)(((__u64)(__x) & (__u64)0xff00ULL) >> 56) )); \ > +}) Isn't this more efficient? n = (x>>32) | (x<<32); n = ((n & 0xLL)<<16) | (n & 0xLL)>>16; n = ((n & 0x00ff00ff00ff00ffLL)<<8) | (n & 0xff00ff00ff00ff00LL)>>8; 6 shift 4 and 3 or instead of 8 shift 8 and 7 or -- Abramo Bagnara mailto:[EMAIL PROTECTED] Opera Unica Via Emilia Interna, 140 Phone: +39.0546.656023 48014 Castel Bolognese (RA) - Italy Fax: +39.0546.656023 ALSA project ishttp://www.alsa-project.org sponsored by SuSE Linuxhttp://www.suse.com It sounds good! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, 16 Oct 2000, Jeff Garzik wrote: > > --- linux-2.4/drivers/scsi/aha152x.cMon Oct 16 13:51:24 2000 > > +++ linux-2.4-fixed/drivers/scsi/aha152x.c Mon Oct 16 14:51:29 2000 > > @@ -1280,7 +1280,8 @@ > > scsi_unregister(shpnt); > > registered_count--; > > release_region(shpnt->io_port, IO_RANGE); > > - aha152x_host[shpnt->irq - IRQ_MIN] = shpnt = 0; > > + aha152x_host[shpnt->irq - IRQ_MIN] = 0; > > + shpnt = 0; > > continue; > > } > > Why this change? aha152x_host[shpnt->irq - IRQ_MIN] = shpnt = 0; reference set Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt wrote: > diff -x log.build -x .* -dru linux-2.4/drivers/net/tulip/tulip_core.c >linux-2.4-fixed/drivers/net/tulip/tulip_core.c > --- linux-2.4/drivers/net/tulip/tulip_core.cMon Oct 16 13:51:23 2000 > +++ linux-2.4-fixed/drivers/net/tulip/tulip_core.c Mon Oct 16 15:40:12 2000 [...] > @@ -944,9 +946,9 @@ > > /* Fill the final entry with our physical address. */ > eaddrs = (u16 *)dev->dev_addr; > - *setup_frm++ = *setup_frm++ = eaddrs[0]; > - *setup_frm++ = *setup_frm++ = eaddrs[1]; > - *setup_frm++ = *setup_frm++ = eaddrs[2]; > + *setup_frm++ = eaddrs[0]; *setup_frm++ = eaddrs[0]; > + *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1]; > + *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2]; > > spin_lock_irqsave(>lock, flags); > Linus/Bernd, please drop this patch to drivers/net/tulip/tulip_core.c, I have a much better change from prumpf for this already in my CVS... > --- linux-2.4/drivers/scsi/aha152x.cMon Oct 16 13:51:24 2000 > +++ linux-2.4-fixed/drivers/scsi/aha152x.c Mon Oct 16 14:51:29 2000 > @@ -1280,7 +1280,8 @@ > scsi_unregister(shpnt); > registered_count--; > release_region(shpnt->io_port, IO_RANGE); > - aha152x_host[shpnt->irq - IRQ_MIN] = shpnt = 0; > + aha152x_host[shpnt->irq - IRQ_MIN] = 0; > + shpnt = 0; > continue; > } Why this change? Other than these two things, the changes look ok. -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt wrote: diff -x log.build -x .* -dru linux-2.4/drivers/net/tulip/tulip_core.c linux-2.4-fixed/drivers/net/tulip/tulip_core.c --- linux-2.4/drivers/net/tulip/tulip_core.cMon Oct 16 13:51:23 2000 +++ linux-2.4-fixed/drivers/net/tulip/tulip_core.c Mon Oct 16 15:40:12 2000 [...] @@ -944,9 +946,9 @@ /* Fill the final entry with our physical address. */ eaddrs = (u16 *)dev-dev_addr; - *setup_frm++ = *setup_frm++ = eaddrs[0]; - *setup_frm++ = *setup_frm++ = eaddrs[1]; - *setup_frm++ = *setup_frm++ = eaddrs[2]; + *setup_frm++ = eaddrs[0]; *setup_frm++ = eaddrs[0]; + *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1]; + *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2]; spin_lock_irqsave(tp-lock, flags); Linus/Bernd, please drop this patch to drivers/net/tulip/tulip_core.c, I have a much better change from prumpf for this already in my CVS... --- linux-2.4/drivers/scsi/aha152x.cMon Oct 16 13:51:24 2000 +++ linux-2.4-fixed/drivers/scsi/aha152x.c Mon Oct 16 14:51:29 2000 @@ -1280,7 +1280,8 @@ scsi_unregister(shpnt); registered_count--; release_region(shpnt-io_port, IO_RANGE); - aha152x_host[shpnt-irq - IRQ_MIN] = shpnt = 0; + aha152x_host[shpnt-irq - IRQ_MIN] = 0; + shpnt = 0; continue; } Why this change? Other than these two things, the changes look ok. -- Jeff Garzik| The difference between laziness and Building 1024 | prioritization is the end result. MandrakeSoft | - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, 16 Oct 2000, Jeff Garzik wrote: --- linux-2.4/drivers/scsi/aha152x.cMon Oct 16 13:51:24 2000 +++ linux-2.4-fixed/drivers/scsi/aha152x.c Mon Oct 16 14:51:29 2000 @@ -1280,7 +1280,8 @@ scsi_unregister(shpnt); registered_count--; release_region(shpnt-io_port, IO_RANGE); - aha152x_host[shpnt-irq - IRQ_MIN] = shpnt = 0; + aha152x_host[shpnt-irq - IRQ_MIN] = 0; + shpnt = 0; continue; } Why this change? aha152x_host[shpnt-irq - IRQ_MIN] = shpnt = 0; reference set Bernd - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Bernd Schmidt wrote: + +#define ___swab64(x) \ +({ \ + __u64 __x = (x); \ + ((__u64)( \ + (__u64)(((__u64)(__x) (__u64)0x00ffULL) 56) | \ + (__u64)(((__u64)(__x) (__u64)0xff00ULL) 40) | \ + (__u64)(((__u64)(__x) (__u64)0x00ffULL) 24) | \ + (__u64)(((__u64)(__x) (__u64)0xff00ULL) 8) | \ + (__u64)(((__u64)(__x) (__u64)0x00ffULL) 8) | \ + (__u64)(((__u64)(__x) (__u64)0xff00ULL) 24) | \ + (__u64)(((__u64)(__x) (__u64)0x00ffULL) 40) | \ + (__u64)(((__u64)(__x) (__u64)0xff00ULL) 56) )); \ +}) Isn't this more efficient? n = (x32) | (x32); n = ((n 0xLL)16) | (n 0xLL)16; n = ((n 0x00ff00ff00ff00ffLL)8) | (n 0xff00ff00ff00ff00LL)8; 6 shift 4 and 3 or instead of 8 shift 8 and 7 or -- Abramo Bagnara mailto:[EMAIL PROTECTED] Opera Unica Via Emilia Interna, 140 Phone: +39.0546.656023 48014 Castel Bolognese (RA) - Italy Fax: +39.0546.656023 ALSA project ishttp://www.alsa-project.org sponsored by SuSE Linuxhttp://www.suse.com It sounds good! - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
At 01:21 PM 10/16/00, Jeff Garzik wrote: Bernd Schmidt wrote: diff -x log.build -x .* -dru linux-2.4/drivers/net/tulip/tulip_core.c linux-2.4-fixed/drivers/net/tulip/tulip_core.c --- linux-2.4/drivers/net/tulip/tulip_core.cMon Oct 16 13:51:23 2000 +++ linux-2.4-fixed/drivers/net/tulip/tulip_core.c Mon Oct 16 15:40:12 2000 [...] @@ -944,9 +946,9 @@ /* Fill the final entry with our physical address. */ eaddrs = (u16 *)dev-dev_addr; - *setup_frm++ = *setup_frm++ = eaddrs[0]; - *setup_frm++ = *setup_frm++ = eaddrs[1]; - *setup_frm++ = *setup_frm++ = eaddrs[2]; + *setup_frm++ = eaddrs[0]; *setup_frm++ = eaddrs[0]; + *setup_frm++ = eaddrs[1]; *setup_frm++ = eaddrs[1]; + *setup_frm++ = eaddrs[2]; *setup_frm++ = eaddrs[2]; spin_lock_irqsave(tp-lock, flags); Looking at the above code, I noticed that there are a lot of ++ operations. I rewrote the code as: setup_from[0] = setup_from[1] = eaddrs[0]; setup_from[2] = setup_from[3] = eaddrs[1]; setup_from[4] = setup_from[5] = eaddrs[2]; setup_from += 6; I compiled using "gcc -S -Wall -O2 -fomit-frame-pointer -m486" to generate the assembler code. The old code is 17 instructions long and the new code is 11 instructions. As well as being shorter, simple timing test indicate that the new code is significantly quicker. David David Relson Osage Software Systems, Inc. [EMAIL PROTECTED] 514 W. Keech Ave. www.osagesoftware.com Ann Arbor, MI 48103 voice: 734.821.8800fax: 734.821.8800 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Also sprach Abramo Bagnara: } } Isn't this more efficient? } n = (x32) | (x32); } n = ((n 0xLL)16) | (n 0xLL)16; } n = ((n 0x00ff00ff00ff00ffLL)8) | (n 0xff00ff00ff00ff00LL)8; } } 6 shift } 4 and } 3 or } Plus 3 assigns...but they may get optimized out. :) } instead of } } 8 shift } 8 and } 7 or } -- || Bill Wendling[EMAIL PROTECTED] - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Jonathan George [EMAIL PROTECTED] writes: This patch has many bogus corrections where new variables were created, but the order of evaluation is already unambiguous. For example each comma separated clause in an expression is guaranteed to be completely evaluated before the next comma separated clause Including Assignments. No, that is not true in general. When the comma in question is the comma operator, it is true. But when the comma separates arguments to a function, it is not: the order of evaluation of function arguments is implementation dependent. See C89 section 6.3.2.2 "Function calls": $ The order of evaluation of the function designator, the $ arguments, and subexpressions within the arguments is $ unspecified, ... -- "Premature optimization is the root of all evil." --D. E. Knuth, "Structured Programming with go to Statements" - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On 16 Oct 2000, Ben Pfaff wrote: Jonathan George [EMAIL PROTECTED] writes: This patch has many bogus corrections where new variables were created, but the order of evaluation is already unambiguous. For example each comma separated clause in an expression is guaranteed to be completely evaluated before the next comma separated clause Including Assignments. No, that is not true in general. When the comma in question is the comma operator, it is true. But when the comma separates arguments to a function, it is not: the order of evaluation of function arguments is implementation dependent. See C89 section 6.3.2.2 "Function calls": [snip] No arguments here, but proposed fixes were remarkably ugly. Example: tmp = *p++; *q = f(tmp, *p++); return p; is equivalent to more idiomatic *q = f(p[0], p[1]); return p+2; And example with copying the string up to the comma... Yuck. Legal C != decent C. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
RE: Patch to remove undefined C code
-Original Message- From: Ben Pfaff [mailto:[EMAIL PROTECTED]] Jonathan George [EMAIL PROTECTED] writes: This patch has many bogus corrections where new variables were created, but the order of evaluation is already unambiguous. For example each comma separated clause in an expression is guaranteed to be completely evaluated before the next comma separated clause Including Assignments. No, that is not true in general. When the comma in question is the comma operator, it is true. But when the comma separates arguments to a function, it is not: the order of evaluation of function arguments is implementation dependent. See C89 section 6.3.2.2 "Function calls": $ The order of evaluation of the function designator, the $ arguments, and subexpressions within the arguments is $ unspecified, ... -- I sit surprised and corrected. With every version of every C compiler on every OS I have ever used (over 100 combinations from AmigaDOS 1.1 using Manx C to E1 using Kai C++) the behavior has been the same for parameter lists as for the comma operator in this respect. Does this imply that even the evaluation of a function pointer is itself undefined in terms of ordering? "Premature optimization is the root of all evil." --D. E. Knuth, "Structured Programming with go to Statements" Absolutely true. However, optimization of well tested, and algorithmically validated kernel code in a time critical section does not constitute premature optimization as I'm sure Donald would agree. I would be really interested to see how well the kernel compiler does at optimizing out all of these new tmp variables in terms of the resulting register utilization and performance. At least an assembler code size comparison is in order for each of these individual cases. That is my only real concern (other than all of that syntactically ambiguous code I have written over the last 15 years ;-) is that critical performance not be compromised in the kernel. --Jonathan-- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
RE: Patch to remove undefined C code
-Original Message- From: Alexander Viro [mailto:[EMAIL PROTECTED]] [snip] No arguments here, but proposed fixes were remarkably ugly. Example: tmp = *p++; *q = f(tmp, *p++); return p; is equivalent to more idiomatic *q = f(p[0], p[1]); return p+2; And example with copying the string up to the comma... Yuck. Legal C != decent C. Strongly agree. If the changes were an elegant solution to the ambiguities I never would have cared. In fact the whole reason I bothered to read the patch in the first place was that I feel strongly that something like that _was_ needed, but the solutions really seemed inferior to the original code in terms of elegance of expression. (Well some of the original code was pretty ugly too :-) --Jonathan-- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Jonathan George [EMAIL PROTECTED] writes: $ The order of evaluation of the function designator, the $ arguments, and subexpressions within the arguments is $ unspecified, ... -- I sit surprised and corrected. With every version of every C compiler on every OS I have ever used (over 100 combinations from AmigaDOS 1.1 using Manx C to E1 using Kai C++) the behavior has been the same for parameter lists as for the comma operator in this respect. Yes. In practice the usual question is whether the compiler will evaluate the operands from left to right or from right to left, but the compiler is within its rights to evaluate the operands in any order it wants. Does this imply that even the evaluation of a function pointer is itself undefined in terms of ordering? Yes. "Premature optimization is the root of all evil." --D. E. Knuth, "Structured Programming with go to Statements" Absolutely true. Good sigmonster, have $TREAT. I would be really interested to see how well the kernel compiler does at optimizing out all of these new tmp variables in terms of the resulting register utilization and performance. At least an assembler code size comparison is in order for each of these individual cases. I agree with you and Al Viro that the proposed fixes were less than desirable. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, Oct 16, 2000 at 04:47:09PM -0400, Alexander Viro wrote: tmp = *p++; *q = f(tmp, *p++); return p; is equivalent to more idiomatic *q = f(p[0], p[1]); return p+2; Which gets better assembler out of various versions of gcc? mrc -- Mike Castle Life is like a clock: You can work constantly [EMAIL PROTECTED] and be right all the time, or not work at all www.netcom.com/~dalgoda/ and be right at least twice a day. -- mrc We are all of us living in the shadow of Manhattan. -- Watchmen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, 16 Oct 2000, Mike Castle wrote: On Mon, Oct 16, 2000 at 04:47:09PM -0400, Alexander Viro wrote: tmp = *p++; *q = f(tmp, *p++); return p; is equivalent to more idiomatic *q = f(p[0], p[1]); return p+2; Which gets better assembler out of various versions of gcc? On which platform? If it would be VAX - sure, autoincrement rocks, but for something like x86 I would expect the second form to do better. Besides, it's more readable and is harder to fsck up when you are modifying the code. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
On Mon, Oct 16, 2000 at 04:55:08PM -0400, Ben Pfaff wrote: $ The order of evaluation of the function designator, the $ arguments, and subexpressions within the arguments is $ unspecified, ... I sit surprised and corrected. With every version of every C compiler on every OS I have ever used (over 100 combinations from AmigaDOS 1.1 using Manx C to E1 using Kai C++) the behavior has been the same for parameter lists as for the comma operator in this respect. Yes. In practice the usual question is whether the compiler will evaluate the operands from left to right or from right to left, but the compiler is within its rights to evaluate the operands in any order it wants. That depends mainly on question: Does your stack grow up or down ? Machines where stack grows up are a bit rare, but they do exist. The CISC archetype, IBM S/360/370/390 has simple instruction to add a small positive offset value into pointer register, but not to substract it (nor have negative offsets). Thus most stackfull languages there have the stack growing up. Doing downwards-growing stack setup requires 1 extra register use, and one extra instruction per frame entry, plus considerably more for parameter push. (But it was a great beast to run Fortran-IV which didn't need stack, just subfunction invocation record -- at a static location unique for each subfunction -- no recursion supported.) Does this imply that even the evaluation of a function pointer is itself undefined in terms of ordering? Yes. For things like lone pointer referral with pre/post inc/dec: *p++ it is well defined, but for things like: *p++ + *p it is not. (Will the second *p be evaluated before or after *p++ post-increment ?) /Matti Aarnio - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/
Re: Patch to remove undefined C code
Matti Aarnio [EMAIL PROTECTED] writes: On Mon, Oct 16, 2000 at 04:55:08PM -0400, Ben Pfaff wrote: Yes. In practice the usual question is whether the compiler will evaluate the operands from left to right or from right to left, but the compiler is within its rights to evaluate the operands in any order it wants. That depends mainly on question: Does your stack grow up or down ? No it doesn't. It depends mainly on whether the ABI for the machine says that arguments are pushed onto the stack in left-to-right or right-to-left order. You could do either on x86, for instance, and it has nothing to do with whether the x86 stack grows up or down. Does this imply that even the evaluation of a function pointer is itself undefined in terms of ordering? Yes. For things like lone pointer referral with pre/post inc/dec: *p++ it is well defined, but for things like: *p++ + *p it is not. (Will the second *p be evaluated before or after *p++ post-increment ?) Non sequitur: I see no function pointers being evaluated here. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] Please read the FAQ at http://www.tux.org/lkml/