lkml.org 
[lkml]   [2015]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.




\
 
 \ /
  Last update: 2015-12-03 04:41    [W:0.084 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site