lkml.org 
[lkml]   [2004]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: Semaphore assembly-code bug


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@Phoenix.com>.
>
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2009-11-18 23:46    [W:1.208 / U:0.760 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site