[6/8] perf annotate: Ensure init/exit for global options

  Now it only cares about the global options so it can just handle it
without the argument.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-annotate.c |  8 ++++----
 tools/perf/builtin-report.c   |  8 ++++----
 tools/perf/builtin-top.c      |  8 ++++----
 tools/perf/util/annotate.c    | 19 +++++++++++--------
 tools/perf/util/annotate.h    |  8 ++++----
 5 files changed, 27 insertions(+), 24 deletions(-)
  

On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Now it only cares about the global options so it can just handle it
> without the argument.

If annotate_opts were accessed by a function then you could
pthread_once the initialization on the first call to get
annotate_opts. Removing annotation_options__init/exit would remove
some potential for error.

Thanks,
Ian


> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-annotate.c |  8 ++++----
>  tools/perf/builtin-report.c   |  8 ++++----
>  tools/perf/builtin-top.c      |  8 ++++----
>  tools/perf/util/annotate.c    | 19 +++++++++++--------
>  tools/perf/util/annotate.h    |  8 ++++----
>  5 files changed, 27 insertions(+), 24 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 87af95634879..9b3dd456a1ee 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -616,13 +616,13 @@ int cmd_annotate(int argc, const char **argv)
>         set_option_flag(options, 0, "show-total-period", PARSE_OPT_EXCLUSIVE);
>         set_option_flag(options, 0, "show-nr-samples", PARSE_OPT_EXCLUSIVE);
>
> -       annotation_options__init(&annotate_opts);
> +       annotation_options__init();
>
>         ret = hists__init();
>         if (ret < 0)
>                 return ret;
>
> -       annotation_config__init(&annotate_opts);
> +       annotation_config__init();
>
>         argc = parse_options(argc, argv, options, annotate_usage, 0);
>         if (argc) {
> @@ -652,7 +652,7 @@ int cmd_annotate(int argc, const char **argv)
>                         return -ENOMEM;
>         }
>
> -       if (annotate_check_args(&annotate_opts) < 0)
> +       if (annotate_check_args() < 0)
>                 return -EINVAL;
>
>  #ifdef HAVE_GTK2_SUPPORT
> @@ -733,7 +733,7 @@ int cmd_annotate(int argc, const char **argv)
>  #ifndef NDEBUG
>         perf_session__delete(annotate.session);
>  #endif
> -       annotation_options__exit(&annotate_opts);
> +       annotation_options__exit();
>
>         return ret;
>  }
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index bc0d986c1e0c..17fb171e898b 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1430,7 +1430,7 @@ int cmd_report(int argc, const char **argv)
>          */
>         symbol_conf.keep_exited_threads = true;
>
> -       annotation_options__init(&annotate_opts);
> +       annotation_options__init();
>
>         ret = perf_config(report__config, &report);
>         if (ret)
> @@ -1464,7 +1464,7 @@ int cmd_report(int argc, const char **argv)
>                         return -ENOMEM;
>         }
>
> -       if (annotate_check_args(&annotate_opts) < 0) {
> +       if (annotate_check_args() < 0) {
>                 ret = -EINVAL;
>                 goto exit;
>         }
> @@ -1696,7 +1696,7 @@ int cmd_report(int argc, const char **argv)
>                          */
>                         symbol_conf.priv_size += sizeof(u32);
>                 }
> -               annotation_config__init(&annotate_opts);
> +               annotation_config__init();
>         }
>
>         if (symbol__init(&session->header.env) < 0)
> @@ -1750,7 +1750,7 @@ int cmd_report(int argc, const char **argv)
>         zstd_fini(&(session->zstd_data));
>         perf_session__delete(session);
>  exit:
> -       annotation_options__exit(&annotate_opts);
> +       annotation_options__exit();
>         free(sort_order_help);
>         free(field_order_help);
>         return ret;
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 60399e4233ee..0de963ca3196 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1608,7 +1608,7 @@ int cmd_top(int argc, const char **argv)
>         if (status < 0)
>                 return status;
>
> -       annotation_options__init(&annotate_opts);
> +       annotation_options__init();
>
>         annotate_opts.min_pcnt = 5;
>         annotate_opts.context  = 4;
> @@ -1660,7 +1660,7 @@ int cmd_top(int argc, const char **argv)
>         if (status)
>                 goto out_delete_evlist;
>
> -       if (annotate_check_args(&annotate_opts) < 0)
> +       if (annotate_check_args() < 0)
>                 goto out_delete_evlist;
>
>         if (!top.evlist->core.nr_entries) {
> @@ -1786,7 +1786,7 @@ int cmd_top(int argc, const char **argv)
>         if (status < 0)
>                 goto out_delete_evlist;
>
> -       annotation_config__init(&annotate_opts);
> +       annotation_config__init();
>
>         symbol_conf.try_vmlinux_path = (symbol_conf.vmlinux_name == NULL);
>         status = symbol__init(NULL);
> @@ -1839,7 +1839,7 @@ int cmd_top(int argc, const char **argv)
>  out_delete_evlist:
>         evlist__delete(top.evlist);
>         perf_session__delete(top.session);
> -       annotation_options__exit(&annotate_opts);
> +       annotation_options__exit();
>
>         return status;
>  }
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index daff9af552f4..626ff3baeb85 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -3416,8 +3416,10 @@ static int annotation__config(const char *var, const char *value, void *data)
>         return 0;
>  }
>
> -void annotation_options__init(struct annotation_options *opt)
> +void annotation_options__init(void)
>  {
> +       struct annotation_options *opt = &annotate_opts;
> +
>         memset(opt, 0, sizeof(*opt));
>
>         /* Default values. */
> @@ -3428,16 +3430,15 @@ void annotation_options__init(struct annotation_options *opt)
>         opt->percent_type = PERCENT_PERIOD_LOCAL;
>  }
>
> -
> -void annotation_options__exit(struct annotation_options *opt)
> +void annotation_options__exit(void)
>  {
> -       zfree(&opt->disassembler_style);
> -       zfree(&opt->objdump_path);
> +       zfree(&annotate_opts.disassembler_style);
> +       zfree(&annotate_opts.objdump_path);
>  }
>
> -void annotation_config__init(struct annotation_options *opt)
> +void annotation_config__init(void)
>  {
> -       perf_config(annotation__config, opt);
> +       perf_config(annotation__config, &annotate_opts);
>  }
>
>  static unsigned int parse_percent_type(char *str1, char *str2)
> @@ -3491,8 +3492,10 @@ int annotate_parse_percent_type(const struct option *opt __maybe_unused, const c
>         return err;
>  }
>
> -int annotate_check_args(struct annotation_options *args)
> +int annotate_check_args(void)
>  {
> +       struct annotation_options *args = &annotate_opts;
> +
>         if (args->prefix_strip && !args->prefix) {
>                 pr_err("--prefix-strip requires --prefix\n");
>                 return -1;
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index 857c5fa0e6b1..4283eb4522b2 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -428,14 +428,14 @@ static inline int symbol__tui_annotate(struct map_symbol *ms __maybe_unused,
>  }
>  #endif
>
> -void annotation_options__init(struct annotation_options *opt);
> -void annotation_options__exit(struct annotation_options *opt);
> +void annotation_options__init(void);
> +void annotation_options__exit(void);
>
> -void annotation_config__init(struct annotation_options *opt);
> +void annotation_config__init(void);
>
>  int annotate_parse_percent_type(const struct option *opt, const char *_str,
>                                 int unset);
>
> -int annotate_check_args(struct annotation_options *args);
> +int annotate_check_args(void);
>
>  #endif /* __PERF_ANNOTATE_H */
> --
> 2.43.0.rc1.413.gea7ed67945-goog
>
  

On Tue, Nov 28, 2023 at 11:21 AM Ian Rogers <irogers@google.com> wrote:
>
> On Tue, Nov 28, 2023 at 9:54 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Now it only cares about the global options so it can just handle it
> > without the argument.
>
> If annotate_opts were accessed by a function then you could
> pthread_once the initialization on the first call to get
> annotate_opts. Removing annotation_options__init/exit would remove
> some potential for error.

Currently all call sites (perf annotate, report and top) initialize the
options and check if it has conflicting options before running the
commands.  So I'm not sure if it needs pthread_once() for that.

Thanks,
Namhyung