[PATCH 00/10] ARM: ftrace: cleanups, Thumb-2, and dynamic ftrace

[PATCH 00/10] ARM: ftrace: cleanups, Thumb-2, and dynamic ftrace

[Rabin Vincent]

This series contains fixes and improvements to the ARM ftrace support.  It adds
Thumb-2 support and re-implements the dynamic ftrace support.

"ftrace: allow building without frame pointers" and "ftrace: pass KBUILD_CFLAGS
to record_mcount.pl" are non-ARM-specific ftrace patches which are required to
support the ARM functionality.

Cleanups / docs:

  ARM: ftrace: clean up mcount assembly indentation
  ARM: ftrace: document mcount formats

Thumb-2:

  ftrace: allow building without frame pointers
  ARM: ftrace: allow building without frame pointers
  ARM: ftrace: add ENDPROC annotations
  ARM: ftrace: add Thumb-2 support

Dynamic ftrace:

  ftrace: pass KBUILD_CFLAGS to record_mcount.pl
  ARM: ftrace: fix and update dynamic ftrace
  ARM: ftrace: add Thumb-2 support to dynamic ftrace
  ARM: ftrace: enable dynamic ftrace

 Makefile                       |    3 +
 arch/arm/Kconfig               |    2 +
 arch/arm/Kconfig.debug         |    5 +
 arch/arm/include/asm/ftrace.h  |   23 +++++-
 arch/arm/kernel/armksyms.c     |    2 +
 arch/arm/kernel/entry-common.S |  167 ++++++++++++++++++++++++-----------
 arch/arm/kernel/ftrace.c       |  188 +++++++++++++++++++++++++++++----------
 kernel/trace/Kconfig           |    2 +-
 scripts/Makefile.build         |    3 +-
 scripts/recordmcount.pl        |    2 +
 10 files changed, 295 insertions(+), 102 deletions(-)

[PATCH 01/10] ARM: ftrace: clean up mcount assembly indentation

[Rabin Vincent]

The mcount implementation currently uses a different indentation style
from the rest of the file (and the rest of the ARM assembly in the
kernel).  Clean it up.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/entry-common.S |   88 ++++++++++++++++++++--------------------
 1 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 2c1db77..0b042bd 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -94,73 +94,73 @@ ENDPROC(ret_from_fork)
 #ifdef CONFIG_FUNCTION_TRACER
 #ifdef CONFIG_DYNAMIC_FTRACE
 ENTRY(mcount)
-	stmdb sp!, {r0-r3, lr}
-	mov r0, lr
-	sub r0, r0, #MCOUNT_INSN_SIZE
+	stmdb	sp!, {r0-r3, lr}
+	mov	r0, lr
+	sub	r0, r0, #MCOUNT_INSN_SIZE
 
 	.globl mcount_call
 mcount_call:
