Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Jan Beulich]

Just like done in the x86-64 patch that I just sent, I'd recommend
moving
the push added yesterday outside of the CFI-covered region (so that
in the unlikely event of being caught at the push there won't be an
ill
assumption that a [fake] return address is already on the stack, or
that
there is a return address at all):

ENTRY(kernel_thread_helper)
	pushl $0			# fake return address
	CFI_STARTPROC
	movl %edx,%eax
	pushl %edx
	CFI_ADJUST_CFA_OFFSET 4
	call *%ebx
	pushl %eax
	CFI_ADJUST_CFA_OFFSET 4
	call do_exit
	CFI_ENDPROC
ENDPROC(kernel_thread_helper)

Jan

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Andi Kleen]

On Tue, 15 Aug 2006 12:36:44 +0200
"Jan Beulich" <jbeulich@novell.com> wrote:

> Just like done in the x86-64 patch that I just sent, I'd recommend
> moving
> the push added yesterday outside of the CFI-covered region (so that
> in the unlikely event of being caught at the push there won't be an
> ill
> assumption that a [fake] return address is already on the stack, or
> that
> there is a return address at all):

Done.

-Andi

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Jan Beulich]

>> And this too - the value now in %eax has no relation with the
>> value known by the caller of this routine (which doesn't expect
>> any return from here anyway).
>
>Ok, but somehow it needs to be annotiated so that the unwinder stops
>and doesn't fall back. Can you please send a replacement patch that 
>does this correctly? 

Actually, with the previous attempt we still didn't achieve the
original
goal of terminating the frame chain at kernel_thread_helper. Thus
another try (the seemingly odd extra push can be avoided once we
start using CFI expressions, which the unwinder currently doesn't
support):

ENTRY(kernel_thread_helper)
	CFI_STARTPROC
	pushl $0			# fake return address
	movl %edx,%eax
	pushl %edx
	CFI_ADJUST_CFA_OFFSET 4
	call *%ebx
	pushl %eax
	CFI_ADJUST_CFA_OFFSET 4
	call do_exit
	CFI_ENDPROC
ENDPROC(kernel_thread_helper)

Jan

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Andi Kleen]

On Monday 14 August 2006 14:36, Jan Beulich wrote:
> >> And this too - the value now in %eax has no relation with the
> >> value known by the caller of this routine (which doesn't expect
> >> any return from here anyway).
> >
> >Ok, but somehow it needs to be annotiated so that the unwinder stops
> >and doesn't fall back. Can you please send a replacement patch that 
> >does this correctly? 
> 
> Actually, with the previous attempt we still didn't achieve the
> original
> goal of terminating the frame chain at kernel_thread_helper. Thus
> another try (the seemingly odd extra push can be avoided once we
> start using CFI expressions, which the unwinder currently doesn't
> support):

