Re: [PATCH] Multiple & vs. && and | vs. || bugs in 2.4.20

[Norbert Kiesel <nkiesel@tbdnetworks.com>]

[-- Attachment #1: Type: text/plain, Size: 3575 bytes --]

Hi,
yes I agree that the code will be slightly less efficient in this case
(though looking a bit around, there are way more places which could be
optimized, e.g. the useless "&& !acm->throttle" in the next line). 
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.

OTOH, I still think that in most (all?) of this cases the bit-op was not
intended.

Anyway, I'll wait another day or two to collect replies from the
maintainers and then resend the remaining patches where the code really
produces wrong results.

so long
	Norbert

On Sun, 2003-03-02 at 10:05, Ulrich Drepper wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Norbert Kiesel wrote:
> 
> > --- linux-2.4.20/drivers/usb/acm.c~	2002-12-03 00:17:50.000000000 -0800
> > +++ linux-2.4.20/drivers/usb/acm.c	2003-03-02 03:03:34.000000000 -0800
> > @@ -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-----
-- 
Norbert Kiesel <nkiesel@tbdnetworks.com>

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]