Re: Semaphore assembly-code bug

[Linus Torvalds]

On Fri, 29 Oct 2004, linux-os wrote:
> 
> Linus, please check this out.

Yes, I concur. However, I'd suggest changing the "addl $4,%esp" into a 
"popl %ecx", which is smaller and apparently faster on some CPU's (ecx 
obviously gets immediately overwritten by the next popl).

Btw, this is another case where we _really_ want "asmlinkage" to mean that
the compiler does not own the argument stack. Is there any chance of
getting a function attribute like that into future versions of gcc?  
Richard, Jan, Andi? Or does it already exist somewhere?

		Linus

--- saved for gcc people commentary ---
>
> asmlinkage void __up(struct semaphore *sem)
> {
>      wake_up(&sem->wait);
> }
> 
> This was from /usr/src/linux-2.6.9/arch/i386/kernel/semaphore.c
> It this case, the value of 'sem' is destroyed which means that
> certain assembly-language helper functions no longer work.
> 
> This was discovered by Aleksey Gorelov <Aleksey_Gorelov@xxxxxxxxxxx>.
> 
> This patch fixes it, but I think somebody may need to rework
> the semaphore code to eliminate the assembly because the newer
> compilers are not consistent in their handling of passed parameters
> so some assembly optimization may no longer be possible.
> 
> 
> --- linux-2.6.9/arch/i386/kernel/semaphore.c.orig	2004-08-14 01:36:56.000000000 -0400
> +++ linux-2.6.9/arch/i386/kernel/semaphore.c	2004-10-19 08:06:15.000000000 -0400
> @@ -198,9 +198,11 @@
>   #endif
>   	"pushl %eax\n\t"
>   	"pushl %edx\n\t"
> -	"pushl %ecx\n\t"
> +	"pushl %ecx\n\t"	// Register to save
> +	"pushl %ecx\n\t"	// Passed parameter
>   	"call __down\n\t"
> -	"popl %ecx\n\t"
> +	"addl $0x04, %esp\t\n"	// Bypass corrupted parameter
> +	"popl %ecx\n\t"		// Restore original
>   	"popl %edx\n\t"
>   	"popl %eax\n\t"
>   #if defined(CONFIG_FRAME_POINTER)
> @@ -220,9 +222,11 @@
>   	"movl  %esp,%ebp\n\t"
>   #endif
>   	"pushl %edx\n\t"
> -	"pushl %ecx\n\t"
> +	"pushl %ecx\n\t"	// Save register
> +	"pushl %ecx\n\t"	// Passed parameter
>   	"call __down_interruptible\n\t"
> -	"popl %ecx\n\t"
> +	"addl $0x04, %esp\n\t"	// Bypass corrupted parameter
> +	"popl %ecx\n\t"		// Restore register
>   	"popl %edx\n\t"
>   #if defined(CONFIG_FRAME_POINTER)
>   	"movl %ebp,%esp\n\t"
> @@ -241,9 +245,11 @@
>   	"movl  %esp,%ebp\n\t"
>   #endif
>   	"pushl %edx\n\t"
> -	"pushl %ecx\n\t"
> +	"pushl %ecx\n\t"		// Save register
> +	"pushl %ecx\n\t"		// Passed parameter
>   	"call __down_trylock\n\t"
> -	"popl %ecx\n\t"
> +	"addl $0x04, %esp\n\t"		// Bypass corrupted parameter
> +	"popl %ecx\n\t"			// Restore register
>   	"popl %edx\n\t"
>   #if defined(CONFIG_FRAME_POINTER)
>   	"movl %ebp,%esp\n\t"
> @@ -259,9 +265,11 @@
>   "__up_wakeup:\n\t"
>   	"pushl %eax\n\t"
>   	"pushl %edx\n\t"
> -	"pushl %ecx\n\t"
> +	"pushl %ecx\n\t"	// Save register
> +	"pushl %ecx\n\t"	// Passed parameter
>   	"call __up\n\t"
> -	"popl %ecx\n\t"
> +	"addl $0x04, %esp\n\t"	// Bypass corrupted parameter
> +	"popl %ecx\n\t"		// Restore register
>   	"popl %edx\n\t"
>   	"popl %eax\n\t"
>   	"ret"
> 
> 
> Cheers,
> Dick Johnson
> Penguin : Linux version 2.6.9 on an i686 machine (5537.79 BogoMips).
>   Notice : All mail here is now cached for review by John Ashcroft.
>                   98.36% of all statistics are fiction.
> 
-
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/