tracing: convert trace_clock_local() as weak function

The implementation of function graph tracer for MIPS is a little different from X86.

in MIPS, gcc(with -pg) only transfer the caller's return address(at) and the _mcount's return address(ra) to us.

move at, ra
jal _mcount

in the function is a leaf, it will no save the return address(ra):

ffffffff80101298 <au1k_wait>:

ffffffff80101298:       67bdfff0        daddiu  sp,sp,-16
ffffffff8010129c:       ffbe0008        sd      s8,8(sp)
ffffffff801012a0:       03a0f02d        move    s8,sp
ffffffff801012a4:       03e0082d        move    at,ra
ffffffff801012a8:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff801012ac:       00020021        nop

so, we can hijack it directly in _mcount, but if the function is non-leaf, the return address is saved in the stack.

ffffffff80133030 <copy_process>:

ffffffff80133030:       67bdff50        daddiu  sp,sp,-176
ffffffff80133034:       ffbe00a0        sd      s8,160(sp)
ffffffff80133038:       03a0f02d        move    s8,sp
ffffffff8013303c:       ffbf00a8        sd      ra,168(sp)
ffffffff80133040:       ffb70098        sd      s7,152(sp)
ffffffff80133044:       ffb60090        sd      s6,144(sp)
ffffffff80133048:       ffb50088        sd      s5,136(sp)
ffffffff8013304c:       ffb40080        sd      s4,128(sp)
ffffffff80133050:       ffb30078        sd      s3,120(sp)
ffffffff80133054:       ffb20070        sd      s2,112(sp)
ffffffff80133058:       ffb10068        sd      s1,104(sp)
ffffffff8013305c:       ffb00060        sd      s0,96(sp)
ffffffff80133060:       03e0082d        move    at,ra
ffffffff80133064:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff80133068:       00020021        nop

but we can not get the exact stack address(which saved ra) directly in _mcount, we need to search the content of at register in the stack space or search the "s{d,w} ra, offset(sp)" instruction in the text. 'Cause we can not prove there is only a match in the stack space, so, we search the text instead.

as we can see, if the first instruction above "move at, ra" is "move s8, sp"(move fp, sp), it is a leaf function, so we hijack the at register directly via put &return_to_handler into it, and otherwise, we search the "s{d,w} ra, offset(sp)" instruction to get the stack offset, and then the stack address. we use the above copy_process() as an example, we at last find "ffbf00a8", 0xa8 is the stack offset, we plus it with s8(fp), that is the stack address, we hijack the content via writing the &return_to_handler in.

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/Kconfig              |    1 +
 arch/mips/kernel/ftrace.c      |  192 ++++++++++++++++++++++++++++++++++++++++
 arch/mips/kernel/mcount.S      |   55 +++++++++++-
 arch/mips/kernel/vmlinux.lds.S |    1 +
 4 files changed, 248 insertions(+), 1 deletions(-)

arch/mips/Kconfig +1 -0

index 147fbbc..de690fd 100644
@@ -8,6 +8,7 @@ config MIPS
 	select HAVE_FUNCTION_TRACE_MCOUNT_TEST
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
+	select HAVE_FUNCTION_GRAPH_TRACER
 	# Horrible source of confusion.  Die, die, die ...
 	select EMBEDDED
 	select RTC_LIB if !LEMOTE_FULOONG2E

arch/mips/kernel/ftrace.c +67 -0

index 1e44865..fddee5b 100644
@@ -13,6 +13,8 @@
 #include <linux/ftrace.h>
 
 #include <asm/cacheflush.h>
+#include <asm/asm.h>
+#include <asm/asm-offsets.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 

@@ -105,3 +107,193 @@ int __init ftrace_dyn_arch_init(void *data)
 	return 0;
 }
 #endif				/* CONFIG_DYNAMIC_FTRACE */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+#define JMP	0x08000000	/* jump to target directly */
+extern void ftrace_graph_call(void);
+
+int ftrace_enable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	int ret;
+
+	/* j ftrace_stub */
+	memcpy(old, (unsigned long *)ip, MCOUNT_INSN_SIZE);
+	new = ftrace_call_replace(JMP, (unsigned long)ftrace_graph_caller);
+
+	ret = ftrace_modify_code(ip, old, new);
+
+	return ret;
+}
+
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	int ret;
+
+	/* j ftrace_graph_caller */
+	memcpy(old, (unsigned long *)ip, MCOUNT_INSN_SIZE);
+	new = ftrace_call_replace(JMP, (unsigned long)ftrace_stub);
+
+	ret = ftrace_modify_code(ip, old, new);
+
+	return ret;
+}
+
+#endif				/* !CONFIG_DYNAMIC_FTRACE */
+
+#define S_RA    (0x2fbf << 16)	/* 32bit: afbf, 64bit: ffbf */
+/* This is only available when enabled -fno-omit-frame-pointer with CONFIG_FRAME_POINTER=y */
+#define MOV_FP_SP       0x03a0f021	/* 32bit: 0x03a0f021, 64bit: 0x03a0f02d */
+#define STACK_OFFSET_MASK	0xfff	/* stack offset range: 0 ~ PT_SIZE(304) */
+
+unsigned long ftrace_get_parent_addr(unsigned long self_addr,
+				     unsigned long parent,
+				     unsigned long parent_addr,
+				     unsigned long fp)
+{
+	unsigned long sp, ip, ra;
+	unsigned int code;
+
+	/* move to the instruction "move ra, at" */
+	ip = self_addr - 8;
+
+	/* search the text until finding the "move s8, sp" instruction or
+	 * "s{d,w} ra, offset(sp)" instruction */
+	do {
+		ip -= 4;
+		/* read the text we want to match */
+		if (probe_kernel_read(&code, (void *)ip, 4)) {
+			WARN_ON(1);
+			panic("read the text failure\n");
+		}

arch/mips/kernel/ftrace.c +115 -0

+	if (ra == parent)
+		return sp;
+	else
+		panic
+		    ("failed on getting stack address of ra\n: addr: 0x%lx, code: 0x%x\n",
+		     ip, code);
+}
+
+/*
+ * Hook the return address and push it in the stack of return addrs
+ * in current thread info.
+ */
+void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
+			   unsigned long fp)
+{
+	unsigned long old;
+	int faulted;
+	struct ftrace_graph_ent trace;
+	unsigned long return_hooker = (unsigned long)
+	    &return_to_handler;
+
+	/*
+	 * Protect against fault, even if it shouldn't
+	 * happen. This tool is too much intrusive to
+	 * ignore such a protection.
+	 *
+	 * old = *parent;
+	 */
+	asm volatile (
+		"1: " STR(PTR_L) " %[old], 0(%[parent])\n"
+		"   li %[faulted], 0\n"
+		"2:\n"
+
+		".section .fixup, \"ax\"\n"
+		"3: li %[faulted], 1\n"
+		"   j 2b\n"
+		".previous\n"
+
+		".section\t__ex_table,\"a\"\n\t"
+		STR(PTR) "\t1b, 3b\n\t"
+		".previous\n"
+
+		: [old] "=&r" (old), [faulted] "=r" (faulted)
+		: [parent] "r" (parent)
+		: "memory"
+	);
+
+	if (unlikely(faulted)) {
+		ftrace_graph_stop();
+		WARN_ON(1);
+		return;
+	}
+
+	/* The argument *parent only work for leaf function, we need to check
+	 * whether the function is leaf, if not, we need to get the real stack
+	 * address which stored it.
+	 *
+	 * and here, we must stop tracing before calling probe_kernel_read().
+	 * after calling it, restart tracing. otherwise, it will hang there.*/
+	tracing_stop();
+	parent =
+	    (unsigned long *)ftrace_get_parent_addr(self_addr, old,
+						    (unsigned long)parent, fp);
+	tracing_start();
+
+	if (unlikely(atomic_read(&current->tracing_graph_pause)))
+		return;
+
+	/*
+	 * Protect against fault, even if it shouldn't
+	 * happen. This tool is too much intrusive to
+	 * ignore such a protection.
+	 *
+	 * *parent = return_hooker;
+	 */
+	asm volatile (
+		"1: " STR(PTR_S) " %[return_hooker], 0(%[parent])\n"
+		"   li %[faulted], 0\n"
+		"2:\n"
+
+		".section .fixup, \"ax\"\n"
+		"3: li %[faulted], 1\n"
+		"   j 2b\n"
+		".previous\n"
+
+		".section\t__ex_table,\"a\"\n\t"
+		STR(PTR) "\t1b, 3b\n\t"
+		".previous\n"
+
+		: [faulted] "=r" (faulted)
+		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)
+		: "memory"
+	);
+
+	if (unlikely(faulted)) {
+		ftrace_graph_stop();
+		WARN_ON(1);
+		return;
+	}
+
+	if (ftrace_push_return_trace(old, self_addr, &trace.depth, fp) ==
+	    -EBUSY) {
+		*parent = old;
+		return;
+	}
+
+	trace.func = self_addr;
+
+	/* Only trace if the calling function expects to */
+	if (!ftrace_graph_entry(&trace)) {
+		current->curr_ret_stack--;
+		*parent = old;
+	}
+}
+#endif				/* CONFIG_FUNCTION_GRAPH_TRACER */

arch/mips/kernel/mcount.S +54 -1

index 30637e6..356e81c 100644
@@ -89,6 +89,14 @@ ftrace_call:
 	nop
 
 	MCOUNT_RESTORE_REGS
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_graph_call
+ftrace_graph_call:
+	j	ftrace_stub
+	nop
+#endif
+
 	.globl ftrace_stub
 ftrace_stub:
 	RETURN_BACK

@@ -106,7 +114,15 @@ NESTED(_mcount, PT_SIZE, ra)
 	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */
 	bne	t0, t1, static_trace
 	nop
-
+#ifdef	CONFIG_FUNCTION_GRAPH_TRACER
+	PTR_L	t2, ftrace_graph_return
+	bne	t0, t2, ftrace_graph_caller
+	nop
+	PTR_LA	t0, ftrace_graph_entry_stub
+	PTR_L	t2, ftrace_graph_entry
+	bne	t0, t2, ftrace_graph_caller
+	nop
+#endif
 	j	ftrace_stub
 	nop
 

@@ -125,5 +141,42 @@ ftrace_stub:
 
 #endif	/* ! CONFIG_DYNAMIC_FTRACE */
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+
+NESTED(ftrace_graph_caller, PT_SIZE, ra)
+	MCOUNT_SAVE_REGS
+
+	PTR_LA	a0, PT_R1(sp)	/* arg1: &AT -> a0 */
+	move	a1, ra		/* arg2: next ip, selfaddr */
+	PTR_SUBU a1, MCOUNT_INSN_SIZE
+	move	a2, fp		/* arg3: frame pointer */
+	jal	prepare_ftrace_return
+	nop
+
+	MCOUNT_RESTORE_REGS
+	RETURN_BACK
+	END(ftrace_graph_caller)
+
+	.align	2
+	.globl	return_to_handler
+return_to_handler:
+	PTR_SUBU	sp, PT_SIZE
+	PTR_S	v0, PT_R2(sp)
+	PTR_S	v1, PT_R3(sp)
+
+	jal	ftrace_return_to_handler
+	nop
+
+	/* restore the real parent address: v0 -> ra */
+	move	ra, v0
+
+	PTR_L	v0, PT_R2(sp)
+	PTR_L	v1, PT_R3(sp)
+	PTR_ADDIU	sp, PT_SIZE
+
+	jr	ra
+	nop
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	.set at
 	.set reorder

arch/mips/kernel/vmlinux.lds.S +1 -0

index 162b299..f25df73 100644
@@ -46,6 +46,7 @@ SECTIONS
 		SCHED_TEXT
 		LOCK_TEXT
 		KPROBES_TEXT
+		IRQENTRY_TEXT
 		*(.text.*)
 		*(.fixup)
 		*(.gnu.warning)

Quoted 26 lines

arch/mips/kernel/ftrace.c +26 -0

