Re: [PATCH v2 1/3] perf jevents: Name the metric when a MetricExpr is missing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf jevents: output files opened before reading tables in
  main()
--

commit e40b6a0ff5427b090df6921a9048cd1849e4b0f0
Author: TAKANO Mitsuhiro <takano32@gmail.com>

perf jevents: Name the metric when a MetricExpr is missing

This commit raises a RuntimeError naming the metric when a JSON metric is
missing its MetricExpr, improves error reporting when parsing JSON tables,
and replaces argparse.FileType with explicit open() calls for output files.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 080e04b51b8c5..86060a228b755 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> @@ -1490,12 +1496,29 @@ such as "arm/cortex-a34".''',
>        type=dir_path,
>        help='Root of tree containing architecture directories containing json files'
>    )
> -  ap.add_argument(
> -      'output_file', type=argparse.FileType('w', encoding='utf-8'), nargs='?', default=sys.stdout)
> -  ap.add_argument(
> -      'output_string_file', type=argparse.FileType('w', encoding='utf-8'), nargs='?', default=None)
> +  ap.add_argument('output_file', nargs='?', default=None,
> +                  help='file to write the tables to, default standard output')
> +  ap.add_argument('output_string_file', nargs='?', default=None,
> +                  help='file to write big_c_string to when it is to be kept '
> +                       'apart from the tables')
>    _args = ap.parse_args()
>  
> +  # Opened here, and not by argparse, so that a failure before the tables are
> +  # written cannot empty a file the caller named.
> +  if _args.output_file in (None, '-'):
> +    _args.output_file = sys.stdout
> +  else:
> +    try:
> +      _args.output_file = open(_args.output_file, 'w', encoding='utf-8')
> +    except OSError as e:
> +      ap.error(f'argument output_file: {e}')
> +  if _args.output_string_file == '-':
> +    _args.output_string_file = sys.stdout
> +  elif _args.output_string_file is not None:
> +    try:
> +      _args.output_string_file = open(_args.output_string_file, 'w', encoding='utf-8')
> +    except OSError as e:
> +      ap.error(f'argument output_string_file: {e}')
>    _args.output_file.write(f"""/* SPDX-License-Identifier: GPL-2.0 */
>  /* THIS FILE WAS AUTOGENERATED BY `jevents.py arch={_args.arch} model={_args.model}` ! */
>  """)

[Severity: Low]
Does opening _args.output_file and _args.output_string_file immediately
after ap.parse_args() still truncate existing output files when reading a
JSON table fails?

preprocess_arch_std_files(), _parallel_read_json_events(), and
preprocess_one_file() all run later in main(), after the files are opened
with 'w' and the header is written:

tools/perf/pmu-events/jevents.py:main() {
    ...
  for arch in archs:
    arch_path = f'{_args.starting_dir}/{arch}'
    preprocess_arch_std_files(arch_path)
    ftw(arch_path, [], collect_json)

  with concurrent.futures.ProcessPoolExecutor(max_workers=_parallel_worker_count(len(tasks)),
                                              initializer=_init_worker,
                                              initargs=(_arch_std_events,)) as executor:
    for path, topic, events in executor.map(_parallel_read_json_events, tasks):
      _json_cache[(path, topic)] = events

  for arch in archs:
    arch_path = f'{_args.starting_dir}/{arch}'
    ftw(arch_path, [], preprocess_one_file)
    ...
}

Any exception while parsing the tables (such as the new RuntimeError for a
missing MetricExpr) will still leave _args.output_file truncated to the
header and _args.output_string_file empty, contradicting the comment and
commit message.  Commit "perf jevents: Check the JSON keys that jevents.py
never reads" later in the series moves preprocess_arch_std_files() and
_parallel_read_json_events() above the open() calls, though
preprocess_one_file() (which parses metricgroups.json) and _bcs.compute()
still run after the files are truncated.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111023448.1970467-1-takano32@gmail.com?part=1