Hi,
Upon random browsing in the kernel tree I noticed in accel.c:
*a++ = byte_rev[*a]
which isn't 100% correct C AFAIK. At least Stroustrup in his C++ book
warns that this kind of code has to be avoided.
Wrote a script to catch similar things all over the tree (attached).
Found some buglets. Here they are:
drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
---------------------------------------------------
Bad code style. Bad name (sounds like 'module inc').
I can't even tell from this define what the hell it is trying to do:
x++ will return unchanged x, then we obtain (x mod y),
then we store it into x... and why x++ then??!
Alan, seems like you can help here...
drivers/isdn/isdn_audio.c: *buff++ = table[*(unsigned char *)buff];
drivers/video/riva/accel.c: *a++ = byte_rev[*a];
drivers/video/riva/accel.c:/* *a++ = byte_rev[*a];
drivers/video/riva/accel.c: *a++ = byte_rev[*a];*/
drivers/usb/se401.c:
*frame++=(((*frame^255)*(*frame^255))/255)^255;
arch/mips/lib/tinycon.c: *(caddr++) = *(caddr + size_x);
arch/mips/lib/tinycon.c: *(caddr++) = (*caddr & 0xff00) | (unsigned short)
' ';
(btw, tinycon.c seriously needs Lindenting)
------------------------------------------------------------------
Undefined behavior by C std: inc/dec may happen before dereference.
Probably GCC is doing inc after right side eval, but standards say nothing
about it AFAIK. Move ++ out of the statement to be safe:
*a++ = byte_rev[*a]; => *a = byte_rev[*a]; a++;
Patch is attached.
drivers/block/paride/pf.c: if (l==0x20) j--; targ[j]=0;
drivers/block/paride/pg.c: if (l==0x20) j--; targ[j]=0;
drivers/block/paride/pt.c: if (l==0x20) j--; targ[j]=0;
(these files need Lindenting too)
----------
Missing {}
Either a bug or a very bad style (so bad that I can even imagine
that it is NOT a bug). Please double check before applying the patch!
--
vda
======= bad_c.diff =======
diff -u --recursive linux-2.4.13-old/arch/mips/lib/tinycon.c
linux-2.4.13-new/arch/mips/lib/tinycon.c
--- linux-2.4.13-old/arch/mips/lib/tinycon.c Thu Jun 26 17:33:37 1997
+++ linux-2.4.13-new/arch/mips/lib/tinycon.c Wed Nov 21 00:54:05 2001
@@ -83,14 +83,18 @@
register int i;
caddr = vram_addr;
- for(i=0; i<size_x * (size_y-1); i++)
- *(caddr++) = *(caddr + size_x);
+ for(i=0; i<size_x * (size_y-1); i++) {
+ *caddr = *(caddr + size_x);
+ caddr++;
+ }
/* blank last line */
caddr = vram_addr + (size_x * (size_y-1));
- for(i=0; i<size_x; i++)
- *(caddr++) = (*caddr & 0xff00) | (unsigned short) ' ';
+ for(i=0; i<size_x; i++) {
+ *caddr = (*caddr & 0xff00) | (unsigned short) ' ';
+ caddr++;
+ }
}
void print_string(const unsigned char *str)
diff -u --recursive linux-2.4.13-old/drivers/isdn/isdn_audio.c
linux-2.4.13-new/drivers/isdn/isdn_audio.c
--- linux-2.4.13-old/drivers/isdn/isdn_audio.c Sun Sep 30 17:26:06 2001
+++ linux-2.4.13-new/drivers/isdn/isdn_audio.c Wed Nov 21 00:49:54 2001
@@ -227,8 +227,10 @@
: "0"((long) table), "1"(n), "2"((long) buff), "3"((long) buff)
: "memory", "ax");
#else
- while (n--)
- *buff++ = table[*(unsigned char *)buff];
+ while (n--) {
+ *buff = table[*(unsigned char *)buff];
+ buff++;
+ }
#endif
}
diff -u --recursive linux-2.4.13-old/drivers/usb/se401.c
linux-2.4.13-new/drivers/usb/se401.c
--- linux-2.4.13-old/drivers/usb/se401.c Fri Sep 14 19:27:10 2001
+++ linux-2.4.13-new/drivers/usb/se401.c Wed Nov 21 00:52:46 2001
@@ -738,7 +738,8 @@
static inline void enhance_picture(unsigned char *frame, int len)
{
while (len--) {
- *frame++=(((*frame^255)*(*frame^255))/255)^255;
+ *frame = (((*frame^255)*(*frame^255))/255) ^ 255;
+ frame++;
}
}
diff -u --recursive linux-2.4.13-old/drivers/video/riva/accel.c
linux-2.4.13-new/drivers/video/riva/accel.c
--- linux-2.4.13-old/drivers/video/riva/accel.c Mon Oct 15 18:47:13 2001
+++ linux-2.4.13-new/drivers/video/riva/accel.c Wed Nov 21 00:50:32 2001
@@ -108,9 +108,9 @@
static inline void fbcon_reverse_order(u32 *l)
{
u8 *a = (u8 *)l;
- *a++ = byte_rev[*a];
-/* *a++ = byte_rev[*a];
- *a++ = byte_rev[*a];*/
+ *a = byte_rev[*a]; a++;
+/* *a = byte_rev[*a]; a++;
+ *a = byte_rev[*a]; a++; */
*a = byte_rev[*a];
}
======= paride.diff =======
diff -u --recursive linux-2.4.13-old/drivers/block/paride/pf.c
linux-2.4.13-new/drivers/block/paride/pf.c
--- linux-2.4.13-old/drivers/block/paride/pf.c Thu Nov 8 23:42:11 2001
+++ linux-2.4.13-new/drivers/block/paride/pf.c Wed Nov 21 01:00:16 2001
@@ -737,10 +737,13 @@
{ int j,k,l;
j=0; l=0;
- for (k=0;k<len;k++)
- if((buf[k+offs]!=0x20)||(buf[k+offs]!=l))
- l=targ[j++]=buf[k+offs];
- if (l==0x20) j--; targ[j]=0;
+ for(k=0;k<len;k++)
+ if((buf[k+offs]!=0x20) || (buf[k+offs]!=l))
+ l = targ[j++] = buf[k+offs];
+ if(l==0x20) {
+ j--;
+ targ[j]=0;
+ }
}
static int xl( char *buf, int offs )
diff -u --recursive linux-2.4.13-old/drivers/block/paride/pg.c
linux-2.4.13-new/drivers/block/paride/pg.c
--- linux-2.4.13-old/drivers/block/paride/pg.c Thu Nov 8 23:42:11 2001
+++ linux-2.4.13-new/drivers/block/paride/pg.c Wed Nov 21 01:00:25 2001
@@ -488,10 +488,13 @@
{ int j,k,l;
j=0; l=0;
- for (k=0;k<len;k++)
- if((buf[k+offs]!=0x20)||(buf[k+offs]!=l))
- l=targ[j++]=buf[k+offs];
- if (l==0x20) j--; targ[j]=0;
+ for(k=0;k<len;k++)
+ if((buf[k+offs]!=0x20) || (buf[k+offs]!=l))
+ l = targ[j++] = buf[k+offs];
+ if(l==0x20) {
+ j--;
+ targ[j]=0;
+ }
}
static int pg_identify( int unit, int log )
diff -u --recursive linux-2.4.13-old/drivers/block/paride/pt.c
linux-2.4.13-new/drivers/block/paride/pt.c
--- linux-2.4.13-old/drivers/block/paride/pt.c Thu Nov 8 23:42:11 2001
+++ linux-2.4.13-new/drivers/block/paride/pt.c Wed Nov 21 00:59:44 2001
@@ -574,10 +574,13 @@
{ int j,k,l;
j=0; l=0;
- for (k=0;k<len;k++)
- if((buf[k+offs]!=0x20)||(buf[k+offs]!=l))
- l=targ[j++]=buf[k+offs];
- if (l==0x20) j--; targ[j]=0;
+ for(k=0;k<len;k++)
+ if((buf[k+offs]!=0x20) || (buf[k+offs]!=l))
+ l = targ[j++] = buf[k+offs];
+ if(l==0x20) {
+ j--;
+ targ[j]=0;
+ }
}
static int xn( char *buf, int offs, int size )
======= bughunter.sh with some lines wrapped by kmail :-( =======
#!/bin/sh
function do_grep() {
pattern="$1"
shift
for i in $@; do
if test -d "$i"; then
do_grep $pattern $i/*
else
#echo "$i"
#echo grep -E $pattern "$i" /dev/null
grep -E $pattern "$i" /dev/null
fi
done
}
# skip lines from doc dir and with for loops (way too many false alarms)
# *var++ ... *var
do_grep
'[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)\+\+.*[^0-9A-Za-z_]\1[^0-9A-Za-z_]' *
2>/dev/null \
| grep -E '(for)|(Documentation)' -v >!vpp_v 2>&1 &
# *var-- ... *var
do_grep
'[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)--.*[^0-9A-Za-z_]\1[^0-9A-Za-z_]' *
2>/dev/null \
| grep -E '(for)|(Documentation)' -v >!vmm_v 2>&1 &
# *var ... *var++
do_grep
'[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)[^0-9A-Za-z_].*[^0-9A-Za-z_]\1\+\+' *
2>/dev/null \
| grep -E '(for)|(Documentation)' -v >!v_vpp 2>&1 &
# *var ... *var--
do_grep
'[^0-9A-Za-z_]([A-Za-z_][0-9A-Za-z_]*)[^0-9A-Za-z_].*[^0-9A-Za-z_]\1--' *
2>/dev/null \
| grep -E '(for)|(Documentation)' -v >!v_vmm 2>&1 &
vda <[email protected]> writes:
|> Upon random browsing in the kernel tree I noticed in accel.c:
|> *a++ = byte_rev[*a]
|> which isn't 100% correct C AFAIK. At least Stroustrup in his C++ book
|> warns that this kind of code has to be avoided.
This is definitely causing undefined behaviour. AFAIK, gcc 3.1 (current
CVS version) can warn about such errors (-Wsequence-point).
|> ======= bad_c.diff =======
|> diff -u --recursive linux-2.4.13-old/arch/mips/lib/tinycon.c
|> linux-2.4.13-new/arch/mips/lib/tinycon.c
|> --- linux-2.4.13-old/arch/mips/lib/tinycon.c Thu Jun 26 17:33:37 1997
|> +++ linux-2.4.13-new/arch/mips/lib/tinycon.c Wed Nov 21 00:54:05 2001
|> @@ -83,14 +83,18 @@
|> register int i;
|>
|> caddr = vram_addr;
|> - for(i=0; i<size_x * (size_y-1); i++)
|> - *(caddr++) = *(caddr + size_x);
|> + for(i=0; i<size_x * (size_y-1); i++) {
|> + *caddr = *(caddr + size_x);
|> + caddr++;
|> + }
Alternatively you can write:
for(i=0; i<size_x * (size_y-1); i++, caddr++)
*caddr = *(caddr + size_x);
or even:
for(i=0; i<size_x * (size_y-1); i++, caddr++)
*caddr = caddr[size_x];
Andreas.
--
Andreas Schwab "And now for something
[email protected] completely different."
SuSE Labs, SuSE GmbH, Schanz?ckerstr. 10, D-90443 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
On Wed, Nov 21, 2001 at 12:40:17PM +0000, vda wrote:
> drivers/block/paride/pf.c: if (l==0x20) j--; targ[j]=0;
> drivers/block/paride/pg.c: if (l==0x20) j--; targ[j]=0;
> drivers/block/paride/pt.c: if (l==0x20) j--; targ[j]=0;
> (these files need Lindenting too)
> ----------
> Missing {}
> Either a bug or a very bad style (so bad that I can even imagine
> that it is NOT a bug). Please double check before applying the patch!
This seems to be just bad indentation rather than a bug. 'targ[j]=0'
seems to have the purpose of zero-terminating the string, if you look
at the callers of those functions. (But yes, the code is extremely
hard to read, and I can't convince myself either way about whether the
|| two lines earlier should or shouldn't be an &&.)
Tim.
*/
vda wrote:
>
> Hi,
>
> Upon random browsing in the kernel tree I noticed in accel.c:
> *a++ = byte_rev[*a]
> which isn't 100% correct C AFAIK. At least Stroustrup in his C++ book
> warns that this kind of code has to be avoided.
It looks perferctly okay to me. Anyway, whenever would you listen to a
C++ book talking about good C coding :p
> Wrote a script to catch similar things all over the tree (attached).
> Found some buglets. Here they are:
>
> drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
> ---------------------------------------------------
> Bad code style. Bad name (sounds like 'module inc').
> I can't even tell from this define what the hell it is trying to do:
> x++ will return unchanged x, then we obtain (x mod y),
> then we store it into x... and why x++ then??!
> Alan, seems like you can help here...
Go read up on C operator precedence. Unary ++ comes before %, so if we
rewrite the #define to make it more "readable" it would be #define
MODINC(x,y) (x = (x+1) % y)
> drivers/isdn/isdn_audio.c: *buff++ = table[*(unsigned char *)buff];
> drivers/video/riva/accel.c: *a++ = byte_rev[*a];
> drivers/video/riva/accel.c:/* *a++ = byte_rev[*a];
> drivers/video/riva/accel.c: *a++ = byte_rev[*a];*/
> drivers/usb/se401.c:
> *frame++=(((*frame^255)*(*frame^255))/255)^255;
> arch/mips/lib/tinycon.c: *(caddr++) = *(caddr + size_x);
> arch/mips/lib/tinycon.c: *(caddr++) = (*caddr & 0xff00) | (unsigned short)
> ' ';
> (btw, tinycon.c seriously needs Lindenting)
> ------------------------------------------------------------------
> Undefined behavior by C std: inc/dec may happen before dereference.
> Probably GCC is doing inc after right side eval, but standards say nothing
> about it AFAIK. Move ++ out of the statement to be safe:
> *a++ = byte_rev[*a]; => *a = byte_rev[*a]; a++;
C std says *always* evaluate from right to left for = operators, so this
will always make perfect sense.
> Patch is attached.
>
> drivers/block/paride/pf.c: if (l==0x20) j--; targ[j]=0;
> drivers/block/paride/pg.c: if (l==0x20) j--; targ[j]=0;
> drivers/block/paride/pt.c: if (l==0x20) j--; targ[j]=0;
> (these files need Lindenting too)
> ----------
> Missing {}
> Either a bug or a very bad style (so bad that I can even imagine
> that it is NOT a bug). Please double check before applying the patch!
> --
> vda
C std says IFF you have one expression after the for() then you can omit
the {}'s. So this is NOT a bug or bad coding style its just saving some
bytes in the source code :)
Vince.
On Wed, Nov 21, 2001 at 12:40:17PM +0000, vda wrote:
> Hi,
>
> Upon random browsing in the kernel tree I noticed in accel.c:
> *a++ = byte_rev[*a]
> which isn't 100% correct C AFAIK. At least Stroustrup in his C++ book
> warns that this kind of code has to be avoided.
> Wrote a script to catch similar things all over the tree (attached).
If you wanna do this type of cleanup, you can take it one step forward;
remember that the order of evaluation of foo and bar doesn't have to be
{foo => bar} so it can be {bar => foo} I hope gcc's behaviour doesn't
change under our feet.
a = foo (i) + bar (j);
.. sprinkle some pointer arithmetic over there for fun ;)
--
____/| Ragnar H?jland Freedom - Linux - OpenGL | Brainbench MVP
\ o.O| PGP94C4B2F0D27DE025BE2302C104B78C56 B72F0822 | for Unix Programming
=(_)= "Thou shalt not follow the NULL pointer for | (http://www.brainbench.com)
U chaos and madness await thee at its end."
> > *a++ = byte_rev[*a]
> It looks perferctly okay to me. Anyway, whenever would you listen to a
> C++ book talking about good C coding :p
AFAIK the ANSI C specification explicitely claims, that it's not defined.
The trick is, that the specification explicitly allows the compiler to
choose wether it does the inc/dec right after/before the fetch, or at the
begin/end of evaluation. Thus the second reference to a might return the
original or incremented value at compiler's will.
> Go read up on C operator precedence. Unary ++ comes before %, so if we
> rewrite the #define to make it more "readable" it would be #define
> MODINC(x,y) (x = (x+1) % y)
*NO*
MODINC(x,y) (x = (x+1) % y)
is correct and beaves as expected. Unfortunately:
MODINC(x,y) (x = x++ % y)
is a nonsence, because the evaluation is something like this
x++ returns x
x++ % y returns x % y
x is assigned the result and it's incremented IN UNDEFINED ORDER!!!
AFAIK the ANSI C spec explicitly undefines the order.
> > *a++ = byte_rev[*a];
> C std says *always* evaluate from right to left for = operators, so this
> will always make perfect sense.
Yes, this should make perfect sense, but I fear the spec talks about
operand used twice, once with side-efect generaly. So to be ANSI C
correct, it's not good.
-------------------------------------------------------------------------------
- Jan Hudec `Bulb' <[email protected]>
> On Wed, Nov 21, 2001 at 12:40:17PM +0000, vda wrote:
> If you wanna do this type of cleanup, you can take it one step forward;
> remember that the order of evaluation of foo and bar doesn't have to be
> {foo => bar} so it can be {bar => foo} I hope gcc's behaviour doesn't
> change under our feet.
>
> a = foo (i) + bar (j);
>
> .. sprinkle some pointer arithmetic over there for fun ;)
AFAIK here the order *IS* defined. + operator is evaluated left to right
unless operands are known not to have side-efects (in which case it doesn't
matter). Functions are considered not having side-efects iff they are defined
with constant atribute.
--------------------------------------------------------------------------------
- Jan Hudec `Bulb' <[email protected]>
On 21-Nov-2001 Jan Hudec wrote:
>> Go read up on C operator precedence. Unary ++ comes before %, so if we
>> rewrite the #define to make it more "readable" it would be #define
>> MODINC(x,y) (x = (x+1) % y)
>
> *NO*
> MODINC(x,y) (x = (x+1) % y)
> is correct and beaves as expected. Unfortunately:
> MODINC(x,y) (x = x++ % y)
> is a nonsence, because the evaluation is something like this
> x++ returns x
> x++ % y returns x % y
> x is assigned the result and it's incremented IN UNDEFINED ORDER!!!
> AFAIK the ANSI C spec explicitly undefines the order.
in fact, gcc does (according to my tests):
MODINC(x,y) (x = (x % y) + 1)
--
me
Jan Hudec <[email protected]> writes:
|> > On Wed, Nov 21, 2001 at 12:40:17PM +0000, vda wrote:
|> > If you wanna do this type of cleanup, you can take it one step forward;
|> > remember that the order of evaluation of foo and bar doesn't have to be
|> > {foo => bar} so it can be {bar => foo} I hope gcc's behaviour doesn't
|> > change under our feet.
|> >
|> > a = foo (i) + bar (j);
|> >
|> > .. sprinkle some pointer arithmetic over there for fun ;)
|>
|> AFAIK here the order *IS* defined. + operator is evaluated left to right
No. It is undefined which of the operator's arguments is evaluated first,
unless it is defined otherwise (only for ||, && and comma).
Andreas.
--
Andreas Schwab "And now for something
[email protected] completely different."
SuSE Labs, SuSE GmbH, Schanz?ckerstr. 10, D-90443 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
On Wed, 21 Nov 2001, Jan Hudec wrote:
> > > *a++ = byte_rev[*a]
> > It looks perferctly okay to me. Anyway, whenever would you listen to a
> > C++ book talking about good C coding :p
>
It's simple. If any object is modified twice without an intervening
sequence point, the results are undefined. The sequence-point in
*a++ = byte_rev[*a];
... is the ';'.
So, we look at 'a' and see if it's modified twice. It isn't. It
gets modified once with '++'. Now we look at the object to which
'a' points. Is it modified twice? No, it's read once in [*a], and
written once in "*a++ =".
So, it's perfectly good code with a well defined behavior as far as
'C' is concerned. I think it is ugly, however, the writer probably
thought it was beautiful. If somebody went around and fixed all
the ugly code, it would still be ugly in someone else's eyes.
Cheers,
Dick Johnson
Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).
I was going to compile a list of innovations that could be
attributed to Microsoft. Once I realized that Ctrl-Alt-Del
was handled in the BIOS, I found that there aren't any.
Vincent Sweeney <[email protected]> writes:
|> vda wrote:
|> > ------------------------------------------------------------------
|> > Undefined behavior by C std: inc/dec may happen before dereference.
|> > Probably GCC is doing inc after right side eval, but standards say nothing
|> > about it AFAIK. Move ++ out of the statement to be safe:
|> > *a++ = byte_rev[*a]; => *a = byte_rev[*a]; a++;
|>
|> C std says *always* evaluate from right to left for = operators, so this
|> will always make perfect sense.
No. It is undefined which of the operator's arguments is evaluated first,
unless it is defined otherwise (only for ||, && and comma).
Andreas.
--
Andreas Schwab "And now for something
[email protected] completely different."
SuSE Labs, SuSE GmbH, Schanz?ckerstr. 10, D-90443 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
The great thing about the C standard is that you don't have to know, or guess,
or remember. I respectfully suggest that all of those who are interested in
this topic buy a copy of K&R Second Edition (ISBN 0-13-110370-9), and read
chapter 2, particularly section 2.12 "Precedence and order of Evaluation".
And take this facinating topic off-line.
Sean
On Wed, Nov 21, 2001 at 02:37:38PM +0100, Jan Hudec wrote:
> > > *a++ = byte_rev[*a]
> > It looks perferctly okay to me. Anyway, whenever would you listen to a
> > C++ book talking about good C coding :p
>
> AFAIK the ANSI C specification explicitely claims, that it's not defined.
> The trick is, that the specification explicitly allows the compiler to
> choose wether it does the inc/dec right after/before the fetch, or at the
> begin/end of evaluation. Thus the second reference to a might return the
> original or incremented value at compiler's will.
>
> > Go read up on C operator precedence. Unary ++ comes before %, so if we
> > rewrite the #define to make it more "readable" it would be #define
> > MODINC(x,y) (x = (x+1) % y)
>
> *NO*
> MODINC(x,y) (x = (x+1) % y)
> is correct and beaves as expected. Unfortunately:
> MODINC(x,y) (x = x++ % y)
> is a nonsence, because the evaluation is something like this
> x++ returns x
> x++ % y returns x % y
> x is assigned the result and it's incremented IN UNDEFINED ORDER!!!
> AFAIK the ANSI C spec explicitly undefines the order.
>
> > > *a++ = byte_rev[*a];
> > C std says *always* evaluate from right to left for = operators, so this
> > will always make perfect sense.
> Yes, this should make perfect sense, but I fear the spec talks about
> operand used twice, once with side-efect generaly. So to be ANSI C
> correct, it's not good.
>
> -------------------------------------------------------------------------------
> - Jan Hudec `Bulb' <[email protected]>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 21 Nov 2001, Andreas Schwab wrote:
> Jan Hudec <[email protected]> writes:
>
> |> > On Wed, Nov 21, 2001 at 12:40:17PM +0000, vda wrote:
> |> > If you wanna do this type of cleanup, you can take it one step forward;
> |> > remember that the order of evaluation of foo and bar doesn't have to be
> |> > {foo => bar} so it can be {bar => foo} I hope gcc's behaviour doesn't
> |> > change under our feet.
> |> >
> |> > a = foo (i) + bar (j);
> |> >
> |> > .. sprinkle some pointer arithmetic over there for fun ;)
> |>
> |> AFAIK here the order *IS* defined. + operator is evaluated left to right
>
> No. It is undefined which of the operator's arguments is evaluated first,
> unless it is defined otherwise (only for ||, && and comma).
I suspect that real source of problem here is confusion with parsing -
+ operators are _grouped_ left to right. I.e. a+b+c is parsed as (a+b)+c.
Which has nothing to order of evaluation of a, b and c...
"Richard B. Johnson" <[email protected]> writes:
> On Wed, 21 Nov 2001, Jan Hudec wrote:
>
> > > > *a++ = byte_rev[*a]
> > > It looks perferctly okay to me. Anyway, whenever would you listen to a
> > > C++ book talking about good C coding :p
> >
>
> It's simple. If any object is modified twice without an intervening
> sequence point, the results are undefined. The sequence-point in
>
> *a++ = byte_rev[*a];
>
> ... is the ';'.
>
> So, we look at 'a' and see if it's modified twice. It isn't. It
> gets modified once with '++'. Now we look at the object to which
> 'a' points. Is it modified twice? No, it's read once in [*a], and
> written once in "*a++ =".
>
> So, it's perfectly good code with a well defined behavior as far as
> 'C' is concerned.
Nope. In particular it isn't defined if [*a] is evaluated before
or after a++ is evaluated.
> I think it is ugly, however, the writer probably
> thought it was beautiful. If somebody went around and fixed all
> the ugly code, it would still be ugly in someone else's eyes.
True, but not significant. You can find a set of people whose opinion
matters (say core kernel maintainers) and not have that looks ugly in
their eyes. In any case broken code is broken.
Eric
On Wednesday 21 November 2001 14:12, Richard B. Johnson wrote:
> On Wed, 21 Nov 2001, Jan Hudec wrote:
> > > > *a++ = byte_rev[*a]
> > >
> > > It looks perferctly okay to me. Anyway, whenever would you listen to a
> > > C++ book talking about good C coding :p
>
> It's simple. If any object is modified twice without an intervening
> sequence point, the results are undefined. The sequence-point in
>
> *a++ = byte_rev[*a];
>
> ... is the ';'.
>
> So, we look at 'a' and see if it's modified twice. It isn't. It
> gets modified once with '++'. Now we look at the object to which
> 'a' points. Is it modified twice? No, it's read once in [*a], and
> written once in "*a++ =".
The question is, byte_rev[*a] gets fetched before or after a is ++'ed?
As you may see from the length of this thread, one has to think
too much on this tiny piece of code and nevertheless can get it wrong.
Let's change code to be obvious:
*a = byte_rev[*a]; a++;
and noone will have to waste his/her time re-checking validity of it
(or worse, figuring out why kernel breaks with new GCC).
--
vda
On Wed, 21 Nov 2001, Richard B. Johnson wrote:
> On Wed, 21 Nov 2001, Jan Hudec wrote:
>
> > > > *a++ = byte_rev[*a]
> > > It looks perferctly okay to me. Anyway, whenever would you listen to a
> > > C++ book talking about good C coding :p
> >
>
> It's simple. If any object is modified twice without an intervening
> sequence point, the results are undefined.
That's not all. There's another case - when modification and use of
the same object happen in undefined order.
And that's precisely what happens here - same as in case of (x + ++x)
"Richard B. Johnson" <[email protected]> writes:
|> On Wed, 21 Nov 2001, Jan Hudec wrote:
|>
|> > > > *a++ = byte_rev[*a]
|> > > It looks perferctly okay to me. Anyway, whenever would you listen to a
|> > > C++ book talking about good C coding :p
|> >
|>
|> It's simple. If any object is modified twice without an intervening
|> sequence point, the results are undefined. The sequence-point in
|>
|> *a++ = byte_rev[*a];
|>
|> ... is the ';'.
|>
|> So, we look at 'a' and see if it's modified twice.
No, the rule much stricter.
-- Between two sequence points, an object is modified more
than once, or is modified and the prior value is
accessed other than to determine the value to be stored
(6.5).
Andreas.
--
Andreas Schwab "And now for something
[email protected] completely different."
SuSE Labs, SuSE GmbH, Schanz?ckerstr. 10, D-90443 N?rnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
On Wednesday 21 November 2001 13:52, Mathijs Mohlmann wrote:
> On 21-Nov-2001 Jan Hudec wrote:
> >> Go read up on C operator precedence. Unary ++ comes before %, so if we
> >> rewrite the #define to make it more "readable" it would be #define
> >> MODINC(x,y) (x = (x+1) % y)
> >
> > *NO*
> > MODINC(x,y) (x = (x+1) % y)
> > is correct and beaves as expected. Unfortunately:
> > MODINC(x,y) (x = x++ % y)
> > is a nonsence, because the evaluation is something like this
> > x++ returns x
> > x++ % y returns x % y
> > x is assigned the result and it's incremented IN UNDEFINED ORDER!!!
> > AFAIK the ANSI C spec explicitly undefines the order.
>
> in fact, gcc does (according to my tests):
> MODINC(x,y) (x = (x % y) + 1)
drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
Alan, can you clarify what this macro is doing?
What about making it less confusing?
--
vda
>>>>> "Andreas" == Andreas Schwab <[email protected]> writes:
Andreas> "Richard B. Johnson" <[email protected]> writes:
Andreas> |> On Wed, 21 Nov 2001, Jan Hudec wrote:
Andreas> |>
Andreas> |> > > > *a++ = byte_rev[*a]
Andreas> |> > > It looks perferctly okay to me. Anyway, whenever would you listen to a
Andreas> |> > > C++ book talking about good C coding :p
Andreas> |> >
Andreas> |>
Andreas> |> It's simple. If any object is modified twice without an intervening
Andreas> |> sequence point, the results are undefined. The sequence-point in
Andreas> |>
Andreas> |> *a++ = byte_rev[*a];
Andreas> |>
Andreas> |> ... is the ';'.
Andreas> |>
Andreas> |> So, we look at 'a' and see if it's modified twice.
Andreas> No, the rule much stricter.
Andreas> -- Between two sequence points, an object is modified more
Andreas> than once, or is modified and the prior value is
Andreas> accessed other than to determine the value to be stored
Andreas> (6.5).
Hmm, I guess some context is missing here. Let's make it
Between the previous and next sequence point an object shall
have its stored value modified at most once by the evaluation of
an expression. Furthermore, the prior value shall be accessed
only to determine the value to be stored.
So, the above ``*a++ = byte_rev[*a]'' is undefined, because the value
of ``a'' is accessed other than to determine the value to be stored in
``a'' (as Andreas pointed out). The side effect of ``a++'' can take
place anywhere before the next sequence point (``;''), that's before
or after the array access, i.e. the statement can be evaluated as
tmp = *a;
*a++ = byte_rev[tmp];
or as
tmp = *(a+1);
*a++ = byte_rev [tmp];
Regards,
-velco
Jan Hudec wrote:-
> AFAIK here the order *IS* defined. + operator is evaluated left to right
> unless operands are known not to have side-efects (in which case it doesn't
> matter). Functions are considered not having side-efects iff they are defined
> with constant atribute.
Nope, the order of evaluation of arithmetic operands is undefined, and is
often different depending upon the optimization setting.
Note that this is a totally separate issue to both precedence and
associativity.
Neil.
At 12:35 PM 11/21/2001 +0000, Vincent Sweeney wrote:
> > Bad code style. Bad name (sounds like 'module inc').
> > I can't even tell from this define what the hell it is trying to do:
> > x++ will return unchanged x, then we obtain (x mod y),
> > then we store it into x... and why x++ then??!
> > Alan, seems like you can help here...
>
>Go read up on C operator precedence. Unary ++ comes before %, so if we
>rewrite the #define to make it more "readable" it would be #define
>MODINC(x,y) (x = (x+1) % y)
But x++ is postincrement though. That means the value of 'x' is inserted,
and after the expression is evaluated, x is incremented. Right?
If we were going to be semiobscure, wouldn't the correct code be
#define MODINC(x,y) (x = ++x % y)
Btw, to the original guy: this loops 'x' around between 0 and (y-1) --
i.e., if y=5, and x=0, successive "calls" to this #define would do
MODINC(x,y); // x=1
MODINC(x,y); // x=2
MODINC(x,y); // x=3
MODINC(x,y); // x=4
MODINC(x,y); // x=0
MODINC(x,y); // x=1
MODINC(x,y); // x=2
MODINC(x,y); // x=3
...
--
Stevie-O
Real programmers use COPY CON PROGRAM.EXE
Stevie O <[email protected]> said:
> At 12:35 PM 11/21/2001 +0000, Vincent Sweeney wrote:
> > > Bad code style. Bad name (sounds like 'module inc').
> > > I can't even tell from this define what the hell it is trying to do:
> > > x++ will return unchanged x, then we obtain (x mod y),
> > > then we store it into x... and why x++ then??!
> > > Alan, seems like you can help here...
> >
> >Go read up on C operator precedence. Unary ++ comes before %, so if we
> >rewrite the #define to make it more "readable" it would be #define
> >MODINC(x,y) (x = (x+1) % y)
>
> But x++ is postincrement though. That means the value of 'x' is inserted,
> and after the expression is evaluated, x is incremented. Right?
Nope. x++ increments x sometime (not defined when) after taking the value.
x = x++ % y
is wrong: There is just one sequence point at the end of the expression,
and x is modified twice in between (++ and =). If this gives the same as:
x = (x + 1) % y
gcc is smoking potent stuff... but it could legally do anything at all.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
On Thu, 22 Nov 2001, Horst von Brand wrote:
> Stevie O <[email protected]> said:
> > But x++ is postincrement though. That means the value of 'x' is inserted,
> > and after the expression is evaluated, x is incremented. Right?
>
> Nope. x++ increments x sometime (not defined when) after taking the value.
>
> x = x++ % y
>
> is wrong: There is just one sequence point at the end of the expression,
> and x is modified twice in between (++ and =).
Or look at it that way: both
tmp = x % y; x++; x = tmp;
and
tmp = x % y; x = tmp; x++;
are possible interpretations with different results. And as usual, compiler
is allowed to do literally anything whenever it sees such beast.
On 20011122 Stevie O wrote:
>At 12:35 PM 11/21/2001 +0000, Vincent Sweeney wrote:
>> > Bad code style. Bad name (sounds like 'module inc').
>> > I can't even tell from this define what the hell it is trying to do:
>> > x++ will return unchanged x, then we obtain (x mod y),
>> > then we store it into x... and why x++ then??!
>> > Alan, seems like you can help here...
>>
>>Go read up on C operator precedence. Unary ++ comes before %, so if we
>>rewrite the #define to make it more "readable" it would be #define
>>MODINC(x,y) (x = (x+1) % y)
>
>But x++ is postincrement though. That means the value of 'x' is inserted,
>and after the expression is evaluated, x is incremented. Right?
>
>If we were going to be semiobscure, wouldn't the correct code be
>
>#define MODINC(x,y) (x = ++x % y)
>
But the question is: Is this kind of code worth the discussion ? AFAIK,
gcc is enough smart to change *2 to <<1, so wouldn't it be smart to
detect a +1 and use and inc (and perhaps to detect that x is overwritten so it
can do the op in place).
So write it as
x = (x+1)%y
and make it readable.
--
J.A. Magallon # Let the source be with you...
mailto:[email protected]
Mandrake Linux release 8.2 (Cooker) for i586
Linux werewolf 2.4.15-pre9 #1 SMP Thu Nov 22 16:16:54 CET 2001 i686
On Wed, 21 Nov 2001, Vincent Sweeney wrote:
>>
>> drivers/block/paride/pf.c: if (l==0x20) j--; targ[j]=0;
>> drivers/block/paride/pg.c: if (l==0x20) j--; targ[j]=0;
>> drivers/block/paride/pt.c: if (l==0x20) j--; targ[j]=0;
>> (these files need Lindenting too)
>> ----------
>> Missing {} Either a bug or a very bad style (so bad that I can even
>> imagine that it is NOT a bug). Please double check before applying
>> the patch! -- vda
>
> C std says IFF you have one expression after the for() then you can
> omit the {}'s. So this is NOT a bug or bad coding style its just
> saving some bytes in the source code :)
The point here is that what is written as
if(l==0x20) j--; targ[j]=0;
is actually
if(l==0x20)
j--;
targ[j]=0;
and not the
if(l==0x20){
j--;
targ[j] = 0;
}
that it appears to be. I wouldn't like to use 'l' as a variable
either, but that's just me.
Cheers,
Chris
On 20011123 vda wrote:
>On Thursday 22 November 2001 18:08, J.A. Magallon wrote:
>> On 20011122 Stevie O wrote:
>> >If we were going to be semiobscure, wouldn't the correct code be
>> >
>> >#define MODINC(x,y) (x = ++x % y)
>>
>> But the question is: Is this kind of code worth the discussion ? AFAIK,
>
>It worth fixing. That macro is really bad: as you can see, people cannot
That was my point, don't ever try to guess if it does what it is intended
to do, just change it.
--
J.A. Magallon # Let the source be with you...
mailto:[email protected]
Mandrake Linux release 8.2 (Cooker) for i586
Linux werewolf 2.4.15-pre9 #1 SMP Thu Nov 22 16:16:54 CET 2001 i686
> > MODINC(x,y) (x = (x % y) + 1)
>
> drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
>
> Alan, can you clarify what this macro is doing?
> What about making it less confusing?
Nothing to do with me 8). I didnt write that bit of the i2o code. I agree
its both confusing and buggy. Send a fix ?
On Monday 26 November 2001 18:28, Alan Cox wrote:
> > > MODINC(x,y) (x = (x % y) + 1)
> >
> > drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
> >
> > Alan, can you clarify what this macro is doing?
> > What about making it less confusing?
>
> Nothing to do with me 8). I didnt write that bit of the i2o code. I agree
> its both confusing and buggy. Send a fix ?
This is a test to be sure my replacement is equivalent:
--------------------
#include <stdio.h>
#define MODINC(x,y) (x = x++ % y)
#define MODULO_INC(x,y) ((x) = ((x)%(y))+1)
int main() {
int x1=1;
int x2=1;
int y=7;
int i;
for(i=0;i<22;i++) {
printf("%d,%d -> ",x1,x2);
MODINC(x1,y);
MODULO_INC(x2,y);
printf("%d,%d\n",x1,x2);
}
}
Patch is below
--
vda
--- i2o_config.c.new Mon Oct 22 13:39:56 2001
+++ i2o_config.c.orig Tue Nov 27 16:03:19 2001
@@ -45,7 +45,7 @@
static spinlock_t i2o_config_lock = SPIN_LOCK_UNLOCKED;
struct wait_queue *i2o_wait_queue;
-#define MODINC(x,y) (x = x++ % y)
+#define MODULO_INC(x,y) ((x) = ((x)%(y))+1)
struct i2o_cfg_info
{
@@ -144,10 +144,10 @@
inf->event_q[inf->q_in].data_size);
spin_lock(&i2o_config_lock);
- MODINC(inf->q_in, I2O_EVT_Q_LEN);
+ MODULO_INC(inf->q_in, I2O_EVT_Q_LEN);
if(inf->q_len == I2O_EVT_Q_LEN)
{
- MODINC(inf->q_out, I2O_EVT_Q_LEN);
+ MODULO_INC(inf->q_out, I2O_EVT_Q_LEN);
inf->q_lost++;
}
else
@@ -803,7 +803,7 @@
}
memcpy(&kget.info, &p->event_q[p->q_out], sizeof(struct i2o_evt_info));
- MODINC(p->q_out, I2O_EVT_Q_LEN);
+ MODULO_INC(p->q_out, I2O_EVT_Q_LEN);
spin_lock_irqsave(&i2o_config_lock, flags);
p->q_len--;
kget.pending = p->q_len;
On Nov 27, 2001 16:03 -0200, vda wrote:
> On Monday 26 November 2001 18:28, Alan Cox wrote:
> > Nothing to do with me 8). I didnt write that bit of the i2o code. I agree
> > its both confusing and buggy. Send a fix ?
>
> This is a test to be sure my replacement is equivalent:
> --------------------
> #include <stdio.h>
> #define MODINC(x,y) (x = x++ % y)
> #define MODULO_INC(x,y) ((x) = ((x)%(y))+1)
Ugh, clearly the code is broken, so we don't want equivalent code, but
correct code. Use the unambiguous "MODINC(x,y) ((x) = ((x) + 1) % (y))"
form and not "have a bug that has properly defined behaviour under ANSI C".
Just looking at the code, it is fairly clear that the desire is to keep
q_in and q_out >= 0 and < I20_EVT_Q_LEN, which is the size of the event_q
array. With the buggy version, it is possible that you could have q_in or
q_out == I2O_EVT_Q_LEN, which is overflowing the array. Bad, bad, bad.
--- i2o_config.c.new Mon Oct 22 13:39:56 2001
+++ i2o_config.c.orig Tue Nov 27 16:03:19 2001
@@ -45,7 +45,7 @@
static spinlock_t i2o_config_lock = SPIN_LOCK_UNLOCKED;
struct wait_queue *i2o_wait_queue;
-#define MODINC(x,y) (x = x++ % y)
+#define MODINC(x,y) ((x) = ((x) + 1) % (y))
struct i2o_cfg_info
{
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/
vda wrote in http://marc.theaimsgroup.com/?l=linux-kernel&m=100687040003540&w=2:
> On Monday 26 November 2001 18:28, Alan Cox wrote:
> > > > MODINC(x,y) (x = (x % y) + 1)
> > >
> > > drivers/message/i2o/i2o_config.c:#define MODINC(x,y) (x = x++ % y)
> > >
> > > Alan, can you clarify what this macro is doing?
> > > What about making it less confusing?
> >
> > Nothing to do with me 8). I didnt write that bit of the i2o code. I agree
> > its both confusing and buggy. Send a fix ?
>
> This is a test to be sure my replacement is equivalent:
> --------------------
> #include <stdio.h>
> #define MODINC(x,y) (x = x++ % y)
> #define MODULO_INC(x,y) ((x) = ((x)%(y))+1)
> ...
If they really are equivalent, you have certainly found a bug.
Examining the code in i2o_config.c, it is expected that the q_in argument
ranges from 0 to I2O_EVT_Q_LEN-1, but the result of MODINC can never
be 0, and may equal I2O_EVT_Q_LEN, overindexing the array member
event_q and clobbering all the remaining members (including q_in).
The correct fix appears to be
#define MODULO_INC(x,y) ((x) = ((x)+1)%(y))
Nathan Myers
ncm at cantrip dot org
On Tuesday 27 November 2001 16:38, Andreas Dilger wrote:
> On Nov 27, 2001 16:03 -0200, vda wrote:
> > On Monday 26 November 2001 18:28, Alan Cox wrote:
> > > Nothing to do with me 8). I didnt write that bit of the i2o code. I
> > > agree its both confusing and buggy. Send a fix ?
> >
> > This is a test to be sure my replacement is equivalent:
> > --------------------
> > #include <stdio.h>
> > #define MODINC(x,y) (x = x++ % y)
> > #define MODULO_INC(x,y) ((x) = ((x)%(y))+1)
>
> Ugh, clearly the code is broken, so we don't want equivalent code, but
> correct code. Use the unambiguous "MODINC(x,y) ((x) = ((x) + 1) % (y))"
> form and not "have a bug that has properly defined behaviour under ANSI C".
>
> Just looking at the code, it is fairly clear that the desire is to keep
> q_in and q_out >= 0 and < I20_EVT_Q_LEN, which is the size of the event_q
> array. With the buggy version, it is possible that you could have q_in or
> q_out == I2O_EVT_Q_LEN, which is overflowing the array. Bad, bad, bad.
You probably right. I know nothing about what it is intended to do, I just
replaced ugly named nonportable macro with the better named portable one
which is doing the same thing.
If you are confident old macro was also buggy (it yields 1,2,3...N,1,2,3...
and should 0,1,2,...N-1,0,1,2...) please feel free to post corrected patch.
(I suggest changing macro name too as I did)
--
vda
> --- i2o_config.c.new Mon Oct 22 13:39:56 2001
> +++ i2o_config.c.orig Tue Nov 27 16:03:19 2001
> @@ -45,7 +45,7 @@
> static spinlock_t i2o_config_lock = SPIN_LOCK_UNLOCKED;
> struct wait_queue *i2o_wait_queue;
>
> -#define MODINC(x,y) (x = x++ % y)
> +#define MODINC(x,y) ((x) = ((x) + 1) % (y))
>
> struct i2o_cfg_info
> {