+unsigned long ftrace_get_parent_addr(unsigned long self_addr,
+				     unsigned long parent,
+				     unsigned long parent_addr,
+				     unsigned long fp)
+{
+	unsigned long sp, ip, ra;
+	unsigned int code;
+
+	/* move to the instruction "move ra, at" */
+	ip = self_addr - 8;
+
+	/* search the text until finding the "move s8, sp" instruction or
+	 * "s{d,w} ra, offset(sp)" instruction */
+	do {
+		ip -= 4;
+		/* read the text we want to match */
+		if (probe_kernel_read(&code, (void *)ip, 4)) {
+			WARN_ON(1);
+			panic("read the text failure\n");
+		}
+
+		/* if the first instruction above "move at, ra" is "move
+		 * s8(fp), sp", means the function is a leaf */
+		if ((code & MOV_FP_SP) == MOV_FP_SP)
+			return parent_addr;
+	} while (((code & S_RA) != S_RA));

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 184

Quoted 4 lines

arch/mips/kernel/ftrace.c +4 -0

+
+	sp = fp + (code & STACK_OFFSET_MASK);
+	ra = *(unsigned long *)sp;
+

Seems missed the fault protection here? is there a need? never met fault in this place and also the following two places, so, are we safe to remove all of the fault protection?

Regards
Wu Zhangjin

Quoted 8 lines

arch/mips/kernel/ftrace.c +8 -0

+	if (ra == parent)
+		return sp;
+	else
+		panic
+		    ("failed on getting stack address of ra\n: addr: 0x%lx, code: 0x%x\n",
+		     ip, code);
+}
+

The implementation of function graph tracer for MIPS is a little different from X86.

in MIPS, gcc(with -pg) only transfer the caller's return address(at) and the _mcount's return address(ra) to us.

move at, ra
jal _mcount

in the function is a leaf, it will no save the return address(ra):

ffffffff80101298 <au1k_wait>:

ffffffff80101298:       67bdfff0        daddiu  sp,sp,-16
ffffffff8010129c:       ffbe0008        sd      s8,8(sp)
ffffffff801012a0:       03a0f02d        move    s8,sp
ffffffff801012a4:       03e0082d        move    at,ra
ffffffff801012a8:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff801012ac:       00020021        nop

so, we can hijack it directly in _mcount, but if the function is non-leaf, the return address is saved in the stack.

ffffffff80133030 <copy_process>:

ffffffff80133030:       67bdff50        daddiu  sp,sp,-176
ffffffff80133034:       ffbe00a0        sd      s8,160(sp)
ffffffff80133038:       03a0f02d        move    s8,sp
ffffffff8013303c:       ffbf00a8        sd      ra,168(sp)
ffffffff80133040:       ffb70098        sd      s7,152(sp)
ffffffff80133044:       ffb60090        sd      s6,144(sp)
ffffffff80133048:       ffb50088        sd      s5,136(sp)
ffffffff8013304c:       ffb40080        sd      s4,128(sp)
ffffffff80133050:       ffb30078        sd      s3,120(sp)
ffffffff80133054:       ffb20070        sd      s2,112(sp)
ffffffff80133058:       ffb10068        sd      s1,104(sp)
ffffffff8013305c:       ffb00060        sd      s0,96(sp)
ffffffff80133060:       03e0082d        move    at,ra
ffffffff80133064:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff80133068:       00020021        nop

but we can not get the exact stack address(which saved ra) directly in _mcount, we need to search the content of at register in the stack space or search the "s{d,w} ra, offset(sp)" instruction in the text. 'Cause we can not prove there is only a match in the stack space, so, we search the text instead.

as we can see, if the first instruction above "move at, ra" is "move s8, sp"(move fp, sp), it is a leaf function, so we hijack the at register

directly via put &return_to_handler into it, and otherwise, we search the "s{d,w} ra, offset(sp)" instruction to get the stack offset, and then the stack address. we use the above copy_process() as an example, we at last find "ffbf00a8", 0xa8 is the stack offset, we plus it with s8(fp), that is the stack address, we hijack the content via writing the &return_to_handler in.

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/Kconfig              |    1 +
 arch/mips/kernel/ftrace.c      |  192 ++++++++++++++++++++++++++++++++++++++++
 arch/mips/kernel/mcount.S      |   55 +++++++++++-
 arch/mips/kernel/vmlinux.lds.S |    1 +
 4 files changed, 248 insertions(+), 1 deletions(-)

arch/mips/Kconfig +1 -0

index 147fbbc..de690fd 100644
@@ -8,6 +8,7 @@ config MIPS
 	select HAVE_FUNCTION_TRACE_MCOUNT_TEST
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
+	select HAVE_FUNCTION_GRAPH_TRACER
 	# Horrible source of confusion.  Die, die, die ...
 	select EMBEDDED
 	select RTC_LIB if !LEMOTE_FULOONG2E

arch/mips/kernel/ftrace.c +54 -0

index 1e44865..fddee5b 100644
@@ -13,6 +13,8 @@
 #include <linux/ftrace.h>
 
 #include <asm/cacheflush.h>
+#include <asm/asm.h>
+#include <asm/asm-offsets.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 

@@ -105,3 +107,193 @@ int __init ftrace_dyn_arch_init(void *data)
 	return 0;
 }
 #endif				/* CONFIG_DYNAMIC_FTRACE */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+#define JMP	0x08000000	/* jump to target directly */
+extern void ftrace_graph_call(void);
+
+int ftrace_enable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	int ret;
+
+	/* j ftrace_stub */
+	memcpy(old, (unsigned long *)ip, MCOUNT_INSN_SIZE);
+	new = ftrace_call_replace(JMP, (unsigned long)ftrace_graph_caller);
+
+	ret = ftrace_modify_code(ip, old, new);
+
+	return ret;
+}
+
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	int ret;
+
+	/* j ftrace_graph_caller */
+	memcpy(old, (unsigned long *)ip, MCOUNT_INSN_SIZE);
+	new = ftrace_call_replace(JMP, (unsigned long)ftrace_stub);
+
+	ret = ftrace_modify_code(ip, old, new);
+
+	return ret;
+}
+
+#endif				/* !CONFIG_DYNAMIC_FTRACE */
+
+#define S_RA    (0x2fbf << 16)	/* 32bit: afbf, 64bit: ffbf */
+/* This is only available when enabled -fno-omit-frame-pointer with CONFIG_FRAME_POINTER=y */
+#define MOV_FP_SP       0x03a0f021	/* 32bit: 0x03a0f021, 64bit: 0x03a0f02d */
+#define STACK_OFFSET_MASK	0xfff	/* stack offset range: 0 ~ PT_SIZE(304) */
+
+unsigned long ftrace_get_parent_addr(unsigned long self_addr,
+				     unsigned long parent,
+				     unsigned long parent_addr,
+				     unsigned long fp)
+{
+	unsigned long sp, ip, ra;
+	unsigned int code;

arch/mips/kernel/ftrace.c +34 -0

+}
+
+/*
+ * Hook the return address and push it in the stack of return addrs
+ * in current thread info.
+ */
+void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
+			   unsigned long fp)
+{
+	unsigned long old;
+	int faulted;
+	struct ftrace_graph_ent trace;
+	unsigned long return_hooker = (unsigned long)
+	    &return_to_handler;
+
+	/*
+	 * Protect against fault, even if it shouldn't
+	 * happen. This tool is too much intrusive to
+	 * ignore such a protection.
+	 *
+	 * old = *parent;
+	 */
+	asm volatile (
+		"1: " STR(PTR_L) " %[old], 0(%[parent])\n"
+		"   li %[faulted], 0\n"
+		"2:\n"
+
+		".section .fixup, \"ax\"\n"
+		"3: li %[faulted], 1\n"
+		"   j 2b\n"
+		".previous\n"
+
+		".section\t__ex_table,\"a\"\n\t"
+		STR(PTR) "\t1b, 3b\n\t"

arch/mips/kernel/ftrace.c +23 -0

+	parent =
+	    (unsigned long *)ftrace_get_parent_addr(self_addr, old,
+						    (unsigned long)parent, fp);
+	tracing_start();
+
+	if (unlikely(atomic_read(&current->tracing_graph_pause)))
+		return;
+
+	/*
+	 * Protect against fault, even if it shouldn't
+	 * happen. This tool is too much intrusive to
+	 * ignore such a protection.
+	 *
+	 * *parent = return_hooker;
+	 */
+	asm volatile (
+		"1: " STR(PTR_S) " %[return_hooker], 0(%[parent])\n"
+		"   li %[faulted], 0\n"
+		"2:\n"
+
+		".section .fixup, \"ax\"\n"
+		"3: li %[faulted], 1\n"
+		"   j 2b\n"

arch/mips/kernel/ftrace.c +3 -0

+	}
+}
+#endif				/* CONFIG_FUNCTION_GRAPH_TRACER */

arch/mips/kernel/mcount.S +17 -1

index 30637e6..356e81c 100644
@@ -89,6 +89,14 @@ ftrace_call:
 	nop
 
 	MCOUNT_RESTORE_REGS
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_graph_call
+ftrace_graph_call:
+	j	ftrace_stub
+	nop
+#endif
+
 	.globl ftrace_stub
 ftrace_stub:
 	RETURN_BACK

@@ -106,7 +114,15 @@ NESTED(_mcount, PT_SIZE, ra)
 	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */
 	bne	t0, t1, static_trace
 	nop
-
+#ifdef	CONFIG_FUNCTION_GRAPH_TRACER
+	PTR_L	t2, ftrace_graph_return
+	bne	t0, t2, ftrace_graph_caller
+	nop
+	PTR_LA	t0, ftrace_graph_entry_stub
+	PTR_L	t2, ftrace_graph_entry
+	bne	t0, t2, ftrace_graph_caller
+	nop
+#endif
 	j	ftrace_stub
 	nop
 

arch/mips/kernel/mcount.S +31 -0

+	move	a1, ra		/* arg2: next ip, selfaddr */
+	PTR_SUBU a1, MCOUNT_INSN_SIZE
+	move	a2, fp		/* arg3: frame pointer */
+	jal	prepare_ftrace_return
+	nop
+
+	MCOUNT_RESTORE_REGS
+	RETURN_BACK
+	END(ftrace_graph_caller)
+
+	.align	2
+	.globl	return_to_handler
+return_to_handler:
+	PTR_SUBU	sp, PT_SIZE
+	PTR_S	v0, PT_R2(sp)
+	PTR_S	v1, PT_R3(sp)
+
+	jal	ftrace_return_to_handler
+	nop
+
+	/* restore the real parent address: v0 -> ra */
+	move	ra, v0
+
+	PTR_L	v0, PT_R2(sp)
+	PTR_L	v1, PT_R3(sp)
+	PTR_ADDIU	sp, PT_SIZE
+
+	jr	ra
+	nop
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	.set at
 	.set reorder

arch/mips/kernel/vmlinux.lds.S +0 -0

index 162b299..f25df73 100644

arch/mips/kernel/vmlinux.lds.S +0 -0

 		*(.text.*)
 		*(.fixup)
 		*(.gnu.warning)

Quoted 45 lines · Wu Zhangjin

The implementation of function graph tracer for MIPS is a little different from X86.

in MIPS, gcc(with -pg) only transfer the caller's return address(at) and the _mcount's return address(ra) to us.

move at, ra
jal _mcount

in the function is a leaf, it will no save the return address(ra):

ffffffff80101298 <au1k_wait>:

ffffffff80101298:       67bdfff0        daddiu  sp,sp,-16
ffffffff8010129c:       ffbe0008        sd      s8,8(sp)
ffffffff801012a0:       03a0f02d        move    s8,sp
ffffffff801012a4:       03e0082d        move    at,ra
ffffffff801012a8:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff801012ac:       00020021        nop

so, we can hijack it directly in _mcount, but if the function is non-leaf, the return address is saved in the stack.

ffffffff80133030 <copy_process>:

ffffffff80133030:       67bdff50        daddiu  sp,sp,-176
ffffffff80133034:       ffbe00a0        sd      s8,160(sp)
ffffffff80133038:       03a0f02d        move    s8,sp
ffffffff8013303c:       ffbf00a8        sd      ra,168(sp)
ffffffff80133040:       ffb70098        sd      s7,152(sp)
ffffffff80133044:       ffb60090        sd      s6,144(sp)
ffffffff80133048:       ffb50088        sd      s5,136(sp)
ffffffff8013304c:       ffb40080        sd      s4,128(sp)
ffffffff80133050:       ffb30078        sd      s3,120(sp)
ffffffff80133054:       ffb20070        sd      s2,112(sp)
ffffffff80133058:       ffb10068        sd      s1,104(sp)
ffffffff8013305c:       ffb00060        sd      s0,96(sp)
ffffffff80133060:       03e0082d        move    at,ra
ffffffff80133064:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff80133068:       00020021        nop

but we can not get the exact stack address(which saved ra) directly in _mcount, we need to search the content of at register in the stack space or search the "s{d,w} ra, offset(sp)" instruction in the text. 'Cause we can not prove there is only a match in the stack space, so, we search the text instead.

Quoted 2 lines

as we can see, if the first instruction above "move at, ra" is "move s8, sp"(move fp, sp), it is a leaf function, so we hijack the at register

Are you sure it will always be the first instruction for leaf registers. You may want to search for that instruction and stop on it. If you have not yet found the storage of ra in the stack, then you know it is a leaf function.

There is no deterministic way to identify MIPS function prologs. This is especially true for leaf functions, but also for functions with multiple return sites.

For certain GCC versions there may be a set of command line options that would give good results, but in general it is not possible. Attempts at fast backtrace generation using code inspection are not reliable and will invariably result in faults and panics when they fail.

Quoted 3 lines · David Daney

There is no deterministic way to identify MIPS function prologs. This is especially true for leaf functions, but also for functions with multiple return sites.

For certain GCC versions there may be a set of command line options that would give good results, but in general it is not possible. Attempts at fast backtrace generation using code inspection are not reliable and will invariably result in faults and panics when they fail.

Thanks for the update.

We can easily protect against panics, since we do fault protection within the code (although currently it will panic on fault, but we can fix that ;-). We can limit the search to a couple of 100 instructions, as well as fail on first panic.

But are you sure that when compiled with -pg, that GCC does not give a reliable prologue. Things are different when GCC is compiled with -pg, it may indeed always have something that we can flag.

We could also add other tests, like the subtraction of the stack too.

I have not used -pg, so I don't know for sure, I think all it does is add the calls to _mcount. Someone could investigate -fno-omit-frame-pointer, with that you may be able to use:

move s8,sp

To identify function prologs, but it would still be ad hoc, as modern versions of GCC will reorder instructions in the prolog for better scheduling.

Quoted 2 lines · David Daney

I have not used -pg, so I don't know for sure, I think all it does is add the calls to _mcount. Someone could investigate -fno-omit-frame-pointer, with that you may be able to use:

Note, -pg assumes -fno-omit-frame-pointer, since -fomit-frame-pointer and -pg are incompatible.

Quoted 3 lines

move s8,sp

To identify function prologs, but it would still be ad hoc, as modern versions of GCC will reorder instructions in the prolog for better scheduling.

I'll have to search the ABI documentation about calling _mcount in MIPS. There are assumptions with _mcount that are made. It may very well be safe to assume that the move s8,sp will always be there before an mcount call.

Although I am quite interested in ftrace for MIPS, I a haven't spent the effort to try to figure out how many levels of backtrace we need to be generating here.

If you only need the caller's address, that is passed in register 'at'.
If you need more than that, then things get tricky. I would do one of
two things:

1) Use the mechanism used by the OOPS dump.

