Message-ID: <20151204124618.GA22102@danjae.kornet>
Date:	Fri, 4 Dec 2015 21:46:18 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	"Wangnan (F)" <wangnan0@...wei.com>
Cc:	acme@...nel.org, lizefan@...wei.com, pi3orama@....com,
	linux-kernel@...r.kernel.org,
	Arnaldo Carvalho de Melo <acme@...hat.com>
Subject: Re: [PATCH 2/2] perf hists browser: Reset selection when refresh

Hi,

On Thu, Dec 03, 2015 at 11:05:13AM +0800, Wangnan (F) wrote:
> 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@...wei.com>
> >>Cc: Arnaldo Carvalho de Melo <acme@...hat.com>
> >>Cc: Namhyung Kim <namhyung@...nel.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

So this is same crash you already fixed.  Now I think that it'd be
better to check hb->he_selection instead of hb->selection in that
patch for the sake of consistency.


> (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@...ry=1,
> helpline=helpline@...ry=0x649038 "For a higher leve
> l overview, try: perf report --sort comm,dso",
> left_exits=left_exits@...ry=false, hbt=hbt@...ry=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@...ry=0x649038 "For a higher level overvie
> w, try: perf report --sort comm,dso", hbt=hbt@...ry=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@...ry=0x8ff9e0 <commands+192>,
> argc=argc@...ry=1, argv=argv@...ry=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.

Agreed.  I also think that the choice 1 is the right thing.

Thanks,
Namhyung
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists