Multiple & vs. && and | vs. || bugs in 2.4.20

drivers/usb/acm.c line 243

Quoted 2 lines

drivers/usb/acm.c renamed from drivers/usb/acm.c~ +1 -1

-	if (!urb->status & !acm->throttle)  {
+	if (!urb->status && !acm->throttle)  {

[...]

Observe the extra 'jne' and the fact that the value of 'throttle'
element cannot be loaded until after the conditional jump. Not even
out of order execution can arrange that.

To summarize, I'd probably not be amused if you would change any of my code which takes advantage of such programming finess.

Quoted 2 lines

drivers/usb/acm.c renamed from drivers/usb/acm.c~ +1 -1

-	if (!urb->status & !acm->throttle)  {
+	if (!urb->status && !acm->throttle)  {

Have you really looked at detail at all these cases? The problem is that you have made the code less efficient on some platforms. The use of && requires shortcut evaluation. I.e., the compiler is forced to not

While I agree with your observation in general, this is actually something the compiler should be able to figure out by itself:

- there's only a side-effect if acm is NULL

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

drivers/usb/acm.c line 246

Quoted 11 lines · Norbert Kiesel

drivers/usb/acm.c renamed from drivers/usb/acm.c~ +1 -1

@@ -240,7 +240,7 @@
 	if (urb->status)
 		dbg("nonzero read bulk status received: %d", urb->status);
 
-	if (!urb->status & !acm->throttle)  {
+	if (!urb->status && !acm->throttle)  {
 		for (i = 0; i < urb->actual_length && !acm->throttle; i++) {
 			/* if we insert more than TTY_FLIPBUF_SIZE characters,
 			 * we drop them. */

Have you really looked at detail at all these cases? The problem is that you have made the code less efficient on some platforms. The use of && requires shortcut evaluation. I.e., the compiler is forced to not evaluate !acm->throttle if !urb->status is true. The causes, unless the architecture has condition bits, a conditional jump.

The original code didn't need and normally has no jump and thi specific case was certainly fine before since the result of the ! operator is either 0 or 1 and therefore the & operator has no strange side effects.

As an example, here is how the code for x86 could have looked before

   movl   status(%edx), %edx
   testl  %edx, %edx
   movl   throttle(%eax), %eax
   sete   %dl
   testl  %eax, %eax
   sete   %al
   andb   %dl, %al
   jne    ...

[if code]

After the change the code must look something like this:

   movl   status(%edx), %edx
   testl  %edx, %edx
   jne    ...
   movl   throttle(%eax), %eax
   testl  %eax, %eax
   jne    ...

[if code]

Observe the extra 'jne' and the fact that the value of 'throttle'
element cannot be loaded until after the conditional jump. Not even
out of order execution can arrange that.

To summarize, I'd probably not be amused if you would change any of my code which takes advantage of such programming finess. I would probably have added appropriate comments to explain the code but nevertheless, replacing the more efficient code with some which is easier to understand should probably be considered on a case by case basis. Incorrect branch prediction is costly.

- --
- --------------. ,-. 444 Castro Street
Ulrich Drepper \ ,-----------------' \ Mountain View, CA 94041 USA
Red Hat --' drepper at redhat.com ---------------------------
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.1 (GNU/Linux)

iD8DBQE+Ykf82ijCOnn/RHQRApJaAKCxM4hwu12mJVbQuD3o+t13YVxrsACgsnVH RZmgjNB5KP3Qu27iqpf5aiU=
=l7xl
-----END PGP SIGNATURE-----

What's IMHO more important is that the original code was producing the correct result, so the patch for acm.c is not really necessary. This is also true for the patches for gus_midi.c, gus-wave.c, and i2c-proc.c.

drivers/usb/acm.c line 244

Quoted 4 lines

drivers/usb/acm.c renamed from drivers/usb/acm.c~ +1 -1

-	if (!urb->status & !acm->throttle)  {
+	if (!urb->status && !acm->throttle)  {
 		for (i = 0; i < urb->actual_length && !acm->throttle; i++) {

To summarize, I'd probably not be amused if you would change any of my code which takes advantage of such programming finess. I would probably have added appropriate comments to explain the code but nevertheless, replacing the more efficient code with some which is easier to understand should probably be considered on a case by case basis.

Hi!

Quoted 10 lines

drivers/usb/acm.c line 244

Quoted 4 lines

drivers/usb/acm.c renamed from drivers/usb/acm.c~ +1 -1

-	if (!urb->status & !acm->throttle)  {
+	if (!urb->status && !acm->throttle)  {
 		for (i = 0; i < urb->actual_length && !acm->throttle; i++) {

To summarize, I'd probably not be amused if you would change any of my code which takes advantage of such programming finess. I would probably have added appropriate comments to explain the code but nevertheless, replacing the more efficient code with some which is easier to understand should probably be considered on a case by case basis.

Actually I feel co-responsible for acm.c, and this *is* typo. acm is for modems, thats *not* performance critical, and certainly not worth code obfuscation.