2) Implement back traces based on DWARF2 unwinding data that is generated by the compiler.

We're not doing back traces. We need to modify the return of the function being called. Note, the above functions that end with ";" are leaf functions. Non leaf functions show "{" and end with "}".

The trick here is to find a reliable way to modify the return address.

Quoted 6 lines

We're not doing back traces. We need to modify the return of the function being called. Note, the above functions that end with ";" are leaf functions. Non leaf functions show "{" and end with "}".

Quoted 2 lines

The trick here is to find a reliable way to modify the return address.

would it not more or less be the same thing if you used -finstrument-functions then and provide a stub __cyg_profile_func_enter/exit initialized to an empty function until you replace it during tracing. This does give you an overhead when you are not tracing - but it would make the tracer implementation quite generic.

We're not doing back traces. We need to modify the return of the function being called. Note, the above functions that end with ";" are leaf functions. Non leaf functions show "{" and end with "}".

The trick here is to find a reliable way to modify the return address.

Quoted 3 lines · David Daney

I have not used -pg, so I don't know for sure, I think all it does is add the calls to _mcount. Someone could investigate -fno-omit-frame-pointer, with that you may be able to use:

Note, -pg assumes -fno-omit-frame-pointer, since -fomit-frame-pointer and -pg are incompatible.

Quoted 6 lines

move s8,sp

To identify function prologs, but it would still be ad hoc, as modern versions of GCC will reorder instructions in the prolog for better scheduling.

I'll have to search the ABI documentation about calling _mcount in MIPS. There are assumptions with _mcount that are made. It may very well be safe to assume that the move s8,sp will always be there before an mcount call.

Hi,

On Wed, 2009-10-21 at 13:23 -0400, Steven Rostedt wrote:

Ralf have told me -pg really works with -fomit-frame-pointer, although the gcc tool tell us they are not incompatible when we use both of them together, but when I remove -fno-omit-frame-pointer in KBUILD_FLAGS(enabled by CONFIG_FRAME_POINTER), it definitely remove the s8(fp) relative source code(Seems -fomit-frame-pionter is used by default by gcc), the leaf function becomes this:

function:

80101144 <au1k_wait>:

80101144:       03e00821        move    at,ra
80101148:       0c04271c        jal     80109c70 <_mcount>

No more instruction,

and the non-leaf function becomes,

80126590 <copy_process>:

80126590:       27bdffa0        addiu   sp,sp,-96
80126594:       afbf005c        sw      ra,92(sp)
80126598:       afbe0058        sw      s8,88(sp)
8012659c:       afb70054        sw      s7,84(sp)
801265a0:       afb60050        sw      s6,80(sp)
801265a4:       afb5004c        sw      s5,76(sp)
801265a8:       afb40048        sw      s4,72(sp)
801265ac:       afb30044        sw      s3,68(sp)
801265b0:       afb20040        sw      s2,64(sp)
801265b4:       afb1003c        sw      s1,60(sp)
801265b8:       afb00038        sw      s0,56(sp)
801265bc:       03e00821        move    at,ra
801265c0:       0c04271c        jal     80109c70 <_mcount>

It may save about two instructions for us.

	sw	s8, offset(sp)
	move	s8, fp

and also, I have tried to just search "Save" instruction, if I find one,

that should be a non-leaf function, otherwise, it's leaf function, but I can not prove no "Save" instruction before the leaf function's "move at, ra", for example:

8010113c:       03e00008        jr      ra
80101140:       00020021        nop

80101144 <au1k_wait>:

80101144:       03e00821        move    at,ra
80101148:       0c04271c        jal     80109c70 <_mcount>

if there is "save" instruction at address 80101140, it will fail. Although, I met not failure with several tries, but no prove on it! any ABI protection for this? if YES, this should be a better solution, for it may works without -fno-omit-frame-pointer and save several instructions for us.

and BTW, -fomit-frame-pointer does help "a lot" for us to get the stack address without any big load:

        sp = fp + (code & STACK_OFFSET_MASK);
        ra = *(unsigned long *)sp;

although we can calculate it via the parent_addr, I have got this in:

NESTED(ftrace_graph_caller, PT_SIZE, ra)
MCOUNT_SAVE_REGS

PTR_LA a0, PT_R1(sp) /* arg1: &AT -> a0 */
move a1, ra /* arg2: next ip, selfaddr */
PTR_SUBU a1, MCOUNT_INSN_SIZE
move a2, fp /* arg3: frame pointer */
jal prepare_ftrace_return
nop

MCOUNT_RESTORE_REGS
RETURN_BACK
END(ftrace_graph_caller)

so, parent_addr is the current sp + PT_R1:

sp = sp - PT_SIZE (did in MCOUNT_SAVE_REGS) parent_addr = sp + PT_R1

so the fp can be calculated like this:

	fp = parent_addr - PT_R1 + PT_SIZE;

this will take use a little time to calculate it.

ooh, so, both -fomit-frame-pointer and -fno-omit-frame-pointer work for us, the left job is that: we need to prove them are really SAFE here.

Quoted 7 lines

Quoted 6 lines

move s8,sp

To identify function prologs, but it would still be ad hoc, as modern versions of GCC will reorder instructions in the prolog for better scheduling.

Quoted 36 lines · Wu Zhangjin

Hi,

On Wed, 2009-10-21 at 13:23 -0400, Steven Rostedt wrote:

Ralf have told me -pg really works with -fomit-frame-pointer, although the gcc tool tell us they are not incompatible when we use both of them together, but when I remove -fno-omit-frame-pointer in KBUILD_FLAGS(enabled by CONFIG_FRAME_POINTER), it definitely remove the s8(fp) relative source code(Seems -fomit-frame-pionter is used by default by gcc), the leaf function becomes this:

function:

80101144 <au1k_wait>:

80101144:       03e00821        move    at,ra
80101148:       0c04271c        jal     80109c70 <_mcount>

No more instruction,

and the non-leaf function becomes,

80126590 <copy_process>:

80126590:       27bdffa0        addiu   sp,sp,-96
80126594:       afbf005c        sw      ra,92(sp)
80126598:       afbe0058        sw      s8,88(sp)
8012659c:       afb70054        sw      s7,84(sp)
801265a0:       afb60050        sw      s6,80(sp)
801265a4:       afb5004c        sw      s5,76(sp)
801265a8:       afb40048        sw      s4,72(sp)
801265ac:       afb30044        sw      s3,68(sp)
801265b0:       afb20040        sw      s2,64(sp)
801265b4:       afb1003c        sw      s1,60(sp)
801265b8:       afb00038        sw      s0,56(sp)
801265bc:       03e00821        move    at,ra
801265c0:       0c04271c        jal     80109c70 <_mcount>

Quoted 6 lines

It may save about two instructions for us.

	sw	s8, offset(sp)
	move	s8, fp

and also, I have tried to just search "Save" instruction, if I find one,

So you look for "sw ..."?

Quoted 3 lines

that should be a non-leaf function, otherwise, it's leaf function, but I can not prove no "Save" instruction before the leaf function's "move at, ra", for example:

Yes but it should never be saving the ra. You can search all instructions before the move at,ra until you find the save of the ra, or you find something that is not a save. If you find the saving of ra, it is not a leaf, but if you don't find the saving of ra, then it is a leaf.

Quoted 8 lines

8010113c:       03e00008        jr      ra
80101140:       00020021        nop

80101144 <au1k_wait>:

80101144:       03e00821        move    at,ra
80101148:       0c04271c        jal     80109c70 <_mcount>

Quoted 5 lines

if there is "save" instruction at address 80101140, it will fail. Although, I met not failure with several tries, but no prove on it! any ABI protection for this? if YES, this should be a better solution, for it may works without -fno-omit-frame-pointer and save several instructions for us.

If we don't stop at just one save, but look for the saving of ra, it should not fail.

Quoted 3 lines

If we don't stop at just one save, but look for the saving of ra, it should not fail.

We can not look for the saving of ra continuously(when should we stop?

if with -fno-omit-fram-pointer, we have "move s8,sp" or "addiu sp, sp, -offset", but without it, we have no "guideboard" to know that is the beginning of the function!), 'Cause we may find the saving of ra of another function, which will fail at that time.

BTW: Just replace probe_kernel_read() and tracing_stop/tracing_start by asm, it works in 32bit, but fails in 64bit, I'm trying to find why!(TLB miss on load or ifetch, will fix it asap! and resend the patchset out!)

Hi,

Quoted 6 lines · Steven Rostedt

Quoted 2 lines · David Daney

I have not used -pg, so I don't know for sure, I think all it does is add the calls to _mcount. Someone could investigate -fno-omit-frame-pointer, with that you may be able to use:

Note, -pg assumes -fno-omit-frame-pointer, since -fomit-frame-pointer and -pg are incompatible.

Ralf have told me -pg really works with -fomit-frame-pointer, although the gcc tool tell us they are not incompatible when we use both of them together, but when I remove -fno-omit-frame-pointer in KBUILD_FLAGS(enabled by CONFIG_FRAME_POINTER), it definitely remove the s8(fp) relative source code(Seems -fomit-frame-pionter is used by default by gcc), the leaf function becomes this:

