RE: [PATCH V4 3/3] perf tool: Add sort key symoff for perf diff

[Liang, Kan]

> 
> On Tue, 18 Nov 2014 11:38:20 -0500, kan liang wrote:
> > From: Kan Liang <kan.liang@xxxxxxxxx>
> >
> > Sometime, especially debugging scaling issue, the function level diff
> > may be high granularity. The user may want to do deeper diff analysis
> > for some cache or lock issue. The "symoff" key can let the user sort
> > differential profile by ips. This feature should only work when the
> > perf.data comes from same binary.
> 
> I think the symoff sort key now works well for different (i.e. modified)
> binaries too.

For different binaries, the function may be changed. So the offset may
point to different code. 
What about this?
"This feature should work when the perf.data comes from
either same binary or same function of different binary."
Or just simply remove this sentence.

> 
> >
> >  -s::
> >  --sort=::
> > -	Sort by key(s): pid, comm, dso, symbol, cpu, parent, srcline.
> > -	Please see description of --sort in the perf-report man page.
> > +	Sort by key(s): pid, comm, dso, symbol, cpu, parent, srcline, symoff.
> > +
> > +	- symoff: exact symbol +  offset address executed at the time of
> sample.
> > +	(for same binary compare)
> 
> Ditto.  And symoff is not only for perf diff, but it should work for normal
> perf report also.  So you'd better move the description to perf report man
> page IMHO.

OK, I will do it, and also remove "(for same binary compare)"

> 
> 
> > +
> > +	For other keys, please see description of --sort in the perf-report
> man page.
> >
> >  -t::
> >  --field-separator=::
> > diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> > index 1ce425d..03a4001 100644
> > --- a/tools/perf/builtin-diff.c
> > +++ b/tools/perf/builtin-diff.c
> > @@ -744,7 +744,7 @@ static const struct option options[] = {
> >  	OPT_STRING('S', "symbols", &symbol_conf.sym_list_str,
> "symbol[,symbol...]",
> >  		   "only consider these symbols"),
> >  	OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
> > -		   "sort by key(s): pid, comm, dso, symbol, parent, cpu,
> srcline, ..."
> > +		   "sort by key(s): pid, comm, dso, symbol, parent, cpu,
> srcline, symoff, ..."
> >  		   " Please refer the man page for the complete list."),
> >  	OPT_STRING('t', "field-separator", &symbol_conf.field_sep,
> "separator",
> >  		   "separator for columns, no spaces will be added between
> "
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c index
> > 6e88b9e..3d8a71b 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -67,13 +67,14 @@ void hists__calc_col_len(struct hists *hists, struct
> hist_entry *h)
> >  		symlen = h->ms.sym->namelen + 4;
> >  		if (verbose)
> >  			symlen += BITS_PER_LONG / 4 + 2 + 3;
> > -		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> >  	} else {
> >  		symlen = unresolved_col_width + 4 + 2;
> > -		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> >  		hists__set_unres_dso_col_len(hists, HISTC_DSO);
> >  	}
> >
> > +	hists__new_col_len(hists, HISTC_SYMBOL, symlen);
> > +	hists__new_col_len(hists, HISTC_SYMOFF, symlen);
> 
> SYMOFF will need larger length at least 6 (for "+0xYYY").  Idealy 3 + symbol
> size in hexdigit?
> 

We also need to handle the case which doesn't have symbol available.
What about this?
        /*
+      * +6 accounts for '"+0xYYY ' symoff info
         * +4 accounts for '[x] ' priv level info
         * +2 accounts for 0x prefix on raw addresses
         * +3 accounts for ' y ' symtab origin info
         */	
                 if (h->ms.sym) {
		symlen = h->ms.sym->namelen + 4;
		if (verbose)
			symlen += BITS_PER_LONG / 4 + 2 + 3;
		hists__new_col_len(hists, HISTC_SYMBOL, symlen);

+                              symlen = h->ms.sym->namelen + 6
+	                hists__new_col_len(hists, HISTC_SYMOFF, symlen);
	} else {
		symlen = unresolved_col_width + 4 + 2;
		hists__new_col_len(hists, HISTC_SYMBOL, symlen);
		hists__set_unres_dso_col_len(hists, HISTC_DSO);
+                             symlen = unresolved_col_width + 2
+                             hists__new_col_len(hists, HISTC_SYMOFF, symlen);
	}

Thanks,
Kan

> Thanks,
> Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/