* [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI
@ 2025-07-25 19:37 Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate() Namhyung Kim
` (8 more replies)
0 siblings, 9 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Hello,
The --code-with-type option is to show normal annotate output with type
information. It was supported only on --stdio, but this change adds it
to TUI as well.
Arnaldo, please tell me if you still don't like the
__hist_entry__tui_annotate() but I don't have a better idea for now.
v4 changes)
* add dso__debuginfo() helper (Ian)
v3 changes)
* hide stack operation and stack canary by default
v2 changes)
* use 'T' key to toggle data type display (Arnaldo)
* display '[Type]' in the title line when it's enabled (Arnaldo)
* show warning when debug info is not available (Arnaldo)
* fix a typo (Arnaldo)
Actually the command line option sets the default behavior and users can
change it by pressing 'T' key in the TUI annotate browser.
The code is also available at 'perf/annotate-code-type-tui-v4' branch at
https://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
Thanks,
Namhyung
Namhyung Kim (9):
perf annotate: Rename to __hist_entry__tui_annotate()
perf annotate: Remove __annotation_line__write()
perf annotate: Pass annotation_print_data to annotation_line__write()
perf annotate: Simplify width calculation in annotation_line__write()
perf annotate: Add --code-with-type support for TUI
perf annotate: Add 'T' hot key to toggle data type display
perf annotate: Show warning when debuginfo is not available
perf annotate: Hide data-type for stack operation and canary
perf annotate: Add dso__debuginfo() helper
tools/perf/Documentation/perf-annotate.txt | 1 -
tools/perf/builtin-annotate.c | 5 -
tools/perf/ui/browsers/annotate.c | 66 +++++++--
tools/perf/ui/browsers/hists.c | 2 +-
tools/perf/util/annotate.c | 161 ++++++++++++++-------
tools/perf/util/annotate.h | 27 ++--
tools/perf/util/dso.h | 21 +++
tools/perf/util/hist.h | 12 +-
8 files changed, 203 insertions(+), 92 deletions(-)
--
2.50.1
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate()
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:16 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 2/9] perf annotate: Remove __annotation_line__write() Namhyung Kim
` (7 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
There are three different but similar functions for annotation on TUI.
Rename it to __hist_entry__tui_annotate() and make sure it passes 'he'.
It's not used for now but it'll be needed for later use.
Also remove map_symbol__tui_annotate() which was a simple wrapper.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 17 +++++++----------
tools/perf/ui/browsers/hists.c | 2 +-
tools/perf/util/annotate.h | 12 ------------
tools/perf/util/hist.h | 12 +++++++-----
4 files changed, 15 insertions(+), 28 deletions(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 183902dac042ecb0..28ef146f29e8e742 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -27,6 +27,7 @@ struct annotate_browser {
struct rb_node *curr_hot;
struct annotation_line *selection;
struct arch *arch;
+ struct hist_entry *he;
bool searching_backwards;
char search_bf[128];
};
@@ -557,7 +558,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
target_ms.map = ms->map;
target_ms.sym = dl->ops.target.sym;
annotation__unlock(notes);
- symbol__tui_annotate(&target_ms, evsel, hbt);
+ __hist_entry__tui_annotate(browser->he, &target_ms, evsel, hbt);
sym_title(ms->sym, ms->map, title, sizeof(title), annotate_opts.percent_type);
ui_browser__show_title(&browser->b, title);
return true;
@@ -1032,12 +1033,6 @@ static int annotate_browser__run(struct annotate_browser *browser,
return key;
}
-int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
- struct hist_browser_timer *hbt)
-{
- return symbol__tui_annotate(ms, evsel, hbt);
-}
-
int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
struct hist_browser_timer *hbt)
{
@@ -1046,11 +1041,12 @@ int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
SLang_init_tty(0, 0, 0);
SLtty_set_suspend_state(true);
- return map_symbol__tui_annotate(&he->ms, evsel, hbt);
+ return __hist_entry__tui_annotate(he, &he->ms, evsel, hbt);
}
-int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
- struct hist_browser_timer *hbt)
+int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
+ struct evsel *evsel,
+ struct hist_browser_timer *hbt)
{
struct symbol *sym = ms->sym;
struct annotation *notes = symbol__annotation(sym);
@@ -1064,6 +1060,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
.priv = ms,
.use_navkeypressed = true,
},
+ .he = he,
};
struct dso *dso;
int ret = -1, err;
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index d9d3fb44477ac6d5..487c0b08c0038710 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2484,8 +2484,8 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
else
evsel = hists_to_evsel(browser->hists);
- err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
he = hist_browser__selected_entry(browser);
+ err = __hist_entry__tui_annotate(he, &act->ms, evsel, browser->hbt);
/*
* offer option to annotate the other branch source or target
* (if they exists) when returning from annotate
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 8b5131d257b01e3e..0f640e4871744262 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -471,18 +471,6 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel);
int hist_entry__tty_annotate(struct hist_entry *he, struct evsel *evsel);
int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel);
-#ifdef HAVE_SLANG_SUPPORT
-int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
- struct hist_browser_timer *hbt);
-#else
-static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
- struct evsel *evsel __maybe_unused,
- struct hist_browser_timer *hbt __maybe_unused)
-{
- return 0;
-}
-#endif
-
void annotation_options__init(void);
void annotation_options__exit(void);
diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
index 70438d03ca9c33b1..c64005278687cb02 100644
--- a/tools/perf/util/hist.h
+++ b/tools/perf/util/hist.h
@@ -713,8 +713,9 @@ struct block_hist {
#include "../ui/keysyms.h"
void attr_to_script(char *buf, struct perf_event_attr *attr);
-int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
- struct hist_browser_timer *hbt);
+int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
+ struct evsel *evsel,
+ struct hist_browser_timer *hbt);
int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
struct hist_browser_timer *hbt);
@@ -742,9 +743,10 @@ int evlist__tui_browse_hists(struct evlist *evlist __maybe_unused,
{
return 0;
}
-static inline int map_symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
- struct evsel *evsel __maybe_unused,
- struct hist_browser_timer *hbt __maybe_unused)
+static inline int __hist_entry__tui_annotate(struct hist_entry *he __maybe_unused,
+ struct map_symbol *ms __maybe_unused,
+ struct evsel *evsel __maybe_unused,
+ struct hist_browser_timer *hbt __maybe_unused)
{
return 0;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 2/9] perf annotate: Remove __annotation_line__write()
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate() Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:20 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write() Namhyung Kim
` (6 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Get rid of the internal function and convert function arguments into
local variables if they are used more than once.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/annotate.c | 46 ++++++++++++++++----------------------
1 file changed, 19 insertions(+), 27 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 0dd475a744b6dfac..69ee83052396b15e 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1935,24 +1935,26 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
return -ENOMEM;
}
-static void __annotation_line__write(struct annotation_line *al, struct annotation *notes,
- bool first_line, bool current_entry, bool change_color, int width,
- void *obj, unsigned int percent_type,
- int (*obj__set_color)(void *obj, int color),
- void (*obj__set_percent_color)(void *obj, double percent, bool current),
- int (*obj__set_jumps_percent_color)(void *obj, int nr, bool current),
- void (*obj__printf)(void *obj, const char *fmt, ...),
- void (*obj__write_graph)(void *obj, int graph))
-
-{
- double percent_max = annotation_line__max_percent(al, percent_type);
- int pcnt_width = annotation__pcnt_width(notes),
- cycles_width = annotation__cycles_width(notes);
+void annotation_line__write(struct annotation_line *al, struct annotation *notes,
+ struct annotation_write_ops *wops)
+{
+ bool current_entry = wops->current_entry;
+ bool change_color = wops->change_color;
+ double percent_max = annotation_line__max_percent(al, annotate_opts.percent_type);
+ int width = wops->width;
+ int pcnt_width = annotation__pcnt_width(notes);
+ int cycles_width = annotation__cycles_width(notes);
bool show_title = false;
char bf[256];
int printed;
-
- if (first_line && (al->offset == -1 || percent_max == 0.0)) {
+ void *obj = wops->obj;
+ int (*obj__set_color)(void *obj, int color) = wops->set_color;
+ void (*obj__set_percent_color)(void *obj, double percent, bool current) = wops->set_percent_color;
+ int (*obj__set_jumps_percent_color)(void *obj, int nr, bool current) = wops->set_jumps_percent_color;
+ void (*obj__printf)(void *obj, const char *fmt, ...) = wops->printf;
+ void (*obj__write_graph)(void *obj, int graph) = wops->write_graph;
+
+ if (wops->first_line && (al->offset == -1 || percent_max == 0.0)) {
if (notes->branch && al->cycles) {
if (al->cycles->ipc == 0.0 && al->cycles->avg == 0)
show_title = true;
@@ -1966,7 +1968,8 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
for (i = 0; i < al->data_nr; i++) {
double percent;
- percent = annotation_data__percent(&al->data[i], percent_type);
+ percent = annotation_data__percent(&al->data[i],
+ annotate_opts.percent_type);
obj__set_percent_color(obj, percent, current_entry);
if (symbol_conf.show_total_period) {
@@ -2115,17 +2118,6 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
}
-void annotation_line__write(struct annotation_line *al, struct annotation *notes,
- struct annotation_write_ops *wops)
-{
- __annotation_line__write(al, notes, wops->first_line, wops->current_entry,
- wops->change_color, wops->width, wops->obj,
- annotate_opts.percent_type,
- wops->set_color, wops->set_percent_color,
- wops->set_jumps_percent_color, wops->printf,
- wops->write_graph);
-}
-
int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
struct arch **parch)
{
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write()
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate() Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 2/9] perf annotate: Remove __annotation_line__write() Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:23 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write() Namhyung Kim
` (5 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
It will be used for data type display later.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 13 ++++++++++--
tools/perf/util/annotate.c | 35 +++++++++++++++++--------------
tools/perf/util/annotate.h | 15 +++++++++++--
3 files changed, 43 insertions(+), 20 deletions(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 28ef146f29e8e742..23bea5b165774ae7 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -28,6 +28,8 @@ struct annotate_browser {
struct annotation_line *selection;
struct arch *arch;
struct hist_entry *he;
+ struct debuginfo *dbg;
+ struct evsel *evsel;
bool searching_backwards;
char search_bf[128];
};
@@ -108,12 +110,18 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
.printf = annotate_browser__printf,
.write_graph = annotate_browser__write_graph,
};
+ struct annotation_print_data apd = {
+ .he = ab->he,
+ .arch = ab->arch,
+ .evsel = ab->evsel,
+ .dbg = ab->dbg,
+ };
/* The scroll bar isn't being used */
if (!browser->navkeypressed)
ops.width += 1;
- annotation_line__write(al, notes, &ops);
+ annotation_line__write(al, notes, &ops, &apd);
if (ops.current_entry)
ab->selection = al;
@@ -976,7 +984,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
continue;
}
case 'P':
- map_symbol__annotation_dump(ms, evsel);
+ map_symbol__annotation_dump(ms, evsel, browser->he);
continue;
case 't':
if (symbol_conf.show_total_period) {
@@ -1061,6 +1069,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
.use_navkeypressed = true,
},
.he = he,
+ .evsel = evsel,
};
struct dso *dso;
int ret = -1, err;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 69ee83052396b15e..c21152710148b68c 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -765,15 +765,6 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
struct debuginfo *dbg, struct disasm_line *dl,
int *type_offset);
-struct annotation_print_data {
- struct hist_entry *he;
- struct evsel *evsel;
- struct arch *arch;
- struct debuginfo *dbg;
- u64 start;
- int addr_fmt_width;
-};
-
static int
annotation_line__print(struct annotation_line *al, struct annotation_print_data *apd,
struct annotation_options *opts, int printed,
@@ -1230,7 +1221,6 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
struct annotation_print_data apd = {
.he = he,
.evsel = evsel,
- .start = map__rip_2objdump(map, sym->start),
};
int printed = 2, queue_len = 0;
int more = 0;
@@ -1357,7 +1347,8 @@ static void FILE__write_graph(void *fp, int graph)
fputs(s, fp);
}
-static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
+static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
+ struct annotation_print_data *apd)
{
struct annotation *notes = symbol__annotation(sym);
struct annotation_write_ops wops = {
@@ -1374,7 +1365,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
list_for_each_entry(al, ¬es->src->source, node) {
if (annotation_line__filter(al))
continue;
- annotation_line__write(al, notes, &wops);
+ annotation_line__write(al, notes, &wops, apd);
fputc('\n', fp);
wops.first_line = false;
}
@@ -1382,13 +1373,18 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
return 0;
}
-int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel)
+int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel,
+ struct hist_entry *he)
{
const char *ev_name = evsel__name(evsel);
char buf[1024];
char *filename;
int err = -1;
FILE *fp;
+ struct annotation_print_data apd = {
+ .he = he,
+ .evsel = evsel,
+ };
if (asprintf(&filename, "%s.annotation", ms->sym->name) < 0)
return -1;
@@ -1404,7 +1400,7 @@ int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel)
fprintf(fp, "%s() %s\nEvent: %s\n\n",
ms->sym->name, dso__long_name(map__dso(ms->map)), ev_name);
- symbol__annotate_fprintf2(ms->sym, fp);
+ symbol__annotate_fprintf2(ms->sym, fp, &apd);
fclose(fp);
err = 0;
@@ -1656,6 +1652,10 @@ int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel)
struct symbol *sym = ms->sym;
struct rb_root source_line = RB_ROOT;
struct hists *hists = evsel__hists(evsel);
+ struct annotation_print_data apd = {
+ .he = he,
+ .evsel = evsel,
+ };
char buf[1024];
int err;
@@ -1678,7 +1678,7 @@ int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel)
hists__scnprintf_title(hists, buf, sizeof(buf));
fprintf(stdout, "%s, [percent: %s]\n%s() %s\n",
buf, percent_type_str(annotate_opts.percent_type), sym->name, dso__long_name(dso));
- symbol__annotate_fprintf2(sym, stdout);
+ symbol__annotate_fprintf2(sym, stdout, &apd);
annotated_source__purge(symbol__annotation(sym)->src);
@@ -1936,7 +1936,8 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
}
void annotation_line__write(struct annotation_line *al, struct annotation *notes,
- struct annotation_write_ops *wops)
+ struct annotation_write_ops *wops,
+ struct annotation_print_data *apd)
{
bool current_entry = wops->current_entry;
bool change_color = wops->change_color;
@@ -2114,6 +2115,8 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
obj__printf(obj, "%-*s", width - pcnt_width - cycles_width - 3 - printed, bf);
+
+ (void)apd;
}
}
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 0f640e4871744262..8fad464a870a2b8e 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -199,8 +199,18 @@ struct annotation_write_ops {
void (*write_graph)(void *obj, int graph);
};
+struct annotation_print_data {
+ struct hist_entry *he;
+ struct evsel *evsel;
+ struct arch *arch;
+ struct debuginfo *dbg;
+ u64 start;
+ int addr_fmt_width;
+};
+
void annotation_line__write(struct annotation_line *al, struct annotation *notes,
- struct annotation_write_ops *ops);
+ struct annotation_write_ops *ops,
+ struct annotation_print_data *apd);
int __annotation__scnprintf_samples_period(struct annotation *notes,
char *bf, size_t size,
@@ -463,7 +473,8 @@ void symbol__annotate_zero_histogram(struct symbol *sym, struct evsel *evsel);
void symbol__annotate_decay_histogram(struct symbol *sym, struct evsel *evsel);
void annotated_source__purge(struct annotated_source *as);
-int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel);
+int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel,
+ struct hist_entry *he);
bool ui__has_annotation(void);
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write()
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
` (2 preceding siblings ...)
2025-07-25 19:37 ` [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write() Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:30 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI Namhyung Kim
` (4 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
The width is updated after each part is printed. It can skip the output
processing if the total printed size is bigger than the width.
No function changes intended.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/annotate.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index c21152710148b68c..d69e406c1bc289cd 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1993,6 +1993,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
symbol_conf.show_nr_samples ? "Samples" : "Percent");
}
}
+ width -= pcnt_width;
if (notes->branch) {
if (al->cycles && al->cycles->ipc)
@@ -2056,11 +2057,12 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
obj__printf(obj, "%*s", ANNOTATION__AVG_IPC_WIDTH, bf);
}
}
+ width -= cycles_width;
obj__printf(obj, " ");
if (!*al->line)
- obj__printf(obj, "%-*s", width - pcnt_width - cycles_width, " ");
+ obj__printf(obj, "%-*s", width, " ");
else if (al->offset == -1) {
if (al->line_nr && annotate_opts.show_linenr)
printed = scnprintf(bf, sizeof(bf), "%-*d ",
@@ -2069,7 +2071,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
printed = scnprintf(bf, sizeof(bf), "%-*s ",
notes->src->widths.addr, " ");
obj__printf(obj, bf);
- obj__printf(obj, "%-*s", width - printed - pcnt_width - cycles_width + 1, al->line);
+ obj__printf(obj, "%-*s", width - printed + 1, al->line);
} else {
u64 addr = al->offset;
int color = -1;
@@ -2112,9 +2114,11 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
if (change_color)
obj__set_color(obj, color);
+ width -= printed + 3;
+
disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
- obj__printf(obj, "%-*s", width - pcnt_width - cycles_width - 3 - printed, bf);
+ obj__printf(obj, "%-*s", width, bf);
(void)apd;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
` (3 preceding siblings ...)
2025-07-25 19:37 ` [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write() Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:31 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 6/9] perf annotate: Add 'T' hot key to toggle data type display Namhyung Kim
` (3 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Until now, the --code-with-type option is available only on stdio.
But it was an artifical limitation because of an implemention issue.
Implement the same logic in annotation_line__write() for stdio2/TUI.
Make disasm_line__write() return the number of printed characters so
that it can skip unnecessary operations when the screen is full.
Remove the limitation and update the man page.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/Documentation/perf-annotate.txt | 1 -
tools/perf/builtin-annotate.c | 5 --
tools/perf/ui/browsers/annotate.c | 6 +++
tools/perf/util/annotate.c | 61 +++++++++++++++++++---
4 files changed, 61 insertions(+), 12 deletions(-)
diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
index 46090c5b42b4762f..547f1a2680185e3c 100644
--- a/tools/perf/Documentation/perf-annotate.txt
+++ b/tools/perf/Documentation/perf-annotate.txt
@@ -170,7 +170,6 @@ include::itrace.txt[]
--code-with-type::
Show data type info in code annotation (for memory instructions only).
- Currently it only works with --stdio option.
SEE ALSO
diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 5d57d2913f3d9a33..646f43b0f7c4c9b0 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -917,11 +917,6 @@ int cmd_annotate(int argc, const char **argv)
symbol_conf.annotate_data_sample = true;
} else if (annotate_opts.code_with_type) {
symbol_conf.annotate_data_member = true;
-
- if (!annotate.use_stdio) {
- pr_err("--code-with-type only works with --stdio.\n");
- goto out_delete;
- }
}
setup_browser(true);
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 23bea5b165774ae7..cdee1969f3131a7c 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -4,6 +4,7 @@
#include "../ui.h"
#include "../../util/annotate.h"
#include "../../util/debug.h"
+#include "../../util/debuginfo.h"
#include "../../util/dso.h"
#include "../../util/hist.h"
#include "../../util/sort.h"
@@ -1101,6 +1102,9 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
ui_helpline__push("Press ESC to exit");
+ if (annotate_opts.code_with_type)
+ browser.dbg = debuginfo__new(dso__long_name(dso));
+
browser.b.width = notes->src->widths.max_line_len;
browser.b.nr_entries = notes->src->nr_entries;
browser.b.entries = ¬es->src->source;
@@ -1111,6 +1115,8 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
ret = annotate_browser__run(&browser, evsel, hbt);
+ if (annotate_opts.code_with_type)
+ debuginfo__delete(browser.dbg);
if (not_annotated && !notes->src->tried_source)
annotated_source__purge(notes->src);
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index d69e406c1bc289cd..06ddc7a9f58722a4 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1362,6 +1362,11 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
};
struct annotation_line *al;
+ if (annotate_opts.code_with_type) {
+ evsel__get_arch(apd->evsel, &apd->arch);
+ apd->dbg = debuginfo__new(dso__long_name(map__dso(apd->he->ms.map)));
+ }
+
list_for_each_entry(al, ¬es->src->source, node) {
if (annotation_line__filter(al))
continue;
@@ -1370,6 +1375,9 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
wops.first_line = false;
}
+ if (annotate_opts.code_with_type)
+ debuginfo__delete(apd->dbg);
+
return 0;
}
@@ -1743,7 +1751,7 @@ static double annotation_line__max_percent(struct annotation_line *al,
return percent_max;
}
-static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
+static int disasm_line__write(struct disasm_line *dl, struct annotation *notes,
void *obj, char *bf, size_t size,
void (*obj__printf)(void *obj, const char *fmt, ...),
void (*obj__write_graph)(void *obj, int graph))
@@ -1771,8 +1779,8 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
obj__printf(obj, " ");
}
- disasm_line__scnprintf(dl, bf, size, !annotate_opts.use_offset,
- notes->src->widths.max_ins_name);
+ return disasm_line__scnprintf(dl, bf, size, !annotate_opts.use_offset,
+ notes->src->widths.max_ins_name);
}
static void ipc_coverage_string(char *bf, int size, struct annotation *notes)
@@ -2116,11 +2124,52 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
width -= printed + 3;
- disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
+ printed = disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf),
+ obj__printf, obj__write_graph);
+
+ obj__printf(obj, "%s", bf);
+ width -= printed;
+
+ if (annotate_opts.code_with_type && apd->dbg) {
+ struct annotated_data_type *data_type;
+ int offset = 0;
+
+ data_type = __hist_entry__get_data_type(apd->he, apd->arch,
+ apd->dbg,
+ disasm_line(al),
+ &offset);
+ if (data_type && data_type != NO_TYPE) {
+ char member[256];
+
+ printed = scnprintf(bf, sizeof(bf),
+ "\t\t# data-type: %s",
+ data_type->self.type_name);
- obj__printf(obj, "%-*s", width, bf);
+ if (data_type != &stackop_type &&
+ data_type != &canary_type &&
+ sizeof(bf) > (size_t)printed) {
+ printed += scnprintf(bf + printed,
+ sizeof(bf) - printed,
+ " +%#x", offset);
+ }
+
+ if (annotated_data_type__get_member_name(data_type,
+ member,
+ sizeof(member),
+ offset) &&
+ sizeof(bf) > (size_t)printed) {
+ printed += scnprintf(bf + printed,
+ sizeof(bf) - printed,
+ " (%s)", member);
+ }
- (void)apd;
+ obj__printf(obj, "%-*s", width, bf);
+ } else {
+ obj__printf(obj, "%-*s", width, " ");
+ }
+ } else {
+ obj__printf(obj, "%-*s", width, " ");
+ }
}
}
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 6/9] perf annotate: Add 'T' hot key to toggle data type display
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
` (4 preceding siblings ...)
2025-07-25 19:37 ` [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:34 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 7/9] perf annotate: Show warning when debuginfo is not available Namhyung Kim
` (2 subsequent siblings)
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
Support data type display with a key press so that users can toggle the
output dynamically on TUI. Also display "[Type]" in the title line if
it's enabled.
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index cdee1969f3131a7c..4b059e0bafd33fcf 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -525,9 +525,10 @@ static void ui_browser__init_asm_mode(struct ui_browser *browser)
static int sym_title(struct symbol *sym, struct map *map, char *title,
size_t sz, int percent_type)
{
- return snprintf(title, sz, "%s %s [Percent: %s]", sym->name,
+ return snprintf(title, sz, "%s %s [Percent: %s] %s", sym->name,
dso__long_name(map__dso(map)),
- percent_type_str(percent_type));
+ percent_type_str(percent_type),
+ annotate_opts.code_with_type ? "[Type]" : "");
}
/*
@@ -901,7 +902,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
"b Toggle percent base [period/hits]\n"
"B Branch counter abbr list (Optional)\n"
"? Search string backwards\n"
- "f Toggle showing offsets to full address\n");
+ "f Toggle showing offsets to full address\n"
+ "T Toggle data type display\n");
continue;
case 'r':
script_browse(NULL, NULL);
@@ -1021,6 +1023,12 @@ static int annotate_browser__run(struct annotate_browser *browser,
case 'f':
annotation__toggle_full_addr(notes, ms);
continue;
+ case 'T':
+ annotate_opts.code_with_type ^= 1;
+ if (browser->dbg == NULL)
+ browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
+ annotate_browser__show(&browser->b, title, help);
+ continue;
case K_LEFT:
case '<':
case '>':
@@ -1115,8 +1123,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
ret = annotate_browser__run(&browser, evsel, hbt);
- if (annotate_opts.code_with_type)
- debuginfo__delete(browser.dbg);
+ debuginfo__delete(browser.dbg);
if (not_annotated && !notes->src->tried_source)
annotated_source__purge(notes->src);
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 7/9] perf annotate: Show warning when debuginfo is not available
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
` (5 preceding siblings ...)
2025-07-25 19:37 ` [PATCH v4 6/9] perf annotate: Add 'T' hot key to toggle data type display Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:38 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper Namhyung Kim
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
When user requests data-type annotation but no DWARF info is available,
show a warning message about it.
Warning:
DWARF debuginfo not found.
Data-type in this DSO will not be displayed.
Please make sure to have debug information.
Press any key...
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 17 +++++++++++++++++
tools/perf/util/dso.h | 11 +++++++++++
2 files changed, 28 insertions(+)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 4b059e0bafd33fcf..2a4db5bdcdb7e9d8 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -804,6 +804,20 @@ static int annotate__scnprintf_title(struct hists *hists, char *bf, size_t size)
return printed;
}
+static void annotate_browser__debuginfo_warning(struct annotate_browser *browser)
+{
+ struct map_symbol *ms = browser->b.priv;
+ struct dso *dso = map__dso(ms->map);
+
+ if (browser->dbg == NULL && annotate_opts.code_with_type &&
+ !dso__debuginfo_warned(dso)) {
+ ui__warning("DWARF debuginfo not found.\n\n"
+ "Data-type in this DSO will not be displayed.\n"
+ "Please make sure to have debug information.");
+ dso__set_debuginfo_warned(dso);
+ }
+}
+
static int annotate_browser__run(struct annotate_browser *browser,
struct evsel *evsel,
struct hist_browser_timer *hbt)
@@ -834,6 +848,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
annotation_br_cntr_abbr_list(&br_cntr_text, evsel, false);
+ annotate_browser__debuginfo_warning(browser);
+
while (1) {
key = ui_browser__run(&browser->b, delay_secs);
@@ -1028,6 +1044,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
if (browser->dbg == NULL)
browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
annotate_browser__show(&browser->b, title, help);
+ annotate_browser__debuginfo_warning(browser);
continue;
case K_LEFT:
case '<':
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 3457d713d3c56df6..7df1673f08d3ddb4 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -299,6 +299,7 @@ DECLARE_RC_STRUCT(dso) {
u8 hit:1;
u8 annotate_warned:1;
u8 auxtrace_warned:1;
+ u8 debuginfo_warned:1;
u8 short_name_allocated:1;
u8 long_name_allocated:1;
u8 is_64_bit:1;
@@ -362,6 +363,16 @@ static inline void dso__set_annotate_warned(struct dso *dso)
RC_CHK_ACCESS(dso)->annotate_warned = 1;
}
+static inline bool dso__debuginfo_warned(const struct dso *dso)
+{
+ return RC_CHK_ACCESS(dso)->debuginfo_warned;
+}
+
+static inline void dso__set_debuginfo_warned(struct dso *dso)
+{
+ RC_CHK_ACCESS(dso)->debuginfo_warned = 1;
+}
+
static inline bool dso__auxtrace_warned(const struct dso *dso)
{
return RC_CHK_ACCESS(dso)->auxtrace_warned;
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
` (6 preceding siblings ...)
2025-07-25 19:37 ` [PATCH v4 7/9] perf annotate: Show warning when debuginfo is not available Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:44 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper Namhyung Kim
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
It's mostly unnecessary to print when it has no actual type information
like in the stack operations and canary. Let's have them if -v option
is given.
Before:
$ perf annotate --code-with-type
...
: 0 0xd640 <_dl_relocate_object>:
0.00 : 0: endbr64
0.00 : 4: pushq %rbp # data-type: (stack operation)
0.00 : 5: movq %rsp, %rbp
0.00 : 8: pushq %r15 # data-type: (stack operation)
0.00 : a: pushq %r14 # data-type: (stack operation)
0.00 : c: pushq %r13 # data-type: (stack operation)
0.00 : e: pushq %r12 # data-type: (stack operation)
0.00 : 10: pushq %rbx # data-type: (stack operation)
0.00 : 11: subq $0xf8, %rsp
...
0.00 : d4: testl %eax, %eax
0.00 : d6: jne 0xf424
0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
0.00 : e3: testq %rbx, %rbx
0.00 : e6: jne 0xf2dd
0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
...
After:
: 0 0xd640 <_dl_relocate_object>:
0.00 : 0: endbr64
0.00 : 4: pushq %rbp
0.00 : 5: movq %rsp, %rbp
0.00 : 8: pushq %r15
0.00 : a: pushq %r14
0.00 : c: pushq %r13
0.00 : e: pushq %r12
0.00 : 10: pushq %rbx
0.00 : 11: subq $0xf8, %rsp
...
0.00 : d4: testl %eax, %eax
0.00 : d6: jne 0xf424
0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
0.00 : e3: testq %rbx, %rbx
0.00 : e6: jne 0xf2dd
0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
...
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/util/annotate.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 06ddc7a9f58722a4..6fc07971631ac8a3 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -765,6 +765,17 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
struct debuginfo *dbg, struct disasm_line *dl,
int *type_offset);
+static bool needs_type_info(struct annotated_data_type *data_type)
+{
+ if (data_type == NULL || data_type == NO_TYPE)
+ return false;
+
+ if (verbose)
+ return true;
+
+ return (data_type != &stackop_type) && (data_type != &canary_type);
+}
+
static int
annotation_line__print(struct annotation_line *al, struct annotation_print_data *apd,
struct annotation_options *opts, int printed,
@@ -844,7 +855,7 @@ annotation_line__print(struct annotation_line *al, struct annotation_print_data
data_type = __hist_entry__get_data_type(apd->he, apd->arch,
apd->dbg, dl, &offset);
- if (data_type && data_type != NO_TYPE) {
+ if (needs_type_info(data_type)) {
char buf[4096];
printf("\t\t# data-type: %s",
@@ -2138,7 +2149,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
apd->dbg,
disasm_line(al),
&offset);
- if (data_type && data_type != NO_TYPE) {
+ if (needs_type_info(data_type)) {
char member[256];
printed = scnprintf(bf, sizeof(bf),
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
` (7 preceding siblings ...)
2025-07-25 19:37 ` [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary Namhyung Kim
@ 2025-07-25 19:37 ` Namhyung Kim
2025-07-26 0:46 ` Ian Rogers
8 siblings, 1 reply; 26+ messages in thread
From: Namhyung Kim @ 2025-07-25 19:37 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users
It'd be great if it can get the correct debug information using DSO
build-Id not just the path name. Instead of adding new callsites of
debuginfo__new(), let's add dso__debuginfo() which can hide the access
using the pathname and help the future conversion.
Suggested-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/ui/browsers/annotate.c | 4 ++--
tools/perf/util/annotate.c | 6 +++---
tools/perf/util/dso.h | 10 ++++++++++
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index 2a4db5bdcdb7e9d8..54610621c5f910fe 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1042,7 +1042,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
case 'T':
annotate_opts.code_with_type ^= 1;
if (browser->dbg == NULL)
- browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
+ browser->dbg = dso__debuginfo(map__dso(ms->map));
annotate_browser__show(&browser->b, title, help);
annotate_browser__debuginfo_warning(browser);
continue;
@@ -1128,7 +1128,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
ui_helpline__push("Press ESC to exit");
if (annotate_opts.code_with_type)
- browser.dbg = debuginfo__new(dso__long_name(dso));
+ browser.dbg = dso__debuginfo(dso);
browser.b.width = notes->src->widths.max_line_len;
browser.b.nr_entries = notes->src->nr_entries;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 6fc07971631ac8a3..05eb19b110ab7dcf 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1270,7 +1270,7 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
apd.addr_fmt_width = annotated_source__addr_fmt_width(¬es->src->source,
apd.start);
evsel__get_arch(evsel, &apd.arch);
- apd.dbg = debuginfo__new(filename);
+ apd.dbg = dso__debuginfo(dso);
list_for_each_entry(pos, ¬es->src->source, node) {
int err;
@@ -1375,7 +1375,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
if (annotate_opts.code_with_type) {
evsel__get_arch(apd->evsel, &apd->arch);
- apd->dbg = debuginfo__new(dso__long_name(map__dso(apd->he->ms.map)));
+ apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
}
list_for_each_entry(al, ¬es->src->source, node) {
@@ -2888,7 +2888,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
di_cache.dso = dso__get(map__dso(ms->map));
debuginfo__delete(di_cache.dbg);
- di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
+ di_cache.dbg = dso__debuginfo(di_cache.dso);
}
if (di_cache.dbg == NULL) {
diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
index 7df1673f08d3ddb4..fd8e95de77f78dfc 100644
--- a/tools/perf/util/dso.h
+++ b/tools/perf/util/dso.h
@@ -10,6 +10,7 @@
#include <stdio.h>
#include <linux/bitops.h>
#include "build-id.h"
+#include "debuginfo.h"
#include "mutex.h"
#include <internal/rc_check.h>
@@ -914,4 +915,13 @@ u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
bool perf_pid_map_tid(const char *dso_name, int *tid);
bool is_perf_pid_map_name(const char *dso_name);
+/*
+ * In the future, we may get debuginfo using build-ID (w/o path).
+ * Add this helper is for the smooth conversion.
+ */
+static inline struct debuginfo *dso__debuginfo(struct dso *dso)
+{
+ return debuginfo__new(dso__long_name(dso));
+}
+
#endif /* __PERF_DSO */
--
2.50.1
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate()
2025-07-25 19:37 ` [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate() Namhyung Kim
@ 2025-07-26 0:16 ` Ian Rogers
2025-07-28 18:39 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:16 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> There are three different but similar functions for annotation on TUI.
> Rename it to __hist_entry__tui_annotate() and make sure it passes 'he'.
> It's not used for now but it'll be needed for later use.
>
> Also remove map_symbol__tui_annotate() which was a simple wrapper.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/ui/browsers/annotate.c | 17 +++++++----------
> tools/perf/ui/browsers/hists.c | 2 +-
> tools/perf/util/annotate.h | 12 ------------
> tools/perf/util/hist.h | 12 +++++++-----
> 4 files changed, 15 insertions(+), 28 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 183902dac042ecb0..28ef146f29e8e742 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -27,6 +27,7 @@ struct annotate_browser {
> struct rb_node *curr_hot;
> struct annotation_line *selection;
> struct arch *arch;
> + struct hist_entry *he;
nit: as you are here is it worth commenting this, for example, who owns "he"?
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> bool searching_backwards;
> char search_bf[128];
> };
> @@ -557,7 +558,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
> target_ms.map = ms->map;
> target_ms.sym = dl->ops.target.sym;
> annotation__unlock(notes);
> - symbol__tui_annotate(&target_ms, evsel, hbt);
> + __hist_entry__tui_annotate(browser->he, &target_ms, evsel, hbt);
> sym_title(ms->sym, ms->map, title, sizeof(title), annotate_opts.percent_type);
> ui_browser__show_title(&browser->b, title);
> return true;
> @@ -1032,12 +1033,6 @@ static int annotate_browser__run(struct annotate_browser *browser,
> return key;
> }
>
> -int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> - struct hist_browser_timer *hbt)
> -{
> - return symbol__tui_annotate(ms, evsel, hbt);
> -}
> -
> int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> struct hist_browser_timer *hbt)
> {
> @@ -1046,11 +1041,12 @@ int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> SLang_init_tty(0, 0, 0);
> SLtty_set_suspend_state(true);
>
> - return map_symbol__tui_annotate(&he->ms, evsel, hbt);
> + return __hist_entry__tui_annotate(he, &he->ms, evsel, hbt);
> }
>
> -int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> - struct hist_browser_timer *hbt)
> +int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> + struct evsel *evsel,
> + struct hist_browser_timer *hbt)
> {
> struct symbol *sym = ms->sym;
> struct annotation *notes = symbol__annotation(sym);
> @@ -1064,6 +1060,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> .priv = ms,
> .use_navkeypressed = true,
> },
> + .he = he,
> };
> struct dso *dso;
> int ret = -1, err;
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index d9d3fb44477ac6d5..487c0b08c0038710 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2484,8 +2484,8 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
> else
> evsel = hists_to_evsel(browser->hists);
>
> - err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
> he = hist_browser__selected_entry(browser);
> + err = __hist_entry__tui_annotate(he, &act->ms, evsel, browser->hbt);
> /*
> * offer option to annotate the other branch source or target
> * (if they exists) when returning from annotate
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 8b5131d257b01e3e..0f640e4871744262 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -471,18 +471,6 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel);
> int hist_entry__tty_annotate(struct hist_entry *he, struct evsel *evsel);
> int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel);
>
> -#ifdef HAVE_SLANG_SUPPORT
> -int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> - struct hist_browser_timer *hbt);
> -#else
> -static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
> - struct evsel *evsel __maybe_unused,
> - struct hist_browser_timer *hbt __maybe_unused)
> -{
> - return 0;
> -}
> -#endif
> -
> void annotation_options__init(void);
> void annotation_options__exit(void);
>
> diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> index 70438d03ca9c33b1..c64005278687cb02 100644
> --- a/tools/perf/util/hist.h
> +++ b/tools/perf/util/hist.h
> @@ -713,8 +713,9 @@ struct block_hist {
> #include "../ui/keysyms.h"
> void attr_to_script(char *buf, struct perf_event_attr *attr);
>
> -int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> - struct hist_browser_timer *hbt);
> +int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> + struct evsel *evsel,
> + struct hist_browser_timer *hbt);
>
> int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> struct hist_browser_timer *hbt);
> @@ -742,9 +743,10 @@ int evlist__tui_browse_hists(struct evlist *evlist __maybe_unused,
> {
> return 0;
> }
> -static inline int map_symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
> - struct evsel *evsel __maybe_unused,
> - struct hist_browser_timer *hbt __maybe_unused)
> +static inline int __hist_entry__tui_annotate(struct hist_entry *he __maybe_unused,
> + struct map_symbol *ms __maybe_unused,
> + struct evsel *evsel __maybe_unused,
> + struct hist_browser_timer *hbt __maybe_unused)
> {
> return 0;
> }
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/9] perf annotate: Remove __annotation_line__write()
2025-07-25 19:37 ` [PATCH v4 2/9] perf annotate: Remove __annotation_line__write() Namhyung Kim
@ 2025-07-26 0:20 ` Ian Rogers
2025-07-28 18:42 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:20 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Get rid of the internal function and convert function arguments into
> local variables if they are used more than once.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/annotate.c | 46 ++++++++++++++++----------------------
> 1 file changed, 19 insertions(+), 27 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 0dd475a744b6dfac..69ee83052396b15e 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1935,24 +1935,26 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
> return -ENOMEM;
> }
>
> -static void __annotation_line__write(struct annotation_line *al, struct annotation *notes,
> - bool first_line, bool current_entry, bool change_color, int width,
> - void *obj, unsigned int percent_type,
> - int (*obj__set_color)(void *obj, int color),
> - void (*obj__set_percent_color)(void *obj, double percent, bool current),
> - int (*obj__set_jumps_percent_color)(void *obj, int nr, bool current),
> - void (*obj__printf)(void *obj, const char *fmt, ...),
> - void (*obj__write_graph)(void *obj, int graph))
> -
> -{
> - double percent_max = annotation_line__max_percent(al, percent_type);
> - int pcnt_width = annotation__pcnt_width(notes),
> - cycles_width = annotation__cycles_width(notes);
> +void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> + struct annotation_write_ops *wops)
nit: constify wops? If its const are the local variables worth it?
Thanks,
Ian
> +{
> + bool current_entry = wops->current_entry;
> + bool change_color = wops->change_color;
> + double percent_max = annotation_line__max_percent(al, annotate_opts.percent_type);
> + int width = wops->width;
> + int pcnt_width = annotation__pcnt_width(notes);
> + int cycles_width = annotation__cycles_width(notes);
> bool show_title = false;
> char bf[256];
> int printed;
> -
> - if (first_line && (al->offset == -1 || percent_max == 0.0)) {
> + void *obj = wops->obj;
> + int (*obj__set_color)(void *obj, int color) = wops->set_color;
> + void (*obj__set_percent_color)(void *obj, double percent, bool current) = wops->set_percent_color;
> + int (*obj__set_jumps_percent_color)(void *obj, int nr, bool current) = wops->set_jumps_percent_color;
> + void (*obj__printf)(void *obj, const char *fmt, ...) = wops->printf;
> + void (*obj__write_graph)(void *obj, int graph) = wops->write_graph;
> +
> + if (wops->first_line && (al->offset == -1 || percent_max == 0.0)) {
> if (notes->branch && al->cycles) {
> if (al->cycles->ipc == 0.0 && al->cycles->avg == 0)
> show_title = true;
> @@ -1966,7 +1968,8 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> for (i = 0; i < al->data_nr; i++) {
> double percent;
>
> - percent = annotation_data__percent(&al->data[i], percent_type);
> + percent = annotation_data__percent(&al->data[i],
> + annotate_opts.percent_type);
>
> obj__set_percent_color(obj, percent, current_entry);
> if (symbol_conf.show_total_period) {
> @@ -2115,17 +2118,6 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
>
> }
>
> -void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> - struct annotation_write_ops *wops)
> -{
> - __annotation_line__write(al, notes, wops->first_line, wops->current_entry,
> - wops->change_color, wops->width, wops->obj,
> - annotate_opts.percent_type,
> - wops->set_color, wops->set_percent_color,
> - wops->set_jumps_percent_color, wops->printf,
> - wops->write_graph);
> -}
> -
> int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
> struct arch **parch)
> {
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write()
2025-07-25 19:37 ` [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write() Namhyung Kim
@ 2025-07-26 0:23 ` Ian Rogers
2025-07-28 18:44 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:23 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It will be used for data type display later.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/ui/browsers/annotate.c | 13 ++++++++++--
> tools/perf/util/annotate.c | 35 +++++++++++++++++--------------
> tools/perf/util/annotate.h | 15 +++++++++++--
> 3 files changed, 43 insertions(+), 20 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 28ef146f29e8e742..23bea5b165774ae7 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -28,6 +28,8 @@ struct annotate_browser {
> struct annotation_line *selection;
> struct arch *arch;
> struct hist_entry *he;
> + struct debuginfo *dbg;
> + struct evsel *evsel;
> bool searching_backwards;
> char search_bf[128];
> };
> @@ -108,12 +110,18 @@ static void annotate_browser__write(struct ui_browser *browser, void *entry, int
> .printf = annotate_browser__printf,
> .write_graph = annotate_browser__write_graph,
> };
> + struct annotation_print_data apd = {
> + .he = ab->he,
> + .arch = ab->arch,
> + .evsel = ab->evsel,
> + .dbg = ab->dbg,
> + };
>
> /* The scroll bar isn't being used */
> if (!browser->navkeypressed)
> ops.width += 1;
>
> - annotation_line__write(al, notes, &ops);
> + annotation_line__write(al, notes, &ops, &apd);
>
> if (ops.current_entry)
> ab->selection = al;
> @@ -976,7 +984,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> continue;
> }
> case 'P':
> - map_symbol__annotation_dump(ms, evsel);
> + map_symbol__annotation_dump(ms, evsel, browser->he);
> continue;
> case 't':
> if (symbol_conf.show_total_period) {
> @@ -1061,6 +1069,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> .use_navkeypressed = true,
> },
> .he = he,
> + .evsel = evsel,
> };
> struct dso *dso;
> int ret = -1, err;
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 69ee83052396b15e..c21152710148b68c 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -765,15 +765,6 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
> struct debuginfo *dbg, struct disasm_line *dl,
> int *type_offset);
>
> -struct annotation_print_data {
> - struct hist_entry *he;
> - struct evsel *evsel;
> - struct arch *arch;
> - struct debuginfo *dbg;
> - u64 start;
> - int addr_fmt_width;
> -};
> -
> static int
> annotation_line__print(struct annotation_line *al, struct annotation_print_data *apd,
> struct annotation_options *opts, int printed,
> @@ -1230,7 +1221,6 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
> struct annotation_print_data apd = {
> .he = he,
> .evsel = evsel,
> - .start = map__rip_2objdump(map, sym->start),
> };
> int printed = 2, queue_len = 0;
> int more = 0;
> @@ -1357,7 +1347,8 @@ static void FILE__write_graph(void *fp, int graph)
> fputs(s, fp);
> }
>
> -static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
> +static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
> + struct annotation_print_data *apd)
> {
> struct annotation *notes = symbol__annotation(sym);
> struct annotation_write_ops wops = {
> @@ -1374,7 +1365,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
> list_for_each_entry(al, ¬es->src->source, node) {
> if (annotation_line__filter(al))
> continue;
> - annotation_line__write(al, notes, &wops);
> + annotation_line__write(al, notes, &wops, apd);
> fputc('\n', fp);
> wops.first_line = false;
> }
> @@ -1382,13 +1373,18 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp)
> return 0;
> }
>
> -int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel)
> +int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel,
> + struct hist_entry *he)
> {
> const char *ev_name = evsel__name(evsel);
> char buf[1024];
> char *filename;
> int err = -1;
> FILE *fp;
> + struct annotation_print_data apd = {
> + .he = he,
> + .evsel = evsel,
> + };
>
> if (asprintf(&filename, "%s.annotation", ms->sym->name) < 0)
> return -1;
> @@ -1404,7 +1400,7 @@ int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel)
>
> fprintf(fp, "%s() %s\nEvent: %s\n\n",
> ms->sym->name, dso__long_name(map__dso(ms->map)), ev_name);
> - symbol__annotate_fprintf2(ms->sym, fp);
> + symbol__annotate_fprintf2(ms->sym, fp, &apd);
>
> fclose(fp);
> err = 0;
> @@ -1656,6 +1652,10 @@ int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel)
> struct symbol *sym = ms->sym;
> struct rb_root source_line = RB_ROOT;
> struct hists *hists = evsel__hists(evsel);
> + struct annotation_print_data apd = {
> + .he = he,
> + .evsel = evsel,
> + };
> char buf[1024];
> int err;
>
> @@ -1678,7 +1678,7 @@ int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel)
> hists__scnprintf_title(hists, buf, sizeof(buf));
> fprintf(stdout, "%s, [percent: %s]\n%s() %s\n",
> buf, percent_type_str(annotate_opts.percent_type), sym->name, dso__long_name(dso));
> - symbol__annotate_fprintf2(sym, stdout);
> + symbol__annotate_fprintf2(sym, stdout, &apd);
>
> annotated_source__purge(symbol__annotation(sym)->src);
>
> @@ -1936,7 +1936,8 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
> }
>
> void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> - struct annotation_write_ops *wops)
> + struct annotation_write_ops *wops,
> + struct annotation_print_data *apd)
> {
> bool current_entry = wops->current_entry;
> bool change_color = wops->change_color;
> @@ -2114,6 +2115,8 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
>
> obj__printf(obj, "%-*s", width - pcnt_width - cycles_width - 3 - printed, bf);
> +
> + (void)apd;
> }
>
> }
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 0f640e4871744262..8fad464a870a2b8e 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -199,8 +199,18 @@ struct annotation_write_ops {
> void (*write_graph)(void *obj, int graph);
> };
>
> +struct annotation_print_data {
> + struct hist_entry *he;
> + struct evsel *evsel;
> + struct arch *arch;
> + struct debuginfo *dbg;
> + u64 start;
> + int addr_fmt_width;
> +};
nit: As the scope of this is now global, is it worth commenting it?
For example, where is start relative to?
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> +
> void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> - struct annotation_write_ops *ops);
> + struct annotation_write_ops *ops,
> + struct annotation_print_data *apd);
>
> int __annotation__scnprintf_samples_period(struct annotation *notes,
> char *bf, size_t size,
> @@ -463,7 +473,8 @@ void symbol__annotate_zero_histogram(struct symbol *sym, struct evsel *evsel);
> void symbol__annotate_decay_histogram(struct symbol *sym, struct evsel *evsel);
> void annotated_source__purge(struct annotated_source *as);
>
> -int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel);
> +int map_symbol__annotation_dump(struct map_symbol *ms, struct evsel *evsel,
> + struct hist_entry *he);
>
> bool ui__has_annotation(void);
>
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write()
2025-07-25 19:37 ` [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write() Namhyung Kim
@ 2025-07-26 0:30 ` Ian Rogers
2025-07-28 18:47 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:30 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The width is updated after each part is printed. It can skip the output
> processing if the total printed size is bigger than the width.
>
> No function changes intended.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/annotate.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index c21152710148b68c..d69e406c1bc289cd 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1993,6 +1993,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> symbol_conf.show_nr_samples ? "Samples" : "Percent");
> }
> }
> + width -= pcnt_width;
>
> if (notes->branch) {
> if (al->cycles && al->cycles->ipc)
> @@ -2056,11 +2057,12 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> obj__printf(obj, "%*s", ANNOTATION__AVG_IPC_WIDTH, bf);
> }
> }
> + width -= cycles_width;
>
> obj__printf(obj, " ");
>
> if (!*al->line)
> - obj__printf(obj, "%-*s", width - pcnt_width - cycles_width, " ");
> + obj__printf(obj, "%-*s", width, " ");
> else if (al->offset == -1) {
> if (al->line_nr && annotate_opts.show_linenr)
> printed = scnprintf(bf, sizeof(bf), "%-*d ",
> @@ -2069,7 +2071,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> printed = scnprintf(bf, sizeof(bf), "%-*s ",
> notes->src->widths.addr, " ");
> obj__printf(obj, bf);
> - obj__printf(obj, "%-*s", width - printed - pcnt_width - cycles_width + 1, al->line);
> + obj__printf(obj, "%-*s", width - printed + 1, al->line);
This doesn't seem to line up with the commit message, should width be
updated prior to this call?
> } else {
> u64 addr = al->offset;
> int color = -1;
> @@ -2112,9 +2114,11 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> if (change_color)
> obj__set_color(obj, color);
>
> + width -= printed + 3;
nit: For constants that like '+3' here and '+1' it'd be nice for a
comment just to say where the adjustment comes from.
Thanks,
Ian
> +
> disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
>
> - obj__printf(obj, "%-*s", width - pcnt_width - cycles_width - 3 - printed, bf);
> + obj__printf(obj, "%-*s", width, bf);
>
> (void)apd;
> }
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI
2025-07-25 19:37 ` [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI Namhyung Kim
@ 2025-07-26 0:31 ` Ian Rogers
2025-07-28 18:47 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:31 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Until now, the --code-with-type option is available only on stdio.
> But it was an artifical limitation because of an implemention issue.
>
> Implement the same logic in annotation_line__write() for stdio2/TUI.
> Make disasm_line__write() return the number of printed characters so
> that it can skip unnecessary operations when the screen is full.
This change seems to be different from the rest of what is the patch,
is it possible to have it is a patch before this one?
Thanks,
Ian
> Remove the limitation and update the man page.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/Documentation/perf-annotate.txt | 1 -
> tools/perf/builtin-annotate.c | 5 --
> tools/perf/ui/browsers/annotate.c | 6 +++
> tools/perf/util/annotate.c | 61 +++++++++++++++++++---
> 4 files changed, 61 insertions(+), 12 deletions(-)
>
> diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
> index 46090c5b42b4762f..547f1a2680185e3c 100644
> --- a/tools/perf/Documentation/perf-annotate.txt
> +++ b/tools/perf/Documentation/perf-annotate.txt
> @@ -170,7 +170,6 @@ include::itrace.txt[]
>
> --code-with-type::
> Show data type info in code annotation (for memory instructions only).
> - Currently it only works with --stdio option.
>
>
> SEE ALSO
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 5d57d2913f3d9a33..646f43b0f7c4c9b0 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -917,11 +917,6 @@ int cmd_annotate(int argc, const char **argv)
> symbol_conf.annotate_data_sample = true;
> } else if (annotate_opts.code_with_type) {
> symbol_conf.annotate_data_member = true;
> -
> - if (!annotate.use_stdio) {
> - pr_err("--code-with-type only works with --stdio.\n");
> - goto out_delete;
> - }
> }
>
> setup_browser(true);
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 23bea5b165774ae7..cdee1969f3131a7c 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -4,6 +4,7 @@
> #include "../ui.h"
> #include "../../util/annotate.h"
> #include "../../util/debug.h"
> +#include "../../util/debuginfo.h"
> #include "../../util/dso.h"
> #include "../../util/hist.h"
> #include "../../util/sort.h"
> @@ -1101,6 +1102,9 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
>
> ui_helpline__push("Press ESC to exit");
>
> + if (annotate_opts.code_with_type)
> + browser.dbg = debuginfo__new(dso__long_name(dso));
> +
> browser.b.width = notes->src->widths.max_line_len;
> browser.b.nr_entries = notes->src->nr_entries;
> browser.b.entries = ¬es->src->source;
> @@ -1111,6 +1115,8 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
>
> ret = annotate_browser__run(&browser, evsel, hbt);
>
> + if (annotate_opts.code_with_type)
> + debuginfo__delete(browser.dbg);
> if (not_annotated && !notes->src->tried_source)
> annotated_source__purge(notes->src);
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index d69e406c1bc289cd..06ddc7a9f58722a4 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1362,6 +1362,11 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
> };
> struct annotation_line *al;
>
> + if (annotate_opts.code_with_type) {
> + evsel__get_arch(apd->evsel, &apd->arch);
> + apd->dbg = debuginfo__new(dso__long_name(map__dso(apd->he->ms.map)));
> + }
> +
> list_for_each_entry(al, ¬es->src->source, node) {
> if (annotation_line__filter(al))
> continue;
> @@ -1370,6 +1375,9 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
> wops.first_line = false;
> }
>
> + if (annotate_opts.code_with_type)
> + debuginfo__delete(apd->dbg);
> +
> return 0;
> }
>
> @@ -1743,7 +1751,7 @@ static double annotation_line__max_percent(struct annotation_line *al,
> return percent_max;
> }
>
> -static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> +static int disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> void *obj, char *bf, size_t size,
> void (*obj__printf)(void *obj, const char *fmt, ...),
> void (*obj__write_graph)(void *obj, int graph))
> @@ -1771,8 +1779,8 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> obj__printf(obj, " ");
> }
>
> - disasm_line__scnprintf(dl, bf, size, !annotate_opts.use_offset,
> - notes->src->widths.max_ins_name);
> + return disasm_line__scnprintf(dl, bf, size, !annotate_opts.use_offset,
> + notes->src->widths.max_ins_name);
> }
>
> static void ipc_coverage_string(char *bf, int size, struct annotation *notes)
> @@ -2116,11 +2124,52 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
>
> width -= printed + 3;
>
> - disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
> + printed = disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf),
> + obj__printf, obj__write_graph);
> +
> + obj__printf(obj, "%s", bf);
> + width -= printed;
> +
> + if (annotate_opts.code_with_type && apd->dbg) {
> + struct annotated_data_type *data_type;
> + int offset = 0;
> +
> + data_type = __hist_entry__get_data_type(apd->he, apd->arch,
> + apd->dbg,
> + disasm_line(al),
> + &offset);
> + if (data_type && data_type != NO_TYPE) {
> + char member[256];
> +
> + printed = scnprintf(bf, sizeof(bf),
> + "\t\t# data-type: %s",
> + data_type->self.type_name);
>
> - obj__printf(obj, "%-*s", width, bf);
> + if (data_type != &stackop_type &&
> + data_type != &canary_type &&
> + sizeof(bf) > (size_t)printed) {
> + printed += scnprintf(bf + printed,
> + sizeof(bf) - printed,
> + " +%#x", offset);
> + }
> +
> + if (annotated_data_type__get_member_name(data_type,
> + member,
> + sizeof(member),
> + offset) &&
> + sizeof(bf) > (size_t)printed) {
> + printed += scnprintf(bf + printed,
> + sizeof(bf) - printed,
> + " (%s)", member);
> + }
>
> - (void)apd;
> + obj__printf(obj, "%-*s", width, bf);
> + } else {
> + obj__printf(obj, "%-*s", width, " ");
> + }
> + } else {
> + obj__printf(obj, "%-*s", width, " ");
> + }
> }
>
> }
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 6/9] perf annotate: Add 'T' hot key to toggle data type display
2025-07-25 19:37 ` [PATCH v4 6/9] perf annotate: Add 'T' hot key to toggle data type display Namhyung Kim
@ 2025-07-26 0:34 ` Ian Rogers
0 siblings, 0 replies; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:34 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Support data type display with a key press so that users can toggle the
> output dynamically on TUI. Also display "[Type]" in the title line if
> it's enabled.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/ui/browsers/annotate.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index cdee1969f3131a7c..4b059e0bafd33fcf 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -525,9 +525,10 @@ static void ui_browser__init_asm_mode(struct ui_browser *browser)
> static int sym_title(struct symbol *sym, struct map *map, char *title,
> size_t sz, int percent_type)
> {
> - return snprintf(title, sz, "%s %s [Percent: %s]", sym->name,
> + return snprintf(title, sz, "%s %s [Percent: %s] %s", sym->name,
> dso__long_name(map__dso(map)),
> - percent_type_str(percent_type));
> + percent_type_str(percent_type),
> + annotate_opts.code_with_type ? "[Type]" : "");
> }
>
> /*
> @@ -901,7 +902,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
> "b Toggle percent base [period/hits]\n"
> "B Branch counter abbr list (Optional)\n"
> "? Search string backwards\n"
> - "f Toggle showing offsets to full address\n");
> + "f Toggle showing offsets to full address\n"
> + "T Toggle data type display\n");
> continue;
> case 'r':
> script_browse(NULL, NULL);
> @@ -1021,6 +1023,12 @@ static int annotate_browser__run(struct annotate_browser *browser,
> case 'f':
> annotation__toggle_full_addr(notes, ms);
> continue;
> + case 'T':
> + annotate_opts.code_with_type ^= 1;
> + if (browser->dbg == NULL)
> + browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
browser->dbg = dso__debuginfo(map__dso(ms->map));
There was a conversation about dso not necessarily having to have
long_name in the future, etc.
Thanks,
Ian
> + annotate_browser__show(&browser->b, title, help);
> + continue;
> case K_LEFT:
> case '<':
> case '>':
> @@ -1115,8 +1123,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
>
> ret = annotate_browser__run(&browser, evsel, hbt);
>
> - if (annotate_opts.code_with_type)
> - debuginfo__delete(browser.dbg);
> + debuginfo__delete(browser.dbg);
> if (not_annotated && !notes->src->tried_source)
> annotated_source__purge(notes->src);
>
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 7/9] perf annotate: Show warning when debuginfo is not available
2025-07-25 19:37 ` [PATCH v4 7/9] perf annotate: Show warning when debuginfo is not available Namhyung Kim
@ 2025-07-26 0:38 ` Ian Rogers
0 siblings, 0 replies; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:38 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> When user requests data-type annotation but no DWARF info is available,
> show a warning message about it.
>
> Warning:
> DWARF debuginfo not found.
>
> Data-type in this DSO will not be displayed.
> Please make sure to have debug information.
>
> Press any key...
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/ui/browsers/annotate.c | 17 +++++++++++++++++
> tools/perf/util/dso.h | 11 +++++++++++
> 2 files changed, 28 insertions(+)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 4b059e0bafd33fcf..2a4db5bdcdb7e9d8 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -804,6 +804,20 @@ static int annotate__scnprintf_title(struct hists *hists, char *bf, size_t size)
> return printed;
> }
>
> +static void annotate_browser__debuginfo_warning(struct annotate_browser *browser)
> +{
> + struct map_symbol *ms = browser->b.priv;
> + struct dso *dso = map__dso(ms->map);
> +
> + if (browser->dbg == NULL && annotate_opts.code_with_type &&
> + !dso__debuginfo_warned(dso)) {
> + ui__warning("DWARF debuginfo not found.\n\n"
> + "Data-type in this DSO will not be displayed.\n"
> + "Please make sure to have debug information.");
> + dso__set_debuginfo_warned(dso);
If there is a dso__debuginfo then this could be encapsulated there, ie
the browser wouldn't need to change dso variables it'd just be a
property of calling dso__debuginfo.
Thanks,
Ian
> + }
> +}
> +
> static int annotate_browser__run(struct annotate_browser *browser,
> struct evsel *evsel,
> struct hist_browser_timer *hbt)
> @@ -834,6 +848,8 @@ static int annotate_browser__run(struct annotate_browser *browser,
>
> annotation_br_cntr_abbr_list(&br_cntr_text, evsel, false);
>
> + annotate_browser__debuginfo_warning(browser);
> +
> while (1) {
> key = ui_browser__run(&browser->b, delay_secs);
>
> @@ -1028,6 +1044,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> if (browser->dbg == NULL)
> browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
> annotate_browser__show(&browser->b, title, help);
> + annotate_browser__debuginfo_warning(browser);
> continue;
> case K_LEFT:
> case '<':
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 3457d713d3c56df6..7df1673f08d3ddb4 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -299,6 +299,7 @@ DECLARE_RC_STRUCT(dso) {
> u8 hit:1;
> u8 annotate_warned:1;
> u8 auxtrace_warned:1;
> + u8 debuginfo_warned:1;
> u8 short_name_allocated:1;
> u8 long_name_allocated:1;
> u8 is_64_bit:1;
> @@ -362,6 +363,16 @@ static inline void dso__set_annotate_warned(struct dso *dso)
> RC_CHK_ACCESS(dso)->annotate_warned = 1;
> }
>
> +static inline bool dso__debuginfo_warned(const struct dso *dso)
> +{
> + return RC_CHK_ACCESS(dso)->debuginfo_warned;
> +}
> +
> +static inline void dso__set_debuginfo_warned(struct dso *dso)
> +{
> + RC_CHK_ACCESS(dso)->debuginfo_warned = 1;
> +}
> +
> static inline bool dso__auxtrace_warned(const struct dso *dso)
> {
> return RC_CHK_ACCESS(dso)->auxtrace_warned;
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary
2025-07-25 19:37 ` [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary Namhyung Kim
@ 2025-07-26 0:44 ` Ian Rogers
2025-07-28 18:51 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:44 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It's mostly unnecessary to print when it has no actual type information
> like in the stack operations and canary. Let's have them if -v option
> is given.
>
> Before:
> $ perf annotate --code-with-type
> ...
> : 0 0xd640 <_dl_relocate_object>:
> 0.00 : 0: endbr64
> 0.00 : 4: pushq %rbp # data-type: (stack operation)
> 0.00 : 5: movq %rsp, %rbp
> 0.00 : 8: pushq %r15 # data-type: (stack operation)
> 0.00 : a: pushq %r14 # data-type: (stack operation)
> 0.00 : c: pushq %r13 # data-type: (stack operation)
> 0.00 : e: pushq %r12 # data-type: (stack operation)
> 0.00 : 10: pushq %rbx # data-type: (stack operation)
I believe the intent in the dwarf is to say where the caller's callee
saves are, but the stack slots should just be saved and restored and
won't be used for anything interesting, perhaps for exception
handling. An annotation like:
# caller's RBX at stack frame offset -48
could perhaps be useful.
> 0.00 : 11: subq $0xf8, %rsp
> ...
> 0.00 : d4: testl %eax, %eax
> 0.00 : d6: jne 0xf424
> 0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
> 0.00 : e3: testq %rbx, %rbx
> 0.00 : e6: jne 0xf2dd
> 0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
> ...
>
> After:
> : 0 0xd640 <_dl_relocate_object>:
> 0.00 : 0: endbr64
> 0.00 : 4: pushq %rbp
> 0.00 : 5: movq %rsp, %rbp
> 0.00 : 8: pushq %r15
> 0.00 : a: pushq %r14
> 0.00 : c: pushq %r13
> 0.00 : e: pushq %r12
> 0.00 : 10: pushq %rbx
> 0.00 : 11: subq $0xf8, %rsp
> ...
> 0.00 : d4: testl %eax, %eax
> 0.00 : d6: jne 0xf424
> 0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
> 0.00 : e3: testq %rbx, %rbx
> 0.00 : e6: jne 0xf2dd
> 0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
> ...
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/annotate.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 06ddc7a9f58722a4..6fc07971631ac8a3 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -765,6 +765,17 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
> struct debuginfo *dbg, struct disasm_line *dl,
> int *type_offset);
>
> +static bool needs_type_info(struct annotated_data_type *data_type)
> +{
> + if (data_type == NULL || data_type == NO_TYPE)
> + return false;
> +
> + if (verbose)
> + return true;
lgtm given the many overloaded meanings of verbose.
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> +
> + return (data_type != &stackop_type) && (data_type != &canary_type);
> +}
> +
> static int
> annotation_line__print(struct annotation_line *al, struct annotation_print_data *apd,
> struct annotation_options *opts, int printed,
> @@ -844,7 +855,7 @@ annotation_line__print(struct annotation_line *al, struct annotation_print_data
>
> data_type = __hist_entry__get_data_type(apd->he, apd->arch,
> apd->dbg, dl, &offset);
> - if (data_type && data_type != NO_TYPE) {
> + if (needs_type_info(data_type)) {
> char buf[4096];
>
> printf("\t\t# data-type: %s",
> @@ -2138,7 +2149,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> apd->dbg,
> disasm_line(al),
> &offset);
> - if (data_type && data_type != NO_TYPE) {
> + if (needs_type_info(data_type)) {
> char member[256];
>
> printed = scnprintf(bf, sizeof(bf),
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper
2025-07-25 19:37 ` [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper Namhyung Kim
@ 2025-07-26 0:46 ` Ian Rogers
2025-07-28 18:54 ` Namhyung Kim
0 siblings, 1 reply; 26+ messages in thread
From: Ian Rogers @ 2025-07-26 0:46 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> It'd be great if it can get the correct debug information using DSO
> build-Id not just the path name. Instead of adding new callsites of
> debuginfo__new(), let's add dso__debuginfo() which can hide the access
> using the pathname and help the future conversion.
>
> Suggested-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
:-) and my prior review comments now make less sense. I think putting
the ui__warning into dso__debugingo makes sense, wdyt?
Thanks,
Ian
> ---
> tools/perf/ui/browsers/annotate.c | 4 ++--
> tools/perf/util/annotate.c | 6 +++---
> tools/perf/util/dso.h | 10 ++++++++++
> 3 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index 2a4db5bdcdb7e9d8..54610621c5f910fe 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -1042,7 +1042,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> case 'T':
> annotate_opts.code_with_type ^= 1;
> if (browser->dbg == NULL)
> - browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
> + browser->dbg = dso__debuginfo(map__dso(ms->map));
> annotate_browser__show(&browser->b, title, help);
> annotate_browser__debuginfo_warning(browser);
> continue;
> @@ -1128,7 +1128,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> ui_helpline__push("Press ESC to exit");
>
> if (annotate_opts.code_with_type)
> - browser.dbg = debuginfo__new(dso__long_name(dso));
> + browser.dbg = dso__debuginfo(dso);
>
> browser.b.width = notes->src->widths.max_line_len;
> browser.b.nr_entries = notes->src->nr_entries;
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 6fc07971631ac8a3..05eb19b110ab7dcf 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1270,7 +1270,7 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
> apd.addr_fmt_width = annotated_source__addr_fmt_width(¬es->src->source,
> apd.start);
> evsel__get_arch(evsel, &apd.arch);
> - apd.dbg = debuginfo__new(filename);
> + apd.dbg = dso__debuginfo(dso);
>
> list_for_each_entry(pos, ¬es->src->source, node) {
> int err;
> @@ -1375,7 +1375,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
>
> if (annotate_opts.code_with_type) {
> evsel__get_arch(apd->evsel, &apd->arch);
> - apd->dbg = debuginfo__new(dso__long_name(map__dso(apd->he->ms.map)));
> + apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
> }
>
> list_for_each_entry(al, ¬es->src->source, node) {
> @@ -2888,7 +2888,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> di_cache.dso = dso__get(map__dso(ms->map));
>
> debuginfo__delete(di_cache.dbg);
> - di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
> + di_cache.dbg = dso__debuginfo(di_cache.dso);
> }
>
> if (di_cache.dbg == NULL) {
> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> index 7df1673f08d3ddb4..fd8e95de77f78dfc 100644
> --- a/tools/perf/util/dso.h
> +++ b/tools/perf/util/dso.h
> @@ -10,6 +10,7 @@
> #include <stdio.h>
> #include <linux/bitops.h>
> #include "build-id.h"
> +#include "debuginfo.h"
> #include "mutex.h"
> #include <internal/rc_check.h>
>
> @@ -914,4 +915,13 @@ u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
> bool perf_pid_map_tid(const char *dso_name, int *tid);
> bool is_perf_pid_map_name(const char *dso_name);
>
> +/*
> + * In the future, we may get debuginfo using build-ID (w/o path).
> + * Add this helper is for the smooth conversion.
> + */
> +static inline struct debuginfo *dso__debuginfo(struct dso *dso)
> +{
> + return debuginfo__new(dso__long_name(dso));
> +}
> +
> #endif /* __PERF_DSO */
> --
> 2.50.1
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate()
2025-07-26 0:16 ` Ian Rogers
@ 2025-07-28 18:39 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:39 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:16:39PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > There are three different but similar functions for annotation on TUI.
> > Rename it to __hist_entry__tui_annotate() and make sure it passes 'he'.
> > It's not used for now but it'll be needed for later use.
> >
> > Also remove map_symbol__tui_annotate() which was a simple wrapper.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/ui/browsers/annotate.c | 17 +++++++----------
> > tools/perf/ui/browsers/hists.c | 2 +-
> > tools/perf/util/annotate.h | 12 ------------
> > tools/perf/util/hist.h | 12 +++++++-----
> > 4 files changed, 15 insertions(+), 28 deletions(-)
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 183902dac042ecb0..28ef146f29e8e742 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -27,6 +27,7 @@ struct annotate_browser {
> > struct rb_node *curr_hot;
> > struct annotation_line *selection;
> > struct arch *arch;
> > + struct hist_entry *he;
>
> nit: as you are here is it worth commenting this, for example, who owns "he"?
Hmm.. good point. The hist entries can be deleted by perf top display
thread so we cannot guarantee it's live during the annotation. Maybe we
need a copy and with proper reference counting on its fields. I'll
think about it more.
>
> Reviewed-by: Ian Rogers <irogers@google.com>
Thanks for your review!
Namhyung
>
> > bool searching_backwards;
> > char search_bf[128];
> > };
> > @@ -557,7 +558,7 @@ static bool annotate_browser__callq(struct annotate_browser *browser,
> > target_ms.map = ms->map;
> > target_ms.sym = dl->ops.target.sym;
> > annotation__unlock(notes);
> > - symbol__tui_annotate(&target_ms, evsel, hbt);
> > + __hist_entry__tui_annotate(browser->he, &target_ms, evsel, hbt);
> > sym_title(ms->sym, ms->map, title, sizeof(title), annotate_opts.percent_type);
> > ui_browser__show_title(&browser->b, title);
> > return true;
> > @@ -1032,12 +1033,6 @@ static int annotate_browser__run(struct annotate_browser *browser,
> > return key;
> > }
> >
> > -int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> > - struct hist_browser_timer *hbt)
> > -{
> > - return symbol__tui_annotate(ms, evsel, hbt);
> > -}
> > -
> > int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> > struct hist_browser_timer *hbt)
> > {
> > @@ -1046,11 +1041,12 @@ int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> > SLang_init_tty(0, 0, 0);
> > SLtty_set_suspend_state(true);
> >
> > - return map_symbol__tui_annotate(&he->ms, evsel, hbt);
> > + return __hist_entry__tui_annotate(he, &he->ms, evsel, hbt);
> > }
> >
> > -int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> > - struct hist_browser_timer *hbt)
> > +int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> > + struct evsel *evsel,
> > + struct hist_browser_timer *hbt)
> > {
> > struct symbol *sym = ms->sym;
> > struct annotation *notes = symbol__annotation(sym);
> > @@ -1064,6 +1060,7 @@ int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> > .priv = ms,
> > .use_navkeypressed = true,
> > },
> > + .he = he,
> > };
> > struct dso *dso;
> > int ret = -1, err;
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index d9d3fb44477ac6d5..487c0b08c0038710 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -2484,8 +2484,8 @@ do_annotate(struct hist_browser *browser, struct popup_action *act)
> > else
> > evsel = hists_to_evsel(browser->hists);
> >
> > - err = map_symbol__tui_annotate(&act->ms, evsel, browser->hbt);
> > he = hist_browser__selected_entry(browser);
> > + err = __hist_entry__tui_annotate(he, &act->ms, evsel, browser->hbt);
> > /*
> > * offer option to annotate the other branch source or target
> > * (if they exists) when returning from annotate
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 8b5131d257b01e3e..0f640e4871744262 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -471,18 +471,6 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel);
> > int hist_entry__tty_annotate(struct hist_entry *he, struct evsel *evsel);
> > int hist_entry__tty_annotate2(struct hist_entry *he, struct evsel *evsel);
> >
> > -#ifdef HAVE_SLANG_SUPPORT
> > -int symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> > - struct hist_browser_timer *hbt);
> > -#else
> > -static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
> > - struct evsel *evsel __maybe_unused,
> > - struct hist_browser_timer *hbt __maybe_unused)
> > -{
> > - return 0;
> > -}
> > -#endif
> > -
> > void annotation_options__init(void);
> > void annotation_options__exit(void);
> >
> > diff --git a/tools/perf/util/hist.h b/tools/perf/util/hist.h
> > index 70438d03ca9c33b1..c64005278687cb02 100644
> > --- a/tools/perf/util/hist.h
> > +++ b/tools/perf/util/hist.h
> > @@ -713,8 +713,9 @@ struct block_hist {
> > #include "../ui/keysyms.h"
> > void attr_to_script(char *buf, struct perf_event_attr *attr);
> >
> > -int map_symbol__tui_annotate(struct map_symbol *ms, struct evsel *evsel,
> > - struct hist_browser_timer *hbt);
> > +int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> > + struct evsel *evsel,
> > + struct hist_browser_timer *hbt);
> >
> > int hist_entry__tui_annotate(struct hist_entry *he, struct evsel *evsel,
> > struct hist_browser_timer *hbt);
> > @@ -742,9 +743,10 @@ int evlist__tui_browse_hists(struct evlist *evlist __maybe_unused,
> > {
> > return 0;
> > }
> > -static inline int map_symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
> > - struct evsel *evsel __maybe_unused,
> > - struct hist_browser_timer *hbt __maybe_unused)
> > +static inline int __hist_entry__tui_annotate(struct hist_entry *he __maybe_unused,
> > + struct map_symbol *ms __maybe_unused,
> > + struct evsel *evsel __maybe_unused,
> > + struct hist_browser_timer *hbt __maybe_unused)
> > {
> > return 0;
> > }
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 2/9] perf annotate: Remove __annotation_line__write()
2025-07-26 0:20 ` Ian Rogers
@ 2025-07-28 18:42 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:42 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:20:28PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Get rid of the internal function and convert function arguments into
> > local variables if they are used more than once.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/util/annotate.c | 46 ++++++++++++++++----------------------
> > 1 file changed, 19 insertions(+), 27 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 0dd475a744b6dfac..69ee83052396b15e 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1935,24 +1935,26 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr,
> > return -ENOMEM;
> > }
> >
> > -static void __annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > - bool first_line, bool current_entry, bool change_color, int width,
> > - void *obj, unsigned int percent_type,
> > - int (*obj__set_color)(void *obj, int color),
> > - void (*obj__set_percent_color)(void *obj, double percent, bool current),
> > - int (*obj__set_jumps_percent_color)(void *obj, int nr, bool current),
> > - void (*obj__printf)(void *obj, const char *fmt, ...),
> > - void (*obj__write_graph)(void *obj, int graph))
> > -
> > -{
> > - double percent_max = annotation_line__max_percent(al, percent_type);
> > - int pcnt_width = annotation__pcnt_width(notes),
> > - cycles_width = annotation__cycles_width(notes);
> > +void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > + struct annotation_write_ops *wops)
>
> nit: constify wops? If its const are the local variables worth it?
Sure, I think it's good.
Thanks,
Namhyung
> > +{
> > + bool current_entry = wops->current_entry;
> > + bool change_color = wops->change_color;
> > + double percent_max = annotation_line__max_percent(al, annotate_opts.percent_type);
> > + int width = wops->width;
> > + int pcnt_width = annotation__pcnt_width(notes);
> > + int cycles_width = annotation__cycles_width(notes);
> > bool show_title = false;
> > char bf[256];
> > int printed;
> > -
> > - if (first_line && (al->offset == -1 || percent_max == 0.0)) {
> > + void *obj = wops->obj;
> > + int (*obj__set_color)(void *obj, int color) = wops->set_color;
> > + void (*obj__set_percent_color)(void *obj, double percent, bool current) = wops->set_percent_color;
> > + int (*obj__set_jumps_percent_color)(void *obj, int nr, bool current) = wops->set_jumps_percent_color;
> > + void (*obj__printf)(void *obj, const char *fmt, ...) = wops->printf;
> > + void (*obj__write_graph)(void *obj, int graph) = wops->write_graph;
> > +
> > + if (wops->first_line && (al->offset == -1 || percent_max == 0.0)) {
> > if (notes->branch && al->cycles) {
> > if (al->cycles->ipc == 0.0 && al->cycles->avg == 0)
> > show_title = true;
> > @@ -1966,7 +1968,8 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> > for (i = 0; i < al->data_nr; i++) {
> > double percent;
> >
> > - percent = annotation_data__percent(&al->data[i], percent_type);
> > + percent = annotation_data__percent(&al->data[i],
> > + annotate_opts.percent_type);
> >
> > obj__set_percent_color(obj, percent, current_entry);
> > if (symbol_conf.show_total_period) {
> > @@ -2115,17 +2118,6 @@ static void __annotation_line__write(struct annotation_line *al, struct annotati
> >
> > }
> >
> > -void annotation_line__write(struct annotation_line *al, struct annotation *notes,
> > - struct annotation_write_ops *wops)
> > -{
> > - __annotation_line__write(al, notes, wops->first_line, wops->current_entry,
> > - wops->change_color, wops->width, wops->obj,
> > - annotate_opts.percent_type,
> > - wops->set_color, wops->set_percent_color,
> > - wops->set_jumps_percent_color, wops->printf,
> > - wops->write_graph);
> > -}
> > -
> > int symbol__annotate2(struct map_symbol *ms, struct evsel *evsel,
> > struct arch **parch)
> > {
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write()
2025-07-26 0:23 ` Ian Rogers
@ 2025-07-28 18:44 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:44 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:23:00PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
[SNIP]
> > diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> > index 0f640e4871744262..8fad464a870a2b8e 100644
> > --- a/tools/perf/util/annotate.h
> > +++ b/tools/perf/util/annotate.h
> > @@ -199,8 +199,18 @@ struct annotation_write_ops {
> > void (*write_graph)(void *obj, int graph);
> > };
> >
> > +struct annotation_print_data {
> > + struct hist_entry *he;
> > + struct evsel *evsel;
> > + struct arch *arch;
> > + struct debuginfo *dbg;
> > + u64 start;
> > + int addr_fmt_width;
> > +};
>
> nit: As the scope of this is now global, is it worth commenting it?
> For example, where is start relative to?
Sure, I'll add comments.
>
> Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Namhyung
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write()
2025-07-26 0:30 ` Ian Rogers
@ 2025-07-28 18:47 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:47 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:30:04PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The width is updated after each part is printed. It can skip the output
> > processing if the total printed size is bigger than the width.
> >
> > No function changes intended.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/util/annotate.c | 10 +++++++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index c21152710148b68c..d69e406c1bc289cd 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1993,6 +1993,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> > symbol_conf.show_nr_samples ? "Samples" : "Percent");
> > }
> > }
> > + width -= pcnt_width;
> >
> > if (notes->branch) {
> > if (al->cycles && al->cycles->ipc)
> > @@ -2056,11 +2057,12 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> > obj__printf(obj, "%*s", ANNOTATION__AVG_IPC_WIDTH, bf);
> > }
> > }
> > + width -= cycles_width;
> >
> > obj__printf(obj, " ");
> >
> > if (!*al->line)
> > - obj__printf(obj, "%-*s", width - pcnt_width - cycles_width, " ");
> > + obj__printf(obj, "%-*s", width, " ");
> > else if (al->offset == -1) {
> > if (al->line_nr && annotate_opts.show_linenr)
> > printed = scnprintf(bf, sizeof(bf), "%-*d ",
> > @@ -2069,7 +2071,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> > printed = scnprintf(bf, sizeof(bf), "%-*s ",
> > notes->src->widths.addr, " ");
> > obj__printf(obj, bf);
> > - obj__printf(obj, "%-*s", width - printed - pcnt_width - cycles_width + 1, al->line);
> > + obj__printf(obj, "%-*s", width - printed + 1, al->line);
>
> This doesn't seem to line up with the commit message, should width be
> updated prior to this call?
Fair enough.
>
> > } else {
> > u64 addr = al->offset;
> > int color = -1;
> > @@ -2112,9 +2114,11 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> > if (change_color)
> > obj__set_color(obj, color);
> >
> > + width -= printed + 3;
>
> nit: For constants that like '+3' here and '+1' it'd be nice for a
> comment just to say where the adjustment comes from.
Sure, will do.
Thanks,
Namhyung
> > +
> > disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
> >
> > - obj__printf(obj, "%-*s", width - pcnt_width - cycles_width - 3 - printed, bf);
> > + obj__printf(obj, "%-*s", width, bf);
> >
> > (void)apd;
> > }
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI
2025-07-26 0:31 ` Ian Rogers
@ 2025-07-28 18:47 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:47 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:31:52PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Until now, the --code-with-type option is available only on stdio.
> > But it was an artifical limitation because of an implemention issue.
> >
> > Implement the same logic in annotation_line__write() for stdio2/TUI.
> > Make disasm_line__write() return the number of printed characters so
> > that it can skip unnecessary operations when the screen is full.
>
> This change seems to be different from the rest of what is the patch,
> is it possible to have it is a patch before this one?
Yep, will do in the next version.
Thanks,
Namhyung
>
> > Remove the limitation and update the man page.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/Documentation/perf-annotate.txt | 1 -
> > tools/perf/builtin-annotate.c | 5 --
> > tools/perf/ui/browsers/annotate.c | 6 +++
> > tools/perf/util/annotate.c | 61 +++++++++++++++++++---
> > 4 files changed, 61 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/Documentation/perf-annotate.txt b/tools/perf/Documentation/perf-annotate.txt
> > index 46090c5b42b4762f..547f1a2680185e3c 100644
> > --- a/tools/perf/Documentation/perf-annotate.txt
> > +++ b/tools/perf/Documentation/perf-annotate.txt
> > @@ -170,7 +170,6 @@ include::itrace.txt[]
> >
> > --code-with-type::
> > Show data type info in code annotation (for memory instructions only).
> > - Currently it only works with --stdio option.
> >
> >
> > SEE ALSO
> > diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> > index 5d57d2913f3d9a33..646f43b0f7c4c9b0 100644
> > --- a/tools/perf/builtin-annotate.c
> > +++ b/tools/perf/builtin-annotate.c
> > @@ -917,11 +917,6 @@ int cmd_annotate(int argc, const char **argv)
> > symbol_conf.annotate_data_sample = true;
> > } else if (annotate_opts.code_with_type) {
> > symbol_conf.annotate_data_member = true;
> > -
> > - if (!annotate.use_stdio) {
> > - pr_err("--code-with-type only works with --stdio.\n");
> > - goto out_delete;
> > - }
> > }
> >
> > setup_browser(true);
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 23bea5b165774ae7..cdee1969f3131a7c 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -4,6 +4,7 @@
> > #include "../ui.h"
> > #include "../../util/annotate.h"
> > #include "../../util/debug.h"
> > +#include "../../util/debuginfo.h"
> > #include "../../util/dso.h"
> > #include "../../util/hist.h"
> > #include "../../util/sort.h"
> > @@ -1101,6 +1102,9 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> >
> > ui_helpline__push("Press ESC to exit");
> >
> > + if (annotate_opts.code_with_type)
> > + browser.dbg = debuginfo__new(dso__long_name(dso));
> > +
> > browser.b.width = notes->src->widths.max_line_len;
> > browser.b.nr_entries = notes->src->nr_entries;
> > browser.b.entries = ¬es->src->source;
> > @@ -1111,6 +1115,8 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> >
> > ret = annotate_browser__run(&browser, evsel, hbt);
> >
> > + if (annotate_opts.code_with_type)
> > + debuginfo__delete(browser.dbg);
> > if (not_annotated && !notes->src->tried_source)
> > annotated_source__purge(notes->src);
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index d69e406c1bc289cd..06ddc7a9f58722a4 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1362,6 +1362,11 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
> > };
> > struct annotation_line *al;
> >
> > + if (annotate_opts.code_with_type) {
> > + evsel__get_arch(apd->evsel, &apd->arch);
> > + apd->dbg = debuginfo__new(dso__long_name(map__dso(apd->he->ms.map)));
> > + }
> > +
> > list_for_each_entry(al, ¬es->src->source, node) {
> > if (annotation_line__filter(al))
> > continue;
> > @@ -1370,6 +1375,9 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
> > wops.first_line = false;
> > }
> >
> > + if (annotate_opts.code_with_type)
> > + debuginfo__delete(apd->dbg);
> > +
> > return 0;
> > }
> >
> > @@ -1743,7 +1751,7 @@ static double annotation_line__max_percent(struct annotation_line *al,
> > return percent_max;
> > }
> >
> > -static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> > +static int disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> > void *obj, char *bf, size_t size,
> > void (*obj__printf)(void *obj, const char *fmt, ...),
> > void (*obj__write_graph)(void *obj, int graph))
> > @@ -1771,8 +1779,8 @@ static void disasm_line__write(struct disasm_line *dl, struct annotation *notes,
> > obj__printf(obj, " ");
> > }
> >
> > - disasm_line__scnprintf(dl, bf, size, !annotate_opts.use_offset,
> > - notes->src->widths.max_ins_name);
> > + return disasm_line__scnprintf(dl, bf, size, !annotate_opts.use_offset,
> > + notes->src->widths.max_ins_name);
> > }
> >
> > static void ipc_coverage_string(char *bf, int size, struct annotation *notes)
> > @@ -2116,11 +2124,52 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> >
> > width -= printed + 3;
> >
> > - disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf), obj__printf, obj__write_graph);
> > + printed = disasm_line__write(disasm_line(al), notes, obj, bf, sizeof(bf),
> > + obj__printf, obj__write_graph);
> > +
> > + obj__printf(obj, "%s", bf);
> > + width -= printed;
> > +
> > + if (annotate_opts.code_with_type && apd->dbg) {
> > + struct annotated_data_type *data_type;
> > + int offset = 0;
> > +
> > + data_type = __hist_entry__get_data_type(apd->he, apd->arch,
> > + apd->dbg,
> > + disasm_line(al),
> > + &offset);
> > + if (data_type && data_type != NO_TYPE) {
> > + char member[256];
> > +
> > + printed = scnprintf(bf, sizeof(bf),
> > + "\t\t# data-type: %s",
> > + data_type->self.type_name);
> >
> > - obj__printf(obj, "%-*s", width, bf);
> > + if (data_type != &stackop_type &&
> > + data_type != &canary_type &&
> > + sizeof(bf) > (size_t)printed) {
> > + printed += scnprintf(bf + printed,
> > + sizeof(bf) - printed,
> > + " +%#x", offset);
> > + }
> > +
> > + if (annotated_data_type__get_member_name(data_type,
> > + member,
> > + sizeof(member),
> > + offset) &&
> > + sizeof(bf) > (size_t)printed) {
> > + printed += scnprintf(bf + printed,
> > + sizeof(bf) - printed,
> > + " (%s)", member);
> > + }
> >
> > - (void)apd;
> > + obj__printf(obj, "%-*s", width, bf);
> > + } else {
> > + obj__printf(obj, "%-*s", width, " ");
> > + }
> > + } else {
> > + obj__printf(obj, "%-*s", width, " ");
> > + }
> > }
> >
> > }
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary
2025-07-26 0:44 ` Ian Rogers
@ 2025-07-28 18:51 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:51 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:44:33PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > It's mostly unnecessary to print when it has no actual type information
> > like in the stack operations and canary. Let's have them if -v option
> > is given.
> >
> > Before:
> > $ perf annotate --code-with-type
> > ...
> > : 0 0xd640 <_dl_relocate_object>:
> > 0.00 : 0: endbr64
> > 0.00 : 4: pushq %rbp # data-type: (stack operation)
> > 0.00 : 5: movq %rsp, %rbp
> > 0.00 : 8: pushq %r15 # data-type: (stack operation)
> > 0.00 : a: pushq %r14 # data-type: (stack operation)
> > 0.00 : c: pushq %r13 # data-type: (stack operation)
> > 0.00 : e: pushq %r12 # data-type: (stack operation)
> > 0.00 : 10: pushq %rbx # data-type: (stack operation)
>
> I believe the intent in the dwarf is to say where the caller's callee
> saves are, but the stack slots should just be saved and restored and
> won't be used for anything interesting, perhaps for exception
> handling. An annotation like:
> # caller's RBX at stack frame offset -48
> could perhaps be useful.
The main purpose of the annotation of the stack operation is to sync
with the output of data type profiling. As it can report lots of
accesses were from the stack operation, this can answer where they are.
>
> > 0.00 : 11: subq $0xf8, %rsp
> > ...
> > 0.00 : d4: testl %eax, %eax
> > 0.00 : d6: jne 0xf424
> > 0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
> > 0.00 : e3: testq %rbx, %rbx
> > 0.00 : e6: jne 0xf2dd
> > 0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
> > ...
> >
> > After:
> > : 0 0xd640 <_dl_relocate_object>:
> > 0.00 : 0: endbr64
> > 0.00 : 4: pushq %rbp
> > 0.00 : 5: movq %rsp, %rbp
> > 0.00 : 8: pushq %r15
> > 0.00 : a: pushq %r14
> > 0.00 : c: pushq %r13
> > 0.00 : e: pushq %r12
> > 0.00 : 10: pushq %rbx
> > 0.00 : 11: subq $0xf8, %rsp
> > ...
> > 0.00 : d4: testl %eax, %eax
> > 0.00 : d6: jne 0xf424
> > 0.00 : dc: movq 0xf0(%r14), %rbx # data-type: struct link_map +0xf0
> > 0.00 : e3: testq %rbx, %rbx
> > 0.00 : e6: jne 0xf2dd
> > 0.00 : ec: cmpq $0, 0xf8(%r14) # data-type: struct link_map +0xf8
> > ...
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> > tools/perf/util/annotate.c | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 06ddc7a9f58722a4..6fc07971631ac8a3 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -765,6 +765,17 @@ __hist_entry__get_data_type(struct hist_entry *he, struct arch *arch,
> > struct debuginfo *dbg, struct disasm_line *dl,
> > int *type_offset);
> >
> > +static bool needs_type_info(struct annotated_data_type *data_type)
> > +{
> > + if (data_type == NULL || data_type == NO_TYPE)
> > + return false;
> > +
> > + if (verbose)
> > + return true;
>
> lgtm given the many overloaded meanings of verbose.
:-)
>
> Reviewed-by: Ian Rogers <irogers@google.com>
Thanks for your careful review!
Namhyung
> > +
> > + return (data_type != &stackop_type) && (data_type != &canary_type);
> > +}
> > +
> > static int
> > annotation_line__print(struct annotation_line *al, struct annotation_print_data *apd,
> > struct annotation_options *opts, int printed,
> > @@ -844,7 +855,7 @@ annotation_line__print(struct annotation_line *al, struct annotation_print_data
> >
> > data_type = __hist_entry__get_data_type(apd->he, apd->arch,
> > apd->dbg, dl, &offset);
> > - if (data_type && data_type != NO_TYPE) {
> > + if (needs_type_info(data_type)) {
> > char buf[4096];
> >
> > printf("\t\t# data-type: %s",
> > @@ -2138,7 +2149,7 @@ void annotation_line__write(struct annotation_line *al, struct annotation *notes
> > apd->dbg,
> > disasm_line(al),
> > &offset);
> > - if (data_type && data_type != NO_TYPE) {
> > + if (needs_type_info(data_type)) {
> > char member[256];
> >
> > printed = scnprintf(bf, sizeof(bf),
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper
2025-07-26 0:46 ` Ian Rogers
@ 2025-07-28 18:54 ` Namhyung Kim
0 siblings, 0 replies; 26+ messages in thread
From: Namhyung Kim @ 2025-07-28 18:54 UTC (permalink / raw)
To: Ian Rogers
Cc: Arnaldo Carvalho de Melo, Kan Liang, Jiri Olsa, Adrian Hunter,
Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users
On Fri, Jul 25, 2025 at 05:46:48PM -0700, Ian Rogers wrote:
> On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > It'd be great if it can get the correct debug information using DSO
> > build-Id not just the path name. Instead of adding new callsites of
> > debuginfo__new(), let's add dso__debuginfo() which can hide the access
> > using the pathname and help the future conversion.
> >
> > Suggested-by: Ian Rogers <irogers@google.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> :-) and my prior review comments now make less sense. I think putting
> the ui__warning into dso__debugingo makes sense, wdyt?
I think it depends on what the caller does. This code is just to get
the debuginfo from DSO and it cannot know if it's ok to print messages.
So it'd be better for callers to handle the case IMHO.
Thanks,
Namhyung
> > ---
> > tools/perf/ui/browsers/annotate.c | 4 ++--
> > tools/perf/util/annotate.c | 6 +++---
> > tools/perf/util/dso.h | 10 ++++++++++
> > 3 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> > index 2a4db5bdcdb7e9d8..54610621c5f910fe 100644
> > --- a/tools/perf/ui/browsers/annotate.c
> > +++ b/tools/perf/ui/browsers/annotate.c
> > @@ -1042,7 +1042,7 @@ static int annotate_browser__run(struct annotate_browser *browser,
> > case 'T':
> > annotate_opts.code_with_type ^= 1;
> > if (browser->dbg == NULL)
> > - browser->dbg = debuginfo__new(dso__long_name(map__dso(ms->map)));
> > + browser->dbg = dso__debuginfo(map__dso(ms->map));
> > annotate_browser__show(&browser->b, title, help);
> > annotate_browser__debuginfo_warning(browser);
> > continue;
> > @@ -1128,7 +1128,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
> > ui_helpline__push("Press ESC to exit");
> >
> > if (annotate_opts.code_with_type)
> > - browser.dbg = debuginfo__new(dso__long_name(dso));
> > + browser.dbg = dso__debuginfo(dso);
> >
> > browser.b.width = notes->src->widths.max_line_len;
> > browser.b.nr_entries = notes->src->nr_entries;
> > diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> > index 6fc07971631ac8a3..05eb19b110ab7dcf 100644
> > --- a/tools/perf/util/annotate.c
> > +++ b/tools/perf/util/annotate.c
> > @@ -1270,7 +1270,7 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
> > apd.addr_fmt_width = annotated_source__addr_fmt_width(¬es->src->source,
> > apd.start);
> > evsel__get_arch(evsel, &apd.arch);
> > - apd.dbg = debuginfo__new(filename);
> > + apd.dbg = dso__debuginfo(dso);
> >
> > list_for_each_entry(pos, ¬es->src->source, node) {
> > int err;
> > @@ -1375,7 +1375,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
> >
> > if (annotate_opts.code_with_type) {
> > evsel__get_arch(apd->evsel, &apd->arch);
> > - apd->dbg = debuginfo__new(dso__long_name(map__dso(apd->he->ms.map)));
> > + apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
> > }
> >
> > list_for_each_entry(al, ¬es->src->source, node) {
> > @@ -2888,7 +2888,7 @@ struct annotated_data_type *hist_entry__get_data_type(struct hist_entry *he)
> > di_cache.dso = dso__get(map__dso(ms->map));
> >
> > debuginfo__delete(di_cache.dbg);
> > - di_cache.dbg = debuginfo__new(dso__long_name(di_cache.dso));
> > + di_cache.dbg = dso__debuginfo(di_cache.dso);
> > }
> >
> > if (di_cache.dbg == NULL) {
> > diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
> > index 7df1673f08d3ddb4..fd8e95de77f78dfc 100644
> > --- a/tools/perf/util/dso.h
> > +++ b/tools/perf/util/dso.h
> > @@ -10,6 +10,7 @@
> > #include <stdio.h>
> > #include <linux/bitops.h>
> > #include "build-id.h"
> > +#include "debuginfo.h"
> > #include "mutex.h"
> > #include <internal/rc_check.h>
> >
> > @@ -914,4 +915,13 @@ u64 dso__findnew_global_type(struct dso *dso, u64 addr, u64 offset);
> > bool perf_pid_map_tid(const char *dso_name, int *tid);
> > bool is_perf_pid_map_name(const char *dso_name);
> >
> > +/*
> > + * In the future, we may get debuginfo using build-ID (w/o path).
> > + * Add this helper is for the smooth conversion.
> > + */
> > +static inline struct debuginfo *dso__debuginfo(struct dso *dso)
> > +{
> > + return debuginfo__new(dso__long_name(dso));
> > +}
> > +
> > #endif /* __PERF_DSO */
> > --
> > 2.50.1
> >
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2025-07-28 18:54 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-25 19:37 [PATCHSET v4 0/9] perf annotate: Support --code-with-type on TUI Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 1/9] perf annotate: Rename to __hist_entry__tui_annotate() Namhyung Kim
2025-07-26 0:16 ` Ian Rogers
2025-07-28 18:39 ` Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 2/9] perf annotate: Remove __annotation_line__write() Namhyung Kim
2025-07-26 0:20 ` Ian Rogers
2025-07-28 18:42 ` Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 3/9] perf annotate: Pass annotation_print_data to annotation_line__write() Namhyung Kim
2025-07-26 0:23 ` Ian Rogers
2025-07-28 18:44 ` Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 4/9] perf annotate: Simplify width calculation in annotation_line__write() Namhyung Kim
2025-07-26 0:30 ` Ian Rogers
2025-07-28 18:47 ` Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 5/9] perf annotate: Add --code-with-type support for TUI Namhyung Kim
2025-07-26 0:31 ` Ian Rogers
2025-07-28 18:47 ` Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 6/9] perf annotate: Add 'T' hot key to toggle data type display Namhyung Kim
2025-07-26 0:34 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 7/9] perf annotate: Show warning when debuginfo is not available Namhyung Kim
2025-07-26 0:38 ` Ian Rogers
2025-07-25 19:37 ` [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary Namhyung Kim
2025-07-26 0:44 ` Ian Rogers
2025-07-28 18:51 ` Namhyung Kim
2025-07-25 19:37 ` [PATCH v4 9/9] perf annotate: Add dso__debuginfo() helper Namhyung Kim
2025-07-26 0:46 ` Ian Rogers
2025-07-28 18:54 ` Namhyung Kim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).