Re: [PATCH -tip v2 7/8] perf probe: Remove die() from probe-finder code

[Arnaldo Carvalho de Melo <acme@infradead.org>]

Em Fri, Apr 09, 2010 at 07:18:38PM -0400, Masami Hiramatsu escreveu:
> Hi Arnaldo,
> 
> Has this code done what you suggested? :)
> I'd like to have your comment.

It improves the current situation, yes, but there are still cases there
where die() is called, I assume that is left for later, right?

More comments below.
 
> Thank you,
> 
> Masami Hiramatsu wrote:
> > Remove die() and DIE_IF() code from util/probe-finder.c since
> > these 'sudden death' in utility functions make reusing it from
> > other code (especially tui/gui) difficult.
> > 
> > Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Paul Mackerras <paulus@samba.org>
> > Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Mike Galbraith <efault@gmx.de>
> > Cc: Frederic Weisbecker <fweisbec@gmail.com>
> > ---
> > 
> >  tools/perf/util/probe-event.c  |    4 
> >  tools/perf/util/probe-finder.c |  517 +++++++++++++++++++++++++---------------
> >  2 files changed, 322 insertions(+), 199 deletions(-)
> > 
> > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > index bef2805..7893b32 100644
> > --- a/tools/perf/util/probe-event.c
> > +++ b/tools/perf/util/probe-event.c
> > @@ -151,10 +151,10 @@ static int try_to_find_kprobe_trace_events(struct perf_probe_event *pev,
> >  
> >  	/* Error path */
> >  	if (need_dwarf) {
> > -		if (ntevs == -ENOENT)
> > +		if (ntevs == -EBADF)
> >  			pr_warning("No dwarf info found in the vmlinux - "
> >  				"please rebuild with CONFIG_DEBUG_INFO=y.\n");
> > -		die("Could not analyze debuginfo.");
> > +		die("Failed to analyze debuginfo.");

Like here, the TUI/GUI can try to add a probe but if it fails it can
still continue providing things like a "perf top" window, analysing
perf.data files, doing 'perf diffs', etc.

> >  	}
> >  	pr_debug("An error occurred in debuginfo analysis."
> >  		 " Try to use symbols.\n");
> > diff --git a/tools/perf/util/probe-finder.c b/tools/perf/util/probe-finder.c
> > index 1f45285..4a57504 100644
> > --- a/tools/perf/util/probe-finder.c
> > +++ b/tools/perf/util/probe-finder.c
> > @@ -196,19 +196,7 @@ static bool die_compare_name(Dwarf_Die *dw_die, const char *tname)
> >  {
> >  	const char *name;
> >  	name = dwarf_diename(dw_die);
> > -	DIE_IF(name == NULL);
> > -	return strcmp(tname, name);
> > -}
> > -
> > -/* Get entry pc(or low pc, 1st entry of ranges)  of the die */
> > -static Dwarf_Addr die_get_entrypc(Dwarf_Die *dw_die)
> > -{
> > -	Dwarf_Addr epc;
> > -	int ret;
> > -
> > -	ret = dwarf_entrypc(dw_die, &epc);
> > -	DIE_IF(ret == -1);
> > -	return epc;
> > +	return name ? strcmp(tname, name) : -1;

This change is correct, with the stated intent :-)

> >  }
> >  
> >  /* Get type die, but skip qualifiers and typedef */
> > @@ -390,7 +378,7 @@ static Dwarf_Die *die_find_member(Dwarf_Die *st_die, const char *name,
> >   */
> >  
> >  /* Show a location */
> > -static void convert_location(Dwarf_Op *op, struct probe_finder *pf)
> > +static int convert_location(Dwarf_Op *op, struct probe_finder *pf)

Yeah, there are still lots of places, in other areas that don't return a
status, just printing a warning and silently failing, like in the trace
parsing routines

> >  {
> >  	unsigned int regn;
> >  	Dwarf_Word offs = 0;
> > @@ -400,8 +388,11 @@ static void convert_location(Dwarf_Op *op, struct probe_finder *pf)
> >  
> >  	/* If this is based on frame buffer, set the offset */
> >  	if (op->atom == DW_OP_fbreg) {
> > -		if (pf->fb_ops == NULL)
> > -			die("The attribute of frame base is not supported.\n");
> > +		if (pf->fb_ops == NULL) {
> > +			pr_warning("The attribute of frame base is not "
> > +				   "supported.\n");
> > +			return -ENOTSUP;
> > +		}

Correct, if you look at tools/perf/util/debug.c, eprintf() looks if
we're in TUI mode, use_browser is true and either fprintfs(stderr) if
not or calls a routine to put it at the "status line" (bottom) in the
TUI.

Having a list with the last messages so that we can have a log window,
or keeping it in a log file that would then be browser is an enhancement
to be made.

> >  		ref = true;
> >  		offs = op->number;
> >  		op = &pf->fb_ops[0];
> > @@ -419,50 +410,63 @@ static void convert_location(Dwarf_Op *op, struct probe_finder *pf)
> >  		ref = true;
> >  	} else if (op->atom == DW_OP_regx) {
> >  		regn = op->number;
> > -	} else
> > -		die("DW_OP %d is not supported.", op->atom);
> > +	} else {
> > +		pr_warning("DW_OP %d is not supported.\n", op->atom);
> > +		return -ENOTSUP;
> > +	}

correct

> >  	regs = get_arch_regstr(regn);
> > -	if (!regs)
> > -		die("%u exceeds max register number.", regn);
> > +	if (!regs) {
> > +		pr_warning("%u exceeds max register number.\n", regn);
> > +		return -ERANGE;
> > +	}
> >  
> >  	tvar->value = xstrdup(regs);
> >  	if (ref) {
> >  		tvar->ref = xzalloc(sizeof(struct kprobe_trace_arg_ref));

We have to kill those xzcalloc, etc, too they are die() in disguise :-)

- Arnaldo