| Message ID | 1449060693-236232-2-git-send-email-wangnan0@huawei.com |
|---|---|
| State | New |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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; for (nd = browser->top; nd; nd = rb_next(nd)) { struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node); @@ -2102,7 +2103,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events, if (browser->he_selection != NULL) { thread = hist_browser__selected_thread(browser); - map = browser->selection->map; + map = browser->selection ? browser->selection->map : NULL; socked_id = browser->he_selection->socket; } switch (key) { @@ -2321,7 +2322,8 @@ skip_annotation: nr_options += add_script_opt(browser, &actions[nr_options], &options[nr_options], - NULL, browser->selection->sym); + NULL, browser->selection ? + browser->selection->sym : NULL); } nr_options += add_script_opt(browser, &actions[nr_options], &options[nr_options], NULL, NULL);
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(-) -- 1.8.3.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/