Re: [PATCH v4 8/9] perf annotate: Hide data-type for stack operation and canary

[Namhyung Kim <namhyung@kernel.org>]

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
> >