Messages in this thread | | | Date | Thu, 3 Dec 2015 11:05:13 +0800 | From | "Wangnan (F)" <> | Subject | Re: [PATCH 2/2] perf hists browser: Reset selection when refresh |
| |
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@huawei.com> >> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> >> Cc: Namhyung Kim <namhyung@kernel.org> >> --- >> >> 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.
| |