linux-kernel - Re: [PATCH v3 5/8] perf annotate: Add --code-with-type support for TUI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fV=E4_9RVvf1CW0GM0VY+ubr8sOvnXc+xhGW66PhMFCnA@mail.gmail.com>
Date: Wed, 16 Jul 2025 13:42:13 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, 
	Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, 
	LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH v3 5/8] perf annotate: Add --code-with-type support for TUI

On Wed, Jul 16, 2025 at 10:27 AM Namhyung Kim <namhyung@...nel.org> wrote:
>
> On Wed, Jul 16, 2025 at 08:00:37AM -0700, Ian Rogers wrote:
> > On Tue, Jul 15, 2025 at 10:01 PM Namhyung Kim <namhyung@...nel.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.
> > >
> > > Remove the limitation and update the man page.
> > >
> > > Signed-off-by: Namhyung Kim <namhyung@...nel.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 9833c2c82a2fee46..6debd725392db4a4 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 = &notes->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)));
> >
> > This API looks unfortunate. A dso may have a long name (it'd be easier
> > to understand if this were called path rather than long name) or a
> > build ID. The API isn't the fault of this change, but I thought I'd
> > mention as we move toward greater use of build IDs.
>
> Are you talking about build-id in MMAP2?  I think it's build-id vs. (dev
> major/minor + inode) and the long name should be available always.

I'm thinking of work I'm doing with build IDs like (unmerged):
https://lore.kernel.org/lkml/20250628045017.1361563-1-irogers@google.com/
Even with mmap2 events the filename shouldn't be necessary as the
build ID should be preferred - if you profile remotely there may be a
file name/path collision on a local machine, but it should be unlikely
for a build ID collision. In those patches the dso_id is changed to
instead of considering inode numbers using build IDs when possible. It
was already the case that the name of a dso could be changed, which
affects sorting. In general I think we should move away from file
paths, inodes and the like as the build IDs will avoid races, work
across systems, etc. I think in this case we could add:
```
apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map))))
```
or possibly just change `apd->dbg` to be the dso and grab the
debuginfo from the dso when needed. For now the dso__debuginfo would
be something like:
```
struct debuginfo *dso__debuginfo(struct dso *dso)
{
     debuginfo__delete(dso->debuginfo);
     dso->debuginfo = debuginfo__new(dso__long_name(dso));
     return dso->debuginfo;
}
```
but in the future we can find the debuginfo off of the build ID and paths, etc.

Thanks,
Ian
> Thanks,
> Namhyung
>