function:

80101144 <au1k_wait>:

80101144:       03e00821        move    at,ra
80101148:       0c04271c        jal     80109c70 <_mcount>

No more instruction,

and the non-leaf function becomes,

80126590 <copy_process>:

80126590:       27bdffa0        addiu   sp,sp,-96
80126594:       afbf005c        sw      ra,92(sp)
80126598:       afbe0058        sw      s8,88(sp)
8012659c:       afb70054        sw      s7,84(sp)
801265a0:       afb60050        sw      s6,80(sp)
801265a4:       afb5004c        sw      s5,76(sp)
801265a8:       afb40048        sw      s4,72(sp)
801265ac:       afb30044        sw      s3,68(sp)
801265b0:       afb20040        sw      s2,64(sp)
801265b4:       afb1003c        sw      s1,60(sp)
801265b8:       afb00038        sw      s0,56(sp)
801265bc:       03e00821        move    at,ra
801265c0:       0c04271c        jal     80109c70 <_mcount>

It may save about two instructions for us.

	sw	s8, offset(sp)
	move	s8, fp

and also, I have tried to just search "Save" instruction, if I find one, that should be a non-leaf function, otherwise, it's leaf function, but I can not prove no "Save" instruction before the leaf function's "move at, ra", for example:

8010113c:       03e00008        jr      ra
80101140:       00020021        nop

80101144 <au1k_wait>:

80101144:       03e00821        move    at,ra
80101148:       0c04271c        jal     80109c70 <_mcount>

if there is "save" instruction at address 80101140, it will fail. Although, I met not failure with several tries, but no prove on it! any ABI protection for this? if YES, this should be a better solution, for it may works without -fno-omit-frame-pointer and save several instructions for us.

This is what I was talking about up-thread. Leaf functions may have no function prolog. If you do code scanning you will fail. While scanning backwards, there is no way to know when you have entered a new function.
Looking for function return sequences 'jr ra' doesn't work as there
may be functions with multiple return sites, functions that never return, or arbitrary data before the function. I think you have to force a frame pointer to be established if you want this to work.

Quoted 3 lines · David Daney

This is what I was talking about up-thread. Leaf functions may have no function prolog. If you do code scanning you will fail. While scanning backwards, there is no way to know when you have entered a new function.
Looking for function return sequences 'jr ra' doesn't work as there
may be functions with multiple return sites, functions that never return, or arbitrary data before the function. I think you have to force a frame pointer to be established if you want this to work.

Functions that run off into another function?? I guess the compiler could do that, but with -pg enable, I would think is broken.

Quoted 3 lines · Steven Rostedt

Functions that run off into another function?? I guess the compiler could do that, but with -pg enable, I would think is broken.

Use of GCC-4.5's __builtin_unreachable() can lead to this, as well as functions that call noreturn functions.

Quoted 39 lines · Wu Zhangjin

The implementation of function graph tracer for MIPS is a little different from X86.

in MIPS, gcc(with -pg) only transfer the caller's return address(at) and the _mcount's return address(ra) to us.

move at, ra
jal _mcount

in the function is a leaf, it will no save the return address(ra):

ffffffff80101298 <au1k_wait>:

ffffffff80101298:       67bdfff0        daddiu  sp,sp,-16
ffffffff8010129c:       ffbe0008        sd      s8,8(sp)
ffffffff801012a0:       03a0f02d        move    s8,sp
ffffffff801012a4:       03e0082d        move    at,ra
ffffffff801012a8:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff801012ac:       00020021        nop

so, we can hijack it directly in _mcount, but if the function is non-leaf, the return address is saved in the stack.

ffffffff80133030 <copy_process>:

ffffffff80133030:       67bdff50        daddiu  sp,sp,-176
ffffffff80133034:       ffbe00a0        sd      s8,160(sp)
ffffffff80133038:       03a0f02d        move    s8,sp
ffffffff8013303c:       ffbf00a8        sd      ra,168(sp)
ffffffff80133040:       ffb70098        sd      s7,152(sp)
ffffffff80133044:       ffb60090        sd      s6,144(sp)
ffffffff80133048:       ffb50088        sd      s5,136(sp)
ffffffff8013304c:       ffb40080        sd      s4,128(sp)
ffffffff80133050:       ffb30078        sd      s3,120(sp)
ffffffff80133054:       ffb20070        sd      s2,112(sp)
ffffffff80133058:       ffb10068        sd      s1,104(sp)
ffffffff8013305c:       ffb00060        sd      s0,96(sp)
ffffffff80133060:       03e0082d        move    at,ra
ffffffff80133064:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff80133068:       00020021        nop

Quoted 8 lines

but we can not get the exact stack address(which saved ra) directly in _mcount, we need to search the content of at register in the stack space or search the "s{d,w} ra, offset(sp)" instruction in the text. 'Cause we can not prove there is only a match in the stack space, so, we search the text instead.

as we can see, if the first instruction above "move at, ra" is "move s8, sp"(move fp, sp), it is a leaf function, so we hijack the at register

Are you sure it will always be the first instruction for leaf registers. You may want to search for that instruction and stop on it. If you have not yet found the storage of ra in the stack, then you know it is a leaf function.

Quoted 96 lines

directly via put &return_to_handler into it, and otherwise, we search the "s{d,w} ra, offset(sp)" instruction to get the stack offset, and then the stack address. we use the above copy_process() as an example, we at last find "ffbf00a8", 0xa8 is the stack offset, we plus it with s8(fp), that is the stack address, we hijack the content via writing the &return_to_handler in.

Signed-off-by Wu Zhangjin <[email protected]>

---
 arch/mips/Kconfig              |    1 +
 arch/mips/kernel/ftrace.c      |  192 ++++++++++++++++++++++++++++++++++++++++
 arch/mips/kernel/mcount.S      |   55 +++++++++++-
 arch/mips/kernel/vmlinux.lds.S |    1 +
 4 files changed, 248 insertions(+), 1 deletions(-)

arch/mips/Kconfig +1 -0

index 147fbbc..de690fd 100644
@@ -8,6 +8,7 @@ config MIPS
 	select HAVE_FUNCTION_TRACE_MCOUNT_TEST
 	select HAVE_DYNAMIC_FTRACE
 	select HAVE_FTRACE_MCOUNT_RECORD
+	select HAVE_FUNCTION_GRAPH_TRACER
 	# Horrible source of confusion.  Die, die, die ...
 	select EMBEDDED
 	select RTC_LIB if !LEMOTE_FULOONG2E

arch/mips/kernel/ftrace.c +54 -0

index 1e44865..fddee5b 100644
@@ -13,6 +13,8 @@
 #include <linux/ftrace.h>
 
 #include <asm/cacheflush.h>
+#include <asm/asm.h>
+#include <asm/asm-offsets.h>
 
 #ifdef CONFIG_DYNAMIC_FTRACE
 

@@ -105,3 +107,193 @@ int __init ftrace_dyn_arch_init(void *data)
 	return 0;
 }
 #endif				/* CONFIG_DYNAMIC_FTRACE */
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+
+#ifdef CONFIG_DYNAMIC_FTRACE
+
+#define JMP	0x08000000	/* jump to target directly */
+extern void ftrace_graph_call(void);
+
+int ftrace_enable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	int ret;
+
+	/* j ftrace_stub */
+	memcpy(old, (unsigned long *)ip, MCOUNT_INSN_SIZE);
+	new = ftrace_call_replace(JMP, (unsigned long)ftrace_graph_caller);
+
+	ret = ftrace_modify_code(ip, old, new);
+
+	return ret;
+}
+
+int ftrace_disable_ftrace_graph_caller(void)
+{
+	unsigned long ip = (unsigned long)(&ftrace_graph_call);
+	unsigned char old[MCOUNT_INSN_SIZE], *new;
+	int ret;
+
+	/* j ftrace_graph_caller */
+	memcpy(old, (unsigned long *)ip, MCOUNT_INSN_SIZE);
+	new = ftrace_call_replace(JMP, (unsigned long)ftrace_stub);
+
+	ret = ftrace_modify_code(ip, old, new);
+
+	return ret;
+}
+
+#endif				/* !CONFIG_DYNAMIC_FTRACE */
+
+#define S_RA    (0x2fbf << 16)	/* 32bit: afbf, 64bit: ffbf */
+/* This is only available when enabled -fno-omit-frame-pointer with CONFIG_FRAME_POINTER=y */
+#define MOV_FP_SP       0x03a0f021	/* 32bit: 0x03a0f021, 64bit: 0x03a0f02d */
+#define STACK_OFFSET_MASK	0xfff	/* stack offset range: 0 ~ PT_SIZE(304) */
+
+unsigned long ftrace_get_parent_addr(unsigned long self_addr,
+				     unsigned long parent,
+				     unsigned long parent_addr,
+				     unsigned long fp)
+{
+	unsigned long sp, ip, ra;
+	unsigned int code;

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 171

Quoted 10 lines

arch/mips/kernel/ftrace.c +10 -0

+
+	/* move to the instruction "move ra, at" */
+	ip = self_addr - 8;
+
+	/* search the text until finding the "move s8, sp" instruction or
+	 * "s{d,w} ra, offset(sp)" instruction */
+	do {
+		ip -= 4;
+		/* read the text we want to match */
+		if (probe_kernel_read(&code, (void *)ip, 4)) {

I thought you had issues with using probe_kernel_read here?

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 177

Quoted 6 lines

arch/mips/kernel/ftrace.c +6 -0

+			WARN_ON(1);
+			panic("read the text failure\n");
+		}
+
+		/* if the first instruction above "move at, ra" is "move
+		 * s8(fp), sp", means the function is a leaf */

Ah, the comment is incorrect. We do search for the move and stop there. So it is not the first instruction. The comment should read...

If we hit the "move s8(fp), sp" instruction before finding where the ra is stored, then this is a leaf function and it does not store the ra on the stack.

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 190

Quoted 13 lines

arch/mips/kernel/ftrace.c +13 -0

+		if ((code & MOV_FP_SP) == MOV_FP_SP)
+			return parent_addr;
+	} while (((code & S_RA) != S_RA));
+
+	sp = fp + (code & STACK_OFFSET_MASK);
+	ra = *(unsigned long *)sp;
+
+	if (ra == parent)
+		return sp;
+	else
+		panic
+		    ("failed on getting stack address of ra\n: addr: 0x%lx, code: 0x%x\n",
+		     ip, code);

I would not panic. I would disable the tracing, and not do the change. The code should work regardless.

Quoted 34 lines

arch/mips/kernel/ftrace.c +34 -0

+}
+
+/*
+ * Hook the return address and push it in the stack of return addrs
+ * in current thread info.
+ */
+void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr,
+			   unsigned long fp)
+{
+	unsigned long old;
+	int faulted;
+	struct ftrace_graph_ent trace;
+	unsigned long return_hooker = (unsigned long)
+	    &return_to_handler;
+
+	/*
+	 * Protect against fault, even if it shouldn't
+	 * happen. This tool is too much intrusive to
+	 * ignore such a protection.
+	 *
+	 * old = *parent;
+	 */
+	asm volatile (
+		"1: " STR(PTR_L) " %[old], 0(%[parent])\n"
+		"   li %[faulted], 0\n"
+		"2:\n"
+
+		".section .fixup, \"ax\"\n"
+		"3: li %[faulted], 1\n"
+		"   j 2b\n"
+		".previous\n"
+
+		".section\t__ex_table,\"a\"\n\t"
+		STR(PTR) "\t1b, 3b\n\t"

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 234

Quoted 10 lines

arch/mips/kernel/ftrace.c +10 -0

+		".previous\n"
+
+		: [old] "=&r" (old), [faulted] "=r" (faulted)
+		: [parent] "r" (parent)
+		: "memory"
+	);
+
+	if (unlikely(faulted)) {
+		ftrace_graph_stop();
+		WARN_ON(1);

See, this is how we kill the tracing.

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 244

Quoted 10 lines

arch/mips/kernel/ftrace.c +10 -0

+		return;
+	}
+
+	/* The argument *parent only work for leaf function, we need to check
+	 * whether the function is leaf, if not, we need to get the real stack
+	 * address which stored it.
+	 *
+	 * and here, we must stop tracing before calling probe_kernel_read().
+	 * after calling it, restart tracing. otherwise, it will hang there.*/
+	tracing_stop();

Ah, here you stop tracing. I still prefer to use a direct asm. This has a little overhead.

Also, this completely negates the above asm, which basically gets the parent the "x86" way.

Quoted 23 lines

arch/mips/kernel/ftrace.c +23 -0

+	parent =
+	    (unsigned long *)ftrace_get_parent_addr(self_addr, old,
+						    (unsigned long)parent, fp);
+	tracing_start();
+
+	if (unlikely(atomic_read(&current->tracing_graph_pause)))
+		return;
+
+	/*
+	 * Protect against fault, even if it shouldn't
+	 * happen. This tool is too much intrusive to
+	 * ignore such a protection.
+	 *
+	 * *parent = return_hooker;
+	 */
+	asm volatile (
+		"1: " STR(PTR_S) " %[return_hooker], 0(%[parent])\n"
+		"   li %[faulted], 0\n"
+		"2:\n"
+
+		".section .fixup, \"ax\"\n"
+		"3: li %[faulted], 1\n"
+		"   j 2b\n"

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 277

Quoted 10 lines

arch/mips/kernel/ftrace.c +10 -0

+		".previous\n"
+
+		".section\t__ex_table,\"a\"\n\t"
+		STR(PTR) "\t1b, 3b\n\t"
+		".previous\n"
+
+		: [faulted] "=r" (faulted)
+		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)
+		: "memory"
+	);

