perf/urgent fixes

From: Arnaldo Carvalho de Melo <[email protected]>

Hi Ingo,

Please consider applying,

- Arnaldo

The following changes since commit 4e93ad601a4308d4a67673c81556580817d56940:

perf: Do not send exit event twice (2015-12-06 12:54:49 +0100)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-urgent-for-mingo

for you to fetch changes up to 4938cf0c7a62025bbfbf3db7bcdcc2c33312bedb:

perf hists browser: Fix segfault if use symbol filter in cmdline (2015-12-07 12:02:35 -0300)

----------------------------------------------------------------
perf/urgent fixes:

User visible:

- Fix showing the running kernel build id using: (Michael Petlan)

    $ perf buildid-list -k
    03c2a89c595616188f02f0282762a75b47069bc0

- hists browser (report, top) symbol filter segfault fixes (Wang Nan)

----------------------------------------------------------------
Michael Petlan (2):
perf buildid-list: Show running kernel build id fix perf buildid-list: Fix return value of perf buildid-list -k

Wang Nan (3):
perf hists browser: Add NULL pointer check to prevent crash perf hists browser: Reset selection when refresh
perf hists browser: Fix segfault if use symbol filter in cmdline

 tools/perf/builtin-buildid-list.c | 2 +-
 tools/perf/ui/browsers/hists.c    | 8 ++++++++
 tools/perf/util/build-id.c        | 2 +-
 3 files changed, 10 insertions(+), 2 deletions(-)

From: Michael Petlan <[email protected]>

The --kernel option of perf buildid-list tool should show the running kernel buildid. The functionality has been lost during other changes of the related code.

The build_id__sprintf() function should return length of the build-id string, but it was the length of the build-id raw data instead. Due to that, some return value checking caused that the final string was not printed out.

With this patch the build_id__sprintf() returns the correct value, so the --kernel option works again.

Before:

	# perf buildid-list --kernel
	#

After:

	# perf buildid-list --kernel
	972c1edab5bdc06cc224af45d510af662a3c6972
	#

LPU-Reference: [email protected]

---
 tools/perf/util/build-id.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 217b5a60e2ab..6a7e273a514a 100644
@@ -91,7 +91,7 @@ int build_id__sprintf(const u8 *build_id, int len, char *bf)
 		bid += 2;
 	}
 
-	return raw - build_id;
+	return (bid - bf) + 1;
 }
 
 int sysfs__sprintf_build_id(const char *root_dir, char *sbuild_id)

2.1.0

From: Michael Petlan <[email protected]>

The buildid string length is returned by perf buildid-list -k command. Since a non-zero return value means an error, perf buildid-list -k cmd should return 0 when successful instead.

Before:

	# perf buildid-list -k
	39356d74e96e02346fe0ec1f3f162b6c522bac62
	# echo $?
	41

After:

	# perf buildid-list -k
	39356d74e96e02346fe0ec1f3f162b6c522bac62
	# echo $?
	0

LPU-Reference: [email protected]

---
 tools/perf/builtin-buildid-list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 918b4de29de4..6419f57b0850 100644
@@ -110,7 +110,7 @@ int cmd_buildid_list(int argc, const char **argv,
 	setup_pager();
 
 	if (show_kernel)
-		return sysfs__fprintf_build_id(stdout);
+		return !(sysfs__fprintf_build_id(stdout) > 0);
 
 	return perf_session__list_build_ids(force, with_hits);
 }

2.1.0

From: Wang Nan <[email protected]>

Before this patch we can trigger a segfault by following steps:

Step 0: Use 'perf record' to generate a perf.data without callchain

Step 1: perf report

Step 2: Use UP/DOWN to select an entry, don't press 'ENTER'

Step 3: Use '/' to filter symbols, use a filter which returns
empty result

Step 4: Press 'ENTER' (notice here that the old selection is still
there. This is another problem)

Step 5: Press 'ENTER' to annotate that symbol

Step 6: Press 'LEFT' to go out.

Result: segfault:

