Re: [PATCH] perf, tools, report: Add support for srcfile sort key

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

Em Mon, Aug 10, 2015 at 01:12:41PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Sun, Aug 09, 2015 at 12:30:49PM +0900, Namhyung Kim escreveu:
> > Hi Andi,
> > 
> > On Sat, Aug 08, 2015 at 04:27:35AM +0200, Andi Kleen wrote:
> > > On Fri, Aug 07, 2015 at 09:02:15PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Fri, Aug 07, 2015 at 08:51:45PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > > Em Fri, Aug 07, 2015 at 03:54:24PM -0700, Andi Kleen escreveu:
> > > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > > 
> > > > > > In some cases it's useful to characterize samples by file. This is useful
> > > > > > to get a higher level categorization, for example to map cost to
> > > > > > subsystems.
> > > > > > 
> > > > > > Add a srcfile sort key to perf report. It builds on top of the existing
> > > > > > srcline support.
> > > > > 
> > > > > Applied
> > > > 
> > > > Humm, holding this up a bit, further testing showed some oddities,
> > > > fedora21, the width of the column is being limited to the lenght of the
> > > > header
> > > 
> > > Yes I've seen that, I just use -w normally. It also happens with --sort
> > > srcline. The column sizing code could probably be somewhat smarter and
> > > always allow the last column to become as wide as needed. But that's
> > > something that should be done separately; I don't think it belongs
> > > into this patch.
> > 
> > Maybe something like this?
> 
> Maybe, but it crashes when right->hists == NULL, perhaps that is some sort of
> placeholder? See the value of right (0x7fffffffc0a0)? looks like some static
> guard for something, will continue after lunch...
> 
> (gdb) fr 2
> #2  0x00000000004bb812 in sort__srcline_cmp (left=0x1bfa090, right=0x7fffffffc0a0) at util/sort.c:306
> 306				hists__new_col_len(right->hists, HISTC_SRCLINE,
> (gdb) p right->hists
> $7 = (struct hists *) 0x0
> (gdb) p left->hists
> $8 = (struct hists *) 0x18d66e0
> (gdb)
> 
> Due to this both "-s srcline" and "-s srcfile" crashes here.
> 
> - Arnaldo

Ok, this one needs to be applied before yours and Andi's:


diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 137f6a92e5d0..c29c333609fe 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -761,6 +761,7 @@ iter_add_next_cumulative_entry(struct hist_entry_iter *iter,
 	struct hist_entry **he_cache = iter->priv;
 	struct hist_entry *he;
 	struct hist_entry he_tmp = {
+		.hists = evsel__hists(evsel),
 		.cpu = al->cpu,
 		.thread = al->thread,
 		.comm = thread__comm(al->thread),

---------

Because you will use that stack synthesized he_tmp entry as a parameter
to the cmp() function, that will end up trying to access he_tmp->hists,
that was NULL, b00m.

- Arnaldo
  
> > 
> > 
> > >From c052d17ae45ac08a381192191473ed3c4308c0c5 Mon Sep 17 00:00:00 2001
> > From: Namhyung Kim <namhyung@kernel.org>
> > Date: Sun, 9 Aug 2015 12:28:01 +0900
> > Subject: [PATCH] perf tools: Update srcline column length
> > 
> > It didn't calculate the column length so the default of header length
> > used only.  Update the length when we get the srcline actually.
> > 
> > Cc: Andi Kleen <andi@firstfloor.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/sort.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index 5177088a71d3..85e7e3e75d39 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -291,6 +291,8 @@ sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
> >  			left->srcline = get_srcline(map->dso,
> >  					   map__rip_2objdump(map, left->ip),
> >  						    left->ms.sym, true);
> > +			hists__new_col_len(left->hists, HISTC_SRCLINE,
> > +					   strlen(left->srcline));
> >  		}
> >  	}
> >  	if (!right->srcline) {
> > @@ -301,6 +303,8 @@ sort__srcline_cmp(struct hist_entry *left, struct hist_entry *right)
> >  			right->srcline = get_srcline(map->dso,
> >  					     map__rip_2objdump(map, right->ip),
> >  						     right->ms.sym, true);
> > +			hists__new_col_len(right->hists, HISTC_SRCLINE,
> > +					   strlen(right->srcline));
> >  		}
> >  	}
> >  	return strcmp(right->srcline, left->srcline);
> > -- 
> > 2.5.0