Date:	Thu, 3 Dec 2015 11:05:13 +0800
From:	"Wangnan (F)" <wangnan0@...wei.com>
To:	Namhyung Kim <namhyung@...nel.org>
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



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
(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.

Thank you.


--
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