Re: [PATCH 2/2] perf hists browser: Reset selection when refresh
From: Wangnan (F)
Date: Wed Dec 02 2015 - 22:13:08 EST
On 2015/12/3 0:17, Namhyung Kim wrote:
On Wed, Dec 02, 2015 at 12:51:33PM +0000, Wang Nan wrote:
With 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 filter all symbols (and the main interface
is empty), pressing 'ENTER' still select one symbol. This behavior
surprise user. This patch resets browser->selection in
hist_browser__refresh() and let it choose default selection. In this
case browser->selection keeps NULL so user won't see annotation item
in menu.
Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
Cc: Arnaldo Carvalho de Melo <acme@xxxxxxxxxx>
Cc: Namhyung Kim <namhyung@xxxxxxxxxx>
---
Note that if this patch is applied before 1/2 then the steps listed in
commit message in 1/2 won't trigger segfault. However I believe patch 1/1
is still required.
---
tools/perf/ui/browsers/hists.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9458da8..523a9ef 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1192,6 +1192,7 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
}
ui_browser__hists_init_top(browser);
+ hb->selection = NULL;
This code assumes that hb->selection is not NULL if hb->he_selection
is not NULL. So I think that the right (and simple) fix is to reset
hb->he_selection rather than hb->selection (or both). It'll make the
change below unnecessary IMHO.
No. Only set hb->he_selection causes crash:
Step 0: user 'perf record ls' create a perf.data without callchain;
Step 1: perf report
Step 2: choose a annotable entry, don't press 'ENTER'
Step 3: use '/' and enter a filter like 'asdfasdf' which ensure no entry
would be left
Step 3: Press 'ENTER' twice
Crash:
# ./perf report
perf: Segmentation fault
-------- backtrace --------
./perf[0x53e588]
/tmp/oxygen_root/lib64/libc.so.6(+0x3545f)[0x7f2b4d6da45f]
./perf[0x539e03]
./perf(perf_evlist__tui_browse_hists+0x96)[0x53d226]
./perf(cmd_report+0x1b9f)[0x442c7f]
./perf[0x47efc2]
./perf(main+0x5f5)[0x432fa5]
/tmp/oxygen_root/lib64/libc.so.6(__libc_start_main+0xf4)[0x7f2b4d6c6bd4]
./perf[0x4330d4]
GDB result:
Program received signal SIGSEGV, Segmentation fault.
hist_browser__toggle_fold (browser=0x1f71160) at ui/browsers/hists.c:352
(gdb) bt
#0 hist_browser__toggle_fold (browser=0x1f71160) at ui/browsers/hists.c:352
#1 hist_browser__run (help=0x649038 "For a higher level overview, try:
perf report --sort comm,dso", browser=0x1f71160) at ui/
browsers/hists.c:539
#2 perf_evsel__hists_browse (evsel=0x19ef140,
nr_events=nr_events@entry=1, helpline=helpline@entry=0x649038 "For a
higher leve
l overview, try: perf report --sort comm,dso",
left_exits=left_exits@entry=false, hbt=hbt@entry=0x0,
min_pcnt=<optimized out>,
env=0x19ed850) at ui/browsers/hists.c:2101
#3 0x000000000053d227 in perf_evlist__tui_browse_hists
(evlist=0x19ee730, help=help@entry=0x649038 "For a higher level overvie
w, try: perf report --sort comm,dso", hbt=hbt@entry=0x0,
min_pcnt=<optimized out>, env=<optimized out>) at ui/browsers/hists.c:
2555
#4 0x0000000000442c80 in report__browse_hists (rep=0x7fffffffca20) at
builtin-report.c:440
#5 __cmd_report (rep=0x7fffffffca20) at builtin-report.c:576
#6 cmd_report (argc=0, argv=0x7fffffffe590, prefix=<optimized out>) at
builtin-report.c:962
#7 0x000000000047efc3 in run_builtin (p=p@entry=0x8ff9e0
<commands+192>, argc=argc@entry=1, argv=argv@entry=0x7fffffffe590) at
perf.c:387
#8 0x0000000000432fa6 in handle_internal_command (argv=0x7fffffffe590,
argc=1) at perf.c:448
#9 run_argv (argv=0x7fffffffe310, argcp=0x7fffffffe31c) at perf.c:492
#10 main (argc=1, argv=0x7fffffffe590) at perf.c:609
But setting both of them to NULL in hist_browser__refresh() can avoid
this crash.
So we have two choice:
1. In hist_browser__refresh() let's set both selection and he_selection
to NULL;
By this way after step 3 we won't see annotation options. The
context menu
(by pressing ENTER or 'm') is:
Run scripts for all samples
Switch to another data file in PWD
Exit
2. In hist_browser__toggle_fold() let's check both he amd ms.
By this way we still get annotation and map options in context menu:
Annotate __strcoll_l
Browse map details
Run scripts for all samples
Switch to another data file in PWD
Exit
I'm not sure which one is better because I don't really understand this
part of
code. But for me the second result is surprising because I have filtered all
entries out in my interface, and I believe I should select nothing, so
pressing
'ENTER' or 'm' I shall not get annotation option because I don't know
which entry
would be annotated.
Thank you.
--
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/