Re: [PATCHSET 0/7] perf annotate: Add --code-with-type option

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Mar 05, 2025 at 03:31:45PM -0800, Ian Rogers wrote:
> On Wed, Mar 5, 2025 at 3:27 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Mon, Mar 3, 2025 at 9:38 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > There are roughly two modes in the perf annotate.  One is normal code
> > > anootation and the other is data type annotation.  I'm proposing a new
> >
> > nit: s/anootation/annotation/

Oops.

> >
> > > way to combine the code and data annotation together.
> > >
> > > With this option, "# data-type: <name> [offset (field)]" part will be
> > > adeded when it found a data type for the given instruction.  This is
> >
> > nit: s/adeded/added/

Thanks for pointing this out.

> >
> > > for every instruction in the function regardless whether it has a
> > > sample or not.
> > >
> > > This will be useful to understand function level data access patterns.
> > > Currently it only works with --stdio, but I can add it to TUI later.
> > >
> > > Here's an example output.
> >
> > At some point I think we might as well have our own disassembler. The
> > annotations remind me of the relocation and other annotation output of
> > objdump.
> >
> > >   $ perf annotate --code-with-type --stdio
> > >   ...
> > >  Percent |      Source code & Disassembly of elf for cpu/mem-loads,ldlat=30/P (4 samples, percent: local period)
> > >   ----------------------------------------------------------------------------------------------------------------
> > >            : 0                0xffffffff81bb7060 <__schedule>:
> > >       0.00 :   ffffffff81bb7060:        pushq   %rbp              # data-type: (stack operation)
> > >       0.00 :   ffffffff81bb7061:        movq    %rsp, %rbp
> > >       0.00 :   ffffffff81bb7064:        pushq   %r15              # data-type: (stack operation)
> > >       0.00 :   ffffffff81bb7066:        pushq   %r14              # data-type: (stack operation)
> > >       0.00 :   ffffffff81bb7068:        pushq   %r13              # data-type: (stack operation)
> > >       0.00 :   ffffffff81bb706a:        pushq   %r12              # data-type: (stack operation)
> > >       0.00 :   ffffffff81bb706c:        pushq   %rbx              # data-type: (stack operation)
> >
> > data-type: looks a bit weird here. I wonder if the
> > .cfi_offset/.cfi_restore could  make this information richer, maybe:
> >        0.00 :   ffffffff81bb7060:        pushq   %rbp              # *(frame-16) = caller rbp
> >        0.00 :   ffffffff81bb7061:        movq    %rsp, %rbp
> >        0.00 :   ffffffff81bb7064:        pushq   %r15              # *(frame-24) = caller r15

Probably possible, but it's also obvious from the instruction.
I didn't try to be smart here as we're interested in types.