-	bl ftrace_stub
-	ldr lr, [fp, #-4]			@ restore lr
-	ldmia sp!, {r0-r3, pc}
+	bl	ftrace_stub
+	ldr	lr, [fp, #-4]			@ restore lr
+	ldmia	sp!, {r0-r3, pc}
 
 ENTRY(ftrace_caller)
-	stmdb sp!, {r0-r3, lr}
-	ldr r1, [fp, #-4]
-	mov r0, lr
-	sub r0, r0, #MCOUNT_INSN_SIZE
+	stmdb	sp!, {r0-r3, lr}
+	ldr	r1, [fp, #-4]
+	mov	r0, lr
+	sub	r0, r0, #MCOUNT_INSN_SIZE
 
 	.globl ftrace_call
 ftrace_call:
-	bl ftrace_stub
-	ldr lr, [fp, #-4]			@ restore lr
-	ldmia sp!, {r0-r3, pc}
+	bl	ftrace_stub
+	ldr	lr, [fp, #-4]			@ restore lr
+	ldmia	sp!, {r0-r3, pc}
 
 #else
 
 ENTRY(__gnu_mcount_nc)
-	stmdb sp!, {r0-r3, lr}
-	ldr r0, =ftrace_trace_function
-	ldr r2, [r0]
-	adr r0, ftrace_stub
-	cmp r0, r2
-	bne gnu_trace
-	ldmia sp!, {r0-r3, ip, lr}
-	mov pc, ip
+	stmdb	sp!, {r0-r3, lr}
+	ldr	r0, =ftrace_trace_function
+	ldr	r2, [r0]
+	adr	r0, ftrace_stub
+	cmp	r0, r2
+	bne	gnu_trace
+	ldmia	sp!, {r0-r3, ip, lr}
+	mov	pc, ip
 
 gnu_trace:
-	ldr r1, [sp, #20]			@ lr of instrumented routine
-	mov r0, lr
-	sub r0, r0, #MCOUNT_INSN_SIZE
-	mov lr, pc
-	mov pc, r2
-	ldmia sp!, {r0-r3, ip, lr}
-	mov pc, ip
+	ldr	r1, [sp, #20]			@ lr of instrumented routine
+	mov	r0, lr
+	sub	r0, r0, #MCOUNT_INSN_SIZE
+	mov	lr, pc
+	mov	pc, r2
+	ldmia	sp!, {r0-r3, ip, lr}
+	mov	pc, ip
 
 ENTRY(mcount)
-	stmdb sp!, {r0-r3, lr}
-	ldr r0, =ftrace_trace_function
-	ldr r2, [r0]
-	adr r0, ftrace_stub
-	cmp r0, r2
-	bne trace
-	ldr lr, [fp, #-4]			@ restore lr
-	ldmia sp!, {r0-r3, pc}
+	stmdb	sp!, {r0-r3, lr}
+	ldr	r0, =ftrace_trace_function
+	ldr	r2, [r0]
+	adr	r0, ftrace_stub
+	cmp	r0, r2
+	bne	trace
+	ldr	lr, [fp, #-4]			@ restore lr
+	ldmia	sp!, {r0-r3, pc}
 
 trace:
-	ldr r1, [fp, #-4]			@ lr of instrumented routine
-	mov r0, lr
-	sub r0, r0, #MCOUNT_INSN_SIZE
-	mov lr, pc
-	mov pc, r2
-	ldr lr, [fp, #-4]			@ restore lr
-	ldmia sp!, {r0-r3, pc}
+	ldr	r1, [fp, #-4]			@ lr of instrumented routine
+	mov	r0, lr
+	sub	r0, r0, #MCOUNT_INSN_SIZE
+	mov	lr, pc
+	mov	pc, r2
+	ldr	lr, [fp, #-4]			@ restore lr
+	ldmia	sp!, {r0-r3, pc}
 
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 	.globl ftrace_stub
 ftrace_stub:
-	mov pc, lr
+	mov	pc, lr
 
 #endif /* CONFIG_FUNCTION_TRACER */
 
-- 
1.6.6

[PATCH 01/10] ARM: ftrace: clean up mcount assembly indentation

[Uwe Kleine-König]

On Sun, Feb 14, 2010 at 01:18:29AM +0530, Rabin Vincent wrote:
> The mcount implementation currently uses a different indentation style
> from the rest of the file (and the rest of the ARM assembly in the
> kernel).  Clean it up.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 01/10] ARM: ftrace: clean up mcount assembly indentation

[Frederic Weisbecker]

On Sun, Feb 14, 2010 at 01:18:29AM +0530, Rabin Vincent wrote:
> The mcount implementation currently uses a different indentation style
> from the rest of the file (and the rest of the ARM assembly in the
> kernel).  Clean it up.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>



>  arch/arm/kernel/entry-common.S |   88 ++++++++++++++++++++--------------------
>  1 files changed, 44 insertions(+), 44 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 2c1db77..0b042bd 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -94,73 +94,73 @@ ENDPROC(ret_from_fork)
>  #ifdef CONFIG_FUNCTION_TRACER
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  ENTRY(mcount)
> -	stmdb sp!, {r0-r3, lr}
> -	mov r0, lr
> -	sub r0, r0, #MCOUNT_INSN_SIZE
> +	stmdb	sp!, {r0-r3, lr}
> +	mov	r0, lr
> +	sub	r0, r0, #MCOUNT_INSN_SIZE
>  
>  	.globl mcount_call
>  mcount_call:
> -	bl ftrace_stub
> -	ldr lr, [fp, #-4]			@ restore lr
> -	ldmia sp!, {r0-r3, pc}
> +	bl	ftrace_stub
> +	ldr	lr, [fp, #-4]			@ restore lr
> +	ldmia	sp!, {r0-r3, pc}
>  
>  ENTRY(ftrace_caller)
> -	stmdb sp!, {r0-r3, lr}
> -	ldr r1, [fp, #-4]
> -	mov r0, lr
> -	sub r0, r0, #MCOUNT_INSN_SIZE
> +	stmdb	sp!, {r0-r3, lr}
> +	ldr	r1, [fp, #-4]
> +	mov	r0, lr
> +	sub	r0, r0, #MCOUNT_INSN_SIZE
>  
>  	.globl ftrace_call
>  ftrace_call:
> -	bl ftrace_stub
> -	ldr lr, [fp, #-4]			@ restore lr
> -	ldmia sp!, {r0-r3, pc}
> +	bl	ftrace_stub
> +	ldr	lr, [fp, #-4]			@ restore lr
> +	ldmia	sp!, {r0-r3, pc}
>  
>  #else
>  
>  ENTRY(__gnu_mcount_nc)
> -	stmdb sp!, {r0-r3, lr}
> -	ldr r0, =ftrace_trace_function
> -	ldr r2, [r0]
> -	adr r0, ftrace_stub
> -	cmp r0, r2
> -	bne gnu_trace
> -	ldmia sp!, {r0-r3, ip, lr}
> -	mov pc, ip
> +	stmdb	sp!, {r0-r3, lr}
> +	ldr	r0, =ftrace_trace_function
> +	ldr	r2, [r0]
> +	adr	r0, ftrace_stub
> +	cmp	r0, r2
> +	bne	gnu_trace
> +	ldmia	sp!, {r0-r3, ip, lr}
> +	mov	pc, ip
>  
>  gnu_trace:
> -	ldr r1, [sp, #20]			@ lr of instrumented routine
> -	mov r0, lr
> -	sub r0, r0, #MCOUNT_INSN_SIZE
> -	mov lr, pc
> -	mov pc, r2
> -	ldmia sp!, {r0-r3, ip, lr}
> -	mov pc, ip
> +	ldr	r1, [sp, #20]			@ lr of instrumented routine
> +	mov	r0, lr
> +	sub	r0, r0, #MCOUNT_INSN_SIZE
> +	mov	lr, pc
> +	mov	pc, r2
> +	ldmia	sp!, {r0-r3, ip, lr}
> +	mov	pc, ip
>  
>  ENTRY(mcount)
> -	stmdb sp!, {r0-r3, lr}
> -	ldr r0, =ftrace_trace_function
> -	ldr r2, [r0]
> -	adr r0, ftrace_stub
> -	cmp r0, r2
> -	bne trace
> -	ldr lr, [fp, #-4]			@ restore lr
> -	ldmia sp!, {r0-r3, pc}
> +	stmdb	sp!, {r0-r3, lr}
> +	ldr	r0, =ftrace_trace_function
> +	ldr	r2, [r0]
> +	adr	r0, ftrace_stub
> +	cmp	r0, r2
> +	bne	trace
> +	ldr	lr, [fp, #-4]			@ restore lr
> +	ldmia	sp!, {r0-r3, pc}
>  
>  trace:
> -	ldr r1, [fp, #-4]			@ lr of instrumented routine
> -	mov r0, lr
> -	sub r0, r0, #MCOUNT_INSN_SIZE
> -	mov lr, pc
> -	mov pc, r2
> -	ldr lr, [fp, #-4]			@ restore lr
> -	ldmia sp!, {r0-r3, pc}
> +	ldr	r1, [fp, #-4]			@ lr of instrumented routine
> +	mov	r0, lr
> +	sub	r0, r0, #MCOUNT_INSN_SIZE
> +	mov	lr, pc
> +	mov	pc, r2
> +	ldr	lr, [fp, #-4]			@ restore lr
> +	ldmia	sp!, {r0-r3, pc}
>  
>  #endif /* CONFIG_DYNAMIC_FTRACE */
>  
>  	.globl ftrace_stub
>  ftrace_stub:
> -	mov pc, lr
> +	mov	pc, lr
>  
>  #endif /* CONFIG_FUNCTION_TRACER */
>  
> -- 
> 1.6.6
> 

[PATCH 02/10] ARM: ftrace: document mcount formats

[Rabin Vincent]

Add a comment describing the mcount variants and how the callsites look
like.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/entry-common.S |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 0b042bd..d412d7c 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -92,6 +92,38 @@ ENDPROC(ret_from_fork)
 #define CALL(x) .long x
 
 #ifdef CONFIG_FUNCTION_TRACER
+/*
+ * When compiling with -pg, gcc inserts a call to the mcount routine at the
+ * start of every function.  In mcount, apart from the function's address (in
+ * lr), we need to get hold of the function's caller's address.
+ *
+ * Older GCCs (pre-4.4) inserted a call to a routine called mcount like this:
+ *
+ *	bl	mcount
+ *
+ * These versions have the limitation that in order for the mcount routine to
+ * be able to determine the function's caller's address, an APCS-style frame
+ * pointer (which is set up with something like the code below) is required.
+ *
+ *	mov     ip, sp
+ *	push    {fp, ip, lr, pc}
+ *	sub     fp, ip, #4
+ *
+ * With EABI, these frame pointers are not available unless -mapcs-frame is
+ * specified, and if building as Thumb-2, not even then.
+ *
+ * Newer GCCs (4.4+) solve this problem by introducing a new version of mcount,
+ * with call sites like:
+ *
+ *	push	{lr}
+ *	bl	__gnu_mcount_nc
+ *
+ * With these compilers, frame pointers are not necessary.
+ *
+ * With both the mcount types, we need to restore the original lr before
+ * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
+ * No other registers should be clobbered.
+ */
 #ifdef CONFIG_DYNAMIC_FTRACE
 ENTRY(mcount)
 	stmdb	sp!, {r0-r3, lr}
-- 
1.6.6

[PATCH 02/10] ARM: ftrace: document mcount formats

[Uwe Kleine-König]

Hello,

On Sun, Feb 14, 2010 at 01:18:30AM +0530, Rabin Vincent wrote:
> Add a comment describing the mcount variants and how the callsites look
> like.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/kernel/entry-common.S |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 0b042bd..d412d7c 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -92,6 +92,38 @@ ENDPROC(ret_from_fork)
>  #define CALL(x) .long x
>  
>  #ifdef CONFIG_FUNCTION_TRACER
> +/*
> + * When compiling with -pg, gcc inserts a call to the mcount routine at the
> + * start of every function.  In mcount, apart from the function's address (in
> + * lr), we need to get hold of the function's caller's address.
> + *
> + * Older GCCs (pre-4.4) inserted a call to a routine called mcount like this:
> + *
> + *	bl	mcount
> + *
> + * These versions have the limitation that in order for the mcount routine to
> + * be able to determine the function's caller's address, an APCS-style frame
> + * pointer (which is set up with something like the code below) is required.
> + *
> + *	mov     ip, sp
> + *	push    {fp, ip, lr, pc}
> + *	sub     fp, ip, #4
> + *
> + * With EABI, these frame pointers are not available unless -mapcs-frame is
> + * specified, and if building as Thumb-2, not even then.
> + *
> + * Newer GCCs (4.4+) solve this problem by introducing a new version of mcount,
> + * with call sites like:
> + *
> + *	push	{lr}
> + *	bl	__gnu_mcount_nc
> + *
> + * With these compilers, frame pointers are not necessary.
> + *
> + * With both the mcount types, we need to restore the original lr before
> + * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
> + * No other registers should be clobbered.
> + */
Very nice.

Maybe make the last two sentences:

In the __gnu_mcount_nc case the ip register is clobbered which is OK as
the calling convention for ARM allow clobbering this value for
subroutines and it doesn't contain parameters.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 02/10] ARM: ftrace: document mcount formats

[Rabin Vincent]

On Sat, Feb 13, 2010 at 09:37:48PM +0100, Uwe Kleine-K?nig wrote:
> On Sun, Feb 14, 2010 at 01:18:30AM +0530, Rabin Vincent wrote:
> > + * With both the mcount types, we need to restore the original lr before
> > + * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
> > + * No other registers should be clobbered.
> > + */
> Very nice.
> 
> Maybe make the last two sentences:
> 
> In the __gnu_mcount_nc case the ip register is clobbered which is OK as
> the calling convention for ARM allow clobbering this value for
> subroutines and it doesn't contain parameters.

Won't quoting calling conventions here be misleading?  The mcounts don't
following normal convention for the other bits (we can't clobber even
the registers that are normally caller-saved, we need to restore lr,
etc.)

Rabin

[PATCH 02/10] ARM: ftrace: document mcount formats

[Uwe Kleine-König]

On Mon, Feb 22, 2010 at 11:36:24PM +0530, Rabin Vincent wrote:
> On Sat, Feb 13, 2010 at 09:37:48PM +0100, Uwe Kleine-K?nig wrote:
> > On Sun, Feb 14, 2010 at 01:18:30AM +0530, Rabin Vincent wrote:
> > > + * With both the mcount types, we need to restore the original lr before
> > > + * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
> > > + * No other registers should be clobbered.
> > > + */
> > Very nice.
> > 
> > Maybe make the last two sentences:
> > 
> > In the __gnu_mcount_nc case the ip register is clobbered which is OK as
> > the calling convention for ARM allow clobbering this value for
> > subroutines and it doesn't contain parameters.
> 
> Won't quoting calling conventions here be misleading?  The mcounts don't
> following normal convention for the other bits (we can't clobber even
> the registers that are normally caller-saved, we need to restore lr,
> etc.)
actually mcount is a function in the middle of a function call.  So it
must be transparent for both the caller and the callee.  As the called
function needs to know the return address, mcount must not clobber it.
And as ip is don't care and can be clobbered mcount can use it as it
likes.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 02/10] ARM: ftrace: document mcount formats

[Rabin Vincent]

On Mon, Feb 22, 2010 at 08:20:09PM +0100, Uwe Kleine-K?nig wrote:
> On Mon, Feb 22, 2010 at 11:36:24PM +0530, Rabin Vincent wrote:
> > On Sat, Feb 13, 2010 at 09:37:48PM +0100, Uwe Kleine-K?nig wrote:
> > > On Sun, Feb 14, 2010 at 01:18:30AM +0530, Rabin Vincent wrote:
> > > > + * With both the mcount types, we need to restore the original lr before
> > > > + * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
> > > > + * No other registers should be clobbered.
> > > > + */
> > > Very nice.
> > > 
> > > Maybe make the last two sentences:
> > > 
> > > In the __gnu_mcount_nc case the ip register is clobbered which is OK as
> > > the calling convention for ARM allow clobbering this value for
> > > subroutines and it doesn't contain parameters.
> > 
> > Won't quoting calling conventions here be misleading?  The mcounts don't
> > following normal convention for the other bits (we can't clobber even
> > the registers that are normally caller-saved, we need to restore lr,
> > etc.)
> actually mcount is a function in the middle of a function call.  So it
> must be transparent for both the caller and the callee.  As the called
> function needs to know the return address, mcount must not clobber it.
> And as ip is don't care and can be clobbered mcount can use it as it
> likes.

OK.  How about this for the last paragraph:

 mcount can be thought of as a function called in the middle of a
 subroutine call.  As such, it needs to be transparent for both the
 caller and the callee: the original lr needs to be restored when
 leaving mcount, and no registers should be clobbered.  (In the
 __gnu_mcount_nc implementation, we clobber the ip register.  This
 is OK because the ARM calling convention allows it to be clobbered
 in subroutines and doesn't use it to hold parameters.)

Rabin

[PATCH 02/10] ARM: ftrace: document mcount formats

[Uwe Kleine-König]

Hi Rabin,

> OK.  How about this for the last paragraph:
> 
>  mcount can be thought of as a function called in the middle of a
>  subroutine call.  As such, it needs to be transparent for both the
>  caller and the callee: the original lr needs to be restored when
>  leaving mcount, and no registers should be clobbered.  (In the
>  __gnu_mcount_nc implementation, we clobber the ip register.  This
>  is OK because the ARM calling convention allows it to be clobbered
>  in subroutines and doesn't use it to hold parameters.)
I rechecked the document describing the calling convention.  It writes
about ip:

Both the ARM- and Thumb-state BL instructions are unable to address the
full 32-bit address space, so it may be necessary for the linker to
insert a veneer between the calling routine and the called subroutine.
Veneers may also be needed to support ARM-Thumb inter-working or dynamic
linking.  Any veneer inserted must preserve the contents of all
registers except IP (r12) and the condition code flags; a conforming
program must assume that a veneer that alters IP may be inserted at any
branch instruction that is exposed to a relocation that supports inter-
working or long branches.

So the last sentence isn't completly accurate.  But describing the
actual situation is a bit overkill, so I'm OK with your suggestion.

Best regards and thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 02/10] ARM: ftrace: document mcount formats

[Frederic Weisbecker]

On Sun, Feb 14, 2010 at 01:18:30AM +0530, Rabin Vincent wrote:
> Add a comment describing the mcount variants and how the callsites look
> like.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>


Cool, this was really missing I think.


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>



> ---
>  arch/arm/kernel/entry-common.S |   32 ++++++++++++++++++++++++++++++++
>  1 files changed, 32 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 0b042bd..d412d7c 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -92,6 +92,38 @@ ENDPROC(ret_from_fork)
>  #define CALL(x) .long x
>  
>  #ifdef CONFIG_FUNCTION_TRACER
> +/*
> + * When compiling with -pg, gcc inserts a call to the mcount routine at the
> + * start of every function.  In mcount, apart from the function's address (in
> + * lr), we need to get hold of the function's caller's address.
> + *
> + * Older GCCs (pre-4.4) inserted a call to a routine called mcount like this:
> + *
> + *	bl	mcount
> + *
> + * These versions have the limitation that in order for the mcount routine to
> + * be able to determine the function's caller's address, an APCS-style frame
> + * pointer (which is set up with something like the code below) is required.
> + *
> + *	mov     ip, sp
> + *	push    {fp, ip, lr, pc}
> + *	sub     fp, ip, #4
> + *
> + * With EABI, these frame pointers are not available unless -mapcs-frame is
> + * specified, and if building as Thumb-2, not even then.
> + *
> + * Newer GCCs (4.4+) solve this problem by introducing a new version of mcount,
> + * with call sites like:
> + *
> + *	push	{lr}
> + *	bl	__gnu_mcount_nc
> + *
> + * With these compilers, frame pointers are not necessary.
> + *
> + * With both the mcount types, we need to restore the original lr before
> + * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
> + * No other registers should be clobbered.
> + */
>  #ifdef CONFIG_DYNAMIC_FTRACE
>  ENTRY(mcount)
>  	stmdb	sp!, {r0-r3, lr}
> -- 
> 1.6.6
> 

[PATCH 03/10] ftrace: allow building without frame pointers

[Rabin Vincent]

With current gcc, compiling with both -pg and -fomit-frame-pointer is
not allowed.  However, -pg can be used to build without actually
specying -fno-omit-frame-pointer, upon which the defaut behaviour for
the target will be used.

On ARM, it is not possible to build a Thumb-2 kernel with
-fno-omit-frame-pointer (FRAME_POINTERS depends on !THUMB2_KERNEL). In
order to support ftrace for Thumb-2, we need to be able to allow a
combination of FUNCTION_TRACER and !FRAME_POINTER.  We do this by
omitting -fomit-frame-pointer if ftrace is enabled.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 Makefile |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index 67d6cff..72d90de 100644
--- a/Makefile
+++ b/Makefile
@@ -546,8 +546,11 @@ endif
 ifdef CONFIG_FRAME_POINTER
 KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
 else
+ifndef CONFIG_FUNCTION_TRACER
+# -fomit-frame-pointer is incompatible with -pg
 KBUILD_CFLAGS	+= -fomit-frame-pointer
 endif
+endif
 
 ifdef CONFIG_DEBUG_INFO
 KBUILD_CFLAGS	+= -g
-- 
1.6.6

[PATCH 03/10] ftrace: allow building without frame pointers

[Frederic Weisbecker]

On Sun, Feb 14, 2010 at 01:18:31AM +0530, Rabin Vincent wrote:
> With current gcc, compiling with both -pg and -fomit-frame-pointer is
> not allowed.  However, -pg can be used to build without actually
> specying -fno-omit-frame-pointer, upon which the defaut behaviour for
> the target will be used.


Doh!....


 
> On ARM, it is not possible to build a Thumb-2 kernel with
> -fno-omit-frame-pointer (FRAME_POINTERS depends on !THUMB2_KERNEL). In
> order to support ftrace for Thumb-2, we need to be able to allow a
> combination of FUNCTION_TRACER and !FRAME_POINTER.  We do this by
> omitting -fomit-frame-pointer if ftrace is enabled.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>



I might be missing something subtle on this one, but it looks
good to me.

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>



> ---
>  Makefile |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 67d6cff..72d90de 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -546,8 +546,11 @@ endif
>  ifdef CONFIG_FRAME_POINTER
>  KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
>  else
> +ifndef CONFIG_FUNCTION_TRACER
> +# -fomit-frame-pointer is incompatible with -pg
>  KBUILD_CFLAGS	+= -fomit-frame-pointer
>  endif
> +endif
>  ifdef CONFIG_DEBUG_INFO
>  KBUILD_CFLAGS	+= -g
> -- 
> 1.6.6
> 

[PATCH 03/10] ftrace: allow building without frame pointers

[Steven Rostedt]

On Sun, 2010-02-14 at 01:18 +0530, Rabin Vincent wrote:
> With current gcc, compiling with both -pg and -fomit-frame-pointer is
> not allowed.  However, -pg can be used to build without actually
> specying -fno-omit-frame-pointer, upon which the defaut behaviour for
> the target will be used.
> 
> On ARM, it is not possible to build a Thumb-2 kernel with
> -fno-omit-frame-pointer (FRAME_POINTERS depends on !THUMB2_KERNEL). In
> order to support ftrace for Thumb-2, we need to be able to allow a
> combination of FUNCTION_TRACER and !FRAME_POINTER.  We do this by
> omitting -fomit-frame-pointer if ftrace is enabled.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  Makefile |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 67d6cff..72d90de 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -546,8 +546,11 @@ endif
>  ifdef CONFIG_FRAME_POINTER
>  KBUILD_CFLAGS	+= -fno-omit-frame-pointer -fno-optimize-sibling-calls
>  else
> +ifndef CONFIG_FUNCTION_TRACER

Is this needed? Selecting FUNCTION_TRACER also selects FRAME_POINTER,
which this change is in the else part of that #if.

-- Steve

> +# -fomit-frame-pointer is incompatible with -pg
>  KBUILD_CFLAGS	+= -fomit-frame-pointer
>  endif
> +endif
>  
>  ifdef CONFIG_DEBUG_INFO
>  KBUILD_CFLAGS	+= -g

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Rabin Vincent]

With a new enough GCC, ARM function tracing can be supported without the
need for frame pointers.  This is essential for Thumb-2 support, since
frame pointers aren't available then.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/Kconfig.debug         |    5 +++++
 arch/arm/kernel/armksyms.c     |    2 ++
 arch/arm/kernel/entry-common.S |    7 +++++++
 kernel/trace/Kconfig           |    2 +-
 4 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
index 5cb9326..5501253 100644
--- a/arch/arm/Kconfig.debug
+++ b/arch/arm/Kconfig.debug
@@ -27,6 +27,11 @@ config ARM_UNWIND
 	  the performance is not affected. Currently, this feature
 	  only works with EABI compilers. If unsure say Y.
 
+config OLD_MCOUNT
+	bool
+	depends on FUNCTION_TRACER && FRAME_POINTER
+	default y
+
 config DEBUG_USER
 	bool "Verbose user fault messages"
 	help
diff --git a/arch/arm/kernel/armksyms.c b/arch/arm/kernel/armksyms.c
index 8214bfe..e5e1e53 100644
--- a/arch/arm/kernel/armksyms.c
+++ b/arch/arm/kernel/armksyms.c
@@ -165,6 +165,8 @@ EXPORT_SYMBOL(_find_next_bit_be);
 #endif
 
 #ifdef CONFIG_FUNCTION_TRACER
+#ifdef CONFIG_OLD_MCOUNT
 EXPORT_SYMBOL(mcount);
+#endif
 EXPORT_SYMBOL(__gnu_mcount_nc);
 #endif
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index d412d7c..c98e3a3 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -169,6 +169,12 @@ gnu_trace:
 	ldmia	sp!, {r0-r3, ip, lr}
 	mov	pc, ip
 
+#ifdef CONFIG_OLD_MCOUNT
+/*
+ * This is under an ifdef in order to force link-time errors for people trying
+ * to build with !FRAME_POINTER with a GCC which doesn't use the new-style
+ * mcount.
+ */
 ENTRY(mcount)
 	stmdb	sp!, {r0-r3, lr}
 	ldr	r0, =ftrace_trace_function
@@ -187,6 +193,7 @@ trace:
 	mov	pc, r2
 	ldr	lr, [fp, #-4]			@ restore lr
 	ldmia	sp!, {r0-r3, pc}
+#endif
 
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
index 6c22d8a..7468ffe 100644
--- a/kernel/trace/Kconfig
+++ b/kernel/trace/Kconfig
@@ -126,7 +126,7 @@ if FTRACE
 config FUNCTION_TRACER
 	bool "Kernel Function Tracer"
 	depends on HAVE_FUNCTION_TRACER
-	select FRAME_POINTER
+	select FRAME_POINTER if (!ARM_UNWIND)
 	select KALLSYMS
 	select GENERIC_TRACER
 	select CONTEXT_SWITCH_TRACER
-- 
1.6.6

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Frederic Weisbecker]

On Sun, Feb 14, 2010 at 01:18:32AM +0530, Rabin Vincent wrote:
> With a new enough GCC, ARM function tracing can be supported without the
> need for frame pointers.  This is essential for Thumb-2 support, since
> frame pointers aren't available then.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/Kconfig.debug         |    5 +++++
>  arch/arm/kernel/armksyms.c     |    2 ++
>  arch/arm/kernel/entry-common.S |    7 +++++++
>  kernel/trace/Kconfig           |    2 +-
>  4 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/Kconfig.debug b/arch/arm/Kconfig.debug
> index 5cb9326..5501253 100644
> --- a/arch/arm/Kconfig.debug
> +++ b/arch/arm/Kconfig.debug
> @@ -27,6 +27,11 @@ config ARM_UNWIND
>  	  the performance is not affected. Currently, this feature
>  	  only works with EABI compilers. If unsure say Y.
>  
> +config OLD_MCOUNT
> +	bool
> +	depends on FUNCTION_TRACER && FRAME_POINTER
> +	default y
> +



Btw, you described that mcount is used in some gcc versions and
__gnu_mcount_nc in newers.
Shouldn't we have a gcc version dependency here? (not sure we can
do this from Kconfig though).



>  config DEBUG_USER
>  	bool "Verbose user fault messages"
>  	help
> diff --git a/arch/arm/kernel/armksyms.c b/arch/arm/kernel/armksyms.c
> index 8214bfe..e5e1e53 100644
> --- a/arch/arm/kernel/armksyms.c
> +++ b/arch/arm/kernel/armksyms.c
> @@ -165,6 +165,8 @@ EXPORT_SYMBOL(_find_next_bit_be);
>  #endif
>  
>  #ifdef CONFIG_FUNCTION_TRACER
> +#ifdef CONFIG_OLD_MCOUNT
>  EXPORT_SYMBOL(mcount);
> +#endif
>  EXPORT_SYMBOL(__gnu_mcount_nc);
>  #endif
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index d412d7c..c98e3a3 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -169,6 +169,12 @@ gnu_trace:
>  	ldmia	sp!, {r0-r3, ip, lr}
>  	mov	pc, ip
>  
> +#ifdef CONFIG_OLD_MCOUNT
> +/*
> + * This is under an ifdef in order to force link-time errors for people trying
> + * to build with !FRAME_POINTER with a GCC which doesn't use the new-style
> + * mcount.
> + */
>  ENTRY(mcount)
>  	stmdb	sp!, {r0-r3, lr}
>  	ldr	r0, =ftrace_trace_function
> @@ -187,6 +193,7 @@ trace:
>  	mov	pc, r2
>  	ldr	lr, [fp, #-4]			@ restore lr
>  	ldmia	sp!, {r0-r3, pc}
> +#endif
>  
>  #endif /* CONFIG_DYNAMIC_FTRACE */
>  
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 6c22d8a..7468ffe 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -126,7 +126,7 @@ if FTRACE
>  config FUNCTION_TRACER
>  	bool "Kernel Function Tracer"
>  	depends on HAVE_FUNCTION_TRACER
> -	select FRAME_POINTER
> +	select FRAME_POINTER if (!ARM_UNWIND)


So, if I understand well. If people have ARM_UNWIND but
FUNCTION_TRACER, it might crash at link time in case
they don't have a recent enough gcc version to
support the new -pg style?

That doesn't look good.

Ideally, we need HAVE_FUNCTION_TRACER to be enabled in arm
only if (old gcc && frame pointers) || new gcc

And then, we need config OLD_MCOUNT:
	 old gcc && FUNCTION_TRACER

and config NEW_MCOUNT:
	new gcc && FUNCTION_TRACER

so that we can selectively build mcount or __gnu_mcount_nc.

Hmm, I fear we can't check gcc version from Kconfig, as I'm
grepping on Kconfig files...

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Steven Rostedt]

On Mon, 2010-02-22 at 20:05 +0100, Frederic Weisbecker wrote:

> 
> Btw, you described that mcount is used in some gcc versions and
> __gnu_mcount_nc in newers.
> Shouldn't we have a gcc version dependency here? (not sure we can
> do this from Kconfig though).
> 

Maybe we can add something to recordmcount.pl?

> 
> 
> >  config DEBUG_USER
> >  	bool "Verbose user fault messages"
> >  	help
> > diff --git a/arch/arm/kernel/armksyms.c b/arch/arm/kernel/armksyms.c
> > index 8214bfe..e5e1e53 100644
> > --- a/arch/arm/kernel/armksyms.c
> > +++ b/arch/arm/kernel/armksyms.c
> > @@ -165,6 +165,8 @@ EXPORT_SYMBOL(_find_next_bit_be);
> >  #endif
> >  
> >  #ifdef CONFIG_FUNCTION_TRACER
> > +#ifdef CONFIG_OLD_MCOUNT
> >  EXPORT_SYMBOL(mcount);
> > +#endif
> >  EXPORT_SYMBOL(__gnu_mcount_nc);
> >  #endif
> > diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> > index d412d7c..c98e3a3 100644
> > --- a/arch/arm/kernel/entry-common.S
> > +++ b/arch/arm/kernel/entry-common.S
> > @@ -169,6 +169,12 @@ gnu_trace:
> >  	ldmia	sp!, {r0-r3, ip, lr}
> >  	mov	pc, ip
> >  
> > +#ifdef CONFIG_OLD_MCOUNT
> > +/*
> > + * This is under an ifdef in order to force link-time errors for people trying
> > + * to build with !FRAME_POINTER with a GCC which doesn't use the new-style
> > + * mcount.
> > + */
> >  ENTRY(mcount)
> >  	stmdb	sp!, {r0-r3, lr}
> >  	ldr	r0, =ftrace_trace_function
> > @@ -187,6 +193,7 @@ trace:
> >  	mov	pc, r2
> >  	ldr	lr, [fp, #-4]			@ restore lr
> >  	ldmia	sp!, {r0-r3, pc}
> > +#endif
> >  
> >  #endif /* CONFIG_DYNAMIC_FTRACE */
> >  
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index 6c22d8a..7468ffe 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -126,7 +126,7 @@ if FTRACE
> >  config FUNCTION_TRACER
> >  	bool "Kernel Function Tracer"
> >  	depends on HAVE_FUNCTION_TRACER
> > -	select FRAME_POINTER
> > +	select FRAME_POINTER if (!ARM_UNWIND)
> 
> 
> So, if I understand well. If people have ARM_UNWIND but
> FUNCTION_TRACER, it might crash at link time in case
> they don't have a recent enough gcc version to
> support the new -pg style?
> 
> That doesn't look good.
> 
> Ideally, we need HAVE_FUNCTION_TRACER to be enabled in arm
> only if (old gcc && frame pointers) || new gcc
> 
> And then, we need config OLD_MCOUNT:
> 	 old gcc && FUNCTION_TRACER
> 
> and config NEW_MCOUNT:
> 	new gcc && FUNCTION_TRACER
> 
> so that we can selectively build mcount or __gnu_mcount_nc.
> 
> Hmm, I fear we can't check gcc version from Kconfig, as I'm
> grepping on Kconfig files...


I would not check gcc version through KCONFIG, but you can have the gcc
version checked and define another macro in a header somewhere, like
asm/ftrace.h. Then we could do something different while it is
compiling.

Disclaimer:
I have to go back and read the entire thread, to know what exactly is
going on ;-)

-- Steve

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Frederic Weisbecker]

On Tue, Feb 23, 2010 at 08:18:33AM -0500, Steven Rostedt wrote:
> On Mon, 2010-02-22 at 20:05 +0100, Frederic Weisbecker wrote:
> 
> > 
> > Btw, you described that mcount is used in some gcc versions and
> > __gnu_mcount_nc in newers.
> > Shouldn't we have a gcc version dependency here? (not sure we can
> > do this from Kconfig though).
> > 
> 
> Maybe we can add something to recordmcount.pl?



May be yeah, with an explicit message that describes the issue.


 
> > 
> > 
> > >  config DEBUG_USER
> > >  	bool "Verbose user fault messages"
> > >  	help
> > > diff --git a/arch/arm/kernel/armksyms.c b/arch/arm/kernel/armksyms.c
> > > index 8214bfe..e5e1e53 100644
> > > --- a/arch/arm/kernel/armksyms.c
> > > +++ b/arch/arm/kernel/armksyms.c
> > > @@ -165,6 +165,8 @@ EXPORT_SYMBOL(_find_next_bit_be);
> > >  #endif
> > >  
> > >  #ifdef CONFIG_FUNCTION_TRACER
> > > +#ifdef CONFIG_OLD_MCOUNT
> > >  EXPORT_SYMBOL(mcount);
> > > +#endif
> > >  EXPORT_SYMBOL(__gnu_mcount_nc);
> > >  #endif
> > > diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> > > index d412d7c..c98e3a3 100644
> > > --- a/arch/arm/kernel/entry-common.S
> > > +++ b/arch/arm/kernel/entry-common.S
> > > @@ -169,6 +169,12 @@ gnu_trace:
> > >  	ldmia	sp!, {r0-r3, ip, lr}
> > >  	mov	pc, ip
> > >  
> > > +#ifdef CONFIG_OLD_MCOUNT
> > > +/*
> > > + * This is under an ifdef in order to force link-time errors for people trying
> > > + * to build with !FRAME_POINTER with a GCC which doesn't use the new-style
> > > + * mcount.
> > > + */
> > >  ENTRY(mcount)
> > >  	stmdb	sp!, {r0-r3, lr}
> > >  	ldr	r0, =ftrace_trace_function
> > > @@ -187,6 +193,7 @@ trace:
> > >  	mov	pc, r2
> > >  	ldr	lr, [fp, #-4]			@ restore lr
> > >  	ldmia	sp!, {r0-r3, pc}
> > > +#endif
> > >  
> > >  #endif /* CONFIG_DYNAMIC_FTRACE */
> > >  
> > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > > index 6c22d8a..7468ffe 100644
> > > --- a/kernel/trace/Kconfig
> > > +++ b/kernel/trace/Kconfig
> > > @@ -126,7 +126,7 @@ if FTRACE
> > >  config FUNCTION_TRACER
> > >  	bool "Kernel Function Tracer"
> > >  	depends on HAVE_FUNCTION_TRACER
> > > -	select FRAME_POINTER
> > > +	select FRAME_POINTER if (!ARM_UNWIND)
> > 
> > 
> > So, if I understand well. If people have ARM_UNWIND but
> > FUNCTION_TRACER, it might crash at link time in case
> > they don't have a recent enough gcc version to
> > support the new -pg style?
> > 
> > That doesn't look good.
> > 
> > Ideally, we need HAVE_FUNCTION_TRACER to be enabled in arm
> > only if (old gcc && frame pointers) || new gcc
> > 
> > And then, we need config OLD_MCOUNT:
> > 	 old gcc && FUNCTION_TRACER
> > 
> > and config NEW_MCOUNT:
> > 	new gcc && FUNCTION_TRACER
> > 
> > so that we can selectively build mcount or __gnu_mcount_nc.
> > 
> > Hmm, I fear we can't check gcc version from Kconfig, as I'm
> > grepping on Kconfig files...
> 
> 
> I would not check gcc version through KCONFIG, but you can have the gcc
> version checked and define another macro in a header somewhere, like
> asm/ftrace.h. Then we could do something different while it is
> compiling.


Yeah. I understand now why we can't do that on Kconfig time, HOSTCC
can be different than CC.

The most important thing is to have a message that describes the issue
if the compiler-config combination is impossible.

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Rabin Vincent]

On Tue, Feb 23, 2010 at 06:11:16PM +0100, Frederic Weisbecker wrote:
> On Tue, Feb 23, 2010 at 08:18:33AM -0500, Steven Rostedt wrote:
> > On Mon, 2010-02-22 at 20:05 +0100, Frederic Weisbecker wrote:
> > > 
> > > Btw, you described that mcount is used in some gcc versions and
> > > __gnu_mcount_nc in newers.
> > > Shouldn't we have a gcc version dependency here? (not sure we can
> > > do this from Kconfig though).
> > > 
> > Maybe we can add something to recordmcount.pl?
> 
> May be yeah, with an explicit message that describes the issue.

Adding something to recordmcount.pl to detect this would be
insufficient, because this affects non-dynamic ftrace too.

> > > > +++ b/arch/arm/kernel/entry-common.S
> > > > @@ -169,6 +169,12 @@ gnu_trace:
> > > >  	ldmia	sp!, {r0-r3, ip, lr}
> > > >  	mov	pc, ip
> > > >  
> > > > +#ifdef CONFIG_OLD_MCOUNT
> > > > +/*
> > > > + * This is under an ifdef in order to force link-time errors for people trying
> > > > + * to build with !FRAME_POINTER with a GCC which doesn't use the new-style
> > > > + * mcount.
> > > > + */
> > > >  ENTRY(mcount)
> > > >  	stmdb	sp!, {r0-r3, lr}
> > > >  	ldr	r0, =ftrace_trace_function
> > > > @@ -187,6 +193,7 @@ trace:
> > > >  	mov	pc, r2
> > > >  	ldr	lr, [fp, #-4]			@ restore lr
> > > >  	ldmia	sp!, {r0-r3, pc}
> > > > +#endif
> > > >  
> > > >  #endif /* CONFIG_DYNAMIC_FTRACE */
> > > >  
> > > > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > > > index 6c22d8a..7468ffe 100644
> > > > --- a/kernel/trace/Kconfig
> > > > +++ b/kernel/trace/Kconfig
> > > > @@ -126,7 +126,7 @@ if FTRACE
> > > >  config FUNCTION_TRACER
> > > >  	bool "Kernel Function Tracer"
> > > >  	depends on HAVE_FUNCTION_TRACER
> > > > -	select FRAME_POINTER
> > > > +	select FRAME_POINTER if (!ARM_UNWIND)
> > > 
> > > 
> > > So, if I understand well. If people have ARM_UNWIND but
> > > FUNCTION_TRACER, it might crash at link time in case
> > > they don't have a recent enough gcc version to
> > > support the new -pg style?

They'd probably get an "undefined reference to mcount" message.  I
forced the link time error because otherwise they'll have runtime
problems.

> > > 
> > > That doesn't look good.
> > > 
> > > Ideally, we need HAVE_FUNCTION_TRACER to be enabled in arm
> > > only if (old gcc && frame pointers) || new gcc
> > > 
> > > And then, we need config OLD_MCOUNT:
> > > 	 old gcc && FUNCTION_TRACER
> > > 
> > > and config NEW_MCOUNT:
> > > 	new gcc && FUNCTION_TRACER
> > > 
> > > so that we can selectively build mcount or __gnu_mcount_nc.

There's more to this.  When we build with FRAME_POINTER, we need to
always build in both version of mcount.  This is to allow modules to be
built with different versions of the compiler (one which uses the old
mcount and one with the new mcount).

> > > 
> > > Hmm, I fear we can't check gcc version from Kconfig, as I'm
> > > grepping on Kconfig files...
> > 
> > 
> > I would not check gcc version through KCONFIG, but you can have the gcc
> > version checked and define another macro in a header somewhere, like
> > asm/ftrace.h. Then we could do something different while it is
> > compiling.
> 
> Yeah. I understand now why we can't do that on Kconfig time, HOSTCC
> can be different than CC.
> 
> The most important thing is to have a message that describes the issue
> if the compiler-config combination is impossible.

The easiest option to have a more obvious message than a linker error
would be to add something like this in entry-common.S:

  #if (__GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 4)) \
	&& !defined(OLD_MCOUNT)
  #warning Ftrace on GCC < 4.4 requires frame pointers
  #endif

Messages like this are also present in other places in ARM (for example
arch/arm/kernel/unwind.c), so this wouldn't be this first.

Note that the above message would help in all cases except the one where
someone builds the kernel with !fp and GCC 4.4+, and then builds a
module with an older GCC.  That would still be only a linker error.

Rabin

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Steven Rostedt]

On Tue, 2010-02-23 at 23:28 +0530, Rabin Vincent wrote:

> The easiest option to have a more obvious message than a linker error
> would be to add something like this in entry-common.S:
> 
>   #if (__GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 4)) \
> 	&& !defined(OLD_MCOUNT)
>   #warning Ftrace on GCC < 4.4 requires frame pointers
>   #endif
> 
> Messages like this are also present in other places in ARM (for example
> arch/arm/kernel/unwind.c), so this wouldn't be this first.
> 
> Note that the above message would help in all cases except the one where
> someone builds the kernel with !fp and GCC 4.4+, and then builds a
> module with an older GCC.  That would still be only a linker error.

I was going to recommend the #if above. But shouldn't it be a #error
instead of a #warning?

-- Steve

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Rabin Vincent]

On Tue, Feb 23, 2010 at 01:03:05PM -0500, Steven Rostedt wrote:
> On Tue, 2010-02-23 at 23:28 +0530, Rabin Vincent wrote:
> > The easiest option to have a more obvious message than a linker error
> > would be to add something like this in entry-common.S:
> > 
> >   #if (__GNUC__ < 4 || (__GNUC__ == 4 && __GNUC_MINOR__ < 4)) \
> > 	&& !defined(OLD_MCOUNT)
> >   #warning Ftrace on GCC < 4.4 requires frame pointers
> >   #endif
> > 
> > Messages like this are also present in other places in ARM (for example
> > arch/arm/kernel/unwind.c), so this wouldn't be this first.
> > 
> > Note that the above message would help in all cases except the one where
> > someone builds the kernel with !fp and GCC 4.4+, and then builds a
> > module with an older GCC.  That would still be only a linker error.
> 
> I was going to recommend the #if above. But shouldn't it be a #error
> instead of a #warning?

Yes, I'll make it a #error.

(I had it at #warning in case some one had a custom compiler that had
the new mcount patch backported, and chose to ignore the warning.  But
it's probably better to leave it at #error and see if someone actually
complains about it, rather than have someone miss the message with it as
a #warning.)

Rabin

[PATCH 04/10] ARM: ftrace: allow building without frame pointers

[Steven Rostedt]

On Sun, 2010-02-14 at 01:18 +0530, Rabin Vincent wrote:
> With a new enough GCC, ARM function tracing can be supported without the
> need for frame pointers.  This is essential for Thumb-2 support, since
> frame pointers aren't available then.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>


>  
> diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> index 6c22d8a..7468ffe 100644
> --- a/kernel/trace/Kconfig
> +++ b/kernel/trace/Kconfig
> @@ -126,7 +126,7 @@ if FTRACE
>  config FUNCTION_TRACER
>  	bool "Kernel Function Tracer"
>  	depends on HAVE_FUNCTION_TRACER
> -	select FRAME_POINTER
> +	select FRAME_POINTER if (!ARM_UNWIND)
>  	select KALLSYMS
>  	select GENERIC_TRACER
>  	select CONTEXT_SWITCH_TRACER

Ah, evolution is keeping these patches out of order, so I did not see
this change before commenting about the change to the Makefile.

A better comment is needed in the Makefile.

-- Steve

[PATCH 05/10] ARM: ftrace: add ENDPROC annotations

[Rabin Vincent]

When building as Thumb-2, the ".type foo, %function" annotation in
ENDPROC seems to be required in order for the assembly routines to be
recognized as Thumb-2 code.  If the ENDPROC annotations are not present,
calls to these routines are generated as BLX instead of BL.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/entry-common.S |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index c98e3a3..42eb166 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -135,6 +135,7 @@ mcount_call:
 	bl	ftrace_stub
 	ldr	lr, [fp, #-4]			@ restore lr
 	ldmia	sp!, {r0-r3, pc}
+ENDPROC(mcount)
 
 ENTRY(ftrace_caller)
 	stmdb	sp!, {r0-r3, lr}
@@ -147,6 +148,7 @@ ftrace_call:
 	bl	ftrace_stub
 	ldr	lr, [fp, #-4]			@ restore lr
 	ldmia	sp!, {r0-r3, pc}
+ENDPROC(ftrace_caller)
 
 #else
 
@@ -168,6 +170,7 @@ gnu_trace:
 	mov	pc, r2
 	ldmia	sp!, {r0-r3, ip, lr}
 	mov	pc, ip
+ENDPROC(__gnu_mcount_nc)
 
 #ifdef CONFIG_OLD_MCOUNT
 /*
@@ -193,13 +196,15 @@ trace:
 	mov	pc, r2
 	ldr	lr, [fp, #-4]			@ restore lr
 	ldmia	sp!, {r0-r3, pc}
+ENDPROC(mcount)
 #endif
 
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 	.globl ftrace_stub
-ftrace_stub:
+ENTRY(ftrace_stub)
 	mov	pc, lr
+ENDPROC(ftrace_stub)
 
 #endif /* CONFIG_FUNCTION_TRACER */
 
-- 
1.6.6

[PATCH 05/10] ARM: ftrace: add ENDPROC annotations

[Catalin Marinas]

Rabin Vincent wrote:
> When building as Thumb-2, the ".type foo, %function" annotation in
> ENDPROC seems to be required in order for the assembly routines to be
> recognized as Thumb-2 code.  If the ENDPROC annotations are not present,
> calls to these routines are generated as BLX instead of BL.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Rabin Vincent]

Fix the mcount routines to build and run on a kernel built with the
Thumb-2 instruction set.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/entry-common.S |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 42eb166..cd2a574 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -156,7 +156,8 @@ ENTRY(__gnu_mcount_nc)
 	stmdb	sp!, {r0-r3, lr}
 	ldr	r0, =ftrace_trace_function
 	ldr	r2, [r0]
-	adr	r0, ftrace_stub
+ THUMB(	orr	r2, r2, #1		)
+	adr	r0, BSYM(ftrace_stub)
 	cmp	r0, r2
 	bne	gnu_trace
 	ldmia	sp!, {r0-r3, ip, lr}
@@ -166,8 +167,9 @@ gnu_trace:
 	ldr	r1, [sp, #20]			@ lr of instrumented routine
 	mov	r0, lr
 	sub	r0, r0, #MCOUNT_INSN_SIZE
-	mov	lr, pc
-	mov	pc, r2
+ ARM(	mov	lr, pc			)
+ ARM(	mov	pc, r2			)
+ THUMB(	blx	r2			)
 	ldmia	sp!, {r0-r3, ip, lr}
 	mov	pc, ip
 ENDPROC(__gnu_mcount_nc)
-- 
1.6.6

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Catalin Marinas]

Rabin Vincent wrote:
> Fix the mcount routines to build and run on a kernel built with the
> Thumb-2 instruction set.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/kernel/entry-common.S |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> index 42eb166..cd2a574 100644
> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -156,7 +156,8 @@ ENTRY(__gnu_mcount_nc)
>  	stmdb	sp!, {r0-r3, lr}
>  	ldr	r0, =ftrace_trace_function
>  	ldr	r2, [r0]
> -	adr	r0, ftrace_stub
> + THUMB(	orr	r2, r2, #1		)
> +	adr	r0, BSYM(ftrace_stub)
>  	cmp	r0, r2

Does this code not give the correct result if not modified?

>  	bne	gnu_trace
>  	ldmia	sp!, {r0-r3, ip, lr}
> @@ -166,8 +167,9 @@ gnu_trace:
>  	ldr	r1, [sp, #20]			@ lr of instrumented routine
>  	mov	r0, lr
>  	sub	r0, r0, #MCOUNT_INSN_SIZE
> -	mov	lr, pc
> -	mov	pc, r2
> + ARM(	mov	lr, pc			)
> + ARM(	mov	pc, r2			)
> + THUMB(	blx	r2			)
>  	ldmia	sp!, {r0-r3, ip, lr}
>  	mov	pc, ip
>  ENDPROC(__gnu_mcount_nc)

As above, what does this need modifying? "mov pc, r2" wouldn't change 
the mode to ARM even if the value in r2 is even. It may need THUMB(nop) 
after this instruction.

I could of course miss something as I haven't actually tried this code.

-- 
Catalin

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Rabin Vincent]

On Sat, Feb 13, 2010 at 11:27:28PM +0000, Catalin Marinas wrote:
> >diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
> >index 42eb166..cd2a574 100644
> >--- a/arch/arm/kernel/entry-common.S
> >+++ b/arch/arm/kernel/entry-common.S
> >@@ -156,7 +156,8 @@ ENTRY(__gnu_mcount_nc)
> > 	stmdb	sp!, {r0-r3, lr}
> > 	ldr	r0, =ftrace_trace_function
> > 	ldr	r2, [r0]
> >-	adr	r0, ftrace_stub
> >+ THUMB(	orr	r2, r2, #1		)
> >+	adr	r0, BSYM(ftrace_stub)
> > 	cmp	r0, r2
> 
> Does this code not give the correct result if not modified?

Without the BSYM, I get assembler errors:

entry-common.S: Assembler messages:
entry-common.S:179: Error: invalid immediate for address calculation (value = 0x00000004)

Without the orr, the lsb is not set on the pointer loaded from
ftrace_trace_function, but is set on BSYM(ftrace_stub), leading to the
comparison failing even when the pointer is pointing to ftrace_stub.

> 
> > 	bne	gnu_trace
> > 	ldmia	sp!, {r0-r3, ip, lr}
> >@@ -166,8 +167,9 @@ gnu_trace:
> > 	ldr	r1, [sp, #20]			@ lr of instrumented routine
> > 	mov	r0, lr
> > 	sub	r0, r0, #MCOUNT_INSN_SIZE
> >-	mov	lr, pc
> >-	mov	pc, r2
> >+ ARM(	mov	lr, pc			)
> >+ ARM(	mov	pc, r2			)
> >+ THUMB(	blx	r2			)
> > 	ldmia	sp!, {r0-r3, ip, lr}
> > 	mov	pc, ip
> > ENDPROC(__gnu_mcount_nc)
> 
> As above, what does this need modifying? "mov pc, r2" wouldn't
> change the mode to ARM even if the value in r2 is even. It may need
> THUMB(nop) after this instruction.

The "mov pc, r2" is not the problem.  The problem is the "mov lr, pc",
which does not set the lsb when storing the pc in lr.  The called
function returns with "bx lr", and the mode changes to ARM.  The blx is
to avoid this.

Rabin

[PATCH 07/10] ftrace: pass KBUILD_CFLAGS to record_mcount.pl

[Rabin Vincent]

On ARM, we have two ABIs, and the ABI used is controlled via a config
option.  Object files built with one ABI can't be merged with object
files built with the other ABI.  So, record_mcount.pl needs to use the
same compiler flags as the kernel when generating the object file with
the mcount locations.  Ensure this by passing CFLAGS to the script.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 scripts/Makefile.build |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 0b94d2f..2535c11 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -209,7 +209,8 @@ ifdef CONFIG_FTRACE_MCOUNT_RECORD
 cmd_record_mcount = set -e ; perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
 	"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
 	"$(if $(CONFIG_64BIT),64,32)" \
-	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" \
+	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC) $(KBUILD_CFLAGS)" \
+	"$(LD)" "$(NM)" "$(RM)" "$(MV)" \
 	"$(if $(part-of-module),1,0)" "$(@)";
 endif
 
-- 
1.6.6

[PATCH 07/10] ftrace: pass KBUILD_CFLAGS to record_mcount.pl

[Steven Rostedt]

On Sun, 2010-02-14 at 01:18 +0530, Rabin Vincent wrote:
> On ARM, we have two ABIs, and the ABI used is controlled via a config
> option.  Object files built with one ABI can't be merged with object
> files built with the other ABI.  So, record_mcount.pl needs to use the
> same compiler flags as the kernel when generating the object file with
> the mcount locations.  Ensure this by passing CFLAGS to the script.

I'm fine with this for now, but I'm wondering if we should pass the
autoconf.h to the script and parse that instead. This would give us all
set config options that we can look at.

But I need to go back and start looking at converting recordmcount.pl to
recordmcount.c again.

> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  scripts/Makefile.build |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 0b94d2f..2535c11 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -209,7 +209,8 @@ ifdef CONFIG_FTRACE_MCOUNT_RECORD
>  cmd_record_mcount = set -e ; perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
>  	"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
>  	"$(if $(CONFIG_64BIT),64,32)" \
> -	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" \
> +	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC) $(KBUILD_CFLAGS)" \
> +	"$(LD)" "$(NM)" "$(RM)" "$(MV)" \

Doesn't this need to be with the change of recordmcount.pl too?
Otherwise applying this patch alone will break it. We need every step of
the patches to work otherwise we risk breaking a kernel bisect.

-- Steve

>  	"$(if $(part-of-module),1,0)" "$(@)";
>  endif
>  

[PATCH 07/10] ftrace: pass KBUILD_CFLAGS to record_mcount.pl

[Rabin Vincent]

On Tue, Feb 23, 2010 at 08:30:46AM -0500, Steven Rostedt wrote:
> On Sun, 2010-02-14 at 01:18 +0530, Rabin Vincent wrote:
> > On ARM, we have two ABIs, and the ABI used is controlled via a config
> > option.  Object files built with one ABI can't be merged with object
> > files built with the other ABI.  So, record_mcount.pl needs to use the
> > same compiler flags as the kernel when generating the object file with
> > the mcount locations.  Ensure this by passing CFLAGS to the script.
> 
> I'm fine with this for now, but I'm wondering if we should pass the
> autoconf.h to the script and parse that instead. This would give us all
> set config options that we can look at.

The correct ABI options are constructed using cc-option and friends, and
that logic would need to be duplicated in recordmcount.pl if we had to
parse autoconf.h.

I originally tried to add a new RECORDMCOUNT_CFLAGS variable in the ARM
Makefile and have that used here instead of KBUILD_CFLAGS (since
KBUILD_CFLAGS includes a whole bunch of possibly irrelevant options),
but coudn't figure out why my new variable wasn't accessible in
Makefile.build.

> 
> But I need to go back and start looking at converting recordmcount.pl to
> recordmcount.c again.
> 
> > 
> > Signed-off-by: Rabin Vincent <rabin@rab.in>
> > ---
> >  scripts/Makefile.build |    3 ++-
> >  1 files changed, 2 insertions(+), 1 deletions(-)
> > 
> > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > index 0b94d2f..2535c11 100644
> > --- a/scripts/Makefile.build
> > +++ b/scripts/Makefile.build
> > @@ -209,7 +209,8 @@ ifdef CONFIG_FTRACE_MCOUNT_RECORD
> >  cmd_record_mcount = set -e ; perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
> >  	"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
> >  	"$(if $(CONFIG_64BIT),64,32)" \
> > -	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" \
> > +	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC) $(KBUILD_CFLAGS)" \
> > +	"$(LD)" "$(NM)" "$(RM)" "$(MV)" \
> 
> Doesn't this need to be with the change of recordmcount.pl too?
> Otherwise applying this patch alone will break it. We need every step of
> the patches to work otherwise we risk breaking a kernel bisect.

Dynamic ftrace for ARM is only enabled in the last patch of the series,
so this woudn't be breaking it.  No modification should be required for
record_mcount.pl for other archs because this doesn't add/remove any
parameters.

Rabin

[PATCH 07/10] ftrace: pass KBUILD_CFLAGS to record_mcount.pl

[Steven Rostedt]

On Tue, 2010-02-23 at 23:53 +0530, Rabin Vincent wrote:

> > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> > > index 0b94d2f..2535c11 100644
> > > --- a/scripts/Makefile.build
> > > +++ b/scripts/Makefile.build
> > > @@ -209,7 +209,8 @@ ifdef CONFIG_FTRACE_MCOUNT_RECORD
> > >  cmd_record_mcount = set -e ; perl $(srctree)/scripts/recordmcount.pl "$(ARCH)" \
> > >  	"$(if $(CONFIG_CPU_BIG_ENDIAN),big,little)" \
> > >  	"$(if $(CONFIG_64BIT),64,32)" \
> > > -	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" \
> > > +	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC) $(KBUILD_CFLAGS)" \
> > > +	"$(LD)" "$(NM)" "$(RM)" "$(MV)" \
> > 
> > Doesn't this need to be with the change of recordmcount.pl too?
> > Otherwise applying this patch alone will break it. We need every step of
> > the patches to work otherwise we risk breaking a kernel bisect.
> 
> Dynamic ftrace for ARM is only enabled in the last patch of the series,
> so this woudn't be breaking it.  No modification should be required for
> record_mcount.pl for other archs because this doesn't add/remove any
> parameters.

OK, it looked to me that "KBUILD_CFLAGS" was a separate parameter. I
miscounted my quotes. ;-)

-- Steve

[PATCH 08/10] ARM: ftrace: fix and update dynamic ftrace

[Rabin Vincent]

This adds mcount recording and updates dynamic ftrace for ARM to work
with the new ftrace dyamic tracing implementation.  It also adds support
for the mcount format used by newer ARM compilers.

With dynamic tracing, mcount() is implemented as a nop.  Callsites are
patched on startup with nops, and dynamically patched to call to the
ftrace_caller() routine as needed.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/include/asm/ftrace.h  |   19 +++++-
 arch/arm/kernel/entry-common.S |   37 +++++++---
 arch/arm/kernel/ftrace.c       |  155 +++++++++++++++++++++++++++------------
 scripts/recordmcount.pl        |    2 +
 4 files changed, 155 insertions(+), 58 deletions(-)

diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index 103f7ee..4a56a2e 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -2,12 +2,29 @@
 #define _ASM_ARM_FTRACE
 
 #ifdef CONFIG_FUNCTION_TRACER
-#define MCOUNT_ADDR		((long)(mcount))
+#define MCOUNT_ADDR		((unsigned long)(__gnu_mcount_nc))
 #define MCOUNT_INSN_SIZE	4 /* sizeof mcount call */
 
 #ifndef __ASSEMBLY__
 extern void mcount(void);
 extern void __gnu_mcount_nc(void);
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+struct dyn_arch_ftrace {
+#ifdef CONFIG_OLD_MCOUNT
+	bool	old_mcount;
+#endif
+};
+
+static inline unsigned long ftrace_call_adjust(unsigned long addr)
+{
+	return addr;
+}
+
+extern void ftrace_caller_old(void);
+extern void ftrace_call_old(void);
+#endif
+
 #endif
 
 #endif
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index cd2a574..736cd75 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -123,32 +123,51 @@ ENDPROC(ret_from_fork)
  * With both the mcount types, we need to restore the original lr before
  * returning.  In the __gnu_mcount_nc, version we're allowed to clobber ip.
  * No other registers should be clobbered.
+ *
+ * When using dynamic ftrace, we patch out the mcount call by a "mov r0, r0"
+ * for the mcount case, and a "pop {lr}" for the __gnu_mcount_nc case (see
+ * arch/arm/kernel/ftrace.c).
  */
 #ifdef CONFIG_DYNAMIC_FTRACE
-ENTRY(mcount)
+ENTRY(__gnu_mcount_nc)
+	mov	ip, lr
+	ldmia	sp!, {lr}
+	mov	pc, ip
+ENDPROC(__gnu_mcount_nc)
+
+ENTRY(ftrace_caller)
 	stmdb	sp!, {r0-r3, lr}
 	mov	r0, lr
 	sub	r0, r0, #MCOUNT_INSN_SIZE
+	ldr	r1, [sp, #20]
 
-	.globl mcount_call
-mcount_call:
+	.global	ftrace_call
+ftrace_call:
 	bl	ftrace_stub
-	ldr	lr, [fp, #-4]			@ restore lr
-	ldmia	sp!, {r0-r3, pc}
+	ldmia	sp!, {r0-r3, ip, lr}
+	mov	pc, ip
+ENDPROC(ftrace_caller)
+
+#ifdef CONFIG_OLD_MCOUNT
+ENTRY(mcount)
+	stmdb	sp!, {lr}
+	ldr	lr, [fp, #-4]
+	ldmia	sp!, {pc}
 ENDPROC(mcount)
 
-ENTRY(ftrace_caller)
+ENTRY(ftrace_caller_old)
 	stmdb	sp!, {r0-r3, lr}
 	ldr	r1, [fp, #-4]
 	mov	r0, lr
 	sub	r0, r0, #MCOUNT_INSN_SIZE
 
-	.globl ftrace_call
-ftrace_call:
+	.globl ftrace_call_old
+ftrace_call_old:
 	bl	ftrace_stub
 	ldr	lr, [fp, #-4]			@ restore lr
 	ldmia	sp!, {r0-r3, pc}
-ENDPROC(ftrace_caller)
+ENDPROC(ftrace_caller_old)
+#endif
 
 #else
 
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index c638427..f09014c 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -2,102 +2,161 @@
  * Dynamic function tracing support.
  *
  * Copyright (C) 2008 Abhishek Sagar <sagar.abhishek@gmail.com>
+ * Copyright (C) 2010 Rabin Vincent <rabin@rab.in>
  *
  * For licencing details, see COPYING.
  *
  * Defines low-level handling of mcount calls when the kernel
  * is compiled with the -pg flag. When using dynamic ftrace, the
- * mcount call-sites get patched lazily with NOP till they are
- * enabled. All code mutation routines here take effect atomically.
+ * mcount call-sites get patched with NOP till they are enabled.
+ * All code mutation routines here are called under stop_machine().
  */
 
 #include <linux/ftrace.h>
+#include <linux/uaccess.h>
 
 #include <asm/cacheflush.h>
 #include <asm/ftrace.h>
 
-#define PC_OFFSET      8
-#define BL_OPCODE      0xeb000000
-#define BL_OFFSET_MASK 0x00ffffff
+#define	NOP		0xe8bd4000	/* pop {lr} */
 
-static unsigned long bl_insn;
-static const unsigned long NOP = 0xe1a00000; /* mov r0, r0 */
+#ifdef CONFIG_OLD_MCOUNT
+#define OLD_MCOUNT_ADDR	((unsigned long) mcount)
+#define OLD_FTRACE_ADDR ((unsigned long) ftrace_caller_old)
 
-unsigned char *ftrace_nop_replace(void)
+#define	OLD_NOP		0xe1a00000	/* mov r0, r0 */
+
+static unsigned long ftrace_nop_replace(struct dyn_ftrace *rec)
+{
+	return rec->arch.old_mcount ? OLD_NOP : NOP;
+}
+
+static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr)
+{
+	if (!rec->arch.old_mcount)
+		return addr;
+
+	if (addr == MCOUNT_ADDR)
+		addr = OLD_MCOUNT_ADDR;
+	else if (addr == FTRACE_ADDR)
+		addr = OLD_FTRACE_ADDR;
+
+	return addr;
+}
+#else
+static unsigned long ftrace_nop_replace(struct dyn_ftrace *rec)
+{
+	return NOP;
+}
+
+static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr)
 {
-	return (char *)&NOP;
+	return addr;
 }
+#endif
 
 /* construct a branch (BL) instruction to addr */
-unsigned char *ftrace_call_replace(unsigned long pc, unsigned long addr)
+static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
 {
 	long offset;
 
-	offset = (long)addr - (long)(pc + PC_OFFSET);
+	offset = (long)addr - (long)(pc + 8);
 	if (unlikely(offset < -33554432 || offset > 33554428)) {
 		/* Can't generate branches that far (from ARM ARM). Ftrace
 		 * doesn't generate branches outside of kernel text.
 		 */
 		WARN_ON_ONCE(1);
-		return NULL;
+		return 0;
 	}
-	offset = (offset >> 2) & BL_OFFSET_MASK;
-	bl_insn = BL_OPCODE | offset;
-	return (unsigned char *)&bl_insn;
-}
 
-int ftrace_modify_code(unsigned long pc, unsigned char *old_code,
-		       unsigned char *new_code)
-{
-	unsigned long err = 0, replaced = 0, old, new;
+	offset = (offset >> 2) & 0x00ffffff;
 
-	old = *(unsigned long *)old_code;
-	new = *(unsigned long *)new_code;
+	return 0xeb000000 | offset;
+}
 
-	__asm__ __volatile__ (
-		"1:  ldr    %1, [%2]  \n"
-		"    cmp    %1, %4    \n"
-		"2:  streq  %3, [%2]  \n"
-		"    cmpne  %1, %3    \n"
-		"    movne  %0, #2    \n"
-		"3:\n"
+static int ftrace_modify_code(unsigned long pc, unsigned long old,
+			      unsigned long new)
+{
+	unsigned long replaced;
 
-		".section .fixup, \"ax\"\n"
-		"4:  mov  %0, #1  \n"
-		"    b    3b      \n"
-		".previous\n"
+	if (probe_kernel_read(&replaced, (void *)pc, MCOUNT_INSN_SIZE))
+		return -EFAULT;
 
-		".section __ex_table, \"a\"\n"
-		"    .long 1b, 4b \n"
-		"    .long 2b, 4b \n"
-		".previous\n"
+	if (replaced != old)
+		return -EINVAL;
 
-		: "=r"(err), "=r"(replaced)
-		: "r"(pc), "r"(new), "r"(old), "0"(err), "1"(replaced)
-		: "memory");
+	if (probe_kernel_write((void *)pc, &new, MCOUNT_INSN_SIZE))
+		return -EPERM;
 
-	if (!err && (replaced == old))
-		flush_icache_range(pc, pc + MCOUNT_INSN_SIZE);
+	flush_icache_range(pc, pc + MCOUNT_INSN_SIZE);
 
-	return err;
+	return 0;
 }
 
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
-	int ret;
 	unsigned long pc, old;
-	unsigned char *new;
+	unsigned long new;
+	int ret;
 
 	pc = (unsigned long)&ftrace_call;
 	memcpy(&old, &ftrace_call, MCOUNT_INSN_SIZE);
 	new = ftrace_call_replace(pc, (unsigned long)func);
-	ret = ftrace_modify_code(pc, (unsigned char *)&old, new);
+
+	ret = ftrace_modify_code(pc, old, new);
+
+#ifdef CONFIG_OLD_MCOUNT
+	if (!ret) {
+		pc = (unsigned long)&ftrace_call_old;
+		memcpy(&old, &ftrace_call_old, MCOUNT_INSN_SIZE);
+		new = ftrace_call_replace(pc, (unsigned long)func);
+
+		ret = ftrace_modify_code(pc, old, new);
+	}
+#endif
+
+	return ret;
+}
+
+int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned long new, old;
+	unsigned long ip = rec->ip;
+
+	old = ftrace_nop_replace(rec);
+	new = ftrace_call_replace(ip, adjust_address(rec, addr));
+
+	return ftrace_modify_code(rec->ip, old, new);
+}
+
+int ftrace_make_nop(struct module *mod,
+		    struct dyn_ftrace *rec, unsigned long addr)
+{
+	unsigned long ip = rec->ip;
+	unsigned long old;
+	unsigned long new;
+	int ret;
+
+	old = ftrace_call_replace(ip, adjust_address(rec, addr));
+	new = ftrace_nop_replace(rec);
+	ret = ftrace_modify_code(ip, old, new);
+
+#ifdef CONFIG_OLD_MCOUNT
+	if (ret == -EINVAL && addr == MCOUNT_ADDR) {
+		rec->arch.old_mcount = true;
+
+		old = ftrace_call_replace(ip, adjust_address(rec, addr));
+		new = ftrace_nop_replace(rec);
+		ret = ftrace_modify_code(ip, old, new);
+	}
+#endif
+
 	return ret;
 }
 
-/* run from ftrace_init with irqs disabled */
 int __init ftrace_dyn_arch_init(void *data)
 {
-	ftrace_mcount_set(data);
+	*(unsigned long *)data = 0;
+
 	return 0;
 }
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index ea6f6e3..66c1c4b 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -267,6 +267,8 @@ if ($arch eq "x86_64") {
 } elsif ($arch eq "arm") {
     $alignment = 2;
     $section_type = '%progbits';
+    $mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*R_ARM_(CALL|PC24)" .
+			"\\s+(__gnu_mcount_nc|mcount)\$";
 
 } elsif ($arch eq "ia64") {
     $mcount_regex = "^\\s*([0-9a-fA-F]+):.*\\s_mcount\$";
-- 
1.6.6

[PATCH 08/10] ARM: ftrace: fix and update dynamic ftrace

[Uwe Kleine-König]

Hello,

On Sun, Feb 14, 2010 at 01:18:36AM +0530, Rabin Vincent wrote:
> This adds mcount recording and updates dynamic ftrace for ARM to work
> with the new ftrace dyamic tracing implementation.  It also adds support
> for the mcount format used by newer ARM compilers.
> 
> With dynamic tracing, mcount() is implemented as a nop.  Callsites are
> patched on startup with nops, and dynamically patched to call to the
> ftrace_caller() routine as needed.
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/include/asm/ftrace.h  |   19 +++++-
>  arch/arm/kernel/entry-common.S |   37 +++++++---
>  arch/arm/kernel/ftrace.c       |  155 +++++++++++++++++++++++++++------------
>  scripts/recordmcount.pl        |    2 +
>  4 files changed, 155 insertions(+), 58 deletions(-)
> 
> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> index 103f7ee..4a56a2e 100644
> --- a/arch/arm/include/asm/ftrace.h
> +++ b/arch/arm/include/asm/ftrace.h
> @@ -2,12 +2,29 @@
>  #define _ASM_ARM_FTRACE
>  
>  #ifdef CONFIG_FUNCTION_TRACER
> -#define MCOUNT_ADDR		((long)(mcount))
> +#define MCOUNT_ADDR		((unsigned long)(__gnu_mcount_nc))
hhmm, does this work with both the old and the new mcount function?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 08/10] ARM: ftrace: fix and update dynamic ftrace

[Rabin Vincent]

On Sun, Feb 14, 2010 at 12:08:40PM +0100, Uwe Kleine-K?nig wrote:
> > diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> > index 103f7ee..4a56a2e 100644
> > --- a/arch/arm/include/asm/ftrace.h
> > +++ b/arch/arm/include/asm/ftrace.h
> > @@ -2,12 +2,29 @@
> >  #define _ASM_ARM_FTRACE
> >  
> >  #ifdef CONFIG_FUNCTION_TRACER
> > -#define MCOUNT_ADDR		((long)(mcount))
> > +#define MCOUNT_ADDR		((unsigned long)(__gnu_mcount_nc))
> hhmm, does this work with both the old and the new mcount function?

Yes, MCOUNT_ADDR is not used by the generic ftrace code directly, but is
instead only passed to our arch specific functions.  We determine the
right mcount address to use there:

> +#define OLD_MCOUNT_ADDR	((unsigned long) mcount)
[..]
> +static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr)
> +{
> +	if (!rec->arch.old_mcount)
> +		return addr;
> +
> +	if (addr == MCOUNT_ADDR)
> +		addr = OLD_MCOUNT_ADDR;
> +	else if (addr == FTRACE_ADDR)
> +		addr = OLD_FTRACE_ADDR;
> +
> +	return addr;
> +}

I changed MCOUNT_ADDR from mcount to __gnu_mcount_nc because we always
build __gnu_mcount_nc in, but don't build mcount when using Thumb-2.

Rabin

[PATCH 09/10] ARM: ftrace: add Thumb-2 support to dynamic ftrace

[Rabin Vincent]

Handle the different nop and call instructions for Thumb-2.  Also, we
need to adjust the recorded mcount_loc addresses because they have the
lsb set.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/include/asm/ftrace.h |    4 ++++
 arch/arm/kernel/ftrace.c      |   33 +++++++++++++++++++++++++++++++++
 scripts/recordmcount.pl       |    2 +-
 3 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
index 4a56a2e..0845f2d 100644
--- a/arch/arm/include/asm/ftrace.h
+++ b/arch/arm/include/asm/ftrace.h
@@ -18,7 +18,11 @@ struct dyn_arch_ftrace {
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
+#ifdef CONFIG_THUMB2_KERNEL
+	return addr & ~1;
+#else
 	return addr;
+#endif
 }
 
 extern void ftrace_caller_old(void);
diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
index f09014c..971ac8c 100644
--- a/arch/arm/kernel/ftrace.c
+++ b/arch/arm/kernel/ftrace.c
@@ -18,7 +18,11 @@
 #include <asm/cacheflush.h>
 #include <asm/ftrace.h>
 
+#ifdef CONFIG_THUMB2_KERNEL
+#define	NOP		0xeb04f85d	/* pop.w {lr} */
+#else
 #define	NOP		0xe8bd4000	/* pop {lr} */
+#endif
 
 #ifdef CONFIG_OLD_MCOUNT
 #define OLD_MCOUNT_ADDR	((unsigned long) mcount)
@@ -56,6 +60,34 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr)
 #endif
 
 /* construct a branch (BL) instruction to addr */
+#ifdef CONFIG_THUMB2_KERNEL
+static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
+{
+	unsigned long s, j1, j2, i1, i2, imm10, imm11;
+	unsigned long first, second;
+	long offset;
+
+	offset = (long)addr - (long)(pc + 4);
+	if (offset < -16777216 || offset > 16777214) {
+		WARN_ON_ONCE(1);
+		return 0;
+	}
+
+	s	= (offset >> 24) & 0x1;
+	i1	= (offset >> 23) & 0x1;
+	i2	= (offset >> 22) & 0x1;
+	imm10	= (offset >> 12) & 0x3ff;
+	imm11	= (offset >>  1) & 0x7ff;
+
+	j1 = (!i1) ^ s;
+	j2 = (!i2) ^ s;
+
+	first = 0xf000 | (s << 10) | imm10;
+	second = 0xd000 | (j1 << 13) | (j2 << 11) | imm11;
+
+	return (second << 16) | first;
+}
+#else
 static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
 {
 	long offset;
@@ -73,6 +105,7 @@ static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
 
 	return 0xeb000000 | offset;
 }
+#endif
 
 static int ftrace_modify_code(unsigned long pc, unsigned long old,
 			      unsigned long new)
diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
index 66c1c4b..88b083b 100755
--- a/scripts/recordmcount.pl
+++ b/scripts/recordmcount.pl
@@ -267,7 +267,7 @@ if ($arch eq "x86_64") {
 } elsif ($arch eq "arm") {
     $alignment = 2;
     $section_type = '%progbits';
-    $mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*R_ARM_(CALL|PC24)" .
+    $mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*R_ARM_(CALL|PC24|THM_CALL)" .
 			"\\s+(__gnu_mcount_nc|mcount)\$";
 
 } elsif ($arch eq "ia64") {
-- 
1.6.6

[PATCH 09/10] ARM: ftrace: add Thumb-2 support to dynamic ftrace

[Steven Rostedt]

On Sun, 2010-02-14 at 01:18 +0530, Rabin Vincent wrote:
> Handle the different nop and call instructions for Thumb-2.  Also, we
> need to adjust the recorded mcount_loc addresses because they have the
> lsb set.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Rabin Vincent <rabin@rab.in>
> ---
>  arch/arm/include/asm/ftrace.h |    4 ++++
>  arch/arm/kernel/ftrace.c      |   33 +++++++++++++++++++++++++++++++++
>  scripts/recordmcount.pl       |    2 +-
>  3 files changed, 38 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/ftrace.h b/arch/arm/include/asm/ftrace.h
> index 4a56a2e..0845f2d 100644
> --- a/arch/arm/include/asm/ftrace.h
> +++ b/arch/arm/include/asm/ftrace.h
> @@ -18,7 +18,11 @@ struct dyn_arch_ftrace {
>  
>  static inline unsigned long ftrace_call_adjust(unsigned long addr)
>  {
> +#ifdef CONFIG_THUMB2_KERNEL
> +	return addr & ~1;

Would it be safe to do that for all arm archs? Instead of adding a
config option around it. Or do some arm archs need the lsb set.

I'm just saying a comment here would look better than #ifdef #else.

> +#else
>  	return addr;
> +#endif
>  }
>  
>  extern void ftrace_caller_old(void);
> diff --git a/arch/arm/kernel/ftrace.c b/arch/arm/kernel/ftrace.c
> index f09014c..971ac8c 100644
> --- a/arch/arm/kernel/ftrace.c
> +++ b/arch/arm/kernel/ftrace.c
> @@ -18,7 +18,11 @@
>  #include <asm/cacheflush.h>
>  #include <asm/ftrace.h>
>  
> +#ifdef CONFIG_THUMB2_KERNEL
> +#define	NOP		0xeb04f85d	/* pop.w {lr} */
> +#else
>  #define	NOP		0xe8bd4000	/* pop {lr} */
> +#endif
>  
>  #ifdef CONFIG_OLD_MCOUNT
>  #define OLD_MCOUNT_ADDR	((unsigned long) mcount)
> @@ -56,6 +60,34 @@ static unsigned long adjust_address(struct dyn_ftrace *rec, unsigned long addr)
>  #endif
>  
>  /* construct a branch (BL) instruction to addr */
> +#ifdef CONFIG_THUMB2_KERNEL
> +static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
> +{
> +	unsigned long s, j1, j2, i1, i2, imm10, imm11;
> +	unsigned long first, second;
> +	long offset;
> +
> +	offset = (long)addr - (long)(pc + 4);
> +	if (offset < -16777216 || offset > 16777214) {
> +		WARN_ON_ONCE(1);
> +		return 0;
> +	}
> +
> +	s	= (offset >> 24) & 0x1;
> +	i1	= (offset >> 23) & 0x1;
> +	i2	= (offset >> 22) & 0x1;
> +	imm10	= (offset >> 12) & 0x3ff;
> +	imm11	= (offset >>  1) & 0x7ff;
> +
> +	j1 = (!i1) ^ s;
> +	j2 = (!i2) ^ s;
> +
> +	first = 0xf000 | (s << 10) | imm10;
> +	second = 0xd000 | (j1 << 13) | (j2 << 11) | imm11;
> +
> +	return (second << 16) | first;
> +}
> +#else
>  static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
>  {
>  	long offset;
> @@ -73,6 +105,7 @@ static unsigned long ftrace_call_replace(unsigned long pc, unsigned long addr)
>  
>  	return 0xeb000000 | offset;
>  }
> +#endif
>  
>  static int ftrace_modify_code(unsigned long pc, unsigned long old,
>  			      unsigned long new)
> diff --git a/scripts/recordmcount.pl b/scripts/recordmcount.pl
> index 66c1c4b..88b083b 100755
> --- a/scripts/recordmcount.pl
> +++ b/scripts/recordmcount.pl

For the recordmcount.pl change;

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> @@ -267,7 +267,7 @@ if ($arch eq "x86_64") {
>  } elsif ($arch eq "arm") {
>      $alignment = 2;
>      $section_type = '%progbits';
> -    $mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*R_ARM_(CALL|PC24)" .
> +    $mcount_regex = "^\\s*([0-9a-fA-F]+):\\s*R_ARM_(CALL|PC24|THM_CALL)" .
>  			"\\s+(__gnu_mcount_nc|mcount)\$";
>  
>  } elsif ($arch eq "ia64") {

[PATCH 09/10] ARM: ftrace: add Thumb-2 support to dynamic ftrace

[Rabin Vincent]

On Tue, Feb 23, 2010 at 08:35:00AM -0500, Steven Rostedt wrote:
> On Sun, 2010-02-14 at 01:18 +0530, Rabin Vincent wrote:
> > --- a/arch/arm/include/asm/ftrace.h
> > +++ b/arch/arm/include/asm/ftrace.h
> > @@ -18,7 +18,11 @@ struct dyn_arch_ftrace {
> >  
> >  static inline unsigned long ftrace_call_adjust(unsigned long addr)
> >  {
> > +#ifdef CONFIG_THUMB2_KERNEL
> > +	return addr & ~1;
> 
> Would it be safe to do that for all arm archs? Instead of adding a
> config option around it. Or do some arm archs need the lsb set.
> 
> I'm just saying a comment here would look better than #ifdef #else.

You're right; I'll change this.  It's safe even when we build for the
ARM instruction set.

Rabin

[PATCH 10/10] ARM: ftrace: enable dynamic ftrace

[Rabin Vincent]

Dynamic ftrace for ARM has been disabled since 07c4cc1cdaa08f ("ftrace:
disable dynamic ftrace for all archs that use daemon").  Now that the
code has been updated, re-enable it.

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/Kconfig |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 4c33ca8..eee22bb 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -17,6 +17,8 @@ config ARM
 	select HAVE_KPROBES if (!XIP_KERNEL)
 	select HAVE_KRETPROBES if (HAVE_KPROBES)
 	select HAVE_FUNCTION_TRACER if (!XIP_KERNEL)
+	select HAVE_FTRACE_MCOUNT_RECORD if (!XIP_KERNEL)
+	select HAVE_DYNAMIC_FTRACE if (!XIP_KERNEL)
 	select HAVE_GENERIC_DMA_COHERENT
 	select HAVE_KERNEL_GZIP
 	select HAVE_KERNEL_LZO
-- 
1.6.6

[PATCH 00/10] ARM: ftrace: cleanups, Thumb-2, and dynamic ftrace

[Rabin Vincent]

Hi Steven / Frederic / Ingo,

On Sun, Feb 14, 2010 at 01:18:28AM +0530, Rabin Vincent wrote:
> This series contains fixes and improvements to the ARM ftrace support.  It adds
> Thumb-2 support and re-implements the dynamic ftrace support.
> 
> "ftrace: allow building without frame pointers" and "ftrace: pass KBUILD_CFLAGS
> to record_mcount.pl" are non-ARM-specific ftrace patches which are required to
> support the ARM functionality.

Ping.  Any comments on this?

Rabin

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Rabin Vincent]

Fix the mcount routines to build and run on a kernel built with the
Thumb-2 instruction set by correcting the following errors using the
fixes suggested by Catalin Marinas:

 - Problem: The following assembler errors appear at the "adr r0,
   ftrace_stub" instruction:

   entry-common.S: Assembler messages:
   entry-common.S:179: Error: invalid immediate for address calculation (value = 0x00000004)

   Fix: The errors don't occur with a non-global symbol, so use one.

 - Problem: The "mov lr, pc" does not set the lsb when storing the pc in
   lr.  The called function returns with "bx lr", and the mode changes
   to ARM.

   Fix: Add a label on the return address and use "adr lr, BSYM(label)".

We don't modify the old mcount because it won't be built when using
Thumb-2.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/entry-common.S |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index c7a8c20..f5e75de 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -167,7 +167,7 @@ ENTRY(__gnu_mcount_nc)
 	stmdb	sp!, {r0-r3, lr}
 	ldr	r0, =ftrace_trace_function
 	ldr	r2, [r0]
-	adr	r0, ftrace_stub
+	adr	r0, .Lftrace_stub
 	cmp	r0, r2
 	bne	gnu_trace
 	ldmia	sp!, {r0-r3, ip, lr}
@@ -177,8 +177,9 @@ gnu_trace:
 	ldr	r1, [sp, #20]			@ lr of instrumented routine
 	mov	r0, lr
 	sub	r0, r0, #MCOUNT_INSN_SIZE
-	mov	lr, pc
+	adr	lr, BSYM(1f)
 	mov	pc, r2
+1:
 	ldmia	sp!, {r0-r3, ip, lr}
 	mov	pc, ip
 ENDPROC(__gnu_mcount_nc)
@@ -213,6 +214,7 @@ ENDPROC(mcount)
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
 ENTRY(ftrace_stub)
+.Lftrace_stub:
 	mov	pc, lr
 ENDPROC(ftrace_stub)
 
-- 
1.7.0

[PATCH 00/10] ARM: ftrace: cleanups, Thumb-2, and dynamic ftrace (v2)

[Rabin Vincent]

v2: Fixes as per review comments.  The only code changes are the #error in 0004
and the removal of the #ifdef in 0009.  The rest are comments and commit
message rewording/expansion.

This series contains fixes and improvements to the ARM ftrace support.  It adds
Thumb-2 support and re-implements the dynamic ftrace support.

"ftrace: allow building without frame pointers" and "ftrace: pass KBUILD_CFLAGS
to record_mcount.pl" are non-ARM-specific ftrace patches which are required to
support the ARM functionality.

Cleanups / docs:

  ARM: ftrace: clean up mcount assembly indentation
  ARM: ftrace: document mcount formats

Thumb-2:

  ftrace: allow building without frame pointers
  ARM: ftrace: allow building without frame pointers
  ARM: ftrace: add ENDPROC annotations
  ARM: ftrace: add Thumb-2 support

Dynamic ftrace:

  ftrace: pass KBUILD_CFLAGS to record_mcount.pl
  ARM: ftrace: fix and update dynamic ftrace
  ARM: ftrace: add Thumb-2 support to dynamic ftrace
  ARM: ftrace: enable dynamic ftrace

 Makefile                       |    7 ++
 arch/arm/Kconfig               |    2 +
 arch/arm/Kconfig.debug         |    5 +
 arch/arm/include/asm/ftrace.h  |   20 ++++-
 arch/arm/kernel/armksyms.c     |    2 +
 arch/arm/kernel/entry-common.S |  178 +++++++++++++++++++++++++++-----------
 arch/arm/kernel/ftrace.c       |  188 +++++++++++++++++++++++++++++----------
 kernel/trace/Kconfig           |    2 +-
 scripts/Makefile.build         |    3 +-
 scripts/recordmcount.pl        |    2 +
 10 files changed, 307 insertions(+), 102 deletions(-)

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Rabin Vincent]

Fix the mcount routines to build and run on a kernel built with the
Thumb-2 instruction set:

 - Without the BSYM, the following assembler errors appear:

   entry-common.S: Assembler messages:
   entry-common.S:179: Error: invalid immediate for address calculation (value = 0x00000004)

 - Without the orr, the lsb is not set on the pointer loaded from
   ftrace_trace_function, but is set on BSYM(ftrace_stub), leading to the
   comparison failing even when the pointer is pointing to ftrace_stub.

 - The problem with the "mov lr, pc", is that it does not set the lsb when
   storing the pc in lr.  The called function returns with "bx lr", and the
   mode changes to ARM.  The blx is to avoid this.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/kernel/entry-common.S |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index c3bdb05..573ed3b 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -167,7 +167,8 @@ ENTRY(__gnu_mcount_nc)
 	stmdb	sp!, {r0-r3, lr}
 	ldr	r0, =ftrace_trace_function
 	ldr	r2, [r0]
-	adr	r0, ftrace_stub
+ THUMB(	orr	r2, r2, #1		)
+	adr	r0, BSYM(ftrace_stub)
 	cmp	r0, r2
 	bne	gnu_trace
 	ldmia	sp!, {r0-r3, ip, lr}
@@ -177,8 +178,9 @@ gnu_trace:
 	ldr	r1, [sp, #20]			@ lr of instrumented routine
 	mov	r0, lr
 	sub	r0, r0, #MCOUNT_INSN_SIZE
-	mov	lr, pc
-	mov	pc, r2
+ ARM(	mov	lr, pc			)
+ ARM(	mov	pc, r2			)
+ THUMB(	blx	r2			)
 	ldmia	sp!, {r0-r3, ip, lr}
 	mov	pc, ip
 ENDPROC(__gnu_mcount_nc)
-- 
1.7.0

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Catalin Marinas]

On Sat, 2010-03-13 at 06:49 +0000, Rabin Vincent wrote:
>  - The problem with the "mov lr, pc", is that it does not set the lsb when
>    storing the pc in lr.  The called function returns with "bx lr", and the
>    mode changes to ARM.  The blx is to avoid this.

I'm not familiar with ftrace but why does the called function returns
using "bx lr". Is this generated by the compiler? I had the impression
that if we don't enable interworking, we wouldn't get this instruction
(but haven't tried this yet).

-- 
Catalin

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Rabin Vincent]

On Sun, Mar 14, 2010 at 10:30:15PM +0000, Catalin Marinas wrote:
> On Sat, 2010-03-13 at 06:49 +0000, Rabin Vincent wrote:
> >  - The problem with the "mov lr, pc", is that it does not set the lsb when
> >    storing the pc in lr.  The called function returns with "bx lr", and the
> >    mode changes to ARM.  The blx is to avoid this.
> 
> I'm not familiar with ftrace but why does the called function returns
> using "bx lr". Is this generated by the compiler? I had the impression
> that if we don't enable interworking, we wouldn't get this instruction
> (but haven't tried this yet).

There's nothing special about the called function: it's just a regular C
function.  GCC uses "bx lr" for the return.

Rabin

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Catalin Marinas]

Hi Rabin,

Rabin Vincent <rabin@rab.in> wrote:
> Fix the mcount routines to build and run on a kernel built with the
> Thumb-2 instruction set:
>
>  - Without the BSYM, the following assembler errors appear:
>
>    entry-common.S: Assembler messages:
>    entry-common.S:179: Error: invalid immediate for address calculation (value = 0x00000004)

I'm still confused by this. I think that's a compiler problem but need
to get some feedback from toolchain people.

> --- a/arch/arm/kernel/entry-common.S
> +++ b/arch/arm/kernel/entry-common.S
> @@ -167,7 +167,8 @@ ENTRY(__gnu_mcount_nc)
>  	stmdb	sp!, {r0-r3, lr}
>  	ldr	r0, =ftrace_trace_function
>  	ldr	r2, [r0]
> -	adr	r0, ftrace_stub
> + THUMB(	orr	r2, r2, #1		)
> +	adr	r0, BSYM(ftrace_stub)

If the ftrace_stub isn't .globl, the code compiles fine. My approach
would be something like this:

diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index d085033..5f5aef6 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -122,7 +122,7 @@ ENTRY(__gnu_mcount_nc)
 	stmdb sp!, {r0-r3, lr}
 	ldr r0, =ftrace_trace_function
 	ldr r2, [r0]
-	adr r0, ftrace_stub
+	adr r0, 1f
 	cmp r0, r2
 	bne gnu_trace
 	ldmia sp!, {r0-r3, ip, lr}
@@ -132,8 +132,9 @@ gnu_trace:
 	ldr r1, [sp, #20]			@ lr of instrumented routine
 	mov r0, lr
 	sub r0, r0, #MCOUNT_INSN_SIZE
-	mov lr, pc
+	adr lr, BSYM(2f)
 	mov pc, r2
+2:
 	ldmia sp!, {r0-r3, ip, lr}
 	mov pc, ip
 
@@ -141,7 +142,7 @@ ENTRY(mcount)
 	stmdb sp!, {r0-r3, lr}
 	ldr r0, =ftrace_trace_function
 	ldr r2, [r0]
-	adr r0, ftrace_stub
+	adr r0, 1f
 	cmp r0, r2
 	bne trace
 	ldr lr, [fp, #-4]			@ restore lr
@@ -160,6 +161,7 @@ trace:
 
 	.globl ftrace_stub
 ftrace_stub:
+1:
 	mov pc, lr
 
 #endif /* CONFIG_FUNCTION_TRACER */

-- 
Catalin

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Rabin Vincent]

On Tue, Mar 16, 2010 at 10:23:54AM +0000, Catalin Marinas wrote:
> If the ftrace_stub isn't .globl, the code compiles fine. My approach
> would be something like this:

New patch below:

[PATCH 06/10] ARM: ftrace: add Thumb-2 support

[Catalin Marinas]

On Wed, 2010-03-31 at 19:25 +0100, Rabin Vincent wrote:
> From bf828a0c069b1bb3f6bf4e68f1dceecab396c286 Mon Sep 17 00:00:00 2001
> From: Rabin Vincent <rabin@rab.in>
> Date: Sun, 14 Feb 2010 01:18:34 +0530
> Subject: [PATCH 06/10] ARM: ftrace: add Thumb-2 support
> 
> Fix the mcount routines to build and run on a kernel built with the
> Thumb-2 instruction set by correcting the following errors using the
> fixes suggested by Catalin Marinas:
> 
>  - Problem: The following assembler errors appear at the "adr r0,
>    ftrace_stub" instruction:
> 
>    entry-common.S: Assembler messages:
>    entry-common.S:179: Error: invalid immediate for address calculation (value = 0x00000004)
> 
>    Fix: The errors don't occur with a non-global symbol, so use one.
> 
>  - Problem: The "mov lr, pc" does not set the lsb when storing the pc in
>    lr.  The called function returns with "bx lr", and the mode changes
>    to ARM.
> 
>    Fix: Add a label on the return address and use "adr lr, BSYM(label)".
> 
> We don't modify the old mcount because it won't be built when using
> Thumb-2.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

-- 
Catalin