Re: [PATCH v2] perf tools annotate: fix a crash when annotate the same symbol with 's' and 'T'
From: Li, Tianyou
Date: Sun Oct 19 2025 - 21:19:37 EST
Hi Namhyung,
On 10/19/2025 11:31 AM, Namhyung Kim wrote:
Hello,
On Fri, Oct 17, 2025 at 12:04:42AM +0800, Li, Tianyou wrote:
On 10/16/2025 11:18 PM, James Clark wrote:Looks like I just overlooked the error handling when I factored out the
Truly appreciated for your instant response, and the suggestions about
On 16/10/2025 4:04 pm, Li, Tianyou wrote:
On 10/16/2025 9:06 PM, James Clark wrote:It may take a little bit of effort to follow the code and look at the
Thanks James. I looked at the code of symbol__annotate, and noticed
On 16/10/2025 4:36 am, Li, Tianyou wrote:
Hi James,Looks like there was a misunderstanding. I'm not saying that the
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, presssymbol_annotate() only fails for negative return values
'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@xxxxxxxxxx>
Signed-off-by: Tianyou Li <tianyou.li@xxxxxxxxx>
---
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;
}
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.
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.
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
git blame to see what happened, but it's really not going to be that
hard.
'Fixes' tag, return value handling etc. I do check the git history about the
code "<0", I still did not quite understand the reason of handling it in
that way.
function. Please feel free to update symbol__annotate() to check != 0.
Thanks for the confirmation. Will send the patch v3 soon.
Agreed. Anyway I can confirm that this patch fixed the crash.
You're basically suggesting to add code that (when expanded) does this:I am suggesting to focus on the 'else' part. If that part of code is
if (first_run) {
if (do_important_thing() < 0)
return err;
} else { // second run
if (do_important_thing() != 0)
return err;
}
It's not going to help anyone who looks at it in the future. It's going
to make future refactors of evsel__get_arch() more difficult, and
without knowing why it's like that, it's possibly introducing another
bug.
correct, then we might need to consider another patch for the "<0" code. I
am eager for the answer as well.
It surely has to be consistent otherwise it doesn't make sense. And ifYes. I am not so sure 'must succeed' could be a right assumption, or for
you sent a patch that did "< 0" I would still say "but it can return
positive errors, so the new code isn't right".
I did suggest in the beginning to not check the error at all and add a
comment saying it must succeed at that point because it's already done
once before, but that's not very defensive and it doesn't fix the other
possible bug.
safety it's better to check the error code.
Tested-by: Namhyung Kim <namhyung@xxxxxxxxxx>
Thanks. Will include and send the patch v3 soon.
Thanks,
Namhyung