> >
> > >       0.00 :   ffffffff81bb706d:        movq    $0x33180, %rbx
> > >       0.00 :   ffffffff81bb7074:        movq    %rbx, %r12
> > >       0.00 :   ffffffff81bb7077:        subq    $0x38, %rsp
> > >       0.00 :   ffffffff81bb707b:        movl    %edi, -0x5c(%rbp)         # data-type: unsigned int +0
> > >       0.00 :   ffffffff81bb707e:        movq    %gs:0x28, %rax            # data-type: (stack canary)
> > >       0.00 :   ffffffff81bb7087:        movq    %rax, -0x30(%rbp)         # data-type: (stack canary)
> > >       0.00 :   ffffffff81bb708b:        xorl    %eax, %eax
> > >       0.00 :   ffffffff81bb708d:        movq    $0x0, -0x58(%rbp)         # data-type: struct rq_flags +0 (flags)
> > >       0.00 :   ffffffff81bb7095:        movq    $0x0, -0x50(%rbp)         # data-type: struct rq_flags +0x8 (clock_update_flags)
> > >       0.00 :   ffffffff81bb709d:        callq   0xffffffff81baa100 <debug_smp_processor_id>               # data-type: (stack operation)
> > >       0.00 :   ffffffff81bb70a2:        cltq
> > >       0.00 :   ffffffff81bb70a4:        addq    -0x7dcf0500(,%rax,8), %r12                # data-type: long unsigned int[] +0
> > >       0.00 :   ffffffff81bb70ac:        movq    0x960(%r12), %r13         # data-type: struct rq +0x960 (curr)
> > >       0.00 :   ffffffff81bb70b4:        movq    0x20(%r13), %rax          # data-type: struct task_struct +0x20 (stack)
> > >       0.00 :   ffffffff81bb70b8:        cmpq    $0x57ac6e9d, (%rax)
> > >       0.00 :   ffffffff81bb70bf:        jne     0xffffffff81bb7bca <__schedule+0xb6a>
> > >       0.00 :   ffffffff81bb70c5:        movl    %gs:0x7e47b87c(%rip), %eax  # 0x32948 <pcpu_hot+0x8>              # data-type: struct pcpu_hot +0x8 (preempt_count)
> >
> > The variable name in the struct is really nice, it is a shame the same
> > information gets repeated a few times on the line.

I guess you meant 'pcpu_hot'.  But the assembly shows the name of the
global variable and the data type shows the name of struct.  It happened
to be the same in this case.

> 
> Outside of the typos in the cover letter, the rest of the patches lgtm.
> 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks for your review!
Namhyung

> 
> > >       0.00 :   ffffffff81bb70cc:        andl    $0x7fffffff, %eax
> > >       0.00 :   ffffffff81bb70d1:        cmpl    $0x1, %eax
> > >       0.00 :   ffffffff81bb70d4:        jne     0xffffffff81bb79de <__schedule+0x97e>
> > >       0.00 :   ffffffff81bb70da:        nopl    (%rax,%rax)
> > >       0.00 :   ffffffff81bb70df:        cmpl    $0x2, 0xe86b3a(%rip)  # 0xffffffff82a3dc20 <prof_on>              # data-type: int +0
> > >       0.00 :   ffffffff81bb70e6:        movq    0x8(%rbp), %rsi
> > >       0.00 :   ffffffff81bb70ea:        je      0xffffffff81bb7a0b <__schedule+0x9ab>
> > >       0.00 :   ffffffff81bb70f0:        nopl    (%rax,%rax)
> > >       0.00 :   ffffffff81bb70f5:        nop
> > >       ...
> > >
> > > The code is also available at 'perf/annotate-code-data-v1' branch in
> > > git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git
> > >
> > > Thanks,
> > > Namhyung
> > >
> > >
> > > Namhyung Kim (7):
> > >   perf annotate-data: Add annotated_data_type__get_member_name()
> > >   perf annotate: Remove unused len parameter from
> > >     annotation_line__print()
> > >   perf annotate: Pass annotation_options to annotation_line__print()
> > >   perf annotate: Pass hist_entry to annotate functions
> > >   perf annotate: Factor out __hist_entry__get_data_type()
> > >   perf annotate: Implement code + data type annotation
> > >   perf annotate: Add --code-with-type option.
> > >
> > >  tools/perf/Documentation/perf-annotate.txt |   4 +
> > >  tools/perf/builtin-annotate.c              |  19 +-
> > >  tools/perf/builtin-top.c                   |   2 +-
> > >  tools/perf/util/annotate-data.c            |  34 +++
> > >  tools/perf/util/annotate-data.h            |   3 +
> > >  tools/perf/util/annotate.c                 | 267 +++++++++++++--------
> > >  tools/perf/util/annotate.h                 |   8 +-
> > >  tools/perf/util/sort.c                     |  39 +--
> > >  8 files changed, 236 insertions(+), 140 deletions(-)
> > >
> > > --
> > > 2.48.1.711.g2feabab25a-goog
> > >