[PATCH] i386/gcc bug with do_test_wp_bit

[PATCH] i386/gcc bug with do_test_wp_bit

[Zachary Amsden]

[-- Attachment #1: Type: text/plain, Size: 620 bytes --]

Playing around with gcc 3.3.3, I compiled a 2.6 series kernel for i386 
and discovered it panics on boot.  The problem was gcc 3.3.3 can inline 
functions even if declared after their call sites.  This causes i386 to 
not boot, since do_test_wp_bit() must not exist in the __init section.  
Similar problems may exist in the boot code for other architectures, but 
I can't confirm that at this time.  x86_64 is not affected.

I've included a small trivial fix that is harmless for users not using 
gcc 3.3.3.  Testing: my 2.6 kernel now boots when compiled with gcc 
3.3.3 compiler.

Cheers,
Zach Amsden
zach@vmware.com

[-- Attachment #2: i386-wp-test.patch --]
[-- Type: text/plain, Size: 588 bytes --]

diff -ru linux-2.6.9-rc3/arch/i386/mm/init.c linux-2.6.9-rc3-za1/arch/i386/mm/init.c
--- linux-2.6.9-rc3/arch/i386/mm/init.c	2004-10-05 18:21:03.000000000 -0700
+++ linux-2.6.9-rc3-za1/arch/i386/mm/init.c	2004-10-05 18:24:08.000000000 -0700
@@ -671,9 +671,9 @@
 
 /*
  * This function cannot be __init, since exceptions don't work in that
- * section.  Put this after the callers, so that it cannot be inlined.
+ * section.  This function must not be inlined.
  */
-static int do_test_wp_bit(void)
+__attribute__((noinline)) static int do_test_wp_bit(void)
 {
 	char tmp_reg;
 	int flag;

[-- Attachment #3: README.i386-wp-test --]
[-- Type: text/plain, Size: 1172 bytes --]

When compiling a Linux 2.6 kernel using gcc 3.3.3, gcc was able to inline the function do_test_wp_bit().  This causes i386 kernels to panic on boot, since the exception handler doesn't work properly.

The fix is straightforward - don't ever allow this function to be inlined.

Here's a disassembly to confirm the bad inline:

c02aecf1 <test_wp_bit>:
c02aecf1:       55                      push   %ebp
c02aecf2:       89 e5                   mov    %esp,%ebp
c02aecf4:       83 ec 0c                sub    $0xc,%esp
c02aecf7:       c7 04 24 0c a0 25 c0    movl   $0xc025a00c,(%esp)
c02aecfe:       e8 63 73 e6 ff          call   c0116066 <printk>
c02aed03:       c7 44 24 08 25 00 00    movl   $0x25,0x8(%esp)
c02aed0a:       00 
c02aed0b:       c7 44 24 04 00 10 10    movl   $0x101000,0x4(%esp)
c02aed12:       00 
c02aed13:       c7 04 24 12 00 00 00    movl   $0x12,(%esp)
c02aed1a:       e8 d5 11 e6 ff          call   c010fef4 <__set_fixmap>
c02aed1f:       b8 01 00 00 00          mov    $0x1,%eax
c02aed24:       8a 15 00 d0 fe ff       mov    0xfffed000,%dl
c02aed2a:       88 15 00 d0 fe ff       mov    %dl,0xfffed000

Cheers,

Zachary Amsden (zach@vmware.com)

Re: [PATCH] i386/gcc bug with do_test_wp_bit

[Linus Torvalds]

On Tue, 5 Oct 2004, Zachary Amsden wrote:
> 
> I've included a small trivial fix that is harmless for users not using 
> gcc 3.3.3.  Testing: my 2.6 kernel now boots when compiled with gcc 
> 3.3.3 compiler.

Thanks. However, it really should use the "noinline" define that we have 
for this, and that doesn't upset older versions of gcc that don't 
understand the "noinline" attribute. 

Also, declaration and definition should match, even if gcc doesn't seem to 
catch that.

So here's the version I committed. It should be obviously ok, but it's 
still a good idea to verify..

		Linus

---
# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/05 18:51:53-07:00 torvalds@evo.osdl.org 
#   i386: mark do_test_wp_bit() noinline
#   
#   As reported by Zachary Amsden <zach@vmware.com>,
#   some gcc versions will inline the function even when
#   it is declared after the call-site. This particular
#   function must not be inlined, since the exception
#   recovery doesn't like __init sections (which the caller
#   is in).
# 
# arch/i386/mm/init.c
#   2004/10/05 18:51:43-07:00 torvalds@evo.osdl.org +2 -2
#   i386: mark do_test_wp_bit() noinline
#   
#   As reported by Zachary Amsden <zach@vmware.com>,
#   some gcc versions will inline the function even when
#   it is declared after the call-site. This particular
#   function must not be inlined, since the exception
#   recovery doesn't like __init sections (which the caller
#   is in).
# 
diff -Nru a/arch/i386/mm/init.c b/arch/i386/mm/init.c
--- a/arch/i386/mm/init.c	2004-10-05 19:04:53 -07:00
+++ b/arch/i386/mm/init.c	2004-10-05 19:04:53 -07:00
@@ -45,7 +45,7 @@
 DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
 unsigned long highstart_pfn, highend_pfn;
 
-static int do_test_wp_bit(void);
+static int noinline do_test_wp_bit(void);
 
 /*
  * Creates a middle page table and puts a pointer to it in the
@@ -673,7 +673,7 @@
  * This function cannot be __init, since exceptions don't work in that
  * section.  Put this after the callers, so that it cannot be inlined.
  */
-static int do_test_wp_bit(void)
+static int noinline do_test_wp_bit(void)
 {
 	char tmp_reg;
 	int flag;

Re: [PATCH] i386/gcc bug with do_test_wp_bit

[Zachary Amsden]

I agree the code obviously looks ok, I will verify against 2.6.9-rc3 
anyways and send an updated patch that gets a non-tweaked gcc 3.3.3 
build to boot.  Additional fixes are needed for regparm compliance 
between prototypes and functions; I've done these for i386, but may have 
missed some in other architectures.  I'd also like to get rid of the 
apparently harmless lvalue warnings I get from gcc 3.3.3.

I checked the gcc docs but couldn't find any versions where noinline was 
unsupported, although I should have assumed otherwise.

Zach

Linus Torvalds wrote:

>On Tue, 5 Oct 2004, Zachary Amsden wrote:
>  
>
>>I've included a small trivial fix that is harmless for users not using 
>>gcc 3.3.3.  Testing: my 2.6 kernel now boots when compiled with gcc 
>>3.3.3 compiler.
>>    
>>
>
>Thanks. However, it really should use the "noinline" define that we have 
>for this, and that doesn't upset older versions of gcc that don't 
>understand the "noinline" attribute. 
>
>Also, declaration and definition should match, even if gcc doesn't seem to 
>catch that.
>
>So here's the version I committed. It should be obviously ok, but it's 
>still a good idea to verify..
>
>		Linus
>
>---
># This is a BitKeeper generated diff -Nru style patch.
>#
># ChangeSet
>#   2004/10/05 18:51:53-07:00 torvalds@evo.osdl.org 
>#   i386: mark do_test_wp_bit() noinline
>#   
>#   As reported by Zachary Amsden <zach@vmware.com>,
>#   some gcc versions will inline the function even when
>#   it is declared after the call-site. This particular
>#   function must not be inlined, since the exception
>#   recovery doesn't like __init sections (which the caller
>#   is in).
># 
># arch/i386/mm/init.c
>#   2004/10/05 18:51:43-07:00 torvalds@evo.osdl.org +2 -2
>#   i386: mark do_test_wp_bit() noinline
>#   
>#   As reported by Zachary Amsden <zach@vmware.com>,
>#   some gcc versions will inline the function even when
>#   it is declared after the call-site. This particular
>#   function must not be inlined, since the exception
>#   recovery doesn't like __init sections (which the caller
>#   is in).
># 
>diff -Nru a/arch/i386/mm/init.c b/arch/i386/mm/init.c
>--- a/arch/i386/mm/init.c	2004-10-05 19:04:53 -07:00
>+++ b/arch/i386/mm/init.c	2004-10-05 19:04:53 -07:00
>@@ -45,7 +45,7 @@
> DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
> unsigned long highstart_pfn, highend_pfn;
> 
>-static int do_test_wp_bit(void);
>+static int noinline do_test_wp_bit(void);
> 
> /*
>  * Creates a middle page table and puts a pointer to it in the
>@@ -673,7 +673,7 @@
>  * This function cannot be __init, since exceptions don't work in that
>  * section.  Put this after the callers, so that it cannot be inlined.
>  */
>-static int do_test_wp_bit(void)
>+static int noinline do_test_wp_bit(void)
> {
> 	char tmp_reg;
> 	int flag;
>  
>

Re: [PATCH] i386/gcc bug with do_test_wp_bit

[Andi Kleen]

On Tue, Oct 05, 2004 at 06:45:05PM -0700, Zachary Amsden wrote:
> Playing around with gcc 3.3.3, I compiled a 2.6 series kernel for i386 
> and discovered it panics on boot.  The problem was gcc 3.3.3 can inline 
> functions even if declared after their call sites.  This causes i386 to 
> not boot, since do_test_wp_bit() must not exist in the __init section.  
> Similar problems may exist in the boot code for other architectures, but 
> I can't confirm that at this time.  x86_64 is not affected.

That should have been fixed long ago by sorting the exception
table. I checked and the code is still there: 

asmlinkage void __init start_kernel(void)
{
	...
        sort_main_extable();


Something must be rotten in your setup. I definitely don't see the
same problems with a unit-at-a-time 3.3 gcc. 

Can you double check that the sort is really done?

The patch is imho not needed.

-Andi

Re: [PATCH] i386/gcc bug with do_test_wp_bit

[Zachary Amsden]

[-- Attachment #1: Type: text/plain, Size: 3065 bytes --]

Confirmed.  As Andi says, sorting the exception table does fix the problem.  Tested 2.6.9-rc3 with gcc 3.3.3 and there are no issues.  My major malfunction was working on an older 2.6 kernel and believing the patches were still relevant.  Linus, please revert the following commit, it is no longer needed and do_test_wp_bit is better inlined into __init and subsequently discarded now that exceptions in __init work.

I've added a nicer patch that inlines the do_test_wp_bit function and deprecates the outdated comment.

Zach


# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/10/05 18:51:53-07:00 torvalds@evo.osdl.org 
#   i386: mark do_test_wp_bit() noinline
#   
#   As reported by Zachary Amsden <zach@vmware.com>,
#   some gcc versions will inline the function even when
#   it is declared after the call-site. This particular
#   function must not be inlined, since the exception
#   recovery doesn't like __init sections (which the caller
#   is in).
# 
# arch/i386/mm/init.c
#   2004/10/05 18:51:43-07:00 torvalds@evo.osdl.org +2 -2
#   i386: mark do_test_wp_bit() noinline
#   
#   As reported by Zachary Amsden <zach@vmware.com>,
#   some gcc versions will inline the function even when
#   it is declared after the call-site. This particular
#   function must not be inlined, since the exception
#   recovery doesn't like __init sections (which the caller
#   is in).
# 
diff -Nru a/arch/i386/mm/init.c b/arch/i386/mm/init.c
--- a/arch/i386/mm/init.c	2004-10-05 19:04:53 -07:00
+++ b/arch/i386/mm/init.c	2004-10-05 19:04:53 -07:00
@@ -45,7 +45,7 @@
 DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
 unsigned long highstart_pfn, highend_pfn;
 
-static int do_test_wp_bit(void);
+static int noinline do_test_wp_bit(void);
 
 /*
  * Creates a middle page table and puts a pointer to it in the
@@ -673,7 +673,7 @@
  * This function cannot be __init, since exceptions don't work in that
  * section.  Put this after the callers, so that it cannot be inlined.
  */
-static int do_test_wp_bit(void)
+static int noinline do_test_wp_bit(void)
 {
 	char tmp_reg;
 	int flag;


Andi Kleen wrote:

>On Tue, Oct 05, 2004 at 06:45:05PM -0700, Zachary Amsden wrote:
>  
>
>>Playing around with gcc 3.3.3, I compiled a 2.6 series kernel for i386 
>>and discovered it panics on boot.  The problem was gcc 3.3.3 can inline 
>>functions even if declared after their call sites.  This causes i386 to 
>>not boot, since do_test_wp_bit() must not exist in the __init section.  
>>Similar problems may exist in the boot code for other architectures, but 
>>I can't confirm that at this time.  x86_64 is not affected.
>>    
>>
>
>That should have been fixed long ago by sorting the exception
>table. I checked and the code is still there: 
>
>asmlinkage void __init start_kernel(void)
>{
>	...
>        sort_main_extable();
>
>
>Something must be rotten in your setup. I definitely don't see the
>same problems with a unit-at-a-time 3.3 gcc. 
>
>Can you double check that the sort is really done?
>
>The patch is imho not needed.
>
>-Andi
>  
>


[-- Attachment #2: i386-wp-bit.patch --]
[-- Type: text/plain, Size: 1841 bytes --]

diff -ru linux-2.6.9-rc3/arch/i386/mm/init.c linux-2.6.9-rc3-za1/arch/i386/mm/init.c
--- linux-2.6.9-rc3/arch/i386/mm/init.c	2004-10-05 18:21:03.000000000 -0700
+++ linux-2.6.9-rc3-za1/arch/i386/mm/init.c	2004-10-06 12:26:03.000000000 -0700
@@ -45,8 +45,6 @@
 DEFINE_PER_CPU(struct mmu_gather, mmu_gathers);
 unsigned long highstart_pfn, highend_pfn;
 
-static int do_test_wp_bit(void);
-
 /*
  * Creates a middle page table and puts a pointer to it in the
  * given global directory entry. This only returns the gd entry
@@ -525,6 +523,28 @@
  * used to involve black magic jumps to work around some nasty CPU bugs,
  * but fortunately the switch to using exceptions got rid of all that.
  */
+static inline int do_test_wp_bit(void)
+{
+	char tmp_reg;
+	int flag;
+
+	__asm__ __volatile__(
+		"	movb %0,%1	\n"
+		"1:	movb %1,%0	\n"
+		"	xorl %2,%2	\n"
+		"2:			\n"
+		".section __ex_table,\"a\"\n"
+		"	.align 4	\n"
+		"	.long 1b,2b	\n"
+		".previous		\n"
+		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
+		 "=q" (tmp_reg),
+		 "=r" (flag)
+		:"2" (1)
+		:"memory");
+	
+	return flag;
+}
 
 void __init test_wp_bit(void)
 {
@@ -669,33 +689,6 @@
 		panic("pgtable_cache_init(): Cannot create pgd cache");
 }
 
-/*
- * This function cannot be __init, since exceptions don't work in that
- * section.  Put this after the callers, so that it cannot be inlined.
- */
-static int do_test_wp_bit(void)
-{
-	char tmp_reg;
-	int flag;
-
-	__asm__ __volatile__(
-		"	movb %0,%1	\n"
-		"1:	movb %1,%0	\n"
-		"	xorl %2,%2	\n"
-		"2:			\n"
-		".section __ex_table,\"a\"\n"
-		"	.align 4	\n"
-		"	.long 1b,2b	\n"
-		".previous		\n"
-		:"=m" (*(char *)fix_to_virt(FIX_WP_TEST)),
-		 "=q" (tmp_reg),
-		 "=r" (flag)
-		:"2" (1)
-		:"memory");
-	
-	return flag;
-}
-
 void free_initmem(void)
 {
 	unsigned long addr;
Only in linux-2.6.9-rc3: .MAINTAINERS.swp