[PATCH] x86: Clean up stack access code in irq_32.c

[PATCH] x86: Clean up stack access code in irq_32.c

[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 <cebbert.lkml@gmail.com>

diff --git a/arch/x86/include/asm/page_32_types.h b/arch/x86/include/asm/page_32_types.h
index f48b17d..a8ca0cb 100644
--- a/arch/x86/include/asm/page_32_types.h
+++ b/arch/x86/include/asm/page_32_types.h
@@ -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
diff --git a/arch/x86/include/asm/page_64_types.h b/arch/x86/include/asm/page_64_types.h
index 6782051..ded89b0 100644
--- a/arch/x86/include/asm/page_64_types.h
+++ b/arch/x86/include/asm/page_64_types.h
@@ -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)
diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
index 63ce838..bef90fc 100644
--- a/arch/x86/kernel/irq_32.c
+++ b/arch/x86/kernel/irq_32.c
@@ -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

Re: [PATCH] x86: Clean up stack access code in irq_32.c

[H. Peter Anvin]

We changed this to asm because the C broke some compilers.  Why are you changing it back?

On October 12, 2014 9:43:53 AM PDT, Chuck Ebbert <cebbert.lkml@gmail.com> wrote:
>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 <cebbert.lkml@gmail.com>
>
>diff --git a/arch/x86/include/asm/page_32_types.h
>b/arch/x86/include/asm/page_32_types.h
>index f48b17d..a8ca0cb 100644
>--- a/arch/x86/include/asm/page_32_types.h
>+++ b/arch/x86/include/asm/page_32_types.h
>@@ -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
>diff --git a/arch/x86/include/asm/page_64_types.h
>b/arch/x86/include/asm/page_64_types.h
>index 6782051..ded89b0 100644
>--- a/arch/x86/include/asm/page_64_types.h
>+++ b/arch/x86/include/asm/page_64_types.h
>@@ -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)
>diff --git a/arch/x86/kernel/irq_32.c b/arch/x86/kernel/irq_32.c
>index 63ce838..bef90fc 100644
>--- a/arch/x86/kernel/irq_32.c
>+++ b/arch/x86/kernel/irq_32.c
>@@ -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

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH] x86: Clean up stack access code in irq_32.c

[Chuck Ebbert]

On Sun, 12 Oct 2014 09:47:53 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:

[replying to the list this time, sigh]

> 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.

> On October 12, 2014 9:43:53 AM PDT, Chuck Ebbert <cebbert.lkml@gmail.com> wrote:
> >Use C instead of asm for accessing the stack pointer. And define some
> >macros to make the code easier to understand.
> >

Re: [PATCH] x86: Clean up stack access code in irq_32.c

[Jeff Epler]

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

Re: [PATCH] x86: Clean up stack access code in irq_32.c

[H. Peter Anvin]

That's not a justification for change.  Claiming no harm is nevessary but not sufficient.

On October 12, 2014 9:53:32 AM PDT, Chuck Ebbert <cebbert.lkml@gmail.com> wrote:
>On Sun, 12 Oct 2014 09:47:53 -0700
>"H. Peter Anvin" <hpa@zytor.com> wrote:
>
>[replying to the list this time, sigh]
>
>> 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.
>
>> On October 12, 2014 9:43:53 AM PDT, Chuck Ebbert
><cebbert.lkml@gmail.com> wrote:
>> >Use C instead of asm for accessing the stack pointer. And define
>some
>> >macros to make the code easier to understand.
>> >

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [PATCH] x86: Clean up stack access code in irq_32.c

[Chuck Ebbert]

On Sun, 12 Oct 2014 12:00:03 -0500
Jeff Epler <jepler@unpythonic.net> 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;
}

Re: [PATCH] x86: Clean up stack access code in irq_32.c

[Chuck Ebbert]

On Sun, 12 Oct 2014 10:13:33 -0700
"H. Peter Anvin" <hpa@zytor.com> 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...

> On October 12, 2014 9:53:32 AM PDT, Chuck Ebbert <cebbert.lkml@gmail.com> wrote:
> >On Sun, 12 Oct 2014 09:47:53 -0700
> >"H. Peter Anvin" <hpa@zytor.com> wrote:
> >
> >[replying to the list this time, sigh]
> >
> >> 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.
> >
> >> On October 12, 2014 9:43:53 AM PDT, Chuck Ebbert
> ><cebbert.lkml@gmail.com> wrote:
> >> >Use C instead of asm for accessing the stack pointer. And define
> >some
> >> >macros to make the code easier to understand.
> >> >
>