I added the extra push thanks (hope there weren't more changes)


-Andi

[PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Andi Kleen]

r

And add proper CFI annotation to it which was previously
impossible. This prevents "stuck" messages by the dwarf2 unwinder
when reaching the top of a kernel stack.

Cc: jbeulich@novell.com

Signed-off-by: Andi Kleen <ak@suse.de>

---
 arch/i386/kernel/entry.S   |   15 +++++++++++++++
 arch/i386/kernel/process.c |    9 ---------
 2 files changed, 15 insertions(+), 9 deletions(-)

Index: linux/arch/i386/kernel/entry.S
===================================================================
--- linux.orig/arch/i386/kernel/entry.S
+++ linux/arch/i386/kernel/entry.S
@@ -946,6 +946,21 @@ ENTRY(arch_unwind_init_running)
 ENDPROC(arch_unwind_init_running)
 #endif
 
+ENTRY(kernel_thread_helper)
+	CFI_STARTPROC
+	movl %edx,%eax
+	CFI_REGISTER edx,eax
+	push %edx
+	CFI_ADJUST_CFA_OFFSET 4
+	CFI_REL_OFFSET edx,0
+	call *%ebx
+	push %eax
+	CFI_ADJUST_CFA_OFFSET 4
+	CFI_REL_OFFSET eax,0
+	call do_exit
+	CFI_ENDPROC
+ENDPROC(kernel_thread_helper)
+
 .section .rodata,"a"
 #include "syscall_table.S"
 
Index: linux/arch/i386/kernel/process.c
===================================================================
--- linux.orig/arch/i386/kernel/process.c
+++ linux/arch/i386/kernel/process.c
@@ -321,15 +321,6 @@ void show_regs(struct pt_regs * regs)
  * the "args".
  */
 extern void kernel_thread_helper(void);
-__asm__(".section .text\n"
-	".align 4\n"
-	"kernel_thread_helper:\n\t"
-	"movl %edx,%eax\n\t"
-	"pushl %edx\n\t"
-	"call *%ebx\n\t"
-	"pushl %eax\n\t"
-	"call do_exit\n"
-	".previous");
 
 /*
  * Create a kernel thread

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Jan Beulich]

>And add proper CFI annotation to it which was previously
>impossible. This prevents "stuck" messages by the dwarf2 unwinder
>when reaching the top of a kernel stack.

>+ENTRY(kernel_thread_helper)
>+	CFI_STARTPROC
>+	movl %edx,%eax
>+	CFI_REGISTER edx,eax

This is pointless, as %eax will be clobbered by the callee of the
subsequent call.

>+	push %edx
>+	CFI_ADJUST_CFA_OFFSET 4
>+	CFI_REL_OFFSET edx,0

This likewise is pointless, as the argument is owned by the callee.

>+	call *%ebx
>+	push %eax
>+	CFI_ADJUST_CFA_OFFSET 4
>+	CFI_REL_OFFSET eax,0

And this too - the value now in %eax has no relation with the
value known by the caller of this routine (which doesn't expect
any return from here anyway).

>+	call do_exit
>+	CFI_ENDPROC
>+ENDPROC(kernel_thread_helper)

Jan

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Andi Kleen]

> And this too - the value now in %eax has no relation with the
> value known by the caller of this routine (which doesn't expect
> any return from here anyway).

Ok, but somehow it needs to be annotiated so that the unwinder stops
and doesn't fall back. Can you please send a replacement patch that 
does this correctly? 

Thanks,
-Andi

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Jan Beulich]

>> And this too - the value now in %eax has no relation with the
>> value known by the caller of this routine (which doesn't expect
>> any return from here anyway).
>
>Ok, but somehow it needs to be annotiated so that the unwinder stops
>and doesn't fall back. Can you please send a replacement patch that 
>does this correctly? 

I would do it this way (untested):

ENTRY(kernel_thread_helper)
	CFI_STARTPROC
	movl %edx,%eax
	pushl %edx
	CFI_ADJUST_CFA_OFFSET 4
	call *%ebx
	pushl %eax
	CFI_ADJUST_CFA_OFFSET 4
	call do_exit
	CFI_ENDPROC
ENDPROC(kernel_thread_helper)

(i.e. tracking the stack pointer movement, but not the register values
other than the return address)

Jan

Re: [PATCH for review] [127/145] i386: move kernel_thread_helper into entry.S

[Andi Kleen]

> ENTRY(kernel_thread_helper)
> 	CFI_STARTPROC
> 	movl %edx,%eax
> 	pushl %edx
> 	CFI_ADJUST_CFA_OFFSET 4
> 	call *%ebx
> 	pushl %eax
> 	CFI_ADJUST_CFA_OFFSET 4
> 	call do_exit
> 	CFI_ENDPROC
> ENDPROC(kernel_thread_helper)
> 
> (i.e. tracking the stack pointer movement, but not the register values
> other than the return address)

Done thanks.

-Andi