Hmm, the above should only be done for non leaf functions.

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 287

Quoted 10 lines

arch/mips/kernel/ftrace.c +10 -0

+
+	if (unlikely(faulted)) {
+		ftrace_graph_stop();
+		WARN_ON(1);
+		return;
+	}
+
+	if (ftrace_push_return_trace(old, self_addr, &trace.depth, fp) ==
+	    -EBUSY) {
+		*parent = old;

And this fails for leaf functions too.

arch/mips/kernel/ftrace.c int __init ftrace_dyn_arch_init(void *data) · line 296

Quoted 9 lines

arch/mips/kernel/ftrace.c +9 -0

+		return;
+	}
+
+	trace.func = self_addr;
+
+	/* Only trace if the calling function expects to */
+	if (!ftrace_graph_entry(&trace)) {
+		current->curr_ret_stack--;
+		*parent = old;

ditto

Quoted 40 lines

arch/mips/kernel/ftrace.c +3 -0

+	}
+}
+#endif				/* CONFIG_FUNCTION_GRAPH_TRACER */

arch/mips/kernel/mcount.S +17 -1

index 30637e6..356e81c 100644
@@ -89,6 +89,14 @@ ftrace_call:
 	nop
 
 	MCOUNT_RESTORE_REGS
+
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+	.globl ftrace_graph_call
+ftrace_graph_call:
+	j	ftrace_stub
+	nop
+#endif
+
 	.globl ftrace_stub
 ftrace_stub:
 	RETURN_BACK

@@ -106,7 +114,15 @@ NESTED(_mcount, PT_SIZE, ra)
 	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */
 	bne	t0, t1, static_trace
 	nop
-
+#ifdef	CONFIG_FUNCTION_GRAPH_TRACER
+	PTR_L	t2, ftrace_graph_return
+	bne	t0, t2, ftrace_graph_caller
+	nop
+	PTR_LA	t0, ftrace_graph_entry_stub
+	PTR_L	t2, ftrace_graph_entry
+	bne	t0, t2, ftrace_graph_caller
+	nop
+#endif
 	j	ftrace_stub
 	nop
 

arch/mips/kernel/mcount.S ftrace_stub: · line 149

Quoted 9 lines

arch/mips/kernel/mcount.S +6 -0

@@ -125,5 +141,42 @@ ftrace_stub:
 
 #endif	/* ! CONFIG_DYNAMIC_FTRACE */
 
+#ifdef CONFIG_FUNCTION_GRAPH_TRACER
+
+NESTED(ftrace_graph_caller, PT_SIZE, ra)
+	MCOUNT_SAVE_REGS
+
+	PTR_LA	a0, PT_R1(sp)	/* arg1: &AT -> a0 */

Ah! You load the address of the stack you stored the ra with!

Good idea. I guess the leaf function update will work there too!

Quoted 37 lines

arch/mips/kernel/mcount.S +31 -0

+	move	a1, ra		/* arg2: next ip, selfaddr */
+	PTR_SUBU a1, MCOUNT_INSN_SIZE
+	move	a2, fp		/* arg3: frame pointer */
+	jal	prepare_ftrace_return
+	nop
+
+	MCOUNT_RESTORE_REGS
+	RETURN_BACK
+	END(ftrace_graph_caller)
+
+	.align	2
+	.globl	return_to_handler
+return_to_handler:
+	PTR_SUBU	sp, PT_SIZE
+	PTR_S	v0, PT_R2(sp)
+	PTR_S	v1, PT_R3(sp)
+
+	jal	ftrace_return_to_handler
+	nop
+
+	/* restore the real parent address: v0 -> ra */
+	move	ra, v0
+
+	PTR_L	v0, PT_R2(sp)
+	PTR_L	v1, PT_R3(sp)
+	PTR_ADDIU	sp, PT_SIZE
+
+	jr	ra
+	nop
+#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
+
 	.set at
 	.set reorder

arch/mips/kernel/vmlinux.lds.S +0 -0

index 162b299..f25df73 100644

arch/mips/kernel/vmlinux.lds.S SECTIONS · line 49

Quoted 5 lines

arch/mips/kernel/vmlinux.lds.S +1 -0

@@ -46,6 +46,7 @@ SECTIONS
 		SCHED_TEXT
 		LOCK_TEXT
 		KPROBES_TEXT
+		IRQENTRY_TEXT

This looks like it should be in a separate patch. I don't see where you explain this?

-- Steve

Quoted 3 lines

arch/mips/kernel/vmlinux.lds.S +0 -0

 		*(.text.*)
 		*(.fixup)
 		*(.gnu.warning)

Quoted 5 lines

Quoted 5 lines

arch/mips/kernel/mcount.S +0 -0

 	.set reorder

arch/mips/kernel/vmlinux.lds.S +0 -0

index 162b299..f25df73 100644

Quoted 9 lines

arch/mips/kernel/vmlinux.lds.S SECTIONS · line 49

Quoted 5 lines

arch/mips/kernel/vmlinux.lds.S +1 -0

@@ -46,6 +46,7 @@ SECTIONS
 		SCHED_TEXT
 		LOCK_TEXT
 		KPROBES_TEXT
+		IRQENTRY_TEXT

This looks like it should be in a separate patch. I don't see where you explain this?

This is used to fix this error:

kernel/built-in.o: In function `print_graph_irq':
trace_functions_graph.c:(.text+0x82f40): undefined reference to __irqentry_text_start' trace_functions_graph.c:(.text+0x82f48): undefined reference to __irqentry_text_start' trace_functions_graph.c:(.text+0x82f70): undefined reference to __irqentry_text_end' trace_functions_graph.c:(.text+0x82f74): undefined reference to __irqentry_text_end'

in MIPS, gcc(with -pg) only transfer the caller's return address(at) and the _mcount's return address(ra) to us.

move at, ra
jal _mcount

in the function is a leaf, it will no save the return address(ra):

ffffffff80101298 <au1k_wait>:

ffffffff80101298:       67bdfff0        daddiu  sp,sp,-16
ffffffff8010129c:       ffbe0008        sd      s8,8(sp)
ffffffff801012a0:       03a0f02d        move    s8,sp
ffffffff801012a4:       03e0082d        move    at,ra
ffffffff801012a8:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff801012ac:       00020021        nop

so, we can hijack it directly in _mcount, but if the function is non-leaf, the
return address is saved in the stack.

ffffffff80133030 <copy_process>:

ffffffff80133030:       67bdff50        daddiu  sp,sp,-176
ffffffff80133034:       ffbe00a0        sd      s8,160(sp)
ffffffff80133038:       03a0f02d        move    s8,sp
ffffffff8013303c:       ffbf00a8        sd      ra,168(sp)
ffffffff80133040:       ffb70098        sd      s7,152(sp)
ffffffff80133044:       ffb60090        sd      s6,144(sp)
ffffffff80133048:       ffb50088        sd      s5,136(sp)
ffffffff8013304c:       ffb40080        sd      s4,128(sp)
ffffffff80133050:       ffb30078        sd      s3,120(sp)
ffffffff80133054:       ffb20070        sd      s2,112(sp)
ffffffff80133058:       ffb10068        sd      s1,104(sp)
ffffffff8013305c:       ffb00060        sd      s0,96(sp)
ffffffff80133060:       03e0082d        move    at,ra
ffffffff80133064:       0c042930        jal     ffffffff8010a4c0 <_mcount>
ffffffff80133068:       00020021        nop

but we can not get the exact stack address(which saved ra) directly in _mcount, we need to search the content of at register in the stack space or search the "s{d,w} ra, offset(sp)" instruction in the text. 'Cause we can not prove there is only a match in the stack space, so, we search the text instead.

Quoted 1 line

All this stuff can be avoided having PROFILE_BEFORE_PROLOGUE enabled in GCC (gcc/config/mips/mips.h), like it is done one several other architectures. Some of them even require it to be set for a working _mcount. Putting the call of _mcount before the function prologue should make no harm (it's working for me), and this way RA can be hooked for function graph tracing
before it is saved to stack in the function prologue. Thus there will be no difference between leaf and non-leaf functions.

This change is no big deal as GCC has to be patched anyway to get dynamic ftracing (and with 4.2.1 I had to patch it to get ftracing in modules working
- using -mlong-calls -, but it's possible that this is fixed in the newer releases).

do you mean if enabling PROFILE_BEFORE_PROLOGUE, there will be some problems with module support using -mlong-calls?

Quoted 4 lines · Wu Zhangjin

do you mean if enabling PROFILE_BEFORE_PROLOGUE, there will be some problems with module support using -mlong-calls?

No, there are no problems. I've tested it on friday, and function graph tracing was working correctly.
I meant to say that 4.2.1 we use does not generate correct profile calls from kernel modules. Maybe this issue was fixed in newer releases, I did not check. I've applied a patch (don't remember where have I found that, maybe it was created by you) to our toolchain several months ago.

I was thinking about dynamic tracing, and I think a toolchain patch can be avoided completely. We only need to make difference between "jal _mcount" and "jalr v1"-style mcount calls when replacing them with "nop" instructions in the code-patching function called by ftrace_convert_nops(). This can be done in 2 ways:
1) keeping old instructions - takes extra memory, not an option
2) using 2 separate instructions to replace with. One of them could be the normal NOP instruction, which expands to "sll r0, r0, 0". For the other we could use "sll r0, r0, 1" but as it has already special meaning (SSNOP) a better candidate could be something like "sll r1, r1, 0". This way we can decide which instruction to patch in when tracing is enabled for a function, eg. when the code patcher encounters a "sll r0, r0, 0" it emits a function call using JAL and when it encounters "sll r1, r1, 0" it emits a function call using "JALR v1".

(Added linux-mips mailing list and the other people to the CC list.)

On Thu, 2009-10-22 at 04:37 -0700, pajko wrote:
[...]

Quoted 10 lines

Quoted 1 line

All this stuff can be avoided having PROFILE_BEFORE_PROLOGUE enabled in GCC (gcc/config/mips/mips.h), like it is done one several other architectures. Some of them even require it to be set for a working _mcount. Putting the call of _mcount before the function prologue should make no harm (it's working for me), and this way RA can be hooked for function graph tracing
before it is saved to stack in the function prologue. Thus there will be no difference between leaf and non-leaf functions.

Good idea! Seems PROFILE_BEFORE_PROLOGUE is commented by default in gcc/config/mips/mips.h of gcc 4.4:

/* #define PROFILE_BEFORE_PROLOGUE */

if we enable this macro, the handling will be the same to non-leaf and leaf function, so, David's patch to gcc is not need again.

MIPS architecture need this argument to handle big/little endian respectively.

Signed-off-by Wu Zhangjin <[email protected]>

---
 scripts/Makefile.build  |    1 +
 scripts/recordmcount.pl |    6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

scripts/Makefile.build +0 -0

index 341b589..0b94d2f 100644

scripts/Makefile.build +0 -0

 	"$(if $(CONFIG_64BIT),64,32)" \
 	"$(OBJDUMP)" "$(OBJCOPY)" "$(CC)" "$(LD)" "$(NM)" "$(RM)" "$(MV)" \
 	"$(if $(part-of-module),1,0)" "$(@)";

scripts/recordmcount.pl +3 -3

index 090d300..daee038 100755
@@ -99,13 +99,13 @@ $P =~ s@.*/@@g;
 
 my $V = '0.1';
 
-if ($#ARGV < 7) {
-	print "usage: $P arch bits objdump objcopy cc ld nm rm mv is_module inputfile\n";
+if ($#ARGV < 8) {
+	print "usage: $P arch endian bits objdump objcopy cc ld nm rm mv is_module inputfile\n";
 	print "version: $V\n";
 	exit(1);
 }
 
-my ($arch, $bits, $objdump, $objcopy, $cc,
+my ($arch, $endian, $bits, $objdump, $objcopy, $cc,
     $ld, $nm, $rm, $mv, $is_module, $inputfile) = @ARGV;
 
 # This file refers to mcount and shouldn't be ftraced, so lets' ignore it

+++ b/arch/mips/kernel/mcount.S

arch/mips/kernel/mcount.S new file +63 -0

@@ -0,0 +1,94 @@
+/*
+ * the mips-specific _mcount implementation
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive for
+ * more details.
+ *
+ * Copyright (C) 2009 DSLab, Lanzhou University, China
+ * Author: Wu Zhangjin <[email protected]>
+ */
+
+#include <asm/regdef.h>
+#include <asm/stackframe.h>
+#include <asm/ftrace.h>
+
+	.text
+	.set noreorder
+	.set noat
+
+	/* since there is a "addiu sp,sp,-8" before "jal _mcount" in 32bit */
+	.macro RESTORE_SP_FOR_32BIT
+#ifdef CONFIG_32BIT
+	PTR_ADDIU	sp, 8
+#endif
+	.endm
+
+	.macro MCOUNT_SAVE_REGS
+	PTR_SUBU	sp, PT_SIZE
+	PTR_S	ra, PT_R31(sp)
+	PTR_S	AT, PT_R1(sp)
+	PTR_S	a0, PT_R4(sp)
+	PTR_S	a1, PT_R5(sp)
+	PTR_S	a2, PT_R6(sp)
+	PTR_S	a3, PT_R7(sp)
+#ifdef CONFIG_64BIT
+	PTR_S	a4, PT_R8(sp)
+	PTR_S	a5, PT_R9(sp)
+	PTR_S	a6, PT_R10(sp)
+	PTR_S	a7, PT_R11(sp)
+#endif
+	.endm
+
+	.macro MCOUNT_RESTORE_REGS
+	PTR_L	ra, PT_R31(sp)
+	PTR_L	AT, PT_R1(sp)
+	PTR_L	a0, PT_R4(sp)
+	PTR_L	a1, PT_R5(sp)
+	PTR_L	a2, PT_R6(sp)
+	PTR_L	a3, PT_R7(sp)
+#ifdef CONFIG_64BIT
+	PTR_L	a4, PT_R8(sp)
+	PTR_L	a5, PT_R9(sp)
+	PTR_L	a6, PT_R10(sp)
+	PTR_L	a7, PT_R11(sp)
+#endif
+	PTR_ADDIU	sp, PT_SIZE
+.endm
+
+	.macro MCOUNT_SET_ARGS
+	move	a0, ra		/* arg1: next ip, selfaddr */
+	move	a1, AT		/* arg2: the caller's next ip, parent */
+	PTR_SUBU a0, MCOUNT_INSN_SIZE
+	.endm

arch/mips/kernel/mcount.S new file +19 -0

+	bne	t0, t1, static_trace
+	nop
+
+	j	ftrace_stub
+	nop
+
+static_trace:
+	MCOUNT_SAVE_REGS
+
+	MCOUNT_SET_ARGS			/* call *ftrace_trace_function */
+	jalr	t1
+	nop
+
+	MCOUNT_RESTORE_REGS
+	.globl ftrace_stub
+ftrace_stub:
+	RETURN_BACK
+	END(_mcount)
+

Quoted 70 lines · Wu Zhangjin

+++ b/arch/mips/kernel/mcount.S

arch/mips/kernel/mcount.S new file +68 -0

@@ -0,0 +1,94 @@
+/*
+ * the mips-specific _mcount implementation
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file "COPYING" in the main directory of this archive for
+ * more details.
+ *
+ * Copyright (C) 2009 DSLab, Lanzhou University, China
+ * Author: Wu Zhangjin <[email protected]>
+ */
+
+#include <asm/regdef.h>
+#include <asm/stackframe.h>
+#include <asm/ftrace.h>
+
+	.text
+	.set noreorder
+	.set noat
+
+	/* since there is a "addiu sp,sp,-8" before "jal _mcount" in 32bit */
+	.macro RESTORE_SP_FOR_32BIT
+#ifdef CONFIG_32BIT
+	PTR_ADDIU	sp, 8
+#endif
+	.endm
+
+	.macro MCOUNT_SAVE_REGS
+	PTR_SUBU	sp, PT_SIZE
+	PTR_S	ra, PT_R31(sp)
+	PTR_S	AT, PT_R1(sp)
+	PTR_S	a0, PT_R4(sp)
+	PTR_S	a1, PT_R5(sp)
+	PTR_S	a2, PT_R6(sp)
+	PTR_S	a3, PT_R7(sp)
+#ifdef CONFIG_64BIT
+	PTR_S	a4, PT_R8(sp)
+	PTR_S	a5, PT_R9(sp)
+	PTR_S	a6, PT_R10(sp)
+	PTR_S	a7, PT_R11(sp)
+#endif
+	.endm
+
+	.macro MCOUNT_RESTORE_REGS
+	PTR_L	ra, PT_R31(sp)
+	PTR_L	AT, PT_R1(sp)
+	PTR_L	a0, PT_R4(sp)
+	PTR_L	a1, PT_R5(sp)
+	PTR_L	a2, PT_R6(sp)
+	PTR_L	a3, PT_R7(sp)
+#ifdef CONFIG_64BIT
+	PTR_L	a4, PT_R8(sp)
+	PTR_L	a5, PT_R9(sp)
+	PTR_L	a6, PT_R10(sp)
+	PTR_L	a7, PT_R11(sp)
+#endif
+	PTR_ADDIU	sp, PT_SIZE
+.endm
+
+	.macro MCOUNT_SET_ARGS
+	move	a0, ra		/* arg1: next ip, selfaddr */
+	move	a1, AT		/* arg2: the caller's next ip, parent */
+	PTR_SUBU a0, MCOUNT_INSN_SIZE
+	.endm
+
+	.macro RETURN_BACK
+	jr ra
+	move ra, AT 
+	.endm

arch/mips/kernel/mcount.S line 73

Quoted 5 lines

arch/mips/kernel/mcount.S new file +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

Quoted 2 lines · Steven Rostedt

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

Quoted 6 lines · Wu Zhangjin

Quoted 2 lines · Steven Rostedt

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

I don't know. I was just asking.

On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:

Quoted 7 lines

arch/mips/kernel/mcount.S line 73

Quoted 5 lines

arch/mips/kernel/mcount.S new file +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

Regards!

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 1 line · Wu Zhangjin

On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:

[...]

Quoted 8 lines

Quoted 7 lines

arch/mips/kernel/mcount.S line 73

Quoted 5 lines

arch/mips/kernel/mcount.S new file +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

Quoted 2 lines

So, perhaps we can use the saved registers(a0,a1...) instead.

a0..a7 may not always be saved.

You can use at, v0, v1 and all the temporary registers. Note that for the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7. So for a 64-bit kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting that at == $1 and contains the callers ra. For a 32-bit kernel you can add $8, $9, $10, and $11

This whole thing seems a little fragile.

I think it might be a good idea to get input from Richard Sandiford, and/or Adam Nemet about this approach (so I add them to the CC).

This e-mail thread starts here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00286.html

and here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html

Quoted 1 line · Wu Zhangjin

On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:

[...]

Quoted 10 lines

Quoted 7 lines

arch/mips/kernel/mcount.S line 73

Quoted 5 lines

arch/mips/kernel/mcount.S new file +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

a0..a7 may not always be saved.

You can use at, v0, v1 and all the temporary registers. Note that for the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7. So for a 64-bit kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting that at == $1 and contains the callers ra. For a 32-bit kernel you can add $8, $9, $10, and $11

This whole thing seems a little fragile.

Maybe scanning the instructions should stop at the beginning of the function based on the kernel's symbol table. I am not sure if we can establish any other stopping condition without affecting performance too much.

Speaking of performance, -pg also affects the instruction scheduling freedom of the compiler in the prologue. With profiling, we limit optimizations not to move instructions in and out of the prologue.

Also note that for functions invoked via tail call you won't get an exit event. E.g. if bar is tail-called from foo:

foo entered
bar entered
foo/bar exited

However, this is not MIPS-specific and you can always disable tail calls with -fno-optimize-sibling-calls.

Quoted 8 lines · Adam Nemet

Speaking of performance, -pg also affects the instruction scheduling freedom of the compiler in the prologue. With profiling, we limit optimizations not to move instructions in and out of the prologue.

Also note that for functions invoked via tail call you won't get an exit event. E.g. if bar is tail-called from foo:

Quoted 6 lines

foo entered
bar entered
foo/bar exited

However, this is not MIPS-specific and you can always disable tail calls with -fno-optimize-sibling-calls.

The question is, would bar have a _mcount call? So far, we have not had any issues with this on either x86 nor PPC.

/me knocks on wood.

Quoted 7 lines · Adam Nemet

Also note that for functions invoked via tail call you won't get an exit event. E.g. if bar is tail-called from foo:

foo entered
bar entered
foo/bar exited

Quoted 2 lines

However, this is not MIPS-specific and you can always disable tail calls with -fno-optimize-sibling-calls.

The question is, would bar have a _mcount call? So far, we have not had any issues with this on either x86 nor PPC.

Quoted 8 lines · Steven Rostedt

Quoted 7 lines · Adam Nemet

Also note that for functions invoked via tail call you won't get an exit event. E.g. if bar is tail-called from foo:

foo entered
bar entered
foo/bar exited

Quoted 5 lines

Quoted 2 lines

However, this is not MIPS-specific and you can always disable tail calls with -fno-optimize-sibling-calls.

The question is, would bar have a _mcount call? So far, we have not had any issues with this on either x86 nor PPC.

Yes, bar will have an _mcount call. The difference is that bar will return to foo's caller directly rather than to foo first.

Quoted 12 lines · Adam Nemet

Quoted 11 lines · Steven Rostedt

Quoted 9 lines · Adam Nemet

Also note that for functions invoked via tail call you won't get an exit event. E.g. if bar is tail-called from foo:

foo entered
bar entered
foo/bar exited

However, this is not MIPS-specific and you can always disable tail calls with -fno-optimize-sibling-calls.

Quoted 5 lines

Quoted 2 lines

The question is, would bar have a _mcount call? So far, we have not had any issues with this on either x86 nor PPC.

Yes, bar will have an _mcount call. The difference is that bar will return to foo's caller directly rather than to foo first.

I guess the best bet is to have CONFIG_FUNCTION_GRAPH enable -fno-optimize-sibling-calls and be done with it.

Quoted 1 line · Wu Zhangjin

On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:

[...]

Quoted 10 lines

Quoted 7 lines

arch/mips/kernel/mcount.S line 73

Quoted 5 lines

arch/mips/kernel/mcount.S new file +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

a0..a7 may not always be saved.

You can use at, v0, v1 and all the temporary registers. Note that for the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7. So for a 64-bit kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting that at == $1 and contains the callers ra. For a 32-bit kernel you can add $8, $9, $10, and $11

This whole thing seems a little fragile.

I think it might be a good idea to get input from Richard Sandiford, and/or Adam Nemet about this approach (so I add them to the CC).

This e-mail thread starts here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00286.html

and here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html

David Daney <[email protected]> writes:

Quoted 3 lines

and here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html

I'm not sure that the "search for a save of RA" thing is really a good idea. The last version of that seemed to be "assume that any register stores will be in a block that immediately precedes the move into RA", but even if that's true now, it might not be in future. And as Wu Zhangjin says, it doesn't cope with long calls, where the target address is loaded into a temporary register before the call.

FWIW, I'd certainly be happy to make GCC pass an additional parameter to _mcount. The parameter could give the address of the return slot, or null for leaf functions. In almost all cases[*], there would be no overhead, since the move would go in the delay slot of the call.

[*] Meaning when the frame is <=32k. ;) I'm guessing you never
get anywhere near that, and if you did, the scan thing wouldn't work anyway.

The new behaviour could be controlled by a command-line option, which would also give linux a cheap way of checking whether the feature is available.

Quoted 20 lines · David Daney

Quoted 1 line · Wu Zhangjin

On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:

[...]

Quoted 9 lines

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

a0..a7 may not always be saved.

You can use at, v0, v1 and all the temporary registers. Note that for the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7. So for a 64-bit kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting that at == $1 and contains the callers ra. For a 32-bit kernel you can add $8, $9, $10, and $11

This whole thing seems a little fragile.

I think it might be a good idea to get input from Richard Sandiford, and/or Adam Nemet about this approach (so I add them to the CC).

Quoted 7 lines

This e-mail thread starts here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00286.html

and here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html

I'm not sure that the "search for a save of RA" thing is really a good idea. The last version of that seemed to be "assume that any register stores will be in a block that immediately precedes the move into RA", but even if that's true now, it might not be in future. And as Wu Zhangjin says, it doesn't cope with long calls, where the target address is loaded into a temporary register before the call.

FWIW, I'd certainly be happy to make GCC pass an additional parameter to _mcount. The parameter could give the address of the return slot, or null for leaf functions. In almost all cases[*], there would be no overhead, since the move would go in the delay slot of the call.

[*] Meaning when the frame is <=32k. ;) I'm guessing you never
get anywhere near that, and if you did, the scan thing wouldn't work anyway.

The new behaviour could be controlled by a command-line option, which would also give linux a cheap way of checking whether the feature is available.

Quoted 46 lines · Richard Sandiford

Quoted 25 lines · David Daney

Quoted 1 line · Wu Zhangjin

On Wed, 2009-10-21 at 11:24 -0400, Steven Rostedt wrote:

[...]

Quoted 9 lines

Quoted 7 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+
+NESTED(_mcount, PT_SIZE, ra)
+	RESTORE_SP_FOR_32BIT
+	PTR_LA	t0, ftrace_stub
+	PTR_L	t1, ftrace_trace_function /* please don't use t1 later, safe? */

Is t0 and t1 safe for mcount to use? Remember, mcount does not follow the dynamics of C function ABI.

So, perhaps we can use the saved registers(a0,a1...) instead.

a0..a7 may not always be saved.

You can use at, v0, v1 and all the temporary registers. Note that for the 64-bit ABIs sometimes the names t0-t4 shadow a4-a7. So for a 64-bit kernel, you can use: $1, $2, $3, $12, $13, $14, $15, $24, $25, noting that at == $1 and contains the callers ra. For a 32-bit kernel you can add $8, $9, $10, and $11

This whole thing seems a little fragile.

I think it might be a good idea to get input from Richard Sandiford, and/or Adam Nemet about this approach (so I add them to the CC).

Quoted 7 lines

This e-mail thread starts here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00286.html

and here:

http://www.linux-mips.org/archives/linux-mips/2009-10/msg00290.html

I'm not sure that the "search for a save of RA" thing is really a good idea. The last version of that seemed to be "assume that any register stores will be in a block that immediately precedes the move into RA", but even if that's true now, it might not be in future. And as Wu Zhangjin says, it doesn't cope with long calls, where the target address is loaded into a temporary register before the call.

FWIW, I'd certainly be happy to make GCC pass an additional parameter to _mcount. The parameter could give the address of the return slot, or null for leaf functions. In almost all cases[*], there would be no overhead, since the move would go in the delay slot of the call.

Quoted 8 lines

[*] Meaning when the frame is <=32k. ;) I'm guessing you never
get anywhere near that, and if you did, the scan thing wouldn't work anyway.

The new behaviour could be controlled by a command-line option, which would also give linux a cheap way of checking whether the feature is available.

How about this patch, I think it does what you suggest.

When we pass -pg -mmcount-raloc, the address of the return address relative to sp is passed in $12 to _mcount. If the return address is not saved, $12 will be zero. I think this will work as registers are never saved with an offset of zero. $12 is a temporary register that is not part of the ABI.

config/mips/mips.c +6 -0

+  if (TARGET_RALOC)
+    {
+      /* If TARGET_RALOC load $12 with the offset of the ra save
+	 location.  */
+      if (mips_raloc_in_delay_slot_p())
+	emit_small_ra_offset = 1;

Thanks for the patch.

Quoted 7 lines

How about this patch, I think it does what you suggest.

When we pass -pg -mmcount-raloc, the address of the return address relative to sp is passed in $12 to _mcount. If the return address is not saved, $12 will be zero. I think this will work as registers are never saved with an offset of zero. $12 is a temporary register that is not part of the ABI.

Hmm, well, the suggestion was to pass a pointer rather than an offset, but both you and Wu Zhangjin seem to prefer the offset. Is there a reason for that? I suggested a pointer because

(a) they're more C-like
(b) they're just as quick and easy to compute
(c) MIPS doesn't have indexed addresses (well, except for a few special cases) so the callee is going to have to compute the pointer at some stage anyway

(It sounds from Wu Zhangjin's reply like he'd alread suggested the offset thing before I sent my message. If so, sorry for not using that earlier message as context.)

Quoted 6 lines

config/mips/mips.c +6 -0

+  if (TARGET_RALOC)
+    {
+      /* If TARGET_RALOC load $12 with the offset of the ra save
+	 location.  */
+      if (mips_raloc_in_delay_slot_p())
+	emit_small_ra_offset = 1;

config/mips/mips.c mips_trampoline_init (rtx m_tramp, tree · line 16142

Quoted 10 lines

config/mips/mips.c +10 -0

+      else
+	{
+	  if (Pmode == DImode)
+	    fprintf (file, "\tdli\t%s,%d\t\t# offset of ra\n", reg_names[12],
+		     cfun->machine->frame.ra_fp_offset);
+	  else
+	    fprintf (file, "\tli\t%s,%d\t\t# offset of ra\n", reg_names[12],
+		     cfun->machine->frame.ra_fp_offset);
+	}
+    }

We shouldn't need to do the delay slot dance. With either the pointer ((D)LA) or offset ((D)LI) approach, the macro won't use $at, so we can insert the new code immediately before the jump, leaving the assembler to fill the delay slot. This is simpler and should mean that the delay slot gets filled more often in the multi-insn-macro cases.

Looks good otherwise, but I'd be interested in other suggestions for the option name. I kept misreading "raloc" as a typo for "reloc".

Thanks for the patch.

Quoted 1 line · David Daney

Richard Sandiford wrote:

Quoted 16 lines

Quoted 6 lines

FWIW, I'd certainly be happy to make GCC pass an additional parameter to _mcount. The parameter could give the address of the return slot, or null for leaf functions. In almost all cases[*], there would be no overhead, since the move would go in the delay slot of the call.

Quoted 8 lines

[*] Meaning when the frame is <=32k. ;) I'm guessing you never
get anywhere near that, and if you did, the scan thing wouldn't work anyway.

The new behaviour could be controlled by a command-line option, which would also give linux a cheap way of checking whether the feature is available.

How about this patch, I think it does what you suggest.

Hmm, well, the suggestion was to pass a pointer rather than an offset, but both you and Wu Zhangjin seem to prefer the offset. Is there a reason for that? I suggested a pointer because

(a) they're more C-like
(b) they're just as quick and easy to compute
(c) MIPS doesn't have indexed addresses (well, except for a few special cases) so the callee is going to have to compute the pointer at some stage anyway

Looks good otherwise, but I'd be interested in other suggestions for the option name. I kept misreading "raloc" as a typo for "reloc".

Quoted 3 lines

Looks good otherwise, but I'd be interested in other suggestions for the option name. I kept misreading "raloc" as a typo for "reloc".

Well how about raaddr? (Return-Address Address)

Still not tested, but how about this version instead?

gcc/
2009-10-26 David Daney <[email protected]>

* doc/invoke.texi (mmcount-raaddr): Document new command line option.
* config/mips/mips.opt (mmcount-raaddr): New option.
* config/mips/mips-protos.h (mips_function_profiler): Declare new function.
* config/mips/mips.c (struct mips_frame_info): Add ra_fp_offset member.
(mips_for_each_saved_gpr_and_fpr): Set ra_fp_offset. (mips_function_profiler): Moved from FUNCTION_PROFILER, and rewritten.
* config/mips/mips.h (FUNCTION_PROFILER): Body of macro moved to mips_function_profiler.

gcc/testsuite/
2009-10-26 David Daney <[email protected]>

* gcc.target/mips/mips.exp (mips_option_groups): Add mcount-raaddr.
* gcc.target/mips/mmcount-raaddr-1.c: New test.
* gcc.target/mips/mmcount-raaddr-2.c: New test.
* gcc.target/mips/mmcount-raaddr-3.c: New test.

plain text document attachment (mcount.patch)
Index: gcc/doc/invoke.texi
===================================================================

doc/invoke.texi +14 -1

@@ -709,7 +709,7 @@ Objective-C and Objective-C++ Dialects}.
 -mbranch-cost=@var{num}  -mbranch-likely  -mno-branch-likely @gol
 -mfp-exceptions -mno-fp-exceptions @gol
 -mvr4130-align -mno-vr4130-align -msynci -mno-synci @gol
--mrelax-pic-calls -mno-relax-pic-calls}
+-mrelax-pic-calls -mno-relax-pic-calls -mmcount-raaddr}
 
 @emph{MMIX Options}
 @gccoptlist{-mlibfuncs  -mno-libfuncs  -mepsilon  -mno-epsilon  -mabi=gnu @gol

@@ -14192,6 +14192,19 @@ an assembler and a linker that supports 
 directive and @code{-mexplicit-relocs} is in effect.  With
 @code{-mno-explicit-relocs}, this optimization can be performed by the
 assembler and the linker alone without help from the compiler.
+
+@item -mmcount-raaddr
+@itemx -mno-mcount-raaddr
+@opindex mmcount-raaddr
+@opindex mno-mcount-raaddr
+Emit (do not emit) code to pass the address of the return address save
+location to @code{_mcount}.  The default is @option{-mno-mcount-raaddr}.
+
+@option{-mmcount-raaddr} can be used in conjunction with @option{-pg}
+and a specially coded @code{_mcount} function to record function exit
+by replacing the saved return address with a pointer to the function
+exit profiling function.
+
 @end table
 
 @node MMIX Options

Index: gcc/testsuite/gcc.target/mips/mips.exp
===================================================================

testsuite/gcc.target/mips/mips.exp +1 -0

@@ -263,6 +263,7 @@ foreach option {
     sym32
     synci
     relax-pic-calls
+    mcount-raaddr
 } {
     lappend mips_option_groups $option "-m(no-|)$option"
 }

Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-1.c
===================================================================

testsuite/gcc.target/mips/mmcount-raaddr-1.c +7 -0

@@ -0,0 +1,7 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
+/* { dg-final { scan-assembler "\tmove\t\\\$12,\\\$0" } } */
+int bazl(int i)
+{
+  return i + 2;
+}

Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-2.c
===================================================================

testsuite/gcc.target/mips/mmcount-raaddr-2.c +8 -0

@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
+/* { dg-final { scan-assembler "\tdaddiu\t\\\$12,\\\$sp,8" } } */
+int foo (int);
+int bar (int i)
+{
+  return foo (i) + 2;
+}

Index: gcc/testsuite/gcc.target/mips/mmcount-raaddr-3.c
===================================================================

testsuite/gcc.target/mips/mmcount-raaddr-3.c +10 -0

@@ -0,0 +1,10 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */
+/* { dg-final { scan-assembler "\tdli\t\\\$12,200008" } } */
+/* { dg-final { scan-assembler "\tdaddu\t\\\$12,\\\$12,\\\$sp" } } */
+int foo (int *);
+int bar(int i)
+{
+  int big[50000];
+  return foo (big) + 2;
+}

Index: gcc/config/mips/mips.opt
===================================================================

config/mips/mips.opt +4 -0

@@ -208,6 +208,10 @@ mlong64
 Target Report RejectNegative Mask(LONG64)
 Use a 64-bit long type
 
+mmcount-raaddr
+Target Report Var(TARGET_MCOUNT_RAADDR)
+Pass the address of the ra save location to _mcount in $12
+
 mmemcpy
 Target Report Mask(MEMCPY)
 Don't optimize block moves

Index: gcc/config/mips/mips-protos.h
===================================================================

config/mips/mips-protos.h +1 -0

@@ -344,5 +344,6 @@ extern bool mips_eh_uses (unsigned int);
 extern bool mips_epilogue_uses (unsigned int);
 extern void mips_final_prescan_insn (rtx, rtx *, int);
 extern int mips_trampoline_code_size (void);
+extern void mips_function_profiler (FILE *);
 
 #endif /* ! GCC_MIPS_PROTOS_H */

Index: gcc/config/mips/mips.c
===================================================================

config/mips/mips.c +74 -0

@@ -319,6 +319,9 @@ struct GTY(())  mips_frame_info {
   HOST_WIDE_INT acc_sp_offset;
   HOST_WIDE_INT cop0_sp_offset;
 
+  /* Similar, but the value passed to _mcount.  */
+  HOST_WIDE_INT ra_fp_offset;
+
   /* The offset of arg_pointer_rtx from the bottom of the frame.  */
   HOST_WIDE_INT arg_pointer_offset;
 

@@ -9616,6 +9619,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WI
   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
     if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
+	/* Record the ra offset for use by mips_function_profiler.  */
+	if (regno == RETURN_ADDR_REGNUM)
+	  cfun->machine->frame.ra_fp_offset = offset + sp_offset;
 	mips_save_restore_reg (word_mode, regno, offset, fn);
 	offset -= UNITS_PER_WORD;
       }

@@ -16088,6 +16094,74 @@ mips_trampoline_init (rtx m_tramp, tree 
   emit_insn (gen_add3_insn (end_addr, addr, GEN_INT (TRAMPOLINE_SIZE)));
   emit_insn (gen_clear_cache (addr, end_addr));
 }
+
+/* Implement FUNCTION_PROFILER.  */
+
+void mips_function_profiler (FILE *file)
+{
+  if (TARGET_MIPS16)
+    sorry ("mips16 function profiling");
+  if (TARGET_LONG_CALLS)
+    {
+      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */
+      if (Pmode == DImode)
+	fprintf (file, "\tdla\t%s,_mcount\n", reg_names[3]);
+      else
+	fprintf (file, "\tla\t%s,_mcount\n", reg_names[3]);
+    }
+  mips_push_asm_switch (&mips_noat);
+  fprintf (file, "\tmove\t%s,%s\t\t# save current return address\n",
+	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[2],
+	     reg_names[STATIC_CHAIN_REGNUM]);
+  if (TARGET_MCOUNT_RAADDR)
+    {
+      /* If TARGET_MCOUNT_RAADDR load $12 with the address of the ra save
+	 location.  */
+      if (cfun->machine->frame.ra_fp_offset == 0)
+	/* ra not saved, pass zero.  */
+	fprintf (file, "\tmove\t%s,%s\t\t# address of ra\n",
+		 reg_names[12], reg_names[0]);
+      else if (SMALL_OPERAND (cfun->machine->frame.ra_fp_offset))
+	fprintf (file,
+		 "\t%s\t%s,%s," HOST_WIDE_INT_PRINT_DEC "\t\t# address of ra\n",
+		 Pmode == DImode ? "daddiu" : "addiu",
+		 reg_names[12], reg_names[STACK_POINTER_REGNUM],
+		 cfun->machine->frame.ra_fp_offset);
+      else
+	{
+	  fprintf (file,
+		   "\t%s\t%s," HOST_WIDE_INT_PRINT_DEC "\n",
+		   Pmode == DImode ? "dli" : "li",
+		   reg_names[12],
+		   cfun->machine->frame.ra_fp_offset);
+	  fprintf (file, "\t%s\t%s,%s,%s\t\t# address of ra\n",
+		   Pmode == DImode ? "daddu" : "addu",
+		   reg_names[12], reg_names[12],
+		   reg_names[STACK_POINTER_REGNUM]);
+	}
+    }
+  if (!TARGET_NEWABI)
+    {
+      fprintf (file,
+	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
+	       TARGET_64BIT ? "dsubu" : "subu",
+	       reg_names[STACK_POINTER_REGNUM],
+	       reg_names[STACK_POINTER_REGNUM],
+	       Pmode == DImode ? 16 : 8);
+    }
+  if (TARGET_LONG_CALLS)
+    fprintf (file, "\tjalr\t%s\n", reg_names[3]);
+  else
+    fprintf (file, "\tjal\t_mcount\n");
+  mips_pop_asm_switch (&mips_noat);
+  /* _mcount treats $2 as the static chain register.  */
+  if (cfun->static_chain_decl != NULL)
+    fprintf (file, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],
+	     reg_names[2]);
+}
 
 /* Initialize the GCC target structure.  */
 #undef TARGET_ASM_ALIGNED_HI_OP

Index: gcc/config/mips/mips.h
===================================================================

config/mips/mips.h +1 -38

@@ -2372,44 +2372,7 @@ typedef struct mips_args {
 /* Output assembler code to FILE to increment profiler label # LABELNO
    for profiling a function entry.  */
 
-#define FUNCTION_PROFILER(FILE, LABELNO)				\
-{									\
-  if (TARGET_MIPS16)							\
-    sorry ("mips16 function profiling");				\
-  if (TARGET_LONG_CALLS)						\
-    {									\
-      /*  For TARGET_LONG_CALLS use $3 for the address of _mcount.  */	\
-      if (Pmode == DImode)						\
-	fprintf (FILE, "\tdla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-      else								\
-	fprintf (FILE, "\tla\t%s,_mcount\n", reg_names[GP_REG_FIRST + 3]); \
-    }									\
-  mips_push_asm_switch (&mips_noat);					\
-  fprintf (FILE, "\tmove\t%s,%s\t\t# save current return address\n",	\
-	   reg_names[AT_REGNUM], reg_names[RETURN_ADDR_REGNUM]);	\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[2],			\
-	     reg_names[STATIC_CHAIN_REGNUM]);				\
-  if (!TARGET_NEWABI)							\
-    {									\
-      fprintf (FILE,							\
-	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n", \
-	       TARGET_64BIT ? "dsubu" : "subu",				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       reg_names[STACK_POINTER_REGNUM],				\
-	       Pmode == DImode ? 16 : 8);				\
-    }									\
-  if (TARGET_LONG_CALLS)						\
-    fprintf (FILE, "\tjalr\t%s\n", reg_names[GP_REG_FIRST + 3]);	\
-  else									\
-    fprintf (FILE, "\tjal\t_mcount\n");					\
-  mips_pop_asm_switch (&mips_noat);					\
-  /* _mcount treats $2 as the static chain register.  */		\
-  if (cfun->static_chain_decl != NULL)					\
-    fprintf (FILE, "\tmove\t%s,%s\n", reg_names[STATIC_CHAIN_REGNUM],	\
-	     reg_names[2]);						\
-}
+#define FUNCTION_PROFILER(FILE, LABELNO) mips_function_profiler ((FILE))
 
 /* The profiler preserves all interesting registers, including $31.  */
 #define MIPS_SAVE_REG_FOR_PROFILING_P(REGNO) false

Quoted 2 lines

Looks good otherwise, but I'd be interested in other suggestions for the option name. I kept misreading "raloc" as a typo for "reloc".

Well how about raaddr? (Return-Address Address)

+@option{-mmcount-raaddr} can be used in conjunction with @option{-pg} +and a specially coded @code{_mcount} function to record function exit

+ /* For TARGET_LONG_CALLS use $3 for the address of _mcount. */

config/mips/mips.c +8 -0

+      else if (SMALL_OPERAND (cfun->machine->frame.ra_fp_offset))
+	fprintf (file,
+		 "\t%s\t%s,%s," HOST_WIDE_INT_PRINT_DEC "\t\t# address of ra\n",
+		 Pmode == DImode ? "daddiu" : "addiu",
+		 reg_names[12], reg_names[STACK_POINTER_REGNUM],
+		 cfun->machine->frame.ra_fp_offset);
+      else
+	{

OK with those changes,

thanks, if it passing testing, and if
Ralf is happy with the linux side.

Hi,

On Tue, 2009-10-27 at 21:20 +0000, Richard Sandiford wrote:
[...]

Quoted 1 line

OK with those changes,

Just applied the above changes, and added one more for not getting the ra_fp_offset when -pg is not enabled:

config/mips/mips.c +3 -0

@@ -9619,6 +9622,9 @@ mips_for_each_saved_gpr_and_fpr (HOST_WIDE_INT
 sp_offset,
   for (regno = GP_REG_LAST; regno >= GP_REG_FIRST; regno--)
     if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
+       /* Record the ra offset for use by mips_function_profiler.  */
+       if (crtl->profile && regno == RETURN_ADDR_REGNUM)
+         cfun->machine->frame.ra_fp_offset = offset + sp_offset;
        mips_save_restore_reg (word_mode, regno, offset, fn);
        offset -= UNITS_PER_WORD;

       }

"crtl->profile &&" was added.

Quoted 3 lines · David Daney

Quoted 2 lines

Looks good otherwise, but I'd be interested in other suggestions for the option name. I kept misreading "raloc" as a typo for "reloc".

Well how about raaddr? (Return-Address Address)

Taking Wu Zhangjin's suggestion of an extra dash, how about -mmcount-ra-address?

Quoted 2 lines

+@option{-mmcount-raaddr} can be used in conjunction with @option{-pg} +and a specially coded @code{_mcount} function to record function exit

Doc conventions require "specially-coded", I think. We should mention the $12 register too, and the treatment of leaf functions. How about:

--------------------------
Allow (do not allow) @code{_mcount} to modify the calling function's return address. When enabled, this option extends the usual @code{_mcount} interface with a new @var{ra-address} parameter, which has type @code{intptr_t *} and is passed in register @code{$12}. @code{_mcount} can then modify the return address by doing both of the following:
@itemize
@item
Returning the new address in register @code{$31}.
@item
Storing the new address in @code{*@var{ra-address}},
if @var{ra-address} is nonnull.
@end @itemize

The default is @option{-mno-mcount-ra-address}.
--------------------------

Review

Quoted 3 lines

(quoted diff) +2 -0

+/* { dg-do compile } */
+/* { dg-options "-O2 -pg -mmcount-raaddr -march=mips64 -mabi=64 -mno-abicalls -msym32" } */

This is a general feature, so I'd rather test with as few special options as possible. Just "-O2 -pg" if we can. Same with the other tests.

Sorry to ask, but while you're here, would you mind fixing a couple of pre-existing formatting problems too? Namely:

Quoted 1 line

+ /* For TARGET_LONG_CALLS use $3 for the address of _mcount. */

Only one space for "For" and

Review

Quoted 9 lines

(quoted diff) +9 -0

+  if (!TARGET_NEWABI)
+    {
+      fprintf (file,
+	       "\t%s\t%s,%s,%d\t\t# _mcount pops 2 words from  stack\n",
+	       TARGET_64BIT ? "dsubu" : "subu",
+	       reg_names[STACK_POINTER_REGNUM],
+	       reg_names[STACK_POINTER_REGNUM],
+	       Pmode == DImode ? 16 : 8);
+    }

No braces here.

As far as the new bits are concerned:

Review

Quoted 6 lines

(quoted diff) +6 -0

+      /* If TARGET_MCOUNT_RAADDR load $12 with the address of the ra save
+	 location.  */
+      if (cfun->machine->frame.ra_fp_offset == 0)
+	/* ra not saved, pass zero.  */
+	fprintf (file, "\tmove\t%s,%s\t\t# address of ra\n",
+		 reg_names[12], reg_names[0]);

Let's drop the "# address of ra" comment. We don't add comments to the other bits.