perf: Segmentation fault
-------- backtrace -------- /home/wangnan/perf[0x53e568] /lib64/libc.so.6(+0x3545f)[0x7fba75d3245f] /home/wangnan/perf[0x537516] /home/wangnan/perf[0x533fef] /home/wangnan/perf[0x53b347] /home/wangnan/perf(perf_evlist__tui_browse_hists+0x96)[0x53d206] /home/wangnan/perf(cmd_report+0x1b9f)[0x442c7f] /home/wangnan/perf[0x47efa2] /home/wangnan/perf(main+0x5f5)[0x432fa5] /lib64/libc.so.6(__libc_start_main+0xf4)[0x7fba75d1ebd4] /home/wangnan/perf[0x4330d4]

This is because in this case 'nd' could be NULL in ui_browser__hists_seek(), but that function never checks it.

This patch adds checker for potential NULL pointer in that function. After this patch the above steps won't segfault.

---
 tools/perf/ui/browsers/hists.c | 3 +++
 1 file changed, 3 insertions(+)

index fa9eb92c9e24..932e13d015b9 100644
@@ -1033,6 +1033,9 @@ static void ui_browser__hists_seek(struct ui_browser *browser,
 	 * and stop when we printed enough lines to fill the screen.
 	 */
 do_offset:
+	if (!nd)
+		return;
+
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);

2.1.0

From: Wang Nan <[email protected]>

With the following steps:

Step 1: perf report

Step 2: Use UP/DOWN to select an entry, don't press 'ENTER'

Step 3: Use '/' to filter symbols, use a filter which returns
empty result

Step 4: Press 'ENTER'

We see that, even if we have filtered all the symbols (and the main interface is empty), pressing 'ENTER' still selects one symbol. This behavior surprises the user.

This patch resets browser->{he_,}selection in hist_browser__refresh() and lets it choose default selection. In this case browser->{he_,}selection keeps NULL so user won't see annotation item in menu.

---
 tools/perf/ui/browsers/hists.c | 2 ++
 1 file changed, 2 insertions(+)

index 932e13d015b9..84c8251f39a1 100644
@@ -928,6 +928,8 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
 	}
 
 	ui_browser__hists_init_top(browser);
+	hb->he_selection = NULL;
+	hb->selection = NULL;
 
 	for (nd = browser->top; nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);

2.1.0

From: Wang Nan <[email protected]>

If feed perf a symbol filter in cmdline and the result is empty, pressing 'Enter' in the hist browser causes crash:

 # ./perf report perf.data   <-- Common mistake for beginners

Then press 'Enter':

perf: Segmentation fault
-------- backtrace -------- /home/wangnan/perf[0x53e578] /lib64/libc.so.6(+0x3545f)[0x7f76bafe045f] /home/wangnan/perf[0x539dd4] /home/wangnan/perf(perf_evlist__tui_browse_hists+0x96)[0x53d216] /home/wangnan/perf(cmd_report+0x1b9f)[0x442c7f] /home/wangnan/perf[0x47efa2] /home/wangnan/perf(main+0x5f5)[0x432fa5] /lib64/libc.so.6(__libc_start_main+0xf4)[0x7f76bafccbd4] /home/wangnan/perf[0x4330d4]

This is because 'perf.data' is interpreted as a symbol filter, and the result is empty, so selection is empty. However, hist_browser__toggle_fold() forgets to check it.

This patch simply return false when selection is NULL.

---
 tools/perf/ui/browsers/hists.c | 3 +++
 1 file changed, 3 insertions(+)

index 84c8251f39a1..81def6c3f24b 100644
@@ -298,6 +298,9 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
 	struct callchain_list *cl = container_of(ms, struct callchain_list, ms);
 	bool has_children;
 
+	if (!he || !ms)
+		return false;
+
 	if (ms == &he->ms)
 		has_children = hist_entry__toggle_fold(he);
 	else

2.1.0

Pulled, thanks a lot Arnaldo!

I also merged the fixes into perf/core and pushed it all out, so you can base subsequent changes on top of the merged result.

Ingo