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

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=fWJxCq97Oss3NXbXEKHeNXbif9-yWvktNRQvwtm3H3jbA@mail.gmail.com>
Date: Fri, 25 Jul 2025 17:44:33 -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 v4 8/9] perf annotate: Hide data-type for stack operation
 and canary

On Fri, Jul 25, 2025 at 12:38 PM Namhyung Kim <namhyung@...nel.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@...nel.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@...gle.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
>