Re: [PATCH] fgraph: record function return value

[Changbin Du]

On Mon, Jan 14, 2019 at 12:11:56PM +0000, Mark Rutland wrote:
> On Sat, Jan 12, 2019 at 02:57:01PM +0800, Changbin Du wrote:
> > This patch adds a new trace option 'funcgraph-retval' and is disabled by
> > default. When this option is enabled, fgraph tracer will show the return
> > value of each function. This is useful to find/analyze a original error
> > source in a call graph.
> > 
> > One limitation is that kernel doesn't know the prototype of functions. So
> > fgraph assumes all functions have a retvalue of type int. You must ignore
> > the value of *void* function. And if the retvalue looks like an error code
> > then both hexadecimal and decimal number are displayed.
> 
> This sounds more confusing than helpful, and it sounds like this has
> overlap with FTRACE_WITH_REGS functionality.
> 
Acctually this is similar to Return Probes. The kprobe has same
situation and it uses regs_return_value() to get retvalue. On x86 it is:
static inline long regs_return_value(struct pt_regs *regs)
{
	return PT_REGS_AX(regs);
}

on arm it is:
static inline long regs_return_value(struct pt_regs *regs)
{
        return regs->ARM_r0;
}
Due to lack of prototype info we cannot handle complex types.

FTRACE_WITH_REGS saves all general registers but here only one.
> > diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> > index 81b8eb5c4633..223f4ad269d4 100644
> > --- a/arch/arm64/kernel/entry-ftrace.S
> > +++ b/arch/arm64/kernel/entry-ftrace.S
> > @@ -202,6 +202,7 @@ ENTRY(return_to_handler)
> >  	stp x4, x5, [sp, #32]
> >  	stp x6, x7, [sp, #48]
> >  
> > +	mov	x1, x0			// return value
> >  	mov	x0, x29			//     parent's fp
> >  	bl	ftrace_return_to_handler// addr = ftrace_return_to_hander(fp);
> >  	mov	x30, x0			// restore the original return address
> 
> What about indirect return values? Those go via x8.
> 
> Additionally, in some cases (e.g. static functions with cross-function
> optimization), the compiler might not follow the usual PCS, so the
> return value might not be in x0 regardless. Maybe such functions aren't
> hooked by ftrace today?
I think these functions have been optimized out so ftrace will not trace
them.

> 
> Generally, I don't think that this is going to be reliable.
> 
> > +config HAVE_FTRACE_RETVAL
> > +	bool
> > +
> >  config HAVE_DYNAMIC_FTRACE
> >  	bool
> >  	help
> > @@ -160,6 +163,7 @@ config FUNCTION_GRAPH_TRACER
> >  	depends on HAVE_FUNCTION_GRAPH_TRACER
> >  	depends on FUNCTION_TRACER
> >  	depends on !X86_32 || !CC_OPTIMIZE_FOR_SIZE
> > +	select HAVE_FTRACE_RETVAL if (X86 || ARM)
> 
> ... but not arm64?
> 
> Thanks,
> Mark.

-- 
Thanks,
Changbin Du