x86: Clean up stack access code in irq_32.c
PATCH 1/1 x86: Clean up stack access code in irq_32.c
Use C instead of asm for accessing the stack pointer. And define some macros to make the code easier to understand.
arch/x86/include/asm/page_32_types.h +2 -0
@@ -19,6 +19,8 @@#define THREAD_SIZE_ORDER 1#define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) +#define THREAD_SIZE_MASK (THREAD_SIZE - 1) +#define CURRENT_MASK (~THREAD_SIZE_MASK)#define STACKFAULT_STACK 0#define DOUBLEFAULT_STACK 1
arch/x86/include/asm/page_64_types.h +3 -2
@@ -2,8 +2,9 @@#define _ASM_X86_PAGE_64_DEFS_H#define THREAD_SIZE_ORDER 2 -#define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) -#define CURRENT_MASK (~(THREAD_SIZE - 1)) +#define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) +#define THREAD_SIZE_MASK (THREAD_SIZE - 1) +#define CURRENT_MASK (~THREAD_SIZE_MASK)#define EXCEPTION_STACK_ORDER 0#define EXCEPTION_STKSZ (PAGE_SIZE << EXCEPTION_STACK_ORDER)
arch/x86/kernel/irq_32.c +9 -14
@@ -27,6 +27,12 @@ EXPORT_PER_CPU_SYMBOL(irq_stat); DEFINE_PER_CPU(struct pt_regs *, irq_regs);EXPORT_PER_CPU_SYMBOL (irq_regs); +/* how to get the current stack pointer from C */ +#define current_stack_pointer ({ \ + register unsigned long sp asm("esp"); \ + sp; \ +}) +#ifdef CONFIG_DEBUG_STACKOVERFLOW int sysctl_panic_on_stackoverflow__read_mostly ;
@@ -34,12 +40,8 @@ int sysctl_panic_on_stackoverflow __read_mostly;/* Debugging check for stack overflow: is there less than 1KB free? */ static int check_stack_overflow (void ) { -long sp; - - __asm__ __volatile__("andl %%esp,%0" : -"=r" (sp) :"0" (THREAD_SIZE -1 )); - -return sp < (sizeof (struct thread_info ) +STACK_WARN ); +return (current_stack_pointer &THREAD_SIZE_MASK ) + <sizeof (struct thread_info ) +STACK_WARN ; }static void print_stack_overflow (void )
@@ -69,16 +71,9 @@ static void call_on_stack(void *func, void *stack) : "memory", "cc", "edx", "ecx", "eax"); } -/* how to get the current stack pointer from C */ -#define current_stack_pointer ({ \ - unsigned long sp; \ - asm("mov %%esp,%0" : "=g" (sp)); \ - sp; \ -}) -static inline void *current_stack (void ) { -return (void *)(current_stack_pointer & ~(THREAD_SIZE -1 )); +return (void *)(current_stack_pointer &CURRENT_MASK ); } static inline int
H. Peter Anvin
reviewerWe changed this to asm because the C broke some compilers. Why are you changing it back?
Quoted 86 lines · Chuck Ebbert
Use C instead of asm for accessing the stack pointer. And define some macros to make the code easier to understand.
Signed-off-by Chuck Ebbert <[email protected]>arch/x86/include/asm/page_32_types.h +2
-0@@ -19,6 +19,8 @@#define THREAD_SIZE_ORDER 1#define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) +#define THREAD_SIZE_MASK (THREAD_SIZE - 1) +#define CURRENT_MASK (~THREAD_SIZE_MASK)#define STACKFAULT_STACK 0#define DOUBLEFAULT_STACK 1arch/x86/include/asm/page_64_types.h +3
-2@@ -2,8 +2,9 @@#define _ASM_X86_PAGE_64_DEFS_H#define THREAD_SIZE_ORDER 2 -#define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) -#define CURRENT_MASK (~(THREAD_SIZE - 1)) +#define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) +#define THREAD_SIZE_MASK (THREAD_SIZE - 1) +#define CURRENT_MASK (~THREAD_SIZE_MASK)#define EXCEPTION_STACK_ORDER 0#define EXCEPTION_STKSZ (PAGE_SIZE << EXCEPTION_STACK_ORDER)arch/x86/kernel/irq_32.c +9
-14@@ -27,6 +27,12 @@ EXPORT_PER_CPU_SYMBOL(irq_stat); DEFINE_PER_CPU(struct pt_regs *, irq_regs);EXPORT_PER_CPU_SYMBOL (irq_regs); +/* how to get the current stack pointer from C */ +#define current_stack_pointer ({ \ + register unsigned long sp asm("esp"); \ + sp; \ +}) +#ifdef CONFIG_DEBUG_STACKOVERFLOW int sysctl_panic_on_stackoverflow__read_mostly ;@@ -34,12 +40,8 @@ int sysctl_panic_on_stackoverflow __read_mostly;/* Debugging check for stack overflow: is there less than 1KB free? */ static int check_stack_overflow (void ) { -long sp; - - __asm__ __volatile__("andl %%esp,%0" : -"=r" (sp) :"0" (THREAD_SIZE -1 )); - -return sp < (sizeof (struct thread_info ) +STACK_WARN ); +return (current_stack_pointer &THREAD_SIZE_MASK ) + <sizeof (struct thread_info ) +STACK_WARN ; }static void print_stack_overflow (void )@@ -69,16 +71,9 @@ static void call_on_stack(void *func, void *stack) : "memory", "cc", "edx", "ecx", "eax"); } -/* how to get the current stack pointer from C */ -#define current_stack_pointer ({ \ - unsigned long sp; \ - asm("mov %%esp,%0" : "=g" (sp)); \ - sp; \ -}) -static inline void *current_stack (void ) { -return (void *)(current_stack_pointer & ~(THREAD_SIZE -1 )); +return (void *)(current_stack_pointer &CURRENT_MASK ); }static inline int
Signature · 1 line
Sent from my mobile phone. Please pardon brevity and lack of formatting.
On Sun, 12 Oct 2014 09:47:53 -0700
"H. Peter Anvin" <[email protected]> wrote:
[replying to the list this time, sigh]
Quoted 2 lines
We changed this to asm because the C broke some compilers. Why are you changing it back?
The C broke some compilers because it was using a global register variable. This is a local one, which the clang documentation says is supported. And I compiled it with clang with no problem.
Quoted 4 lines · Chuck Ebbert
Quoted 3 lines
Use C instead of asm for accessing the stack pointer. And define some macros to make the code easier to understand.
H. Peter Anvin
reviewerThat's not a justification for change. Claiming no harm is nevessary but not sufficient.
Quoted 19 lines · Chuck Ebbert
On Sun, 12 Oct 2014 09:47:53 -0700
"H. Peter Anvin" <[email protected]> wrote:[replying to the list this time, sigh]
Quoted 1 line
We changed this to asm because the C broke some compilers. Why are
you changing it back?
Quoted 1 line
The C broke some compilers because it was using a global register variable. This is a local one, which the clang documentation says is supported. And I compiled it with clang with no problem.
Quoted 1 line
On October 12, 2014 9:43:53 AM PDT, Chuck Ebbert
Quoted 1 line · <[email protected]> wrote:
Quoted 1 line
Use C instead of asm for accessing the stack pointer. And define
some
Quoted 2 lines
Quoted 2 lines
macros to make the code easier to understand.
Signature · 1 line
Sent from my mobile phone. Please pardon brevity and lack of formatting.
Quoted 2 lines · On Sun, 12 Oct 2014 10:13:33 -0700 "H. Peter Anvin" <[email protected]> wrote:
That's not a justification for change. Claiming no harm is nevessary but not sufficient.
The optimization is also a little better with GCC when using C instead of asm for current_stack_pointer. Probably not enough better to do different macros for gcc and other compilers though.
clang actually moves %esp to memory and then into another register instead of moving it directly when using the current macro. Their optimizer really needs some work...
Quoted 21 lines · Chuck Ebbert
Quoted 19 lines
On Sun, 12 Oct 2014 09:47:53 -0700
"H. Peter Anvin" <[email protected]> wrote:[replying to the list this time, sigh]
Quoted 1 line
We changed this to asm because the C broke some compilers. Why are
you changing it back?
Quoted 1 line
The C broke some compilers because it was using a global register variable. This is a local one, which the clang documentation says is supported. And I compiled it with clang with no problem.
Quoted 1 line
On October 12, 2014 9:43:53 AM PDT, Chuck Ebbert
Quoted 1 line · <[email protected]> wrote:
Quoted 1 line
Use C instead of asm for accessing the stack pointer. And define
some
Quoted 2 lines
Quoted 2 lines
macros to make the code easier to understand.
It looks like the proposed variant still miscompiles in clang 3.4 and 3.5, the two versions I had handy to test.
I extracted your code to a simple standalone C translation unit and inspected various compilers' results via objdump.
// cut here for cso.c
struct thread_info { long l[32]; }; // who knows
#define STACK_WARN (1024)
#define PAGE_SIZE (4096)
#define THREAD_SIZE_ORDER 2 #define THREAD_SIZE (PAGE_SIZE << THREAD_SIZE_ORDER) #define THREAD_SIZE_MASK (THREAD_SIZE - 1) #define CURRENT_MASK (~THREAD_SIZE_MASK)
/* how to get the current stack pointer from C */
#define current_stack_pointer ({ \
register unsigned long sp asm("esp"); \
sp; \})
int check_stack_overflow0(void)
{
long sp;
__asm__ __volatile__("andl %%esp,%0" :
"=r" (sp) : "0" (THREAD_SIZE - 1));
return sp < (sizeof(struct thread_info) + STACK_WARN);
}int check_stack_overflow1(void)
{
return (current_stack_pointer & THREAD_SIZE_MASK)
< sizeof(struct thread_info) + STACK_WARN;
}
// end cso.c
Typical compiler invocation:
clang-3.5 -m32 -Os -c cso.c
Both clang-3.4 and clang-3.5 as packaged for debian jessie seem to get check_stack_overflow1 wrong, yielding a function which always returns true:
00000000 <check_stack_overflow1>:
0: b8 01 00 00 00 mov $0x1,%eax
5: c3 ret Jeff
Quoted 6 lines · On Sun, 12 Oct 2014 12:00:03 -0500 Jeff Epler <[email protected]> wrote:
It looks like the proposed variant still miscompiles in clang 3.4 and 3.5, the two versions I had handy to test.
I extracted your code to a simple standalone C translation unit and inspected various compilers' results via objdump.
Wow, my little test program below worked with clang by accident. I was building it with both printf() calls enabled and it printed out the same results on both output lines. But commenting out the first line reveals that it simply leaves whatever junk is on the stack there for the first arg when it calls printf().
#define _GNU_SOURCE
#include <string.h>
#include <stdio.h>
#include <unistd.h>
#define current_stack_pointer ({ \
unsigned long sp; \
asm("mov %%esp,%0" : "=g" (sp)); \
sp; \
})#define current_stack_pointer2 ({ \
register unsigned long sp asm("esp"); \
sp; \
})int main(int argc, char **argv)
{
// printf("%X %X\n", current_stack_pointer , __builtin_frame_address(0));
printf("%X %X\n", current_stack_pointer2, __builtin_frame_address(0));
return 0;
}