RE: [BUG mm] "fixed" i386 memcpy inlining buggy

[Dave Korn]

----Original Message----
>From: Denis Vlasenko
>Sent: 06 April 2005 11:14

  Is this someone's idea of an April Fool's joke?  Because if it is, I've
suffered a serious sense-of-humour failure.

> Oh shit. I was trying to be too clever. I still run with this patch,
> so it must be happening very rarely.

  The kernel is way too important for cross-your-fingers-and-hope
engineering techniques to be applied.  This patch should never have been
permitted.  How on earth could anything like this hope to make it through a
strict review?

> Does this one compile ok?

> 	{
> 		/* load esi/edi */
> 		__asm__ __volatile__(
> 			""
> 			: "=&D" (edi), "=&S" (esi)
> 			: "0" ((long) to),"1" ((long) from)
> 			: "memory"
> 		);
> 	}
> 	if (n >= 5*4) {
> 		/* large block: use rep prefix */
> 		int ecx;
> 		__asm__ __volatile__(
> 			"rep ; movsl"
> 			: "=&c" (ecx), "=&D" (edi), "=&S" (esi)
> 			: "0" (n/4), "1" (edi),"2" (esi)
> 			: "memory"
> 		);


  It doesn't matter if it compiles or not, it's still *utterly* invalid.
You can NOT make assumptions about registers keeping their values between
one asm block and another.  Immediately after the closing quote of the first
asm, the compiler can do ANYTHING IT WANTS and to just _hope_ that it won't
use the registers you want is voodoo programming.  Even if it works when you
try it once, there are zero guarantees that another version or revision of
the compiler or even just a tiny change to the source that affects the
behaviour of the scheduler when compiling the function won't produce
something completely different, meaning that this code is appallingly
fragile.  This code should be completely discarded and rewritten properly.


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/