Re: [PATCH v4 1/2] perf script: Fix perf script -F +metric

[Namhyung Kim <namhyung@kernel.org>]

Hi Andi,

On Sat, Jul 06, 2024 at 12:18:56PM -0700, Andi Kleen wrote:
> This fixes a regression with perf script -F +metric originally caused by :
> 
> commit 37cc8ad77cf81f3ffd226856c367b0e15333a738
> Author: Ian Rogers <irogers@google.com>
> Date:   Sun Feb 19 01:28:46 2023 -0800
> 
>     perf metric: Directly use counts rather than saved_value
> 
> In the perf script environment the evsel wouldn't allocate an aggr
> values array, which led to a -1 reference because the metric
> evaluation would try to reference NULL - 1 (for aggr_idx)
> 
> Give the perf script evsels a single CPU aggr setup. That's
> enough because the groups are always contiguous, so no need
> to store more than one CPU's worth of values.
> 
> Before
> 
> % perf record -e '{cycles,instructions}:S' perf bench  mem memcpy
> % perf script -F +metric
> Segmentation fault (core dumped)
> 
> After:
> 
> % perf record -e '{cycles,instructions}:S' perf bench  mem memcpy
> ...
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.028 MB perf.data (90 samples) ]
> % perf script -F +metric
>        perf-exec 1847557 264658.180789:       3009       cycles:  ffffffff990a579a native_write_msr+0xa ([kernel.kallsyms])
>        perf-exec 1847557 264658.180789:        382 instructions:  ffffffff990a579a native_write_msr+0xa ([kernel.kallsyms])
>        perf-exec 1847557 264658.180789:         metric:    0.13  insn per cycle
> ...
> 
> Fixes: 37cc8ad77cf8 ("perf metric: Directly use counts rather ...")
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

This makes perf stat test fail.

  $ sudo ./perf test -v 104
  104: perf stat tests:
  --- start ---
  test child forked, pid 2762796
  Basic stat command test
  Basic stat command test [Success]
  stat record and report test
  stat record and report test [Success]
  stat record and script test
  stat record and script test [Failed]
  stat repeat weak groups test
  stat repeat weak groups test [Success]
  Topdown event group test
  Topdown event group test [Skipped event parsing failed]
  Topdown weak groups test
  Topdown weak groups test [Skipped event parsing failed]
  cputype test
  cputype test [Success]
  ---- end(-1) ----
  104: perf stat tests                                                 : FAILED!

And it gets a segfault to run manually.

  $ ./perf stat record -o- true | ./perf script -i-
  Segmentation fault

Thanks,
Namhyung

> 
> ----
> 
> v2: Reformat code
> v3: Avoid warning on some compilers
> ---
>  tools/perf/builtin-script.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index c16224b1fef3..95ba3f3cff60 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -2133,12 +2133,14 @@ static void perf_sample__fprint_metric(struct perf_script *script,
>  	if (evsel_script(leader)->gnum++ == 0)
>  		perf_stat__reset_shadow_stats();
>  	val = sample->period * evsel->scale;
> +	/* Always use CPU 0 storage because the groups are contiguous. */
> +	evsel->stats->aggr[0].counts.val = val;
>  	evsel_script(evsel)->val = val;
>  	if (evsel_script(leader)->gnum == leader->core.nr_members) {
>  		for_each_group_member (ev2, leader) {
>  			perf_stat__print_shadow_stats(&stat_config, ev2,
>  						      evsel_script(ev2)->val,
> -						      sample->cpu,
> +						      0,
>  						      &ctx,
>  						      NULL);
>  		}
> @@ -4084,6 +4086,7 @@ int cmd_script(int argc, const char **argv)
>  
>  	perf_set_singlethreaded();
>  
> +	stat_config.aggr_map = &(struct cpu_aggr_map){ .nr = 1 };
>  	setup_scripting();
>  
>  	argc = parse_options_subcommand(argc, argv, options, script_subcommands, script_usage,
> -- 
> 2.45.2
>