Message-ID: <9f843a5b-4fa1-4abf-9c4b-1e5433ab9704@intel.com>
Date: Thu, 16 Oct 2025 23:04:38 +0800
From: "Li, Tianyou" <tianyou.li@...el.com>
To: James Clark <james.clark@...aro.org>
CC: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Arnaldo Carvalho de Melo <acme@...nel.org>, 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>, "Namhyung
 Kim" <namhyung@...nel.org>
Subject: Re: [PATCH v2] perf tools annotate: fix a crash when annotate the
 same symbol with 's' and 'T'


On 10/16/2025 9:06 PM, James Clark wrote:
>
>
> On 16/10/2025 4:36 am, Li, Tianyou wrote:
>> Hi James,
>>
>> Thanks for your time to review. Please see my comments inlined.
>>
>> Regards,
>>
>> Tianyou
>>
>> On 10/16/2025 1:30 AM, James Clark wrote:
>>>
>>>
>>> On 15/10/2025 6:20 pm, Tianyou Li wrote:
>>>> When perf report with annotation for a symbol, press 's' and 'T', 
>>>> then exit
>>>> the annotate browser. Once annotate the same symbol, the annotate 
>>>> browser
>>>> will crash.
>>>>
>>>> The browser.arch was required to be correctly updated when data type
>>>> feature was enabled by 'T'. Usually it was initialized by 
>>>> symbol__annotate2
>>>> function. If a symbol has already been correctly annotated at the 
>>>> first
>>>> time, it should not call the symbol__annotate2 function again, thus 
>>>> the
>>>> browser.arch will not get initialized. Then at the second time to 
>>>> show the
>>>> annotate browser, the data type needs to be displayed but the 
>>>> browser.arch
>>>> is empty.
>>>>
>>>> Stack trace as below:
>>>>
>>>> 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]
>>>>
>>>> Fixes: 1d4374afd000 ("perf annotate: Add 'T' hot key to toggle data 
>>>> type display")
>>>> Reviewed-by: James Clark <james.clark@...aro.org>
>>>> 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..3b27ef1e8490 100644
>>>> --- a/tools/perf/ui/browsers/annotate.c
>>>> +++ b/tools/perf/ui/browsers/annotate.c
>>>> @@ -1161,6 +1161,9 @@ int __hist_entry__tui_annotate(struct 
>>>> hist_entry *he, struct map_symbol *ms,
>>>>               if (!annotation__has_source(notes))
>>>>                   ui__warning("Annotation has no source code.");
>>>>           }
>>>> +    } else if (evsel__get_arch(evsel, &browser.arch)) {
>>>> +        ui__error("Couldn't get architecture for event '%s'", 
>>>> evsel- >name);
>>>> +        return -1;
>>>>       }
>>>
>>> symbol_annotate() only fails for negative return values of 
>>> evsel__get_arch(), but evsel__get_arch() has at least two positive 
>>> error return values.
>>>
>>> If symbol_annotate() is wrong and it should be != 0 like you have, 
>>> then maybe symbol_annotate() should be fixed in another commit in 
>>> the same patchset as this one. Otherwise you have two calls to the 
>>> same thing right next to each other that handle errors differently.
>>
>>
>> Thanks James. I will give a try on handling the error message with 
>> symbol__strerror_disassemble. I am conservative to change the code in 
>> symbol_annotate, agreed it should be considered in another patch. 
>> Would like to focus this particular issue and get it fixed properly. 
>> Thanks.
>>
>>
>
> Looks like there was a misunderstanding. I'm not saying that the error 
> is _reported_ differently, it's that the condition that triggers the 
> error is different.
>
> symbol__annotate():
>
>   err = evsel__get_arch(evsel, &arch);
>   if (err < 0)
>       return err;
>
> You added:
>
>   if (evsel__get_arch(evsel, &browser.arch))
>      ...
>
> evsel__get_arch() returns positive error values (and maybe also 
> negative?), so "< 0" behaves differently to "!= 0".
>
> You either have to assume that "< 0" is correct and not change it, but 
> then you have to also check the return value in the same way. Or if by 
> doing "!= 0" you're implying that symbol__annotate() is wrong to do "< 
> 0", then you should fix it now to not leave 
> __hist_entry__tui_annotate() doing the same thing two different ways 
> at different times.
>
Thanks James. I looked at the code of symbol__annotate, and noticed the 
if (err<0) statement. I did not mean to change the code in 
symbol__annotate because I did not understand why it handled the error 
code that way. The positive return value of evsel__get_arch indicates 
some error happens, eg in arm__annotate_init, so I use the 
symbol__strerror_disassemble function to handle both positive and 
negative error code.

I do agree we should check the error code of evsel__get_arch, but I am 
hesitate to touch the code which I am not sure the consequences. I agree 
it may deserve another patch but not in this patchset if we have clear 
answers on why "<0" is not correct, or we have a case to break the 
current code as a evidence. Thanks.


Regards,

Tianyou

>>>
>>>>         /* Copy necessary information when it's called from perf 
>>>> top */
>>>> 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