Message-ID: <9f8f8acc-6b0b-4edb-bf69-25d7bbd9c147@intel.com>
Date: Thu, 16 Oct 2025 00:49:06 +0800
From: "Li, Tianyou" <tianyou.li@...el.com>
To: James Clark <james.clark@...aro.org>, Namhyung Kim <namhyung@...nel.org>
CC: Mark Rutland <mark.rutland@....com>, Alexander Shishkin
	<alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, "Ian
 Rogers" <irogers@...gle.com>, Adrian Hunter <adrian.hunter@...el.com>, "Kan
 Liang" <kan.liang@...ux.intel.com>, Ravi Bangoria <ravi.bangoria@....com>,
	<wangyang.guo@...el.com>, <pan.deng@...el.com>, <zhiguo.zhou@...el.com>,
	<jiebin.sun@...el.com>, <thomas.falcon@...el.com>, <dapeng1.mi@...el.com>,
	<linux-perf-users@...r.kernel.org>, <linux-kernel@...r.kernel.org>, "Peter
 Zijlstra" <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>, "Arnaldo
 Carvalho de Melo" <acme@...nel.org>
Subject: Re: [PATCH] perf tools annotate: fix a crash when annotate the same
 symbol with 's' and 'T'

Hi James,

Appreciated for your review comments. All great suggestions. Thanks.

On 10/15/2025 8:30 PM, James Clark wrote:
>
>
> On 13/10/2025 5:10 pm, Tianyou Li wrote:
>> When perf report with annotation for a symbol, press 's' and 'T', 
>> then exit
>> the annotate browser. Once annoate the same symbol, the annoate browser
>> will crash. Stack trace as below:
>>
>
> Hi Tianyou,
>
> Can you explain why this only happens the second time and what you did 
> to fix it in the commit message. It took me a minute to work out.
>

Sure, will update the commit message and fix typos in patch v2.


>> Perf: Segmentation fault
>> -------- backtrace --------
>>      #0 0x55d365 in ui__signal_backtrace setup.c:0
>>      #1 0x7f5ff1a3e930 in __restore_rt libc.so.6[3e930]
>>      #2 0x570f08 in arch__is perf[570f08]
>>      #3 0x562186 in annotate_get_insn_location perf[562186]
>>      #4 0x562626 in __hist_entry__get_data_type annotate.c:0
>>      #5 0x56476d in annotation_line__write perf[56476d]
>>      #6 0x54e2db in annotate_browser__write annotate.c:0
>>      #7 0x54d061 in ui_browser__list_head_refresh perf[54d061]
>>      #8 0x54dc9e in annotate_browser__refresh annotate.c:0
>>      #9 0x54c03d in __ui_browser__refresh browser.c:0
>>      #10 0x54ccf8 in ui_browser__run perf[54ccf8]
>>      #11 0x54eb92 in __hist_entry__tui_annotate perf[54eb92]
>>      #12 0x552293 in do_annotate hists.c:0
>>      #13 0x55941c in evsel__hists_browse hists.c:0
>>      #14 0x55b00f in evlist__tui_browse_hists perf[55b00f]
>>      #15 0x42ff02 in cmd_report perf[42ff02]
>>      #16 0x494008 in run_builtin perf.c:0
>>      #17 0x494305 in handle_internal_command perf.c:0
>>      #18 0x410547 in main perf[410547]
>>      #19 0x7f5ff1a295d0 in __libc_start_call_main libc.so.6[295d0]
>>      #20 0x7f5ff1a29680 in __libc_start_main@@GLIBC_2.34 
>> libc.so.6[29680]
>>      #21 0x410b75 in _start perf[410b75]
>>
>
> This should have a fixes tag. Maybe commit 1d4374afd000 ("perf 
> annotate: Add 'T' hot key to toggle data type display")? Or earlier if 
> only 's' and not 's' and 'T' are required to reproduce.
>

My apology. Will add the fixes tag in patch v2.


>> Signed-off-by: Tianyou Li <tianyou.li@...el.com>
>> ---
>>   tools/perf/ui/browsers/annotate.c | 3 +++
>>   tools/perf/util/annotate.c        | 2 +-
>>   tools/perf/util/annotate.h        | 2 ++
>>   3 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/ui/browsers/annotate.c 
>> b/tools/perf/ui/browsers/annotate.c
>> index 8fe699f98542..1e0873194217 100644
>> --- a/tools/perf/ui/browsers/annotate.c
>> +++ b/tools/perf/ui/browsers/annotate.c
>> @@ -1163,6 +1163,9 @@ int __hist_entry__tui_annotate(struct 
>> hist_entry *he, struct map_symbol *ms,
>>           }
>>       }
>>   +    if (browser.arch == NULL)
>> +        evsel__get_arch(evsel, &browser.arch);
>> +
>
> This technically only needs to be called if symbol_annotate2() doesn't 
> get called. So it could be on an "else" after "if (not_annotated || 
> !sym->annotate2)".
>

I have a local version use the 'else' but I thought it might be clearer 
by make a NULL check then call the evsel__get_arch. Your suggestion 
definitely make sense especially the if/else statement could serve the 
whole purpose of initializing the necessary data structures. I will 
update the code in patch v2.


> I looked to see if it's better to save the arch in *notes, seeing as 
> that's where the annotation is cached, but it doesn't feel any nicer 
> than the way you've done it.
>
> You don't need to check the return value for errors because we only 
> get here if it worked previously and dso__annotate_warned() is false. 
> But it might be worth a comment or checking it anyway to avoid doubt.
>

Yes, I do need to check the return value. Appreciated for your kind 
words. I will update the code in patch v2. Thanks.


> Other than that, looks good.
>
> Thanks
> James
>
>>       /* Copy necessary information when it's called from perf top */
>>       if (hbt != NULL && he != &annotate_he) {
>>           annotate_he.hists = he->hists;
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index a2e34f149a07..39d6594850f1 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -980,7 +980,7 @@ void symbol__calc_percent(struct symbol *sym, 
>> struct evsel *evsel)
>>       annotation__calc_percent(notes, evsel, symbol__size(sym));
>>   }
>>   -static int evsel__get_arch(struct evsel *evsel, struct arch **parch)
>> +int evsel__get_arch(struct evsel *evsel, struct arch **parch)
>>   {
>>       struct perf_env *env = evsel__env(evsel);
>>       const char *arch_name = perf_env__arch(env);
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index eaf6c8aa7f47..d4990bff29a7 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -585,4 +585,6 @@ void debuginfo_cache__delete(void);
>>   int annotation_br_cntr_entry(char **str, int br_cntr_nr, u64 *br_cntr,
>>                    int num_aggr, struct evsel *evsel);
>>   int annotation_br_cntr_abbr_list(char **str, struct evsel *evsel, 
>> bool header);
>> +
>> +int evsel__get_arch(struct evsel *evsel, struct arch **parch);
>>   #endif    /* __PERF_ANNOTATE_H */
>
>

Powered by blists - more mailing lists