Re: [PATCH -v5 10/11] tracing: add function graph tracer support for MIPS

[Steven Rostedt <rostedt@goodmis.org>]

On Sun, 2009-10-25 at 23:17 +0800, Wu Zhangjin wrote:

> +
> +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;
> +
> +		/* get the code at "ip" */
> +		code = *(unsigned int *)ip;

Probably want to put the above in an asm with exception handling.

> +
> +		/* 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. */
> +		if ((code & MOV_FP_SP) == MOV_FP_SP)
> +			return parent_addr;
> +	} while (((code & S_RA) != S_RA));

Hmm, that condition also looks worrisome. Should we just always search
for s{d,w} R,X(sp)?

Since there should only be stores of registers into the sp above the
jump to mcount. The break out loop is a check for move. I think it would
be safer to have the break out loop is a check for non storing of a
register into SP.

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

Also might want to make the above into a asm with exception handling.

> +
> +	if (ra == parent)
> +		return sp;
> +
> +	ftrace_graph_stop();
> +	WARN_ON(1);
> +	return parent_addr;

Hmm, may need to do more than this. See below.

> +}
> +
> +/*
> + * 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;
> +	struct ftrace_graph_ent trace;
> +	unsigned long return_hooker = (unsigned long)
> +	    &return_to_handler;
> +
> +	if (unlikely(atomic_read(&current->tracing_graph_pause)))
> +		return;
> +
> +	/* "parent" is the stack address saved the return address of the caller
> +	 * of _mcount, for a leaf function not save the return address in the
> +	 * stack address, so, we "emulate" one in _mcount's stack space, and
> +	 * hijack it directly, but for a non-leaf function, it will save the
> +	 * return address to the its stack space, so, we can not hijack the
> +	 * "parent" directly, but need to find the real stack address,
> +	 * ftrace_get_parent_addr() does it!
> +	 */
> +
> +	old = *parent;
> +
> +	parent = (unsigned long *)ftrace_get_parent_addr(self_addr, old,
> +							 (unsigned long)parent,
> +							 fp);
> +
> +	*parent = return_hooker;

Although you may have turned off fgraph tracer in
ftrace_get_parent_addr, nothing stops the below from messing with the
stack. The return stack may get off sync and break later. If you fail
the above, you should not be calling the push function below.


-- Steve

> +
> +	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 */