Re: [PATCH] perf report: Fix some option handling on --stdio

Arnaldo Carvalho de Melo
2015-05-13T16:07:31Z

Em Thu, May 14, 2015 at 12:03:26AM +0900, Namhyung Kim escreveu:
> There's a bug that perf report sometimes ignore some options on --stdio
> output. This bug is triggered only if a related config variable is
> set. For example, let's assume we have a following config file.

Testing, please next time indicate against what branch this is to be
applied, as I thought about perf/core, but it doesn't apply there, so
tried perf/urgent, where it applies.

- Arnaldo

> $ cat ~/.perfconfig
> [call-graph]
> print-type = graph
> [hist]
> percentage = absolute
>
> Then, following perf config will not honor some options.
>
> $ perf record -ag sleep 1
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.199 MB perf.data (77 samples) ]
>
> $ perf report -g none --stdio
> # To display the perf.data header info, please use --header/--header-only options.
> #
> # Samples: 77 of event 'cycles'
> # Event count (approx.): 25425383
> #
> # Overhead Command Shared Object Symbol
> # ........ ............... ....................... ..............
> #
> 16.34% swapper [kernel.vmlinux] [k] intel_idle
> |
> ---intel_idle
> cpuidle_enter_state
> cpuidle_enter
> cpu_startup_entry
> ...
>
> With '-g none' option, it should not show callchains, but it still shows
> callchains. However it works as expected on --tui output.
>
> Similarly, '--percentage relative' option is not work and still shows a
> absolute percentage values.
>
> Looking at the source, I found that those setting were overwritten by
> config variables when setup_pager() called. The setup_pager() is to
> start a pager process so that it can manage long lines of output on the
> stdio mode. But as it calls the perf_config() after parsing arguments,
> the settings were overwritten regardless of command line options.
>
> The reason it calls perf_config() is to find the 'pager_program' which
> might be set by a config variable, I guess. However current perf code
> does not provide the config variable for it, so it's just meaningless
> IMHO. Eliminating the call makes the option working as expected.
>
> Cc: Taeung Song <treeze.taeung@gmail.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
> tools/perf/util/cache.h | 1 -
> tools/perf/util/environment.c | 1 -
> tools/perf/util/pager.c | 5 -----
> 3 files changed, 7 deletions(-)
>
> diff --git a/tools/perf/util/cache.h b/tools/perf/util/cache.h
> index fbcca21d66ab..c861373aaed3 100644
> --- a/tools/perf/util/cache.h
> +++ b/tools/perf/util/cache.h
> @@ -30,7 +30,6 @@ extern const char *perf_config_dirname(const char *, const char *);
>
> /* pager.c */
> extern void setup_pager(void);
> -extern const char *pager_program;
> extern int pager_in_use(void);
> extern int pager_use_color;
>
> diff --git a/tools/perf/util/environment.c b/tools/perf/util/environment.c
> index 275b0ee345f5..7405123692f1 100644
> --- a/tools/perf/util/environment.c
> +++ b/tools/perf/util/environment.c
> @@ -5,5 +5,4 @@
> */
> #include "cache.h"
>
> -const char *pager_program;
> int pager_use_color = 1;
> diff --git a/tools/perf/util/pager.c b/tools/perf/util/pager.c
> index 31ee02d4e988..53ef006a951c 100644
> --- a/tools/perf/util/pager.c
> +++ b/tools/perf/util/pager.c
> @@ -50,11 +50,6 @@ void setup_pager(void)
>
> if (!isatty(1))
> return;
> - if (!pager) {
> - if (!pager_program)
> - perf_config(perf_default_config, NULL);
> - pager = pager_program;
> - }
> if (!pager)
> pager = getenv("PAGER");
> if (!(pager || access("/usr/bin/pager", X_OK)))
> --
> 2.4.0