Re: [PATCH] perf script: add --addr2line option

[Ian Rogers]

On Thu, Jul 18, 2024 at 7:30 AM <m.liska@xxxxxxxxxx> wrote:
>
> From: Martin Liska <martin.liska@xxxxxxx>
>
> Similarly to other subcommands (like report, top), it would be handy to
> provide a path for addr2line command.

Thanks Martin, lgtm but you did put a Signed-off-by tag in your commit
message. There is an option in git commit and git format-patch to add
this for you.

> ---
>  tools/perf/Documentation/perf-script.txt |  3 +++
>  tools/perf/builtin-script.c              | 10 +++++++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
> index ff086ef05a0c..5abb960c4960 100644
> --- a/tools/perf/Documentation/perf-script.txt
> +++ b/tools/perf/Documentation/perf-script.txt
> @@ -369,6 +369,9 @@ OPTIONS
>  --demangle-kernel::
>         Demangle kernel symbol names to human readable form (for C++ kernels).
>
> +--addr2line=<path>::
> +       Path to addr2line binary.
> +
>  --header
>         Show perf.data header.
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c16224b1fef3..301ea1c98e36 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -3897,7 +3897,7 @@ int cmd_script(int argc, const char **argv)
>         };
>         struct utsname uts;
>         char *script_path = NULL;
> -       const char *dlfilter_file = NULL;
> +       const char *dlfilter_file = NULL, *addr2line_path = NULL;
>         const char **__argv;
>         int i, j, err = 0;
>         struct perf_script script = {
> @@ -4052,6 +4052,8 @@ int cmd_script(int argc, const char **argv)
>                         "Enable symbol demangling"),
>         OPT_BOOLEAN(0, "demangle-kernel", &symbol_conf.demangle_kernel,
>                         "Enable kernel symbol demangling"),
> +       OPT_STRING(0, "addr2line", &addr2line_path, "path",

Thinking out loud. I'm kind of wondering why we use a local variable
and not just &symbol_conf.addr2line_path here. I see you've used the
same logic that is elsewhere like builtin-top.c, so I think it is
okay.

Thanks,
Ian

> +                       "addr2line binary to use for line numbers"),
>         OPT_STRING(0, "time", &script.time_str, "str",
>                    "Time span of interest (start,stop)"),
>         OPT_BOOLEAN(0, "inline", &symbol_conf.inline_name,
> @@ -4138,6 +4140,12 @@ int cmd_script(int argc, const char **argv)
>             itrace_synth_opts.callchain_sz > scripting_max_stack)
>                 scripting_max_stack = itrace_synth_opts.callchain_sz;
>
> +       if (addr2line_path) {
> +               symbol_conf.addr2line_path = strdup(addr2line_path);
> +               if (!symbol_conf.addr2line_path)
> +                       return -ENOMEM;
> +       }
> +
>         /* make sure PERF_EXEC_PATH is set for scripts */
>         set_argv_exec_path(get_argv_exec_path());
>
> --
> 2.45.2
>