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

[Wu Zhangjin <wuzhangjin@gmail.com>]

Hi,

On Mon, 2009-10-26 at 11:13 -0400, Steven Rostedt wrote:
[...]
> > +
> > +		/* get the code at "ip" */
> > +		code = *(unsigned int *)ip;
> 
> Probably want to put the above in an asm with exception handling.
> 

Seems that exception handling in an asm is really "awful"(un-readable)
and the above ip is what we have got from the ftrace_graph_caller, it
should be okay. but if exception handling is necessary, I will send a
new patch for the places(including the following one) which need it. 

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


Okay, let's look at this with -mlong-calls,

leaf function:

ffffffff80243cd8 <oops_may_print>:
ffffffff80243cd8:       67bdfff0        daddiu  sp,sp,-16
ffffffff80243cdc:       ffbe0008        sd      s8,8(sp)
ffffffff80243ce0:       03a0f02d        move    s8,sp
ffffffff80243ce4:       3c038021        lui     v1,0x8021
ffffffff80243ce8:       646316b0        daddiu  v1,v1,5808
ffffffff80243cec:       03e0082d        move    at,ra
ffffffff80243cf0:       0060f809        jalr    v1
ffffffff80243cf4:       00020021        nop

non-leaf function:

ffffffff802414c0 <copy_process>:
ffffffff802414c0:       67bdff40        daddiu  sp,sp,-192
ffffffff802414c4:       ffbe00b0        sd      s8,176(sp)
ffffffff802414c8:       03a0f02d        move    s8,sp
ffffffff802414cc:       ffbf00b8        sd      ra,184(sp)
ffffffff802414d0:       ffb700a8        sd      s7,168(sp)
ffffffff802414d4:       ffb600a0        sd      s6,160(sp)
ffffffff802414d8:       ffb50098        sd      s5,152(sp)
ffffffff802414dc:       ffb40090        sd      s4,144(sp)
ffffffff802414e0:       ffb30088        sd      s3,136(sp)
ffffffff802414e4:       ffb20080        sd      s2,128(sp)
ffffffff802414e8:       ffb10078        sd      s1,120(sp)
ffffffff802414ec:       ffb00070        sd      s0,112(sp)
ffffffff802414f0:       3c038021        lui     v1,0x8021
ffffffff802414f4:       646316b0        daddiu  v1,v1,5808
ffffffff802414f8:       03e0082d        move    at,ra
ffffffff802414fc:       0060f809        jalr    v1
ffffffff80241500:       00020021        nop
ip -->  

At first, we move to "lui, v1, HI_16BIT_OF_MCOUNT", ip = ip - 12(not 8
when without -mlong-calls, i need to update the source code later).

and then, we check whether there is a "Store" instruction, if it's not a
"Store" instruction, the function should be a leaf? otherwise, we
continue the searching until finding the "s{d,w} ra, offset(sp)"
instruction, get the offset, calculate the stack address, and finish?

So, we just need to replace this:

		if ((code & MOV_FP_SP) == MOV_FP_SP)
			return parent_addr;	
	
by

#define S_INSN	(0xafb0 << 16)

		if ((code & S_INSN) != S_INSN)
			return parent_addr;

> 
> > +
> > +	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.
> 

We need to really stop before ftrace_push_return_trace to avoid messing
with the stack :-) but if we have stopped the tracer, is it important to
mess with the stack or not?

Regards,
	Wu Zhangjin