Re: [PATCH v3 3/3] perf jevents: Check the JSON keys that jevents.py never reads

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf jevents: module-scope variable e in __main__ breaks
  pylint W0621
- [Low] perf jevents: empty tasks check in main() skips arch std errors
  and arch=none
--

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

perf jevents: Check the JSON keys that jevents.py never reads

This commit adds a --check mode to jevents.py (along with a jevents-check
make target and shell test) to report unread JSON keys that closely match
known key names and metrics missing a MetricExpr.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index f4dca47eb49b2..3381ade201fd9 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py

[ ... ]

> @@ -1510,7 +1729,12 @@ model must include the implementor such as "arm/cortex-a34".''',
>      if item.name == _args.arch or _args.arch == 'all' or item.name == 'test' or item.name == 'common':
>        archs.append(item.name)
>  
> -  if len(archs) < 2 and _args.arch != 'none':
> +  if _args.check and _args.arch not in ('all', 'none') and _args.arch not in [
> +      item.name for item in os.scandir(_args.starting_dir) if item.is_dir()]:
> +    print(f'jevents: error: no architecture directory named \'{_args.arch}\' '
> +          f'under \'{_args.starting_dir}\'', file=sys.stderr)
> +    sys.exit(2)
> +  if not archs and _args.arch != 'none':
>      raise IOError(f'Missing architecture directory \'{_args.arch}\'')
>  
>    archs.sort()
> @@ -1522,16 +1746,51 @@ model must include the implementor such as "arm/cortex-a34".''',
>      if item.is_file() and item.name.endswith('.json') and not item.name.endswith('metricgroups.json'):
>        tasks.append((item.path, get_topic(item.name)))
>  
> -  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
> +  failed: List[Tuple[str, Exception]] = []
> +  try:
> +    for arch in archs:
> +      arch_path = f'{_args.starting_dir}/{arch}'
> +      preprocess_arch_std_files(arch_path, failed if _args.check else None)
> +      ftw(arch_path, [], collect_json)
> +
> +    if not tasks:
> +      print(f'jevents: error: no event table under \'{_args.starting_dir}\' '
> +            f'matched arch \'{_args.arch}\'', file=sys.stderr)
> +      sys.exit(2 if _args.check else 1)

[Severity: Low]
Does exiting early when tasks is empty discard errors and mistyped keys
already collected from architecture standard files, and break arch='none'
builds?

collect_json() only queues JSON files in subdirectories (len(parents) > 0).
If an architecture directory only contains top-level arch std JSON files,
exiting here in --check mode skips reporting any parse failures in failed
or mistyped keys in _json_cache from preprocess_arch_std_files().  In build
mode, when _args.arch == 'none' is used on a directory without common or
test subdirectories, this exits with status 1 instead of generating empty C
tables as the _args.arch != 'none' check above allows.

[ ... ]

> @@ -1608,4 +1869,16 @@ struct pmu_table_entry {
>      _args.output_string_file.close()
>  
>  if __name__ == '__main__':
> -  main()
> +  try:
> +    main()
> +  except SystemExit:
> +    raise
> +  except KeyboardInterrupt:
> +    raise
> +  except BaseException as e:
> +    # A build should see the traceback; a check gives its caller a status,
> +    # and 2 is the one that says the tool could not do its job.
> +    if _args is not None and getattr(_args, 'check', False):
> +      print(f'jevents: error: {e}', file=sys.stderr)
> +      sys.exit(2)
> +    raise

[Severity: Medium]
Does binding e in the top-level __main__ block break the perf build when
pylint is enabled?

Because if __name__ == '__main__': runs at module scope, binding e here
causes pylint to report W0621 (redefined-outer-name) on every function and
lambda in jevents.py that uses a local variable or argument named e, such as
add_events_table_entries():

tools/perf/pmu-events/jevents.py:add_events_table_entries() {
    ...
  for e in read_json_events(item.path, topic):
    ...
}

The default perf build runs pylint on jevents.py with E and W enabled
whenever pylint >= 2.16.0 is installed, so these W0621 warnings fail the
pylint-pmu-events build target.  Moving this try/except inside main() or
using a distinct name like _err avoids shadowing.

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