perf report: Make --branch-history work without callgraphs(-g) option in perf record

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 <[email protected]>

---
  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(-)

tools/perf/builtin-report.c +7 -5

index 22478ff..4ceb2d2 100644
@@ -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");
  	}
 

tools/perf/util/callchain.c +4 -3

index 81fc29a..08d3abf 100644
@@ -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);
  }

tools/perf/util/hist.c +2 -0

index cf0186a..8b045a5 100644
@@ -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);
  }
 

tools/perf/util/machine.c +12 -1

index d97e014..7b47080 100644
@@ -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;
  	}
 

Hi Arnaldo,

Is this patch OK for merging? It's more than 2 months no more comments.

Thanks
Jin Yao

Quoted 155 lines · Jin Yao

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 <[email protected]>

---
  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(-)

tools/perf/builtin-report.c +7 -5

index 22478ff..4ceb2d2 100644
@@ -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");
  	}

tools/perf/util/callchain.c +4 -3

index 81fc29a..08d3abf 100644
@@ -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);
  }

tools/perf/util/hist.c +2 -0

index cf0186a..8b045a5 100644
@@ -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);
  }

tools/perf/util/machine.c +12 -1

index d97e014..7b47080 100644
@@ -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;
  	}

Em Mon, Jul 24, 2017 at 11:49:30AM +0800, Jin, Yao escreveu:

Quoted 3 lines

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.