Re: [PATCH v2] perf report: Make --branch-history work without callgraphs(-g) option in perf record

[Arnaldo Carvalho de Melo]

Em Mon, Jul 24, 2017 at 11:49:30AM +0800, Jin, Yao escreveu:
> Hi Arnaldo,
> 
> Is this patch OK for merging? It's more than 2 months no more comments.

This is not an area I'm completely familiar with, so having other people
vouch for it, reviewing, acking it surely should speed merging, can you
find someone willing to provide that? If not I'll try and do it.

- Arnaldo
 
> Thanks
> Jin Yao
> 
> 
> On 5/8/2017 6:43 PM, Jin Yao wrote:
> > perf record -b -g <command>
> > perf report --branch-history
> > 
> > This merges the LBRs with the callgraphs.
> > 
> > However it would be nice if it also works without callgraphs (-g)
> > set in perf record, so that only the LBRs are displayed.
> > But currently perf report errors in this case. For example,
> > 
> > perf record -b <command>
> > perf report --branch-history
> > 
> > Error:
> > Selected -g or --branch-history but no callchain data. Did
> > you call 'perf record' without -g?
> > 
> > This patch displays the LBRs only even if callgraphs(-g) is not
> > enabled in perf record.
> > 
> > Change log:
> > ----------
> > v2: According to Milian Wolff's comment, change the obsolete error
> > message. Now the error message is:
> > 
> >                   ââError:ââââââââââââââââââââââââââââââââââââââ
> >                   âSelected -g or --branch-history.            â
> >                   âBut no callchain or branch data.            â
> >                   âDid you call 'perf record' without -g or -b?â
> >                   â                                            â
> >                   â                                            â
> >                   âPress any key...                            â
> >                   ââââââââââââââââââââââââââââââââââââââââââââââ
> > 
> > When passing the last parameter to hists__fprintf,
> > changes "|" to "||".
> > 
> > hsts__fprintf(hists, !quiet, 0, 0, rep->min_percent, stdout,
> >                symbol_conf.use_callchain ||
> >                symbol_conf.show_branchflag_count);
> > 
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > ---
> >   tools/perf/builtin-report.c | 12 +++++++-----
> >   tools/perf/util/callchain.c |  7 ++++---
> >   tools/perf/util/hist.c      |  2 ++
> >   tools/perf/util/machine.c   | 13 ++++++++++++-
> >   4 files changed, 25 insertions(+), 9 deletions(-)
> > 
> > diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> > index 22478ff..4ceb2d2 100644
> > --- a/tools/perf/builtin-report.c
> > +++ b/tools/perf/builtin-report.c
> > @@ -259,10 +259,11 @@ static int report__setup_sample_type(struct report *rep)
> >   				    "'perf record' without -g?\n");
> >   			return -EINVAL;
> >   		}
> > -		if (symbol_conf.use_callchain) {
> > -			ui__error("Selected -g or --branch-history but no "
> > -				  "callchain data. Did\n"
> > -				  "you call 'perf record' without -g?\n");
> > +		if (symbol_conf.use_callchain &&
> > +			!symbol_conf.show_branchflag_count) {
> > +			ui__error("Selected -g or --branch-history.\n"
> > +				  "But no callchain or branch data.\n"
> > +				  "Did you call 'perf record' without -g or -b?\n");
> >   			return -1;
> >   		}
> >   	} else if (!callchain_param.enabled &&
> > @@ -397,7 +398,8 @@ static int perf_evlist__tty_browse_hists(struct perf_evlist *evlist,
> >   		hists__fprintf_nr_sample_events(hists, rep, evname, stdout);
> >   		hists__fprintf(hists, !quiet, 0, 0, rep->min_percent, stdout,
> > -			       symbol_conf.use_callchain);
> > +			       symbol_conf.use_callchain ||
> > +			       symbol_conf.show_branchflag_count);
> >   		fprintf(stdout, "\n\n");
> >   	}
> > diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
> > index 81fc29a..08d3abf 100644
> > --- a/tools/perf/util/callchain.c
> > +++ b/tools/perf/util/callchain.c
> > @@ -993,11 +993,11 @@ int sample__resolve_callchain(struct perf_sample *sample,
> >   			      struct perf_evsel *evsel, struct addr_location *al,
> >   			      int max_stack)
> >   {
> > -	if (sample->callchain == NULL)
> > +	if (sample->callchain == NULL && !symbol_conf.show_branchflag_count)
> >   		return 0;
> >   	if (symbol_conf.use_callchain || symbol_conf.cumulate_callchain ||
> > -	    perf_hpp_list.parent) {
> > +	    perf_hpp_list.parent || symbol_conf.show_branchflag_count) {
> >   		return thread__resolve_callchain(al->thread, cursor, evsel, sample,
> >   						 parent, al, max_stack);
> >   	}
> > @@ -1006,7 +1006,8 @@ int sample__resolve_callchain(struct perf_sample *sample,
> >   int hist_entry__append_callchain(struct hist_entry *he, struct perf_sample *sample)
> >   {
> > -	if (!symbol_conf.use_callchain || sample->callchain == NULL)
> > +	if ((!symbol_conf.use_callchain || sample->callchain == NULL) &&
> > +		!symbol_conf.show_branchflag_count)
> >   		return 0;
> >   	return callchain_append(he->callchain, &callchain_cursor, sample->period);
> >   }
> > diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> > index cf0186a..8b045a5 100644
> > --- a/tools/perf/util/hist.c
> > +++ b/tools/perf/util/hist.c
> > @@ -1762,6 +1762,8 @@ void perf_evsel__output_resort(struct perf_evsel *evsel, struct ui_progress *pro
> >   	else
> >   		use_callchain = symbol_conf.use_callchain;
> > +	use_callchain |= symbol_conf.show_branchflag_count;
> > +
> >   	output_resort(evsel__hists(evsel), prog, use_callchain, NULL);
> >   }
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index d97e014..7b47080 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1901,13 +1901,16 @@ static int thread__resolve_callchain_sample(struct thread *thread,
> >   {
> >   	struct branch_stack *branch = sample->branch_stack;
> >   	struct ip_callchain *chain = sample->callchain;
> > -	int chain_nr = chain->nr;
> > +	int chain_nr = 0;
> >   	u8 cpumode = PERF_RECORD_MISC_USER;
> >   	int i, j, err, nr_entries;
> >   	int skip_idx = -1;
> >   	int first_call = 0;
> >   	int nr_loop_iter;
> > +	if (chain)
> > +		chain_nr = chain->nr;
> > +
> >   	if (perf_evsel__has_branch_callstack(evsel)) {
> >   		err = resolve_lbr_callchain_sample(thread, cursor, sample, parent,
> >   						   root_al, max_stack);
> > @@ -1945,6 +1948,10 @@ static int thread__resolve_callchain_sample(struct thread *thread,
> >   		for (i = 0; i < nr; i++) {
> >   			if (callchain_param.order == ORDER_CALLEE) {
> >   				be[i] = branch->entries[i];
> > +
> > +				if (chain == NULL)
> > +					continue;
> > +
> >   				/*
> >   				 * Check for overlap into the callchain.
> >   				 * The return address is one off compared to
> > @@ -1998,6 +2005,10 @@ static int thread__resolve_callchain_sample(struct thread *thread,
> >   			if (err)
> >   				return err;
> >   		}
> > +
> > +		if (chain_nr == 0)
> > +			return 0;
> > +
> >   		chain_nr -= nr;
> >   	}