[PATCH 0/3] perf jevents: Report the JSON keys that jevents.py never reads

[PATCH 0/3] perf jevents: Report the JSON keys that jevents.py never reads

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key spelled differently is dropped without a word
and the object is built without whatever that field held.

Patches 1 and 2 make the generator name what went wrong, and let the
model argument be omitted. Patch 3 adds a --check mode that reports the
keys jevents.py never asks for, along with a make target and a perf test
so that it runs without anyone having to remember it.

This is the second posting of the check. It went out alone in November
(https://lore.kernel.org/linux-perf-
users/20261110193239.1741014-1-takano32@gmail.com/) and the review found
two ways it could call a tree clean that no build can use: a MetricName
with a dash in it that loses its MetricExpr was invisible to both the
reader and the report, and a metric without an expression was skipped
rather than reported unless its key was spelled close enough to be a near
miss. Both are fixed here, and the failure-path changes the check needs
are no longer mixed into it.

The test runs the check over a fixture table rather than over the tree,
so it is green whatever the tables happen to look like, and it checks the
three exit statuses: 0 for a clean table, 1 for a reported key, 2 for a
table that does not parse.

The two misspellings the check reports in the tree today were posted
separately in November:

https://lore.kernel.org/linux-perf-users/20261108184600.3810059-1-takano32@gmail.com/
https://lore.kernel.org/linux-perf-users/20261108183846.3804875-1-takano32@gmail.com/

TAKANO Mitsuhiro (3):
  perf jevents: Name the metric when a MetricExpr is missing
  perf jevents: Make the model argument optional
  perf jevents: Check the JSON keys that jevents.py never reads

 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 394 +++++++++++++++++++++---
 tools/perf/tests/shell/jevents_check.sh |  68 ++++
 4 files changed, 428 insertions(+), 49 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

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

[TAKANO Mitsuhiro]

jevents.py reads each field it wants out of a JSON object by name, so a
MetricExpr key that is missing or spelled another way leaves the object
with a MetricName and no expression. Two different failures followed,
neither of which named the metric: the rewrite in _read_json_events_impl()
raised AttributeError on the None, and a metric that got that far reached
print_pending_metrics(), whose assert fired on it.

Raise in the reader instead, naming the metric, before either of them can
happen. The check is not inside the '-' test that decides whether a metric
takes part in the rewrite: a name with a dash in it can lose its
expression the same way.

Two smaller changes on the same path:

- An architecture standard table that does not parse now carries the
   reason in its message ("Failure processing '<file>' in '<dir>':
   <cause>").  The traceback used to end with the file name alone, and
   only the chained exception said why.

- A table that does not parse is reported on stderr with a "jevents:
   error:" prefix instead of on stdout, where it was mixed with the
   generated output.

And the output file is opened after the tables have been read, and not by
argparse, so a build that fails while reading them leaves a file the
caller named as it was instead of truncating it to the header.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/pmu-events/jevents.py | 35 ++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..86060a228b75 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -460,11 +460,17 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f"Exception processing {path}")
+    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
+    if event.metric_name and event.metric_expr is None:
+      # A MetricName without a MetricExpr leaves nothing to put in the
+      # generated tables, whether or not the name has a dash, so the build
+      # must not go on.  Name the metric instead of letting the table printer
+      # assert on the None.
+      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -493,7 +499,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-        raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\'') from e
+      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -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}` ! */
 """)
-- 
2.53.0

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

[sashiko-bot]

> jevents.py reads each field it wants out of a JSON object by name, so a
> MetricExpr key that is missing or spelled another way leaves the object
> with a MetricName and no expression. Two different failures followed,
> neither of which named the metric: the rewrite in _read_json_events_impl()
> raised AttributeError on the None, and a metric that got that far reached
> print_pending_metrics(), whose assert fired on it.
> 
> Raise in the reader instead, naming the metric, before either of them can
> happen. The check is not inside the '-' test that decides whether a metric
> takes part in the rewrite: a name with a dash in it can lose its
> expression the same way.
> 
> Two smaller changes on the same path:
> 
> - An architecture standard table that does not parse now carries the
>    reason in its message ("Failure processing '<file>' in '<dir>':
>    <cause>").  The traceback used to end with the file name alone, and
>    only the chained exception said why.
> 
> - A table that does not parse is reported on stderr with a "jevents:
>    error:" prefix instead of on stdout, where it was mixed with the
>    generated output.
> 
> And the output file is opened after the tables have been read, and not by
> argparse, so a build that fails while reading them leaves a file the
> caller named as it was instead of truncating it to the header.
> 
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH 2/3] perf jevents: Make the model argument optional

[TAKANO Mitsuhiro]

The model argument carries a default of "all", but a positional argument
with a default and no nargs is still required: "jevents.py x86 <dir>" exits
with "the following arguments are required: model", and so does any caller
that only wants the tables for an architecture.

Make it nargs='?' so the default is reachable and an omitted model means
"all".  Builds pass it explicitly, so the generated tables do not change.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/pmu-events/jevents.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 86060a228b75..844fa82887bb 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -1486,10 +1486,10 @@ def main() -> None:
 
   ap = argparse.ArgumentParser()
   ap.add_argument('arch', help='Architecture name like x86')
-  ap.add_argument('model', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all". For architectures like
-ARM64 with an implementor/model, the model must include the implementor
-such as "arm/cortex-a34".''',
+  ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
+reduce the code size.  Normally set to "all", and that is what an omitted
+model means. For architectures like ARM64 with an implementor/model, the
+model must include the implementor such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
-- 
2.53.0

Re: [PATCH 2/3] perf jevents: Make the model argument optional

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf jevents: missing Fixes trailer for unreachable model
  default
--

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

> perf jevents: Make the model argument optional
>
> The model argument carries a default of "all", but a positional argument
> with a default and no nargs is still required: "jevents.py x86 <dir>" exits
> with "the following arguments are required: model", and so does any caller
> that only wants the tables for an architecture.
>
> Make it nargs='?' so the default is reachable and an omitted model means
> "all".  Builds pass it explicitly, so the generated tables do not change.
>
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

[Severity: Low]
Since 5a09b1fd1b1f ("perf jevents: Add model list option") added the
positional model argument with default='all' without nargs='?', should this
include:

  Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")

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

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

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name:
jd.get('BriefDescription'), 'EventCode' in jd, and the names in
event_fields. A key spelled another way is dropped without a word, and
the object is built without whatever that field held.

Two of those are in the tree now. "BriefDescript6ion" is in one entry
each of amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-
store.json, and since those objects carry no other description,
ls_inef_sw_pref.all is generated with an empty text field.
"PublicDescrition" was in 16 of the 28 entries of
arm64/ampere/ampereonex/mmu.json; nothing a user saw changed there,
because each of the 16 also had a BriefDescription with the same text,
which is why the typo could sit there unnoticed.

Add a --check mode that reads every architecture, not just the one being
built: a typo is cheapest to fix by whoever reads that table next, and
that reader may be building something else. It records the keys
JsonEvent was never asked for and reports the ones whose spelling is a
near miss for a name jevents.py does read (difflib ratio 0.8). In this
tree 52 distinct key names occur, jevents.py reads 46 of them, the two
misspellings score 0.97, and the highest score among the remaining
unread names is 0.78 (Counter against CounterMask), so any cutoff in
that gap reports the same two.

It also reports a metric that carries a name but no expression, by
metric name and however the lost MetricExpr key was spelled: the key
report needs the spelling to be close to a name jevents.py reads, and a
metric without its expression cannot be generated at all.

The tracking is off unless --check asked for it, so a build parses each
object exactly as before and the generated tables are byte-identical
(x86, arm64 and all architectures compared against the same tree without
this patch). The check itself reads the 973 json files under arch/ and
takes seconds of CPU.

Exit status is 0 when nothing was reported, 1 when something was, and 2
when a table could not be read or the command line was wrong; make folds
both failures into its own status, so a script that needs the difference
runs jevents.py directly. Status 0 means the tables under that directory
were read and no unread key looks like a name jevents.py reads. It is
not a promise that a build will succeed: mapfile.csv coverage, the model
that selects a subset of the tables, and the C generation are not part
of it.

It is not a schema either: it checks nothing about types, required
fields or value ranges, and looks at key names only, so a misspelled
description in a value is out of scope. It does not read mapfile.csv,
which has no keys, or metricgroups.json, whose keys are group names.
Names that only metric.py or the C code read are outside it too, so an
"Expermental" key is not reported, and file names and topics are not
keys.

The check runs from the new make target "make -C tools/perf jevents-
check" (JEVENTS_CHECK_ARCH=x86 narrows it to one architecture) and from
perf test: tests/shell/jevents_check.sh runs it over a fixture table and
checks the status for a clean table (0), a misspelled key (1) and a
table that does not parse (2), and skips with status 2 when pmu-events/
is not next to the binary.

Written with Hermes Agent, an LLM assistant, which found the
misspellings and wrote this check, its test and this changelog. I built
perf with it, ran the perf test above, ran the check over the whole
tree, compared the generated tables with those from an unpatched tree on
x86, arm64 and all architectures at once, and ran mypy on jevents.py.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 383 ++++++++++++++++++++----
 tools/perf/tests/shell/jevents_check.sh |  68 +++++
 4 files changed, 411 insertions(+), 55 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 00baf90e5bda..0b13f1e12899 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -140,6 +140,18 @@ check-headers:
 sync-headers:
 	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
 
+# The pmu-events tables are data, so a key nothing reads is invisible until a
+# description goes missing.  Named in the wrapper like check-headers: this is
+# a data check, not a build, so it needs neither config nor fixdep and reads
+# the source tree whatever O= says.  JEVENTS_CHECK_ARCH=x86 reads x86 and the
+# common and test tables every build reads.
+# Makefile.perf picks the interpreter for the build; the wrapper does not
+# include it, so default here and let PYTHON= win when it is given.
+JEVENTS_CHECK_ARCH ?= all
+PYTHON ?= python3
+jevents-check:
+	@$(PYTHON) ./pmu-events/jevents.py --check '$(JEVENTS_CHECK_ARCH)' all pmu-events/arch
+
 #
 # All other targets get passed through:
 #
@@ -147,4 +159,4 @@ sync-headers:
 	$(print_msg)
 	$(make)
 
-.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers
+.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers jevents-check
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b74b534e1153..a330cd9de0fc 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -871,6 +871,7 @@ help:
 	@echo 'Perf maintainer targets:'
 	@echo '  clean			- clean all binary objects and build output'
 	@echo '  check-headers		- check which header copies are out of sync with the kernel'
+	@echo '  jevents-check		- report the pmu-events json keys that jevents.py never reads'
 	@echo '  sync-headers		- sync those header copies with the kernel, creating commits'
 	@echo '			  (SYNC_HEADERS_ARGS="..." passes options to the script)'
 
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 844fa82887bb..3063282599a0 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -3,15 +3,16 @@
 """Convert directories of JSON events to C code."""
 import argparse
 import csv
+import difflib
 import json
 import metric
 import os
 import sys
-from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
+from typing import (Any, Callable, Dict, List, Optional, Sequence, Set, Tuple)
 import collections
 
 # Global command line arguments.
-_args = None
+_args: Optional[argparse.Namespace] = None
 # List of regular event tables.
 _event_tables: list[str] = []
 # List of event tables generated from "/sys" directories.
@@ -190,11 +191,78 @@ class BigCString:
 
 _bcs = BigCString()
 
+# Names that some object in this process has been asked for.  Used only to
+# find the spelling an unread name was meant to be.  The set is per process
+# and the workers of the pool never share it, so a name is in it once some
+# object in the same worker has been built; every object asks for the same
+# names, so they are there before the first object is reported, but a read
+# that moved behind a condition would make a report depend on which worker
+# happened to build which file.
+_read_keys: Set[str] = set()
+
+# Unread name -> the names it could be a misspelling of, or () for none.  A table
+# repeats the same few names tens of thousands of times, and the name that
+# misses is the common case (Counter, many thousands of objects), so the
+# misses are kept too; _mark empties the cache when a name is read for the
+# first time, so a hint computed while _read_keys was still growing cannot
+# stick.
+_hints: Dict[str, Tuple[str, ...]] = {}
+
+# Whether the objects are wrapped at all.  Only the check reads what the
+# tracking records, so a build does not wrap anything.  Set in
+# main() and, because the workers may be fresh interpreters, passed to them
+# through _init_worker() rather than read from _args there.
+_track: bool = False
+
+
+class _JsonObject(dict):
+  """A JSON object that remembers which of its keys were looked at.
+
+  jevents.py picks the fields it wants out of each object by name and
+  ignores the rest, so a key it never names - a misspelled
+  "BriefDescript6ion", say - is dropped without a word and the event
+  silently loses its description.  Handing the parsed object to this
+  class and asking it afterwards which keys were never looked at makes
+  the misspelling visible.  Only get(), [] and 'in' are tracked: a read
+  through items(), values() or pop() would not be, so a new read of a
+  field belongs in one of the three.  The alternative, a list of the key names
+  jevents.py reads, would be a second copy of the code below that goes
+  stale the first time someone adds a field.
+  """
+
+  def __init__(self, jd: dict):
+    super().__init__(jd)
+    # dict.keys() rather than the object itself: iterating the object would
+    # go through __iter__, should that ever be hooked below.
+    self.unread: Set[str] = set(dict.keys(jd))
+
+  def _mark(self, key: str) -> None:
+    self.unread.discard(key)
+    if key not in _read_keys:
+      _read_keys.add(key)
+      _hints.clear()
+
+  def get(self, key: Any, default: Any = None) -> Any:
+    self._mark(key)
+    return dict.get(self, key, default)
+
+  def __getitem__(self, key: Any) -> Any:
+    self._mark(key)
+    return dict.__getitem__(self, key)
+
+  def __contains__(self, key: object) -> bool:
+    if isinstance(key, str):
+      self._mark(key)
+    return dict.__contains__(self, key)
+
+
 class JsonEvent:
   """Representation of an event loaded from a json file dictionary."""
 
   def __init__(self, jd: dict):
     """Constructor passed the dictionary of parsed json values."""
+    if _track:
+      jd = _JsonObject(jd)
 
     def llx(x: int) -> str:
       """Convert an int to a string similar to a printf modifier of %#llx."""
@@ -416,6 +484,42 @@ class JsonEvent:
         raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
 
     self.event = real_event(self.name, event)
+    # Keys of the JSON object that no code above looked at, set last so that
+    # the copy of an architecture standard event above does not overwrite it.
+    # A name that is a near miss for one the code reads, and whose name the
+    # object does not carry with the same text, is almost certainly a
+    # misspelling of it: the object lost the field the reader was looking for.
+    # A name that is not a near miss is left alone, because a table is free to
+    # carry metadata jevents.py has no use for.
+    self.mistyped: Optional[Dict[str, Tuple[str, ...]]] = None
+    if not _track:
+      return
+    assert isinstance(jd, _JsonObject)
+    unread = jd.unread
+    if unread:
+      found = {}
+      for key in unread:
+        if key not in _hints:
+          lowered = {name.lower(): name for name in _read_keys}
+          # Two names can be equally close - RetirementLatencyMax and its Min -
+          # and naming one of them would be a coin toss presented as an answer.
+          match = difflib.get_close_matches(key.lower(), sorted(lowered), n=2, cutoff=0.8)
+          _hints[key] = tuple(lowered[m] for m in match)
+        hints = _hints[key]
+        if not hints:
+          continue
+        # Nothing was lost if the object carries one of those names with the
+        # same text: that is a table with the field twice, not a misspelling.
+        # dict.__contains__ so that the look is not recorded as a read.
+        lost = True
+        for name in hints:
+          if dict.__contains__(jd, name) and dict.get(jd, key) == dict.get(jd, name):
+            lost = False
+            break
+        if lost:
+          found[key] = hints
+      if found:
+        self.mistyped = found
 
   def __repr__(self) -> str:
     """String representation primarily for debugging."""
@@ -453,6 +557,83 @@ class JsonEvent:
     return f'{make_comment(s)}\t{{ { _bcs.offsets[s] } }},\n'
 
 
+def _report_mistyped_keys() -> bool:
+  """Report the JSON keys that jevents.py did not look for.
+
+  jevents.py takes the fields it wants out of each object by name.  A key
+  spelled differently from the name it looks for is never read, the object is
+  built without whatever the field held, and the build says nothing; that is
+  how "BriefDescript6ion" in the AMD tables and "PublicDescrition" in the
+  AmpereOneX mmu table got in.  A name that is a near miss for one jevents.py
+  reads is reported, with the spelling it was probably meant to be and the
+  files carrying it.  A name that is not a near miss is not reported: a
+  table may carry metadata for another reader, and reporting it would mean
+  keeping a list of such names in this file that every vendor adding one
+  would have to edit.
+
+  A table that cannot be parsed is reported the same way and does not stop
+  the rest of the tree from being read, so one broken file does not hide the
+  misspellings in the other 1000.
+  """
+  assert _args is not None
+  found: Dict[str, collections.Counter] = {}
+  hints: Dict[str, Tuple[str, ...]] = {}
+  objects = 0
+  for (path, _), events in _json_cache.items():
+    for event in events:
+      if not event.mistyped:
+        continue
+      objects += 1
+      for key, hint in event.mistyped.items():
+        found.setdefault(key, collections.Counter())[path] += 1
+        hints.setdefault(key, hint)
+  if not found:
+    return False
+
+  paths = {path for files in found.values() for path in files}
+  start = _args.starting_dir
+  print(f'jevents: error: {objects} object(s) in '
+        f'{len(paths)} file(s) carry a key that looks like a misspelling:', file=sys.stderr)
+  for key in sorted(found):
+    files = found[key]
+    guess = ' or '.join(repr(name) for name in hints[key])
+    print(f'  {key!r}, did you mean {guess}? ({sum(files.values())} object(s) in '
+          f'{len(files)} file(s)):', file=sys.stderr)
+    where = sorted(os.path.relpath(path, start) for path in files)
+    for path in where[:5]:
+      print(f'    {path}', file=sys.stderr)
+    if len(where) > 5:
+      print(f'    ... and {len(where) - 5} more file(s)', file=sys.stderr)
+
+  return True
+
+
+def _report_metrics_without_expr() -> bool:
+  """Report the metrics that carry a name but no expression.
+
+  A MetricExpr key spelled another way is not read, so the object keeps its
+  name and loses its expression.  Unlike the key report above this does not
+  depend on how close a spelling is: a metric the rewrite cannot handle would
+  fail a build however the lost key was spelled.
+  """
+  assert _args is not None
+  start = _args.starting_dir
+  missing = sorted((os.path.relpath(path, start), event.metric_name)
+                   for (path, _), events in _json_cache.items()
+                   for event in events
+                   if event.metric_name and event.metric_expr is None)
+  if not missing:
+    return False
+  print(f'jevents: error: {len(missing)} metric(s) have a name but no '
+        f'expression, so their MetricExpr key is missing or misspelled:',
+        file=sys.stderr)
+  for where, name in missing[:20]:
+    print(f'  {name!r} in {where}', file=sys.stderr)
+  if len(missing) > 20:
+    print(f'    ... and {len(missing) - 20} more metric(s)', file=sys.stderr)
+  return True
+
+
 _json_cache: Dict[Tuple[str, str], Sequence[JsonEvent]] = {}
 def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
   """Read json events from the specified file."""
@@ -460,17 +641,20 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
+    if not _track:
+      print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
     if event.metric_name and event.metric_expr is None:
       # A MetricName without a MetricExpr leaves nothing to put in the
-      # generated tables, whether or not the name has a dash, so the build
-      # must not go on.  Name the metric instead of letting the table printer
-      # assert on the None.
-      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      # generated tables, whether or not the name has a dash, so a build must
+      # not go on.  Name the metric instead of letting the table printer
+      # assert on the None; --check reports it and keeps reading the rest.
+      if not _track:
+        raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      continue
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -487,8 +671,12 @@ def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
     _json_cache[key] = _read_json_events_impl(path, topic)
   return _json_cache[key]
 
-def preprocess_arch_std_files(archpath: str) -> None:
-  """Read in all architecture standard events."""
+def preprocess_arch_std_files(archpath: str, failures: Optional[List[Tuple[str, Exception]]] = None) -> None:
+  """Read in all architecture standard events.
+
+  A failure is raised, or appended to failures when the caller can carry on
+  without the table; either way the tables after it are still read.
+  """
   for item in os.scandir(archpath):
     if not item.is_file() or not item.name.endswith('.json'):
       continue
@@ -499,7 +687,10 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
+        failure = RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}')
+        if failures is None:
+          raise failure from e
+        failures.append((item.path, failure))
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1448,16 +1639,27 @@ def _parallel_worker_count(num_tasks: int) -> int:
   return max(1, min(getattr(os, 'process_cpu_count', os.cpu_count)() or 1,
                     _max_parallel_workers, num_tasks))
 
-def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Sequence[JsonEvent]]:
+def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Any]:
   path, topic = task
-  return path, topic, _read_json_events_impl(path, topic)
-
-def _init_worker(std_events: dict) -> None:
-  global _arch_std_events
+  try:
+    return path, topic, _read_json_events_impl(path, topic)
+  except Exception as e:
+    if not _track:
+      # The build stops here: keep the file and the reason in one message,
+      # the pool adds the traceback.
+      raise RuntimeError(f'Failure processing \'{path}\': {e}') from None
+    # Carried back rather than raised: the parent knows the tree and can
+    # report the file and keep going, which is the point of reading the
+    # tables this build does not use.
+    return path, topic, e
+
+def _init_worker(std_events: dict, track: bool) -> None:
+  global _arch_std_events, _track
   _arch_std_events = std_events
+  _track = track
 
 def main() -> None:
-  global _args
+  global _args, _track
 
   def dir_path(path: str) -> str:
     """Validate path is a directory for argparse."""
@@ -1484,12 +1686,12 @@ def main() -> None:
       if item.is_dir():
         ftw(item.path, list(parents) + [item.name], action)
 
-  ap = argparse.ArgumentParser()
+  ap = argparse.ArgumentParser(description='Generate pmu-events.c from the json tables')
   ap.add_argument('arch', help='Architecture name like x86')
   ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all", and that is what an omitted
-model means. For architectures like ARM64 with an implementor/model, the
-model must include the implementor such as "arm/cortex-a34".''',
+reduce the code size.  Normally set to "all". For architectures like
+ARM64 with an implementor/model, the model must include the implementor
+such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
@@ -1497,14 +1699,104 @@ model must include the implementor such as "arm/cortex-a34".''',
       help='Root of tree containing architecture directories containing json files'
   )
   ap.add_argument('output_file', nargs='?', default=None,
-                  help='file to write the tables to, default standard output')
+                  help='file to write the tables to, default standard output; '
+                       'not written, and not opened, with --check')
   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')
+                       'apart from the tables; ignored with --check')
+  ap.add_argument('--check', action='store_true',
+                  help='read the tables, report the keys that look like a '
+                       'misspelling of a name this file reads on standard '
+                       'error, and exit without writing any table; pass "all" '
+                       'as both arch and model to cover the whole tree.  '
+                       'Nothing is reported when every key is read and every '
+                       'table parses.  The exit status is 0 when the tree is '
+                       'clean, 1 when something was reported and 2 when a '
+                       'table could not be read, so it can gate a script')
   _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.check and (_args.output_file is not None or _args.output_string_file is not None):
+    ap.error('--check writes no table; drop the output arguments')
+  if _args.check:
+    if _args.arch == 'none':
+      ap.error('--check needs an architecture to read')
+    # The model only picks the tables a build needs; a check is about the
+    # tree, so it reads all of it and the model means nothing here.
+    _args.model = 'all'
+    _track = True
+
+  archs = []
+  for item in os.scandir(_args.starting_dir):
+    if not item.is_dir():
+      continue
+    if item.name == _args.arch or _args.arch == 'all' or item.name == 'test' or item.name == 'common':
+      archs.append(item.name)
+
+  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()
+  import concurrent.futures
+  tasks = []
+  def collect_json(parents: Sequence[str], item: os.DirEntry) -> None:
+    if len(parents) == 0:
+      return
+    if item.is_file() and item.name.endswith('.json') and not item.name.endswith('metricgroups.json'):
+      tasks.append((item.path, get_topic(item.name)))
+
+  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)
+
+    with concurrent.futures.ProcessPoolExecutor(max_workers=_parallel_worker_count(len(tasks)),
+                                                initializer=_init_worker,
+                                                initargs=(_arch_std_events, _args.check)) as executor:
+      for path, topic, events in executor.map(_parallel_read_json_events, tasks):
+        if isinstance(events, Exception):
+          if not _args.check:
+            raise RuntimeError(f'Failure processing \'{path}\'') from events
+          failed.append((path, events))
+        else:
+          _json_cache[(path, topic)] = events
+  except (RuntimeError, ValueError, OSError, argparse.ArgumentTypeError) as e:
+    if not _args.check:
+      raise
+    failed.append((_args.starting_dir, e))
+
+  if _args.check:
+    # A table this check does not need is reported, not fatal: the check reads
+    # the whole tree and a caller only asked about one part of it.
+    for path, error in failed:
+      text = str(error)
+      # The architecture standard preprocess names the file and its directory
+      # already, so prefixing the path here only repeats it.
+      if text.startswith('Failure processing'):
+        print(f'jevents: error: {text}', file=sys.stderr)
+      else:
+        print(f'jevents: error: '
+              f'{os.path.relpath(path, _args.starting_dir)}: {text}',
+              file=sys.stderr)
+    reported = _report_mistyped_keys()
+    reported = _report_metrics_without_expr() or reported
+    if failed:
+      sys.exit(2)
+    sys.exit(1 if reported else 0)
+
+  # Opened here, and not by argparse, so that --check leaves the files it was
+  # given alone and a failure before the tables are written cannot empty one.
   if _args.output_file in (None, '-'):
     _args.output_file = sys.stdout
   else:
@@ -1540,35 +1832,6 @@ struct pmu_table_entry {
 };
 
 """)
-  archs = []
-  for item in os.scandir(_args.starting_dir):
-    if not item.is_dir():
-      continue
-    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':
-    raise IOError(f'Missing architecture directory \'{_args.arch}\'')
-
-  archs.sort()
-  import concurrent.futures
-  tasks = []
-  def collect_json(parents: Sequence[str], item: os.DirEntry) -> None:
-    if len(parents) == 0:
-      return
-    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
 
   for arch in archs:
     arch_path = f'{_args.starting_dir}/{arch}'
@@ -1607,4 +1870,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
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..2254bc96352c
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Check that jevents.py reports unread json keys and refuses a broken table
+
+shelldir=$(dirname "$0")
+. "${shelldir}"/lib/setup_python.sh
+
+# The check reads the tables and needs no build and no hardware, but an
+# installed perf has no pmu-events/ next to the binary.
+pmudir=$(readlink -f "${shelldir}/../../pmu-events")
+if [ ! -f "${pmudir}/jevents.py" ] || [ ! -f "${pmudir}/metric.py" ]; then
+	echo "Skipping: no pmu-events/jevents.py under ${pmudir}"
+	exit 2
+fi
+
+workdir=$(mktemp -d /tmp/jevents_check.XXXXXX)
+trap 'rm -rf "${workdir}"' EXIT
+
+# A table with one key jevents.py reads and one event, so that a clean run
+# says nothing and exits 0.
+mkdir -p "${workdir}/arch/x86/model"
+cat > "${workdir}/arch/x86/model/events.json" <<'JSON'
+[
+  {
+    "EventName": "CHECK_TEST_EVENT",
+    "EventCode": "0x1",
+    "BriefDescription": "a table the check should accept"
+  }
+]
+JSON
+
+run_check() {
+	"${PYTHON}" "${pmudir}/jevents.py" --check x86 all "${workdir}/arch"
+}
+
+run_check
+if [ $? -ne 0 ]; then
+	echo "pmu-events json keys [Failed: a clean table was reported]"
+	exit 1
+fi
+
+# A near miss for a name jevents.py reads is reported, and the status is 1.
+sed -i 's/"BriefDescription"/"BriefDescript6ion"/' \
+	"${workdir}/arch/x86/model/events.json"
+output=$(run_check 2>&1)
+ret=$?
+case "${ret}" in
+1) ;;
+*)	echo "pmu-events json keys [Failed: a misspelled key exited ${ret}]"
+	exit 1 ;;
+esac
+if ! echo "${output}" | grep -q "BriefDescript6ion"; then
+	echo "pmu-events json keys [Failed: the misspelled key was not reported]"
+	exit 1
+fi
+
+# A table that does not parse is a failure of the check and not a finding:
+# the status is 2, which this script must not pass on as a skip.
+echo '{ "truncated": ' > "${workdir}/arch/x86/model/broken.json"
+run_check >/dev/null 2>&1
+ret=$?
+if [ "${ret}" -ne 2 ]; then
+	echo "pmu-events json keys [Failed: an unreadable table exited ${ret}]"
+	exit 1
+fi
+
+echo "pmu-events json keys [Success]"
+exit 0
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf jevents: JsonEvent.__init__() suppresses mistyped keys
  matching a sibling hint
--

commit 618a7a7e812ef95bd110f0becbecdb78f73a1397
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, a jevents-check make target,
and a shell test to report unread JSON keys that are near misses for keys
read by jevents.py, as well as metrics missing a MetricExpr.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 844fa82887bbf..3063282599a00 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> @@ -416,6 +484,42 @@ class JsonEvent:
>          raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
>  
>      self.event = real_event(self.name, event)
> +    # Keys of the JSON object that no code above looked at, set last so that
> +    # the copy of an architecture standard event above does not overwrite it.
> +    # A name that is a near miss for one the code reads, and whose name the
> +    # object does not carry with the same text, is almost certainly a
> +    # misspelling of it: the object lost the field the reader was looking for.
> +    # A name that is not a near miss is left alone, because a table is free to
> +    # carry metadata jevents.py has no use for.
> +    self.mistyped: Optional[Dict[str, Tuple[str, ...]]] = None
> +    if not _track:
> +      return
> +    assert isinstance(jd, _JsonObject)
> +    unread = jd.unread
> +    if unread:
> +      found = {}
> +      for key in unread:
> +        if key not in _hints:
> +          lowered = {name.lower(): name for name in _read_keys}
> +          # Two names can be equally close - RetirementLatencyMax and its Min -
> +          # and naming one of them would be a coin toss presented as an answer.
> +          match = difflib.get_close_matches(key.lower(), sorted(lowered), n=2, cutoff=0.8)
> +          _hints[key] = tuple(lowered[m] for m in match)
> +        hints = _hints[key]
> +        if not hints:
> +          continue
> +        # Nothing was lost if the object carries one of those names with the
> +        # same text: that is a table with the field twice, not a misspelling.
> +        # dict.__contains__ so that the look is not recorded as a read.
> +        lost = True
> +        for name in hints:
> +          if dict.__contains__(jd, name) and dict.get(jd, key) == dict.get(jd, name):
> +            lost = False
> +            break

[Severity: Low]
Does setting lost = False when any name in hints has the same value in jd
hide mistyped sibling keys?

Because get_close_matches() collects up to two matches (n=2), hints for a
typo in a closely-named sibling field (such as RetirementLatencyMax alongside
RetirementLatencyMin) contains both sibling names.  If both fields have the
same value on an event object and one key is mistyped, the other valid
sibling in hints matches in jd, setting lost = False so --check silently
misses the typo even though that field was dropped from the generated event.

In addition, json.load() already collapses identically-spelled duplicate JSON
keys into a single dictionary entry that is removed from jd.unread when read,
so any key remaining in jd.unread is spelled differently from every key
jevents.py reads and suppresses a misspelled duplicate key whenever the valid
key has the same value.

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

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

[TAKANO Mitsuhiro]

Both findings are in my tree.

The missing Fixes: tag, which you raised against 2/3: adding nargs='?'
to the model argument is fixing a hole in 5a09b1fd1b1f ("perf jevents:
Add model list option"), so the tag belongs there.  It is in the commit
now, so the next revision of 2/3 carries it.

The sibling hint, which you raised against 3/3: you are right, and the
suppression has to go rather than be narrowed.  get_close_matches(n=2)
on a key such as RetirementLatencyMxi returns both RetirementLatencyMin
and RetirementLatencyMax, and the code kept quiet when the other
candidate was present and carried the same value in its table.  But the
value does not say which of the two names the author meant, and the JSON
object can hold only one of them, so a metric written next to a correct
sibling with the same value was the one case that disappeared: measured
on a table that has RetirementLatencyMin and a misspelled
RetirementLatencyMxi with the same value, the check exited 0 and
reported nothing before the change, and reports the key and exits 1
after it.

Reporting both names is noisier, which is the trade: the report names
the key that was not read, and the two candidates are the nearest names
to it.  A table with no typo produces no output either way.

[PATCH v2 0/3] perf jevents: Report the JSON keys that jevents.py never reads

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key spelled differently is dropped without a word
and the object is built without whatever that field held.

Patches 1 and 2 make the generator name what went wrong, and let the
model argument be omitted. Patch 3 adds a --check mode that reports the
keys jevents.py never asks for, along with a make target and a perf test
so that it runs without anyone having to remember it.

This is the second posting of the check. It went out alone in November
(https://lore.kernel.org/linux-perf-
users/20261110193239.1741014-1-takano32@gmail.com/) and the review found
two ways it could call a tree clean that no build can use: a MetricName
with a dash in it that loses its MetricExpr was invisible to both the
reader and the report, and a metric without an expression was skipped
rather than reported unless its key was spelled close enough to be a near
miss. Both are fixed here, and the failure-path changes the check needs
are no longer mixed into it.

The test runs the check over a fixture table rather than over the tree,
so it is green whatever the tables happen to look like, and it checks the
three exit statuses: 0 for a clean table, 1 for a reported key, 2 for a
table that does not parse.

The two misspellings the check reports in the tree today were posted
separately in November:

https://lore.kernel.org/linux-perf-users/20261108184600.3810059-1-takano32@gmail.com/
https://lore.kernel.org/linux-perf-users/20261108183846.3804875-1-takano32@gmail.com/

TAKANO Mitsuhiro (3):
  perf jevents: Name the metric when a MetricExpr is missing
  perf jevents: Make the model argument optional
  perf jevents: Check the JSON keys that jevents.py never reads

 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 391 +++++++++++++++++++++---
 tools/perf/tests/shell/jevents_check.sh |  68 +++++
 4 files changed, 425 insertions(+), 49 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

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

[TAKANO Mitsuhiro]

jevents.py reads each field it wants out of a JSON object by name, so a
MetricExpr key that is missing or spelled another way leaves the object
with a MetricName and no expression. Two different failures followed,
neither of which named the metric: the rewrite in _read_json_events_impl()
raised AttributeError on the None, and a metric that got that far reached
print_pending_metrics(), whose assert fired on it.

Raise in the reader instead, naming the metric, before either of them can
happen. The check is not inside the '-' test that decides whether a metric
takes part in the rewrite: a name with a dash in it can lose its
expression the same way.

Two smaller changes on the same path:

- An architecture standard table that does not parse now carries the
   reason in its message ("Failure processing '<file>' in '<dir>':
   <cause>").  The traceback used to end with the file name alone, and
   only the chained exception said why.

- A table that does not parse is reported on stderr with a "jevents:
   error:" prefix instead of on stdout, where it was mixed with the
   generated output.

And the output file is opened after the tables have been read, and not by
argparse, so a build that fails while reading them leaves a file the
caller named as it was instead of truncating it to the header.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/pmu-events/jevents.py | 35 ++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..86060a228b75 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -460,11 +460,17 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f"Exception processing {path}")
+    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
+    if event.metric_name and event.metric_expr is None:
+      # A MetricName without a MetricExpr leaves nothing to put in the
+      # generated tables, whether or not the name has a dash, so the build
+      # must not go on.  Name the metric instead of letting the table printer
+      # assert on the None.
+      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -493,7 +499,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-        raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\'') from e
+      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -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}` ! */
 """)
-- 
2.53.0

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

[sashiko-bot]

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

[PATCH v2 2/3] perf jevents: Make the model argument optional

[TAKANO Mitsuhiro]

The model argument carries a default of "all", but a positional argument
with a default and no nargs is still required: "jevents.py x86 <dir>" exits
with "the following arguments are required: model", and so does any caller
that only wants the tables for an architecture.

Make it nargs='?' so the default is reachable and an omitted model means
"all".  Builds pass it explicitly, so the generated tables do not change.

Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/pmu-events/jevents.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 86060a228b75..844fa82887bb 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -1486,10 +1486,10 @@ def main() -> None:
 
   ap = argparse.ArgumentParser()
   ap.add_argument('arch', help='Architecture name like x86')
-  ap.add_argument('model', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all". For architectures like
-ARM64 with an implementor/model, the model must include the implementor
-such as "arm/cortex-a34".''',
+  ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
+reduce the code size.  Normally set to "all", and that is what an omitted
+model means. For architectures like ARM64 with an implementor/model, the
+model must include the implementor such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
-- 
2.53.0

Re: [PATCH v2 2/3] perf jevents: Make the model argument optional

[sashiko-bot]

> The model argument carries a default of "all", but a positional argument
> with a default and no nargs is still required: "jevents.py x86 <dir>" exits
> with "the following arguments are required: model", and so does any caller
> that only wants the tables for an architecture.
> 
> Make it nargs='?' so the default is reachable and an omitted model means
> "all".  Builds pass it explicitly, so the generated tables do not change.
> 
> Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

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

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name:
jd.get('BriefDescription'), 'EventCode' in jd, and the names in
event_fields. A key spelled another way is dropped without a word, and
the object is built without whatever that field held.

Two of those are in the tree now. "BriefDescript6ion" is in one entry
each of amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-
store.json, and since those objects carry no other description,
ls_inef_sw_pref.all is generated with an empty text field.
"PublicDescrition" was in 16 of the 28 entries of
arm64/ampere/ampereonex/mmu.json; nothing a user saw changed there,
because each of the 16 also had a BriefDescription with the same text,
which is why the typo could sit there unnoticed.

Add a --check mode that reads every architecture, not just the one being
built: a typo is cheapest to fix by whoever reads that table next, and
that reader may be building something else. It records the keys
JsonEvent was never asked for and reports the ones whose spelling is a
near miss for a name jevents.py does read (difflib ratio 0.8). In this
tree 52 distinct key names occur, jevents.py reads 46 of them, the two
misspellings score 0.97, and the highest score among the remaining
unread names is 0.78 (Counter against CounterMask), so any cutoff in
that gap reports the same two.

It also reports a metric that carries a name but no expression, by
metric name and however the lost MetricExpr key was spelled: the key
report needs the spelling to be close to a name jevents.py reads, and a
metric without its expression cannot be generated at all.

A key that is a near miss for two names at once is reported as well: the
object may carry one of them with the same value, but that does not say
which one the key was meant to be, and the key is one nothing reads
either way.

The tracking is off unless --check asked for it, so a build parses each
object exactly as before and the generated tables are byte-identical
(x86, arm64 and all architectures compared against the same tree without
this patch). The check itself reads the 973 json files under arch/ and
takes seconds of CPU.

Exit status is 0 when nothing was reported, 1 when something was, and 2
when a table could not be read or the command line was wrong; make folds
both failures into its own status, so a script that needs the difference
runs jevents.py directly. Status 0 means the tables under that directory
were read and no unread key looks like a name jevents.py reads. It is
not a promise that a build will succeed: mapfile.csv coverage, the model
that selects a subset of the tables, and the C generation are not part
of it.

It is not a schema either: it checks nothing about types, required
fields or value ranges, and looks at key names only, so a misspelled
description in a value is out of scope. It does not read mapfile.csv,
which has no keys, or metricgroups.json, whose keys are group names.
Names that only metric.py or the C code read are outside it too, so an
"Expermental" key is not reported, and file names and topics are not
keys.

The check runs from the new make target "make -C tools/perf jevents-
check" (JEVENTS_CHECK_ARCH=x86 narrows it to one architecture) and from
perf test, where tests/shell/jevents_check.sh skips with status 2 when
pmu-events/arch is not next to the binary.

Written with Hermes Agent, an LLM assistant, which found the
misspellings and wrote this check, its test and this changelog. I built
perf with it, ran the perf test above, ran the check over the whole
tree, compared the generated tables with those from an unpatched tree on
x86, arm64 and all architectures at once, and ran mypy on jevents.py.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 380 ++++++++++++++++++++----
 tools/perf/tests/shell/jevents_check.sh |  68 +++++
 4 files changed, 408 insertions(+), 55 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 00baf90e5bda..0b13f1e12899 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -140,6 +140,18 @@ check-headers:
 sync-headers:
 	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
 
+# The pmu-events tables are data, so a key nothing reads is invisible until a
+# description goes missing.  Named in the wrapper like check-headers: this is
+# a data check, not a build, so it needs neither config nor fixdep and reads
+# the source tree whatever O= says.  JEVENTS_CHECK_ARCH=x86 reads x86 and the
+# common and test tables every build reads.
+# Makefile.perf picks the interpreter for the build; the wrapper does not
+# include it, so default here and let PYTHON= win when it is given.
+JEVENTS_CHECK_ARCH ?= all
+PYTHON ?= python3
+jevents-check:
+	@$(PYTHON) ./pmu-events/jevents.py --check '$(JEVENTS_CHECK_ARCH)' all pmu-events/arch
+
 #
 # All other targets get passed through:
 #
@@ -147,4 +159,4 @@ sync-headers:
 	$(print_msg)
 	$(make)
 
-.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers
+.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers jevents-check
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b74b534e1153..a330cd9de0fc 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -871,6 +871,7 @@ help:
 	@echo 'Perf maintainer targets:'
 	@echo '  clean			- clean all binary objects and build output'
 	@echo '  check-headers		- check which header copies are out of sync with the kernel'
+	@echo '  jevents-check		- report the pmu-events json keys that jevents.py never reads'
 	@echo '  sync-headers		- sync those header copies with the kernel, creating commits'
 	@echo '			  (SYNC_HEADERS_ARGS="..." passes options to the script)'
 
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 844fa82887bb..d31f8e2fd6d3 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -3,15 +3,16 @@
 """Convert directories of JSON events to C code."""
 import argparse
 import csv
+import difflib
 import json
 import metric
 import os
 import sys
-from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
+from typing import (Any, Callable, Dict, List, Optional, Sequence, Set, Tuple)
 import collections
 
 # Global command line arguments.
-_args = None
+_args: Optional[argparse.Namespace] = None
 # List of regular event tables.
 _event_tables: list[str] = []
 # List of event tables generated from "/sys" directories.
@@ -190,11 +191,78 @@ class BigCString:
 
 _bcs = BigCString()
 
+# Names that some object in this process has been asked for.  Used only to
+# find the spelling an unread name was meant to be.  The set is per process
+# and the workers of the pool never share it, so a name is in it once some
+# object in the same worker has been built; every object asks for the same
+# names, so they are there before the first object is reported, but a read
+# that moved behind a condition would make a report depend on which worker
+# happened to build which file.
+_read_keys: Set[str] = set()
+
+# Unread name -> the names it could be a misspelling of, or () for none.  A table
+# repeats the same few names tens of thousands of times, and the name that
+# misses is the common case (Counter, many thousands of objects), so the
+# misses are kept too; _mark empties the cache when a name is read for the
+# first time, so a hint computed while _read_keys was still growing cannot
+# stick.
+_hints: Dict[str, Tuple[str, ...]] = {}
+
+# Whether the objects are wrapped at all.  Only the check reads what the
+# tracking records, so a build does not wrap anything.  Set in
+# main() and, because the workers may be fresh interpreters, passed to them
+# through _init_worker() rather than read from _args there.
+_track: bool = False
+
+
+class _JsonObject(dict):
+  """A JSON object that remembers which of its keys were looked at.
+
+  jevents.py picks the fields it wants out of each object by name and
+  ignores the rest, so a key it never names - a misspelled
+  "BriefDescript6ion", say - is dropped without a word and the event
+  silently loses its description.  Handing the parsed object to this
+  class and asking it afterwards which keys were never looked at makes
+  the misspelling visible.  Only get(), [] and 'in' are tracked: a read
+  through items(), values() or pop() would not be, so a new read of a
+  field belongs in one of the three.  The alternative, a list of the key names
+  jevents.py reads, would be a second copy of the code below that goes
+  stale the first time someone adds a field.
+  """
+
+  def __init__(self, jd: dict):
+    super().__init__(jd)
+    # dict.keys() rather than the object itself: iterating the object would
+    # go through __iter__, should that ever be hooked below.
+    self.unread: Set[str] = set(dict.keys(jd))
+
+  def _mark(self, key: str) -> None:
+    self.unread.discard(key)
+    if key not in _read_keys:
+      _read_keys.add(key)
+      _hints.clear()
+
+  def get(self, key: Any, default: Any = None) -> Any:
+    self._mark(key)
+    return dict.get(self, key, default)
+
+  def __getitem__(self, key: Any) -> Any:
+    self._mark(key)
+    return dict.__getitem__(self, key)
+
+  def __contains__(self, key: object) -> bool:
+    if isinstance(key, str):
+      self._mark(key)
+    return dict.__contains__(self, key)
+
+
 class JsonEvent:
   """Representation of an event loaded from a json file dictionary."""
 
   def __init__(self, jd: dict):
     """Constructor passed the dictionary of parsed json values."""
+    if _track:
+      jd = _JsonObject(jd)
 
     def llx(x: int) -> str:
       """Convert an int to a string similar to a printf modifier of %#llx."""
@@ -416,6 +484,39 @@ class JsonEvent:
         raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
 
     self.event = real_event(self.name, event)
+    # Keys of the JSON object that no code above looked at, set last so that
+    # the copy of an architecture standard event above does not overwrite it.
+    # A name that is a near miss for one the code reads, and whose name the
+    # object does not carry with the same text, is almost certainly a
+    # misspelling of it: the object lost the field the reader was looking for.
+    # A name that is not a near miss is left alone, because a table is free to
+    # carry metadata jevents.py has no use for.
+    self.mistyped: Optional[Dict[str, Tuple[str, ...]]] = None
+    if not _track:
+      return
+    assert isinstance(jd, _JsonObject)
+    unread = jd.unread
+    if unread:
+      found = {}
+      for key in unread:
+        if key not in _hints:
+          lowered = {name.lower(): name for name in _read_keys}
+          # Two names can be equally close - RetirementLatencyMax and its Min -
+          # and naming one of them would be a coin toss presented as an answer.
+          match = difflib.get_close_matches(key.lower(), sorted(lowered), n=2, cutoff=0.8)
+          _hints[key] = tuple(lowered[m] for m in match)
+        hints = _hints[key]
+        if not hints:
+          continue
+        # Reported even when the object also carries a name it could be a
+        # misspelling of: two names can be equally close (RetirementLatencyMax
+        # next to its Min, which can hold the same value), so a matching value
+        # does not say which one this key was meant to be, and suppressing on
+        # it hid a typo of the other.  Either way this is a key that nothing
+        # reads, which is what the check reports.
+        found[key] = hints
+      if found:
+        self.mistyped = found
 
   def __repr__(self) -> str:
     """String representation primarily for debugging."""
@@ -453,6 +554,83 @@ class JsonEvent:
     return f'{make_comment(s)}\t{{ { _bcs.offsets[s] } }},\n'
 
 
+def _report_mistyped_keys() -> bool:
+  """Report the JSON keys that jevents.py did not look for.
+
+  jevents.py takes the fields it wants out of each object by name.  A key
+  spelled differently from the name it looks for is never read, the object is
+  built without whatever the field held, and the build says nothing; that is
+  how "BriefDescript6ion" in the AMD tables and "PublicDescrition" in the
+  AmpereOneX mmu table got in.  A name that is a near miss for one jevents.py
+  reads is reported, with the spelling it was probably meant to be and the
+  files carrying it.  A name that is not a near miss is not reported: a
+  table may carry metadata for another reader, and reporting it would mean
+  keeping a list of such names in this file that every vendor adding one
+  would have to edit.
+
+  A table that cannot be parsed is reported the same way and does not stop
+  the rest of the tree from being read, so one broken file does not hide the
+  misspellings in the other 1000.
+  """
+  assert _args is not None
+  found: Dict[str, collections.Counter] = {}
+  hints: Dict[str, Tuple[str, ...]] = {}
+  objects = 0
+  for (path, _), events in _json_cache.items():
+    for event in events:
+      if not event.mistyped:
+        continue
+      objects += 1
+      for key, hint in event.mistyped.items():
+        found.setdefault(key, collections.Counter())[path] += 1
+        hints.setdefault(key, hint)
+  if not found:
+    return False
+
+  paths = {path for files in found.values() for path in files}
+  start = _args.starting_dir
+  print(f'jevents: error: {objects} object(s) in '
+        f'{len(paths)} file(s) carry a key that looks like a misspelling:', file=sys.stderr)
+  for key in sorted(found):
+    files = found[key]
+    guess = ' or '.join(repr(name) for name in hints[key])
+    print(f'  {key!r}, did you mean {guess}? ({sum(files.values())} object(s) in '
+          f'{len(files)} file(s)):', file=sys.stderr)
+    where = sorted(os.path.relpath(path, start) for path in files)
+    for path in where[:5]:
+      print(f'    {path}', file=sys.stderr)
+    if len(where) > 5:
+      print(f'    ... and {len(where) - 5} more file(s)', file=sys.stderr)
+
+  return True
+
+
+def _report_metrics_without_expr() -> bool:
+  """Report the metrics that carry a name but no expression.
+
+  A MetricExpr key spelled another way is not read, so the object keeps its
+  name and loses its expression.  Unlike the key report above this does not
+  depend on how close a spelling is: a metric the rewrite cannot handle would
+  fail a build however the lost key was spelled.
+  """
+  assert _args is not None
+  start = _args.starting_dir
+  missing = sorted((os.path.relpath(path, start), event.metric_name)
+                   for (path, _), events in _json_cache.items()
+                   for event in events
+                   if event.metric_name and event.metric_expr is None)
+  if not missing:
+    return False
+  print(f'jevents: error: {len(missing)} metric(s) have a name but no '
+        f'expression, so their MetricExpr key is missing or misspelled:',
+        file=sys.stderr)
+  for where, name in missing[:20]:
+    print(f'  {name!r} in {where}', file=sys.stderr)
+  if len(missing) > 20:
+    print(f'    ... and {len(missing) - 20} more metric(s)', file=sys.stderr)
+  return True
+
+
 _json_cache: Dict[Tuple[str, str], Sequence[JsonEvent]] = {}
 def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
   """Read json events from the specified file."""
@@ -460,17 +638,20 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
+    if not _track:
+      print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
     if event.metric_name and event.metric_expr is None:
       # A MetricName without a MetricExpr leaves nothing to put in the
-      # generated tables, whether or not the name has a dash, so the build
-      # must not go on.  Name the metric instead of letting the table printer
-      # assert on the None.
-      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      # generated tables, whether or not the name has a dash, so a build must
+      # not go on.  Name the metric instead of letting the table printer
+      # assert on the None; --check reports it and keeps reading the rest.
+      if not _track:
+        raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      continue
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -487,8 +668,12 @@ def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
     _json_cache[key] = _read_json_events_impl(path, topic)
   return _json_cache[key]
 
-def preprocess_arch_std_files(archpath: str) -> None:
-  """Read in all architecture standard events."""
+def preprocess_arch_std_files(archpath: str, failures: Optional[List[Tuple[str, Exception]]] = None) -> None:
+  """Read in all architecture standard events.
+
+  A failure is raised, or appended to failures when the caller can carry on
+  without the table; either way the tables after it are still read.
+  """
   for item in os.scandir(archpath):
     if not item.is_file() or not item.name.endswith('.json'):
       continue
@@ -499,7 +684,10 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
+        failure = RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}')
+        if failures is None:
+          raise failure from e
+        failures.append((item.path, failure))
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1448,16 +1636,27 @@ def _parallel_worker_count(num_tasks: int) -> int:
   return max(1, min(getattr(os, 'process_cpu_count', os.cpu_count)() or 1,
                     _max_parallel_workers, num_tasks))
 
-def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Sequence[JsonEvent]]:
+def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Any]:
   path, topic = task
-  return path, topic, _read_json_events_impl(path, topic)
-
-def _init_worker(std_events: dict) -> None:
-  global _arch_std_events
+  try:
+    return path, topic, _read_json_events_impl(path, topic)
+  except Exception as e:
+    if not _track:
+      # The build stops here: keep the file and the reason in one message,
+      # the pool adds the traceback.
+      raise RuntimeError(f'Failure processing \'{path}\': {e}') from None
+    # Carried back rather than raised: the parent knows the tree and can
+    # report the file and keep going, which is the point of reading the
+    # tables this build does not use.
+    return path, topic, e
+
+def _init_worker(std_events: dict, track: bool) -> None:
+  global _arch_std_events, _track
   _arch_std_events = std_events
+  _track = track
 
 def main() -> None:
-  global _args
+  global _args, _track
 
   def dir_path(path: str) -> str:
     """Validate path is a directory for argparse."""
@@ -1484,12 +1683,12 @@ def main() -> None:
       if item.is_dir():
         ftw(item.path, list(parents) + [item.name], action)
 
-  ap = argparse.ArgumentParser()
+  ap = argparse.ArgumentParser(description='Generate pmu-events.c from the json tables')
   ap.add_argument('arch', help='Architecture name like x86')
   ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all", and that is what an omitted
-model means. For architectures like ARM64 with an implementor/model, the
-model must include the implementor such as "arm/cortex-a34".''',
+reduce the code size.  Normally set to "all". For architectures like
+ARM64 with an implementor/model, the model must include the implementor
+such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
@@ -1497,14 +1696,104 @@ model must include the implementor such as "arm/cortex-a34".''',
       help='Root of tree containing architecture directories containing json files'
   )
   ap.add_argument('output_file', nargs='?', default=None,
-                  help='file to write the tables to, default standard output')
+                  help='file to write the tables to, default standard output; '
+                       'not written, and not opened, with --check')
   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')
+                       'apart from the tables; ignored with --check')
+  ap.add_argument('--check', action='store_true',
+                  help='read the tables, report the keys that look like a '
+                       'misspelling of a name this file reads on standard '
+                       'error, and exit without writing any table; pass "all" '
+                       'as both arch and model to cover the whole tree.  '
+                       'Nothing is reported when every key is read and every '
+                       'table parses.  The exit status is 0 when the tree is '
+                       'clean, 1 when something was reported and 2 when a '
+                       'table could not be read, so it can gate a script')
   _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.check and (_args.output_file is not None or _args.output_string_file is not None):
+    ap.error('--check writes no table; drop the output arguments')
+  if _args.check:
+    if _args.arch == 'none':
+      ap.error('--check needs an architecture to read')
+    # The model only picks the tables a build needs; a check is about the
+    # tree, so it reads all of it and the model means nothing here.
+    _args.model = 'all'
+    _track = True
+
+  archs = []
+  for item in os.scandir(_args.starting_dir):
+    if not item.is_dir():
+      continue
+    if item.name == _args.arch or _args.arch == 'all' or item.name == 'test' or item.name == 'common':
+      archs.append(item.name)
+
+  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()
+  import concurrent.futures
+  tasks = []
+  def collect_json(parents: Sequence[str], item: os.DirEntry) -> None:
+    if len(parents) == 0:
+      return
+    if item.is_file() and item.name.endswith('.json') and not item.name.endswith('metricgroups.json'):
+      tasks.append((item.path, get_topic(item.name)))
+
+  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)
+
+    with concurrent.futures.ProcessPoolExecutor(max_workers=_parallel_worker_count(len(tasks)),
+                                                initializer=_init_worker,
+                                                initargs=(_arch_std_events, _args.check)) as executor:
+      for path, topic, events in executor.map(_parallel_read_json_events, tasks):
+        if isinstance(events, Exception):
+          if not _args.check:
+            raise RuntimeError(f'Failure processing \'{path}\'') from events
+          failed.append((path, events))
+        else:
+          _json_cache[(path, topic)] = events
+  except (RuntimeError, ValueError, OSError, argparse.ArgumentTypeError) as e:
+    if not _args.check:
+      raise
+    failed.append((_args.starting_dir, e))
+
+  if _args.check:
+    # A table this check does not need is reported, not fatal: the check reads
+    # the whole tree and a caller only asked about one part of it.
+    for path, error in failed:
+      text = str(error)
+      # The architecture standard preprocess names the file and its directory
+      # already, so prefixing the path here only repeats it.
+      if text.startswith('Failure processing'):
+        print(f'jevents: error: {text}', file=sys.stderr)
+      else:
+        print(f'jevents: error: '
+              f'{os.path.relpath(path, _args.starting_dir)}: {text}',
+              file=sys.stderr)
+    reported = _report_mistyped_keys()
+    reported = _report_metrics_without_expr() or reported
+    if failed:
+      sys.exit(2)
+    sys.exit(1 if reported else 0)
+
+  # Opened here, and not by argparse, so that --check leaves the files it was
+  # given alone and a failure before the tables are written cannot empty one.
   if _args.output_file in (None, '-'):
     _args.output_file = sys.stdout
   else:
@@ -1540,35 +1829,6 @@ struct pmu_table_entry {
 };
 
 """)
-  archs = []
-  for item in os.scandir(_args.starting_dir):
-    if not item.is_dir():
-      continue
-    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':
-    raise IOError(f'Missing architecture directory \'{_args.arch}\'')
-
-  archs.sort()
-  import concurrent.futures
-  tasks = []
-  def collect_json(parents: Sequence[str], item: os.DirEntry) -> None:
-    if len(parents) == 0:
-      return
-    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
 
   for arch in archs:
     arch_path = f'{_args.starting_dir}/{arch}'
@@ -1607,4 +1867,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
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..2254bc96352c
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Check that jevents.py reports unread json keys and refuses a broken table
+
+shelldir=$(dirname "$0")
+. "${shelldir}"/lib/setup_python.sh
+
+# The check reads the tables and needs no build and no hardware, but an
+# installed perf has no pmu-events/ next to the binary.
+pmudir=$(readlink -f "${shelldir}/../../pmu-events")
+if [ ! -f "${pmudir}/jevents.py" ] || [ ! -f "${pmudir}/metric.py" ]; then
+	echo "Skipping: no pmu-events/jevents.py under ${pmudir}"
+	exit 2
+fi
+
+workdir=$(mktemp -d /tmp/jevents_check.XXXXXX)
+trap 'rm -rf "${workdir}"' EXIT
+
+# A table with one key jevents.py reads and one event, so that a clean run
+# says nothing and exits 0.
+mkdir -p "${workdir}/arch/x86/model"
+cat > "${workdir}/arch/x86/model/events.json" <<'JSON'
+[
+  {
+    "EventName": "CHECK_TEST_EVENT",
+    "EventCode": "0x1",
+    "BriefDescription": "a table the check should accept"
+  }
+]
+JSON
+
+run_check() {
+	"${PYTHON}" "${pmudir}/jevents.py" --check x86 all "${workdir}/arch"
+}
+
+run_check
+if [ $? -ne 0 ]; then
+	echo "pmu-events json keys [Failed: a clean table was reported]"
+	exit 1
+fi
+
+# A near miss for a name jevents.py reads is reported, and the status is 1.
+sed -i 's/"BriefDescription"/"BriefDescript6ion"/' \
+	"${workdir}/arch/x86/model/events.json"
+output=$(run_check 2>&1)
+ret=$?
+case "${ret}" in
+1) ;;
+*)	echo "pmu-events json keys [Failed: a misspelled key exited ${ret}]"
+	exit 1 ;;
+esac
+if ! echo "${output}" | grep -q "BriefDescript6ion"; then
+	echo "pmu-events json keys [Failed: the misspelled key was not reported]"
+	exit 1
+fi
+
+# A table that does not parse is a failure of the check and not a finding:
+# the status is 2, which this script must not pass on as a skip.
+echo '{ "truncated": ' > "${workdir}/arch/x86/model/broken.json"
+run_check >/dev/null 2>&1
+ret=$?
+if [ "${ret}" -ne 2 ]; then
+	echo "pmu-events json keys [Failed: an unreadable table exited ${ret}]"
+	exit 1
+fi
+
+echo "pmu-events json keys [Success]"
+exit 0
-- 
2.53.0

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

[sashiko-bot]

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 75a86d8bd9af5645c902739baea7b69e539c8eea
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 make target and
shell test, to detect unread JSON keys that look like misspellings of known
field names and metrics missing a MetricExpr.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 844fa82887bbf..d31f8e2fd6d3f 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py

[ ... ]

> @@ -1497,14 +1696,104 @@ model must include the implementor such as "arm/cortex-a34".''',
> +  if not archs and _args.arch != 'none':
> +    raise IOError(f'Missing architecture directory \'{_args.arch}\'')
> +
> +  archs.sort()
> +  import concurrent.futures
> +  tasks = []
> +  def collect_json(parents: Sequence[str], item: os.DirEntry) -> None:
> +    if len(parents) == 0:
> +      return
> +    if item.is_file() and item.name.endswith('.json') and not item.name.endswith('metricgroups.json'):
> +      tasks.append((item.path, get_topic(item.name)))
> +
> +  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 when tasks is empty skip reporting errors and mistyped keys
collected by preprocess_arch_std_files() in --check mode?

collect_json() only adds JSON files in subdirectories (len(parents) > 0) to
tasks, whereas preprocess_arch_std_files() reads top-level JSON files into
_json_cache and records parse errors in failed.  If an architecture directory
contains only top-level standard event files, main() exits here before
printing failed or running _report_mistyped_keys() and
_report_metrics_without_expr().  Also, in build mode with _args.arch ==
'none' on a directory without common or test subdirectories, this exits with
status 1 instead of generating the empty C tables that the _args.arch !=
'none' guard above allows.

[ ... ]

> @@ -1607,4 +1867,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 module-level __main__ block break the default 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 parameter named e, such
as add_events_table_entries(), failing the pylint-pmu-events build target in
tools/perf/Makefile.perf.

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

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

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key spelled differently is dropped without a word
and the object is built without whatever that field held.

Patches 1 and 2 make the generator name what went wrong, and let the
model argument be omitted. Patch 3 adds a --check mode that reports the
keys jevents.py never asks for, along with a make target and a perf test
so that it runs without anyone having to remember it.

This is the second posting of the check. It went out alone in November
(https://lore.kernel.org/linux-perf-
users/20261110193239.1741014-1-takano32@gmail.com/) and the review found
two ways it could call a tree clean that no build can use: a MetricName
with a dash in it that loses its MetricExpr was invisible to both the
reader and the report, and a metric without an expression was skipped
rather than reported unless its key was spelled close enough to be a near
miss. Both are fixed here, and the failure-path changes the check needs
are no longer mixed into it.

The test runs the check over a fixture table rather than over the tree,
so it is green whatever the tables happen to look like, and it checks the
three exit statuses: 0 for a clean table, 1 for a reported key, 2 for a
table that does not parse.

The two misspellings the check reports in the tree today were posted
separately in November:

https://lore.kernel.org/linux-perf-users/20261108184600.3810059-1-takano32@gmail.com/
https://lore.kernel.org/linux-perf-users/20261108183846.3804875-1-takano32@gmail.com/

TAKANO Mitsuhiro (3):
  perf jevents: Name the metric when a MetricExpr is missing
  perf jevents: Make the model argument optional
  perf jevents: Check the JSON keys that jevents.py never reads

 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 397 +++++++++++++++++++++---
 tools/perf/tests/shell/jevents_check.sh |  68 ++++
 4 files changed, 429 insertions(+), 51 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

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

[TAKANO Mitsuhiro]

jevents.py reads each field it wants out of a JSON object by name, so a
MetricExpr key that is missing or spelled another way leaves the object
with a MetricName and no expression. Two different failures followed,
neither of which named the metric: the rewrite in _read_json_events_impl()
raised AttributeError on the None, and a metric that got that far reached
print_pending_metrics(), whose assert fired on it.

Raise in the reader instead, naming the metric, before either of them can
happen. The check is not inside the '-' test that decides whether a metric
takes part in the rewrite: a name with a dash in it can lose its
expression the same way.

Two smaller changes on the same path:

- An architecture standard table that does not parse now carries the
   reason in its message ("Failure processing '<file>' in '<dir>':
   <cause>").  The traceback used to end with the file name alone, and
   only the chained exception said why.

- A table that does not parse is reported on stderr with a "jevents:
   error:" prefix instead of on stdout, where it was mixed with the
   generated output.

And the output files are opened after every table has been read, and not
by argparse, so a build that fails while reading leaves a file the caller
named as it was instead of truncating it to the header.  "Every table"
includes metricgroups.json, whose first pass runs before this point, and
the string table built from all of them: nothing that parses a file runs
after the output is opened.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v3: the pass that reads metricgroups.json, and the string table built
    from it, now run before the output files are opened.  Sashiko asked
    whether a failure there still truncates them: with this ordering
    nothing that parses a table runs after the open, and a table that
    does not parse leaves a named output file as it was.

 tools/perf/pmu-events/jevents.py | 78 +++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..61f778dcc1e5 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -460,11 +460,17 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f"Exception processing {path}")
+    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
+    if event.metric_name and event.metric_expr is None:
+      # A MetricName without a MetricExpr leaves nothing to put in the
+      # generated tables, whether or not the name has a dash, so the build
+      # must not go on.  Name the metric instead of letting the table printer
+      # assert on the None.
+      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -493,7 +499,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-        raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\'') from e
+      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1490,33 +1496,13 @@ 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()
 
-  _args.output_file.write(f"""/* SPDX-License-Identifier: GPL-2.0 */
-/* THIS FILE WAS AUTOGENERATED BY `jevents.py arch={_args.arch} model={_args.model}` ! */
-""")
-  _args.output_file.write("""
-#include <pmu-events/pmu-events.h>
-#include "util/header.h"
-#include "util/pmu.h"
-#include <string.h>
-#include <stddef.h>
-
-struct compact_pmu_event {
-\tint offset;
-};
-
-struct pmu_table_entry {
-\tconst struct compact_pmu_event *entries;
-\tuint32_t num_entries;
-\tstruct compact_pmu_event pmu_name;
-};
-
-""")
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
@@ -1553,6 +1539,44 @@ struct pmu_table_entry {
 
   assert _bcs is not None
   _bcs.compute()
+
+  # Every table has been read by now, so opening the output here cannot truncate
+  # a file the caller named when a table turns out not to parse.
+  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}` ! */
+""")
+  _args.output_file.write("""
+#include <pmu-events/pmu-events.h>
+#include "util/header.h"
+#include "util/pmu.h"
+#include <string.h>
+#include <stddef.h>
+
+struct compact_pmu_event {
+\tint offset;
+};
+
+struct pmu_table_entry {
+\tconst struct compact_pmu_event *entries;
+\tuint32_t num_entries;
+\tstruct compact_pmu_event pmu_name;
+};
+
+""")
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
     _args.output_file.write('static const char *const big_c_string =\n')
-- 
2.53.0

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

[sashiko-bot]

> jevents.py reads each field it wants out of a JSON object by name, so a
> MetricExpr key that is missing or spelled another way leaves the object
> with a MetricName and no expression. Two different failures followed,
> neither of which named the metric: the rewrite in _read_json_events_impl()
> raised AttributeError on the None, and a metric that got that far reached
> print_pending_metrics(), whose assert fired on it.
> 
> Raise in the reader instead, naming the metric, before either of them can
> happen. The check is not inside the '-' test that decides whether a metric
> takes part in the rewrite: a name with a dash in it can lose its
> expression the same way.
> 
> Two smaller changes on the same path:
> 
> - An architecture standard table that does not parse now carries the
> [ ... ]
> includes metricgroups.json, whose first pass runs before this point, and
> the string table built from all of them: nothing that parses a file runs
> after the output is opened.
> 
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v3 2/3] perf jevents: Make the model argument optional

[TAKANO Mitsuhiro]

The model argument carries a default of "all", but a positional argument
with a default and no nargs is still required: "jevents.py x86 <dir>" exits
with "the following arguments are required: model", and so does any caller
that only wants the tables for an architecture.

Make it nargs='?' so the default is reachable and an omitted model means
"all".  Builds pass it explicitly, so the generated tables do not change.

Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v3: the pass that reads metricgroups.json, and the string table built
    from it, now run before the output files are opened.  Sashiko asked
    whether a failure there still truncates them: with this ordering
    nothing that parses a table runs after the open, and a table that
    does not parse leaves a named output file as it was.

 tools/perf/pmu-events/jevents.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 61f778dcc1e5..f4dca47eb49b 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -1486,10 +1486,10 @@ def main() -> None:
 
   ap = argparse.ArgumentParser()
   ap.add_argument('arch', help='Architecture name like x86')
-  ap.add_argument('model', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all". For architectures like
-ARM64 with an implementor/model, the model must include the implementor
-such as "arm/cortex-a34".''',
+  ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
+reduce the code size.  Normally set to "all", and that is what an omitted
+model means. For architectures like ARM64 with an implementor/model, the
+model must include the implementor such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
-- 
2.53.0

Re: [PATCH v3 2/3] perf jevents: Make the model argument optional

[sashiko-bot]

> The model argument carries a default of "all", but a positional argument
> with a default and no nargs is still required: "jevents.py x86 <dir>" exits
> with "the following arguments are required: model", and so does any caller
> that only wants the tables for an architecture.
> 
> Make it nargs='?' so the default is reachable and an omitted model means
> "all".  Builds pass it explicitly, so the generated tables do not change.
> 
> Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

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

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name:
jd.get('BriefDescription'), 'EventCode' in jd, and the names in
event_fields. A key spelled another way is dropped without a word, and
the object is built without whatever that field held.

Two of those are in the tree now. "BriefDescript6ion" is in one entry
each of amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-
store.json, and since those objects carry no other description,
ls_inef_sw_pref.all is generated with an empty text field.
"PublicDescrition" was in 16 of the 28 entries of
arm64/ampere/ampereonex/mmu.json; nothing a user saw changed there,
because each of the 16 also had a BriefDescription with the same text,
which is why the typo could sit there unnoticed.

Add a --check mode that reads every architecture, not just the one being
built: a typo is cheapest to fix by whoever reads that table next, and
that reader may be building something else. It records the keys
JsonEvent was never asked for and reports the ones whose spelling is a
near miss for a name jevents.py does read (difflib ratio 0.8). In this
tree 52 distinct key names occur, jevents.py reads 46 of them, the two
misspellings score 0.97, and the highest score among the remaining
unread names is 0.78 (Counter against CounterMask), so any cutoff in
that gap reports the same two.

It also reports a metric that carries a name but no expression, by
metric name and however the lost MetricExpr key was spelled: the key
report needs the spelling to be close to a name jevents.py reads, and a
metric without its expression cannot be generated at all.

A key that is a near miss for two names at once is reported as well: the
object may carry one of them with the same value, but that does not say
which one the key was meant to be, and the key is one nothing reads
either way.

The tracking is off unless --check asked for it, so a build parses each
object exactly as before and the generated tables are byte-identical
(x86, arm64 and all architectures compared against the same tree without
this patch). The check itself reads the 973 json files under arch/ and
takes seconds of CPU.

Exit status is 0 when nothing was reported, 1 when something was, and 2
when a table could not be read or the command line was wrong; make folds
both failures into its own status, so a script that needs the difference
runs jevents.py directly. Status 0 means the tables under that directory
were read and no unread key looks like a name jevents.py reads. It is
not a promise that a build will succeed: mapfile.csv coverage, the model
that selects a subset of the tables, and the C generation are not part
of it.

It is not a schema either: it checks nothing about types, required
fields or value ranges, and looks at key names only, so a misspelled
description in a value is out of scope. It does not read mapfile.csv,
which has no keys, or metricgroups.json, whose keys are group names.
Names that only metric.py or the C code read are outside it too, so an
"Expermental" key is not reported, and file names and topics are not
keys.

The check runs from the new make target "make -C tools/perf jevents-
check" (JEVENTS_CHECK_ARCH=x86 narrows it to one architecture) and from
perf test, where tests/shell/jevents_check.sh skips with status 2 when
pmu-events/arch is not next to the binary.

Written with Hermes Agent, an LLM assistant, which found the
misspellings and wrote this check, its test and this changelog. I built
perf with it, ran the perf test above, ran the check over the whole
tree, compared the generated tables with those from an unpatched tree on
x86, arm64 and all architectures at once, and ran mypy on jevents.py.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v3: the pass that reads metricgroups.json, and the string table built
    from it, now run before the output files are opened.  Sashiko asked
    whether a failure there still truncates them: with this ordering
    nothing that parses a table runs after the open, and a table that
    does not parse leaves a named output file as it was.

 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 345 +++++++++++++++++++++---
 tools/perf/tests/shell/jevents_check.sh |  68 +++++
 4 files changed, 391 insertions(+), 37 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 00baf90e5bda..0b13f1e12899 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -140,6 +140,18 @@ check-headers:
 sync-headers:
 	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
 
+# The pmu-events tables are data, so a key nothing reads is invisible until a
+# description goes missing.  Named in the wrapper like check-headers: this is
+# a data check, not a build, so it needs neither config nor fixdep and reads
+# the source tree whatever O= says.  JEVENTS_CHECK_ARCH=x86 reads x86 and the
+# common and test tables every build reads.
+# Makefile.perf picks the interpreter for the build; the wrapper does not
+# include it, so default here and let PYTHON= win when it is given.
+JEVENTS_CHECK_ARCH ?= all
+PYTHON ?= python3
+jevents-check:
+	@$(PYTHON) ./pmu-events/jevents.py --check '$(JEVENTS_CHECK_ARCH)' all pmu-events/arch
+
 #
 # All other targets get passed through:
 #
@@ -147,4 +159,4 @@ sync-headers:
 	$(print_msg)
 	$(make)
 
-.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers
+.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers jevents-check
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b74b534e1153..a330cd9de0fc 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -871,6 +871,7 @@ help:
 	@echo 'Perf maintainer targets:'
 	@echo '  clean			- clean all binary objects and build output'
 	@echo '  check-headers		- check which header copies are out of sync with the kernel'
+	@echo '  jevents-check		- report the pmu-events json keys that jevents.py never reads'
 	@echo '  sync-headers		- sync those header copies with the kernel, creating commits'
 	@echo '			  (SYNC_HEADERS_ARGS="..." passes options to the script)'
 
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index f4dca47eb49b..3381ade201fd 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -3,15 +3,16 @@
 """Convert directories of JSON events to C code."""
 import argparse
 import csv
+import difflib
 import json
 import metric
 import os
 import sys
-from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
+from typing import (Any, Callable, Dict, List, Optional, Sequence, Set, Tuple)
 import collections
 
 # Global command line arguments.
-_args = None
+_args: Optional[argparse.Namespace] = None
 # List of regular event tables.
 _event_tables: list[str] = []
 # List of event tables generated from "/sys" directories.
@@ -190,11 +191,78 @@ class BigCString:
 
 _bcs = BigCString()
 
+# Names that some object in this process has been asked for.  Used only to
+# find the spelling an unread name was meant to be.  The set is per process
+# and the workers of the pool never share it, so a name is in it once some
+# object in the same worker has been built; every object asks for the same
+# names, so they are there before the first object is reported, but a read
+# that moved behind a condition would make a report depend on which worker
+# happened to build which file.
+_read_keys: Set[str] = set()
+
+# Unread name -> the names it could be a misspelling of, or () for none.  A table
+# repeats the same few names tens of thousands of times, and the name that
+# misses is the common case (Counter, many thousands of objects), so the
+# misses are kept too; _mark empties the cache when a name is read for the
+# first time, so a hint computed while _read_keys was still growing cannot
+# stick.
+_hints: Dict[str, Tuple[str, ...]] = {}
+
+# Whether the objects are wrapped at all.  Only the check reads what the
+# tracking records, so a build does not wrap anything.  Set in
+# main() and, because the workers may be fresh interpreters, passed to them
+# through _init_worker() rather than read from _args there.
+_track: bool = False
+
+
+class _JsonObject(dict):
+  """A JSON object that remembers which of its keys were looked at.
+
+  jevents.py picks the fields it wants out of each object by name and
+  ignores the rest, so a key it never names - a misspelled
+  "BriefDescript6ion", say - is dropped without a word and the event
+  silently loses its description.  Handing the parsed object to this
+  class and asking it afterwards which keys were never looked at makes
+  the misspelling visible.  Only get(), [] and 'in' are tracked: a read
+  through items(), values() or pop() would not be, so a new read of a
+  field belongs in one of the three.  The alternative, a list of the key names
+  jevents.py reads, would be a second copy of the code below that goes
+  stale the first time someone adds a field.
+  """
+
+  def __init__(self, jd: dict):
+    super().__init__(jd)
+    # dict.keys() rather than the object itself: iterating the object would
+    # go through __iter__, should that ever be hooked below.
+    self.unread: Set[str] = set(dict.keys(jd))
+
+  def _mark(self, key: str) -> None:
+    self.unread.discard(key)
+    if key not in _read_keys:
+      _read_keys.add(key)
+      _hints.clear()
+
+  def get(self, key: Any, default: Any = None) -> Any:
+    self._mark(key)
+    return dict.get(self, key, default)
+
+  def __getitem__(self, key: Any) -> Any:
+    self._mark(key)
+    return dict.__getitem__(self, key)
+
+  def __contains__(self, key: object) -> bool:
+    if isinstance(key, str):
+      self._mark(key)
+    return dict.__contains__(self, key)
+
+
 class JsonEvent:
   """Representation of an event loaded from a json file dictionary."""
 
   def __init__(self, jd: dict):
     """Constructor passed the dictionary of parsed json values."""
+    if _track:
+      jd = _JsonObject(jd)
 
     def llx(x: int) -> str:
       """Convert an int to a string similar to a printf modifier of %#llx."""
@@ -416,6 +484,39 @@ class JsonEvent:
         raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
 
     self.event = real_event(self.name, event)
+    # Keys of the JSON object that no code above looked at, set last so that
+    # the copy of an architecture standard event above does not overwrite it.
+    # A name that is a near miss for one the code reads, and whose name the
+    # object does not carry with the same text, is almost certainly a
+    # misspelling of it: the object lost the field the reader was looking for.
+    # A name that is not a near miss is left alone, because a table is free to
+    # carry metadata jevents.py has no use for.
+    self.mistyped: Optional[Dict[str, Tuple[str, ...]]] = None
+    if not _track:
+      return
+    assert isinstance(jd, _JsonObject)
+    unread = jd.unread
+    if unread:
+      found = {}
+      for key in unread:
+        if key not in _hints:
+          lowered = {name.lower(): name for name in _read_keys}
+          # Two names can be equally close - RetirementLatencyMax and its Min -
+          # and naming one of them would be a coin toss presented as an answer.
+          match = difflib.get_close_matches(key.lower(), sorted(lowered), n=2, cutoff=0.8)
+          _hints[key] = tuple(lowered[m] for m in match)
+        hints = _hints[key]
+        if not hints:
+          continue
+        # Reported even when the object also carries a name it could be a
+        # misspelling of: two names can be equally close (RetirementLatencyMax
+        # next to its Min, which can hold the same value), so a matching value
+        # does not say which one this key was meant to be, and suppressing on
+        # it hid a typo of the other.  Either way this is a key that nothing
+        # reads, which is what the check reports.
+        found[key] = hints
+      if found:
+        self.mistyped = found
 
   def __repr__(self) -> str:
     """String representation primarily for debugging."""
@@ -453,6 +554,83 @@ class JsonEvent:
     return f'{make_comment(s)}\t{{ { _bcs.offsets[s] } }},\n'
 
 
+def _report_mistyped_keys() -> bool:
+  """Report the JSON keys that jevents.py did not look for.
+
+  jevents.py takes the fields it wants out of each object by name.  A key
+  spelled differently from the name it looks for is never read, the object is
+  built without whatever the field held, and the build says nothing; that is
+  how "BriefDescript6ion" in the AMD tables and "PublicDescrition" in the
+  AmpereOneX mmu table got in.  A name that is a near miss for one jevents.py
+  reads is reported, with the spelling it was probably meant to be and the
+  files carrying it.  A name that is not a near miss is not reported: a
+  table may carry metadata for another reader, and reporting it would mean
+  keeping a list of such names in this file that every vendor adding one
+  would have to edit.
+
+  A table that cannot be parsed is reported the same way and does not stop
+  the rest of the tree from being read, so one broken file does not hide the
+  misspellings in the other 1000.
+  """
+  assert _args is not None
+  found: Dict[str, collections.Counter] = {}
+  hints: Dict[str, Tuple[str, ...]] = {}
+  objects = 0
+  for (path, _), events in _json_cache.items():
+    for event in events:
+      if not event.mistyped:
+        continue
+      objects += 1
+      for key, hint in event.mistyped.items():
+        found.setdefault(key, collections.Counter())[path] += 1
+        hints.setdefault(key, hint)
+  if not found:
+    return False
+
+  paths = {path for files in found.values() for path in files}
+  start = _args.starting_dir
+  print(f'jevents: error: {objects} object(s) in '
+        f'{len(paths)} file(s) carry a key that looks like a misspelling:', file=sys.stderr)
+  for key in sorted(found):
+    files = found[key]
+    guess = ' or '.join(repr(name) for name in hints[key])
+    print(f'  {key!r}, did you mean {guess}? ({sum(files.values())} object(s) in '
+          f'{len(files)} file(s)):', file=sys.stderr)
+    where = sorted(os.path.relpath(path, start) for path in files)
+    for path in where[:5]:
+      print(f'    {path}', file=sys.stderr)
+    if len(where) > 5:
+      print(f'    ... and {len(where) - 5} more file(s)', file=sys.stderr)
+
+  return True
+
+
+def _report_metrics_without_expr() -> bool:
+  """Report the metrics that carry a name but no expression.
+
+  A MetricExpr key spelled another way is not read, so the object keeps its
+  name and loses its expression.  Unlike the key report above this does not
+  depend on how close a spelling is: a metric the rewrite cannot handle would
+  fail a build however the lost key was spelled.
+  """
+  assert _args is not None
+  start = _args.starting_dir
+  missing = sorted((os.path.relpath(path, start), event.metric_name)
+                   for (path, _), events in _json_cache.items()
+                   for event in events
+                   if event.metric_name and event.metric_expr is None)
+  if not missing:
+    return False
+  print(f'jevents: error: {len(missing)} metric(s) have a name but no '
+        f'expression, so their MetricExpr key is missing or misspelled:',
+        file=sys.stderr)
+  for where, name in missing[:20]:
+    print(f'  {name!r} in {where}', file=sys.stderr)
+  if len(missing) > 20:
+    print(f'    ... and {len(missing) - 20} more metric(s)', file=sys.stderr)
+  return True
+
+
 _json_cache: Dict[Tuple[str, str], Sequence[JsonEvent]] = {}
 def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
   """Read json events from the specified file."""
@@ -460,17 +638,20 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
+    if not _track:
+      print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
     if event.metric_name and event.metric_expr is None:
       # A MetricName without a MetricExpr leaves nothing to put in the
-      # generated tables, whether or not the name has a dash, so the build
-      # must not go on.  Name the metric instead of letting the table printer
-      # assert on the None.
-      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      # generated tables, whether or not the name has a dash, so a build must
+      # not go on.  Name the metric instead of letting the table printer
+      # assert on the None; --check reports it and keeps reading the rest.
+      if not _track:
+        raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      continue
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -487,8 +668,12 @@ def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
     _json_cache[key] = _read_json_events_impl(path, topic)
   return _json_cache[key]
 
-def preprocess_arch_std_files(archpath: str) -> None:
-  """Read in all architecture standard events."""
+def preprocess_arch_std_files(archpath: str, failures: Optional[List[Tuple[str, Exception]]] = None) -> None:
+  """Read in all architecture standard events.
+
+  A failure is raised, or appended to failures when the caller can carry on
+  without the table; either way the tables after it are still read.
+  """
   for item in os.scandir(archpath):
     if not item.is_file() or not item.name.endswith('.json'):
       continue
@@ -499,7 +684,10 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
+        failure = RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}')
+        if failures is None:
+          raise failure from e
+        failures.append((item.path, failure))
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1448,16 +1636,27 @@ def _parallel_worker_count(num_tasks: int) -> int:
   return max(1, min(getattr(os, 'process_cpu_count', os.cpu_count)() or 1,
                     _max_parallel_workers, num_tasks))
 
-def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Sequence[JsonEvent]]:
+def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Any]:
   path, topic = task
-  return path, topic, _read_json_events_impl(path, topic)
-
-def _init_worker(std_events: dict) -> None:
-  global _arch_std_events
+  try:
+    return path, topic, _read_json_events_impl(path, topic)
+  except Exception as e:
+    if not _track:
+      # The build stops here: keep the file and the reason in one message,
+      # the pool adds the traceback.
+      raise RuntimeError(f'Failure processing \'{path}\': {e}') from None
+    # Carried back rather than raised: the parent knows the tree and can
+    # report the file and keep going, which is the point of reading the
+    # tables this build does not use.
+    return path, topic, e
+
+def _init_worker(std_events: dict, track: bool) -> None:
+  global _arch_std_events, _track
   _arch_std_events = std_events
+  _track = track
 
 def main() -> None:
-  global _args
+  global _args, _track
 
   def dir_path(path: str) -> str:
     """Validate path is a directory for argparse."""
@@ -1484,12 +1683,12 @@ def main() -> None:
       if item.is_dir():
         ftw(item.path, list(parents) + [item.name], action)
 
-  ap = argparse.ArgumentParser()
+  ap = argparse.ArgumentParser(description='Generate pmu-events.c from the json tables')
   ap.add_argument('arch', help='Architecture name like x86')
   ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all", and that is what an omitted
-model means. For architectures like ARM64 with an implementor/model, the
-model must include the implementor such as "arm/cortex-a34".''',
+reduce the code size.  Normally set to "all". For architectures like
+ARM64 with an implementor/model, the model must include the implementor
+such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
@@ -1497,12 +1696,32 @@ model must include the implementor such as "arm/cortex-a34".''',
       help='Root of tree containing architecture directories containing json files'
   )
   ap.add_argument('output_file', nargs='?', default=None,
-                  help='file to write the tables to, default standard output')
+                  help='file to write the tables to, default standard output; '
+                       'not written, and not opened, with --check')
   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')
+                       'apart from the tables; ignored with --check')
+  ap.add_argument('--check', action='store_true',
+                  help='read the tables, report the keys that look like a '
+                       'misspelling of a name this file reads on standard '
+                       'error, and exit without writing any table; pass "all" '
+                       'as both arch and model to cover the whole tree.  '
+                       'Nothing is reported when every key is read and every '
+                       'table parses.  The exit status is 0 when the tree is '
+                       'clean, 1 when something was reported and 2 when a '
+                       'table could not be read, so it can gate a script')
   _args = ap.parse_args()
 
+  if _args.check and (_args.output_file is not None or _args.output_string_file is not None):
+    ap.error('--check writes no table; drop the output arguments')
+  if _args.check:
+    if _args.arch == 'none':
+      ap.error('--check needs an architecture to read')
+    # The model only picks the tables a build needs; a check is about the
+    # tree, so it reads all of it and the model means nothing here.
+    _args.model = 'all'
+    _track = True
+
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
@@ -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)
+
+    with concurrent.futures.ProcessPoolExecutor(max_workers=_parallel_worker_count(len(tasks)),
+                                                initializer=_init_worker,
+                                                initargs=(_arch_std_events, _args.check)) as executor:
+      for path, topic, events in executor.map(_parallel_read_json_events, tasks):
+        if isinstance(events, Exception):
+          if not _args.check:
+            raise RuntimeError(f'Failure processing \'{path}\'') from events
+          failed.append((path, events))
+        else:
+          _json_cache[(path, topic)] = events
+  except (RuntimeError, ValueError, OSError, argparse.ArgumentTypeError) as e:
+    if not _args.check:
+      raise
+    failed.append((_args.starting_dir, e))
+
+  if _args.check:
+    # A table this check does not need is reported, not fatal: the check reads
+    # the whole tree and a caller only asked about one part of it.
+    for path, error in failed:
+      text = str(error)
+      # The architecture standard preprocess names the file and its directory
+      # already, so prefixing the path here only repeats it.
+      if text.startswith('Failure processing'):
+        print(f'jevents: error: {text}', file=sys.stderr)
+      else:
+        print(f'jevents: error: '
+              f'{os.path.relpath(path, _args.starting_dir)}: {text}',
+              file=sys.stderr)
+    reported = _report_mistyped_keys()
+    reported = _report_metrics_without_expr() or reported
+    if failed:
+      sys.exit(2)
+    sys.exit(1 if reported else 0)
 
   for arch in archs:
     arch_path = f'{_args.starting_dir}/{arch}'
@@ -1540,8 +1799,9 @@ model must include the implementor such as "arm/cortex-a34".''',
   assert _bcs is not None
   _bcs.compute()
 
-  # Every table has been read by now, so opening the output here cannot truncate
-  # a file the caller named when a table turns out not to parse.
+  # Everything that reads a table has run by now, so opening the output here
+  # cannot truncate one on a failure: --check leaves the files it was given
+  # alone, and a table that does not parse stops before this point.
   if _args.output_file in (None, '-'):
     _args.output_file = sys.stdout
   else:
@@ -1577,6 +1837,7 @@ struct pmu_table_entry {
 };
 
 """)
+
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
     _args.output_file.write('static const char *const big_c_string =\n')
@@ -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
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..2254bc96352c
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Check that jevents.py reports unread json keys and refuses a broken table
+
+shelldir=$(dirname "$0")
+. "${shelldir}"/lib/setup_python.sh
+
+# The check reads the tables and needs no build and no hardware, but an
+# installed perf has no pmu-events/ next to the binary.
+pmudir=$(readlink -f "${shelldir}/../../pmu-events")
+if [ ! -f "${pmudir}/jevents.py" ] || [ ! -f "${pmudir}/metric.py" ]; then
+	echo "Skipping: no pmu-events/jevents.py under ${pmudir}"
+	exit 2
+fi
+
+workdir=$(mktemp -d /tmp/jevents_check.XXXXXX)
+trap 'rm -rf "${workdir}"' EXIT
+
+# A table with one key jevents.py reads and one event, so that a clean run
+# says nothing and exits 0.
+mkdir -p "${workdir}/arch/x86/model"
+cat > "${workdir}/arch/x86/model/events.json" <<'JSON'
+[
+  {
+    "EventName": "CHECK_TEST_EVENT",
+    "EventCode": "0x1",
+    "BriefDescription": "a table the check should accept"
+  }
+]
+JSON
+
+run_check() {
+	"${PYTHON}" "${pmudir}/jevents.py" --check x86 all "${workdir}/arch"
+}
+
+run_check
+if [ $? -ne 0 ]; then
+	echo "pmu-events json keys [Failed: a clean table was reported]"
+	exit 1
+fi
+
+# A near miss for a name jevents.py reads is reported, and the status is 1.
+sed -i 's/"BriefDescription"/"BriefDescript6ion"/' \
+	"${workdir}/arch/x86/model/events.json"
+output=$(run_check 2>&1)
+ret=$?
+case "${ret}" in
+1) ;;
+*)	echo "pmu-events json keys [Failed: a misspelled key exited ${ret}]"
+	exit 1 ;;
+esac
+if ! echo "${output}" | grep -q "BriefDescript6ion"; then
+	echo "pmu-events json keys [Failed: the misspelled key was not reported]"
+	exit 1
+fi
+
+# A table that does not parse is a failure of the check and not a finding:
+# the status is 2, which this script must not pass on as a skip.
+echo '{ "truncated": ' > "${workdir}/arch/x86/model/broken.json"
+run_check >/dev/null 2>&1
+ret=$?
+if [ "${ret}" -ne 2 ]; then
+	echo "pmu-events json keys [Failed: an unreadable table exited ${ret}]"
+	exit 1
+fi
+
+echo "pmu-events json keys [Success]"
+exit 0
-- 
2.53.0

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

[sashiko-bot]

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

[PATCH v4 0/3] perf jevents: Report the JSON keys that jevents.py never reads

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key spelled differently is dropped without a word
and the object is built without whatever that field held.

Patches 1 and 2 make the generator name what went wrong, and let the
model argument be omitted. Patch 3 adds a --check mode that reports the
keys jevents.py never asks for, along with a make target and a perf test
so that it runs without anyone having to remember it.

This is the second posting of the check. It went out alone in November
(https://lore.kernel.org/linux-perf-
users/20261110193239.1741014-1-takano32@gmail.com/) and the review found
two ways it could call a tree clean that no build can use: a MetricName
with a dash in it that loses its MetricExpr was invisible to both the
reader and the report, and a metric without an expression was skipped
rather than reported unless its key was spelled close enough to be a near
miss. Both are fixed here, and the failure-path changes the check needs
are no longer mixed into it.

The test runs the check over a fixture table rather than over the tree,
so it is green whatever the tables happen to look like, and it checks the
three exit statuses: 0 for a clean table, 1 for a reported key, 2 for a
table that does not parse.

The two misspellings the check reports in the tree today were posted
separately in November:

https://lore.kernel.org/linux-perf-users/20261108184600.3810059-1-takano32@gmail.com/
https://lore.kernel.org/linux-perf-users/20261108183846.3804875-1-takano32@gmail.com/

TAKANO Mitsuhiro (3):
  perf jevents: Name the metric when a MetricExpr is missing
  perf jevents: Make the model argument optional
  perf jevents: Check the JSON keys that jevents.py never reads

 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 407 +++++++++++++++++++++---
 tools/perf/tests/shell/jevents_check.sh |  68 ++++
 4 files changed, 439 insertions(+), 51 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

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

[TAKANO Mitsuhiro]

jevents.py reads each field it wants out of a JSON object by name, so a
MetricExpr key that is missing or spelled another way leaves the object
with a MetricName and no expression. Two different failures followed,
neither of which named the metric: the rewrite in _read_json_events_impl()
raised AttributeError on the None, and a metric that got that far reached
print_pending_metrics(), whose assert fired on it.

Raise in the reader instead, naming the metric, before either of them can
happen. The check is not inside the '-' test that decides whether a metric
takes part in the rewrite: a name with a dash in it can lose its
expression the same way.

Two smaller changes on the same path:

- An architecture standard table that does not parse now carries the
   reason in its message ("Failure processing '<file>' in '<dir>':
   <cause>").  The traceback used to end with the file name alone, and
   only the chained exception said why.

- A table that does not parse is reported on stderr with a "jevents:
   error:" prefix instead of on stdout, where it was mixed with the
   generated output.

And the output files are opened after every table has been read, and not
by argparse, so a build that fails while reading leaves a file the caller
named as it was instead of truncating it to the header.  "Every table"
includes metricgroups.json, whose first pass runs before this point, and
the string table built from all of them: nothing that parses a file runs
after the output is opened.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v4: the check's try/except moves into a function, so the name it binds
    is not a module global that makes every local `e` trip pylint's
    redefined-outer-name in the pylint-pmu-events target.  An empty
    table list is an error only for a build that named an architecture:
    --check still reports the failures it collected, and arch=none
    writes the empty tables it wrote before.  Both raised by Sashiko.
v3: metricgroups.json and the string table are read before the output
    files are opened, so a table that does not parse leaves them alone.

 tools/perf/pmu-events/jevents.py | 78 +++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..61f778dcc1e5 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -460,11 +460,17 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f"Exception processing {path}")
+    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
+    if event.metric_name and event.metric_expr is None:
+      # A MetricName without a MetricExpr leaves nothing to put in the
+      # generated tables, whether or not the name has a dash, so the build
+      # must not go on.  Name the metric instead of letting the table printer
+      # assert on the None.
+      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -493,7 +499,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-        raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\'') from e
+      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1490,33 +1496,13 @@ 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()
 
-  _args.output_file.write(f"""/* SPDX-License-Identifier: GPL-2.0 */
-/* THIS FILE WAS AUTOGENERATED BY `jevents.py arch={_args.arch} model={_args.model}` ! */
-""")
-  _args.output_file.write("""
-#include <pmu-events/pmu-events.h>
-#include "util/header.h"
-#include "util/pmu.h"
-#include <string.h>
-#include <stddef.h>
-
-struct compact_pmu_event {
-\tint offset;
-};
-
-struct pmu_table_entry {
-\tconst struct compact_pmu_event *entries;
-\tuint32_t num_entries;
-\tstruct compact_pmu_event pmu_name;
-};
-
-""")
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
@@ -1553,6 +1539,44 @@ struct pmu_table_entry {
 
   assert _bcs is not None
   _bcs.compute()
+
+  # Every table has been read by now, so opening the output here cannot truncate
+  # a file the caller named when a table turns out not to parse.
+  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}` ! */
+""")
+  _args.output_file.write("""
+#include <pmu-events/pmu-events.h>
+#include "util/header.h"
+#include "util/pmu.h"
+#include <string.h>
+#include <stddef.h>
+
+struct compact_pmu_event {
+\tint offset;
+};
+
+struct pmu_table_entry {
+\tconst struct compact_pmu_event *entries;
+\tuint32_t num_entries;
+\tstruct compact_pmu_event pmu_name;
+};
+
+""")
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
     _args.output_file.write('static const char *const big_c_string =\n')
-- 
2.53.0

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

[sashiko-bot]

> jevents.py reads each field it wants out of a JSON object by name, so a
> MetricExpr key that is missing or spelled another way leaves the object
> with a MetricName and no expression. Two different failures followed,
> neither of which named the metric: the rewrite in _read_json_events_impl()
> raised AttributeError on the None, and a metric that got that far reached
> print_pending_metrics(), whose assert fired on it.
> 
> Raise in the reader instead, naming the metric, before either of them can
> happen. The check is not inside the '-' test that decides whether a metric
> takes part in the rewrite: a name with a dash in it can lose its
> expression the same way.
> 
> Two smaller changes on the same path:
> 
> - An architecture standard table that does not parse now carries the
> [ ... ]
> includes metricgroups.json, whose first pass runs before this point, and
> the string table built from all of them: nothing that parses a file runs
> after the output is opened.
> 
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v4 2/3] perf jevents: Make the model argument optional

[TAKANO Mitsuhiro]

The model argument carries a default of "all", but a positional argument
with a default and no nargs is still required: "jevents.py x86 <dir>" exits
with "the following arguments are required: model", and so does any caller
that only wants the tables for an architecture.

Make it nargs='?' so the default is reachable and an omitted model means
"all".  Builds pass it explicitly, so the generated tables do not change.

Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v4: the check's try/except moves into a function, so the name it binds
    is not a module global that makes every local `e` trip pylint's
    redefined-outer-name in the pylint-pmu-events target.  An empty
    table list is an error only for a build that named an architecture:
    --check still reports the failures it collected, and arch=none
    writes the empty tables it wrote before.  Both raised by Sashiko.
v3: metricgroups.json and the string table are read before the output
    files are opened, so a table that does not parse leaves them alone.

 tools/perf/pmu-events/jevents.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 61f778dcc1e5..f4dca47eb49b 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -1486,10 +1486,10 @@ def main() -> None:
 
   ap = argparse.ArgumentParser()
   ap.add_argument('arch', help='Architecture name like x86')
-  ap.add_argument('model', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all". For architectures like
-ARM64 with an implementor/model, the model must include the implementor
-such as "arm/cortex-a34".''',
+  ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
+reduce the code size.  Normally set to "all", and that is what an omitted
+model means. For architectures like ARM64 with an implementor/model, the
+model must include the implementor such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
-- 
2.53.0

Re: [PATCH v4 2/3] perf jevents: Make the model argument optional

[sashiko-bot]

> The model argument carries a default of "all", but a positional argument
> with a default and no nargs is still required: "jevents.py x86 <dir>" exits
> with "the following arguments are required: model", and so does any caller
> that only wants the tables for an architecture.
> 
> Make it nargs='?' so the default is reachable and an omitted model means
> "all".  Builds pass it explicitly, so the generated tables do not change.
> 
> Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

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

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name:
jd.get('BriefDescription'), 'EventCode' in jd, and the names in
event_fields. A key spelled another way is dropped without a word, and
the object is built without whatever that field held.

Two of those are in the tree now. "BriefDescript6ion" is in one entry
each of amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-
store.json, and since those objects carry no other description,
ls_inef_sw_pref.all is generated with an empty text field.
"PublicDescrition" was in 16 of the 28 entries of
arm64/ampere/ampereonex/mmu.json; nothing a user saw changed there,
because each of the 16 also had a BriefDescription with the same text,
which is why the typo could sit there unnoticed.

Add a --check mode that reads every architecture, not just the one being
built: a typo is cheapest to fix by whoever reads that table next, and
that reader may be building something else. It records the keys
JsonEvent was never asked for and reports the ones whose spelling is a
near miss for a name jevents.py does read (difflib ratio 0.8). In this
tree 52 distinct key names occur, jevents.py reads 46 of them, the two
misspellings score 0.97, and the highest score among the remaining
unread names is 0.78 (Counter against CounterMask), so any cutoff in
that gap reports the same two.

It also reports a metric that carries a name but no expression, by
metric name and however the lost MetricExpr key was spelled: the key
report needs the spelling to be close to a name jevents.py reads, and a
metric without its expression cannot be generated at all.

A key that is a near miss for two names at once is reported as well: the
object may carry one of them with the same value, but that does not say
which one the key was meant to be, and the key is one nothing reads
either way.

The tracking is off unless --check asked for it, so a build parses each
object exactly as before and the generated tables are byte-identical
(x86, arm64 and all architectures compared against the same tree without
this patch). The check itself reads the 973 json files under arch/ and
takes seconds of CPU.

Exit status is 0 when nothing was reported, 1 when something was, and 2
when a table could not be read or the command line was wrong; make folds
both failures into its own status, so a script that needs the difference
runs jevents.py directly. Status 0 means the tables under that directory
were read and no unread key looks like a name jevents.py reads. It is
not a promise that a build will succeed: mapfile.csv coverage, the model
that selects a subset of the tables, and the C generation are not part
of it.

It is not a schema either: it checks nothing about types, required
fields or value ranges, and looks at key names only, so a misspelled
description in a value is out of scope. It does not read mapfile.csv,
which has no keys, or metricgroups.json, whose keys are group names.
Names that only metric.py or the C code read are outside it too, so an
"Expermental" key is not reported, and file names and topics are not
keys.

The check runs from the new make target "make -C tools/perf jevents-
check" (JEVENTS_CHECK_ARCH=x86 narrows it to one architecture) and from
perf test, where tests/shell/jevents_check.sh skips with status 2 when
pmu-events/arch is not next to the binary.

The try/except that turns a failed check into status 2 lives in a
function, not at module scope: there the name binds a global, and every
function here that uses `e` as a local then trips pylint's redefined-
outer-name (W0621) in the pylint-pmu-events target.

An empty table list is an error only for a build that named an
architecture. A check still has the parse failures it collected to report,
and arch=none writes the empty tables it always wrote.

Written with Hermes Agent, an LLM assistant, which found the
misspellings and wrote this check, its test and this changelog. I built
perf with it, ran the perf test above, ran the check over the whole
tree, compared the generated tables with those from an unpatched tree on
x86, arm64 and all architectures at once, and ran mypy on jevents.py.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v4: the check's try/except moves into a function, so the name it binds
    is not a module global that makes every local `e` trip pylint's
    redefined-outer-name in the pylint-pmu-events target.  An empty
    table list is an error only for a build that named an architecture:
    --check still reports the failures it collected, and arch=none
    writes the empty tables it wrote before.  Both raised by Sashiko.
v3: metricgroups.json and the string table are read before the output
    files are opened, so a table that does not parse leaves them alone.

 tools/perf/Makefile                     |  14 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 355 +++++++++++++++++++++---
 tools/perf/tests/shell/jevents_check.sh |  68 +++++
 4 files changed, 401 insertions(+), 37 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 00baf90e5bda..0b13f1e12899 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -140,6 +140,18 @@ check-headers:
 sync-headers:
 	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
 
+# The pmu-events tables are data, so a key nothing reads is invisible until a
+# description goes missing.  Named in the wrapper like check-headers: this is
+# a data check, not a build, so it needs neither config nor fixdep and reads
+# the source tree whatever O= says.  JEVENTS_CHECK_ARCH=x86 reads x86 and the
+# common and test tables every build reads.
+# Makefile.perf picks the interpreter for the build; the wrapper does not
+# include it, so default here and let PYTHON= win when it is given.
+JEVENTS_CHECK_ARCH ?= all
+PYTHON ?= python3
+jevents-check:
+	@$(PYTHON) ./pmu-events/jevents.py --check '$(JEVENTS_CHECK_ARCH)' all pmu-events/arch
+
 #
 # All other targets get passed through:
 #
@@ -147,4 +159,4 @@ sync-headers:
 	$(print_msg)
 	$(make)
 
-.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers
+.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers jevents-check
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b74b534e1153..a330cd9de0fc 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -871,6 +871,7 @@ help:
 	@echo 'Perf maintainer targets:'
 	@echo '  clean			- clean all binary objects and build output'
 	@echo '  check-headers		- check which header copies are out of sync with the kernel'
+	@echo '  jevents-check		- report the pmu-events json keys that jevents.py never reads'
 	@echo '  sync-headers		- sync those header copies with the kernel, creating commits'
 	@echo '			  (SYNC_HEADERS_ARGS="..." passes options to the script)'
 
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index f4dca47eb49b..31e1eaca4917 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -3,15 +3,16 @@
 """Convert directories of JSON events to C code."""
 import argparse
 import csv
+import difflib
 import json
 import metric
 import os
 import sys
-from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
+from typing import (Any, Callable, Dict, List, Optional, Sequence, Set, Tuple)
 import collections
 
 # Global command line arguments.
-_args = None
+_args: Optional[argparse.Namespace] = None
 # List of regular event tables.
 _event_tables: list[str] = []
 # List of event tables generated from "/sys" directories.
@@ -190,11 +191,78 @@ class BigCString:
 
 _bcs = BigCString()
 
+# Names that some object in this process has been asked for.  Used only to
+# find the spelling an unread name was meant to be.  The set is per process
+# and the workers of the pool never share it, so a name is in it once some
+# object in the same worker has been built; every object asks for the same
+# names, so they are there before the first object is reported, but a read
+# that moved behind a condition would make a report depend on which worker
+# happened to build which file.
+_read_keys: Set[str] = set()
+
+# Unread name -> the names it could be a misspelling of, or () for none.  A table
+# repeats the same few names tens of thousands of times, and the name that
+# misses is the common case (Counter, many thousands of objects), so the
+# misses are kept too; _mark empties the cache when a name is read for the
+# first time, so a hint computed while _read_keys was still growing cannot
+# stick.
+_hints: Dict[str, Tuple[str, ...]] = {}
+
+# Whether the objects are wrapped at all.  Only the check reads what the
+# tracking records, so a build does not wrap anything.  Set in
+# main() and, because the workers may be fresh interpreters, passed to them
+# through _init_worker() rather than read from _args there.
+_track: bool = False
+
+
+class _JsonObject(dict):
+  """A JSON object that remembers which of its keys were looked at.
+
+  jevents.py picks the fields it wants out of each object by name and
+  ignores the rest, so a key it never names - a misspelled
+  "BriefDescript6ion", say - is dropped without a word and the event
+  silently loses its description.  Handing the parsed object to this
+  class and asking it afterwards which keys were never looked at makes
+  the misspelling visible.  Only get(), [] and 'in' are tracked: a read
+  through items(), values() or pop() would not be, so a new read of a
+  field belongs in one of the three.  The alternative, a list of the key names
+  jevents.py reads, would be a second copy of the code below that goes
+  stale the first time someone adds a field.
+  """
+
+  def __init__(self, jd: dict):
+    super().__init__(jd)
+    # dict.keys() rather than the object itself: iterating the object would
+    # go through __iter__, should that ever be hooked below.
+    self.unread: Set[str] = set(dict.keys(jd))
+
+  def _mark(self, key: str) -> None:
+    self.unread.discard(key)
+    if key not in _read_keys:
+      _read_keys.add(key)
+      _hints.clear()
+
+  def get(self, key: Any, default: Any = None) -> Any:
+    self._mark(key)
+    return dict.get(self, key, default)
+
+  def __getitem__(self, key: Any) -> Any:
+    self._mark(key)
+    return dict.__getitem__(self, key)
+
+  def __contains__(self, key: object) -> bool:
+    if isinstance(key, str):
+      self._mark(key)
+    return dict.__contains__(self, key)
+
+
 class JsonEvent:
   """Representation of an event loaded from a json file dictionary."""
 
   def __init__(self, jd: dict):
     """Constructor passed the dictionary of parsed json values."""
+    if _track:
+      jd = _JsonObject(jd)
 
     def llx(x: int) -> str:
       """Convert an int to a string similar to a printf modifier of %#llx."""
@@ -416,6 +484,39 @@ class JsonEvent:
         raise argparse.ArgumentTypeError('Cannot find arch std event:', arch_std)
 
     self.event = real_event(self.name, event)
+    # Keys of the JSON object that no code above looked at, set last so that
+    # the copy of an architecture standard event above does not overwrite it.
+    # A name that is a near miss for one the code reads, and whose name the
+    # object does not carry with the same text, is almost certainly a
+    # misspelling of it: the object lost the field the reader was looking for.
+    # A name that is not a near miss is left alone, because a table is free to
+    # carry metadata jevents.py has no use for.
+    self.mistyped: Optional[Dict[str, Tuple[str, ...]]] = None
+    if not _track:
+      return
+    assert isinstance(jd, _JsonObject)
+    unread = jd.unread
+    if unread:
+      found = {}
+      for key in unread:
+        if key not in _hints:
+          lowered = {name.lower(): name for name in _read_keys}
+          # Two names can be equally close - RetirementLatencyMax and its Min -
+          # and naming one of them would be a coin toss presented as an answer.
+          match = difflib.get_close_matches(key.lower(), sorted(lowered), n=2, cutoff=0.8)
+          _hints[key] = tuple(lowered[m] for m in match)
+        hints = _hints[key]
+        if not hints:
+          continue
+        # Reported even when the object also carries a name it could be a
+        # misspelling of: two names can be equally close (RetirementLatencyMax
+        # next to its Min, which can hold the same value), so a matching value
+        # does not say which one this key was meant to be, and suppressing on
+        # it hid a typo of the other.  Either way this is a key that nothing
+        # reads, which is what the check reports.
+        found[key] = hints
+      if found:
+        self.mistyped = found
 
   def __repr__(self) -> str:
     """String representation primarily for debugging."""
@@ -453,6 +554,83 @@ class JsonEvent:
     return f'{make_comment(s)}\t{{ { _bcs.offsets[s] } }},\n'
 
 
+def _report_mistyped_keys() -> bool:
+  """Report the JSON keys that jevents.py did not look for.
+
+  jevents.py takes the fields it wants out of each object by name.  A key
+  spelled differently from the name it looks for is never read, the object is
+  built without whatever the field held, and the build says nothing; that is
+  how "BriefDescript6ion" in the AMD tables and "PublicDescrition" in the
+  AmpereOneX mmu table got in.  A name that is a near miss for one jevents.py
+  reads is reported, with the spelling it was probably meant to be and the
+  files carrying it.  A name that is not a near miss is not reported: a
+  table may carry metadata for another reader, and reporting it would mean
+  keeping a list of such names in this file that every vendor adding one
+  would have to edit.
+
+  A table that cannot be parsed is reported the same way and does not stop
+  the rest of the tree from being read, so one broken file does not hide the
+  misspellings in the other 1000.
+  """
+  assert _args is not None
+  found: Dict[str, collections.Counter] = {}
+  hints: Dict[str, Tuple[str, ...]] = {}
+  objects = 0
+  for (path, _), events in _json_cache.items():
+    for event in events:
+      if not event.mistyped:
+        continue
+      objects += 1
+      for key, hint in event.mistyped.items():
+        found.setdefault(key, collections.Counter())[path] += 1
+        hints.setdefault(key, hint)
+  if not found:
+    return False
+
+  paths = {path for files in found.values() for path in files}
+  start = _args.starting_dir
+  print(f'jevents: error: {objects} object(s) in '
+        f'{len(paths)} file(s) carry a key that looks like a misspelling:', file=sys.stderr)
+  for key in sorted(found):
+    files = found[key]
+    guess = ' or '.join(repr(name) for name in hints[key])
+    print(f'  {key!r}, did you mean {guess}? ({sum(files.values())} object(s) in '
+          f'{len(files)} file(s)):', file=sys.stderr)
+    where = sorted(os.path.relpath(path, start) for path in files)
+    for path in where[:5]:
+      print(f'    {path}', file=sys.stderr)
+    if len(where) > 5:
+      print(f'    ... and {len(where) - 5} more file(s)', file=sys.stderr)
+
+  return True
+
+
+def _report_metrics_without_expr() -> bool:
+  """Report the metrics that carry a name but no expression.
+
+  A MetricExpr key spelled another way is not read, so the object keeps its
+  name and loses its expression.  Unlike the key report above this does not
+  depend on how close a spelling is: a metric the rewrite cannot handle would
+  fail a build however the lost key was spelled.
+  """
+  assert _args is not None
+  start = _args.starting_dir
+  missing = sorted((os.path.relpath(path, start), event.metric_name)
+                   for (path, _), events in _json_cache.items()
+                   for event in events
+                   if event.metric_name and event.metric_expr is None)
+  if not missing:
+    return False
+  print(f'jevents: error: {len(missing)} metric(s) have a name but no '
+        f'expression, so their MetricExpr key is missing or misspelled:',
+        file=sys.stderr)
+  for where, name in missing[:20]:
+    print(f'  {name!r} in {where}', file=sys.stderr)
+  if len(missing) > 20:
+    print(f'    ... and {len(missing) - 20} more metric(s)', file=sys.stderr)
+  return True
+
+
 _json_cache: Dict[Tuple[str, str], Sequence[JsonEvent]] = {}
 def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
   """Read json events from the specified file."""
@@ -460,17 +638,20 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
+    if not _track:
+      print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
     if event.metric_name and event.metric_expr is None:
       # A MetricName without a MetricExpr leaves nothing to put in the
-      # generated tables, whether or not the name has a dash, so the build
-      # must not go on.  Name the metric instead of letting the table printer
-      # assert on the None.
-      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      # generated tables, whether or not the name has a dash, so a build must
+      # not go on.  Name the metric instead of letting the table printer
+      # assert on the None; --check reports it and keeps reading the rest.
+      if not _track:
+        raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
+      continue
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -487,8 +668,12 @@ def read_json_events(path: str, topic: str) -> Sequence[JsonEvent]:
     _json_cache[key] = _read_json_events_impl(path, topic)
   return _json_cache[key]
 
-def preprocess_arch_std_files(archpath: str) -> None:
-  """Read in all architecture standard events."""
+def preprocess_arch_std_files(archpath: str, failures: Optional[List[Tuple[str, Exception]]] = None) -> None:
+  """Read in all architecture standard events.
+
+  A failure is raised, or appended to failures when the caller can carry on
+  without the table; either way the tables after it are still read.
+  """
   for item in os.scandir(archpath):
     if not item.is_file() or not item.name.endswith('.json'):
       continue
@@ -499,7 +684,10 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
+        failure = RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}')
+        if failures is None:
+          raise failure from e
+        failures.append((item.path, failure))
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1448,16 +1636,27 @@ def _parallel_worker_count(num_tasks: int) -> int:
   return max(1, min(getattr(os, 'process_cpu_count', os.cpu_count)() or 1,
                     _max_parallel_workers, num_tasks))
 
-def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Sequence[JsonEvent]]:
+def _parallel_read_json_events(task: Tuple[str, str]) -> Tuple[str, str, Any]:
   path, topic = task
-  return path, topic, _read_json_events_impl(path, topic)
-
-def _init_worker(std_events: dict) -> None:
-  global _arch_std_events
+  try:
+    return path, topic, _read_json_events_impl(path, topic)
+  except Exception as e:
+    if not _track:
+      # The build stops here: keep the file and the reason in one message,
+      # the pool adds the traceback.
+      raise RuntimeError(f'Failure processing \'{path}\': {e}') from None
+    # Carried back rather than raised: the parent knows the tree and can
+    # report the file and keep going, which is the point of reading the
+    # tables this build does not use.
+    return path, topic, e
+
+def _init_worker(std_events: dict, track: bool) -> None:
+  global _arch_std_events, _track
   _arch_std_events = std_events
+  _track = track
 
 def main() -> None:
-  global _args
+  global _args, _track
 
   def dir_path(path: str) -> str:
     """Validate path is a directory for argparse."""
@@ -1484,12 +1683,12 @@ def main() -> None:
       if item.is_dir():
         ftw(item.path, list(parents) + [item.name], action)
 
-  ap = argparse.ArgumentParser()
+  ap = argparse.ArgumentParser(description='Generate pmu-events.c from the json tables')
   ap.add_argument('arch', help='Architecture name like x86')
   ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all", and that is what an omitted
-model means. For architectures like ARM64 with an implementor/model, the
-model must include the implementor such as "arm/cortex-a34".''',
+reduce the code size.  Normally set to "all". For architectures like
+ARM64 with an implementor/model, the model must include the implementor
+such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
@@ -1497,12 +1696,32 @@ model must include the implementor such as "arm/cortex-a34".''',
       help='Root of tree containing architecture directories containing json files'
   )
   ap.add_argument('output_file', nargs='?', default=None,
-                  help='file to write the tables to, default standard output')
+                  help='file to write the tables to, default standard output; '
+                       'not written, and not opened, with --check')
   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')
+                       'apart from the tables; ignored with --check')
+  ap.add_argument('--check', action='store_true',
+                  help='read the tables, report the keys that look like a '
+                       'misspelling of a name this file reads on standard '
+                       'error, and exit without writing any table; pass "all" '
+                       'as both arch and model to cover the whole tree.  '
+                       'Nothing is reported when every key is read and every '
+                       'table parses.  The exit status is 0 when the tree is '
+                       'clean, 1 when something was reported and 2 when a '
+                       'table could not be read, so it can gate a script')
   _args = ap.parse_args()
 
+  if _args.check and (_args.output_file is not None or _args.output_string_file is not None):
+    ap.error('--check writes no table; drop the output arguments')
+  if _args.check:
+    if _args.arch == 'none':
+      ap.error('--check needs an architecture to read')
+    # The model only picks the tables a build needs; a check is about the
+    # tree, so it reads all of it and the model means nothing here.
+    _args.model = 'all'
+    _track = True
+
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
@@ -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 and not _args.check and _args.arch != 'none':
+      print(f'jevents: error: no event table under \'{_args.starting_dir}\' '
+            f'matched arch \'{_args.arch}\'', file=sys.stderr)
+      sys.exit(1)
+
+    with concurrent.futures.ProcessPoolExecutor(max_workers=_parallel_worker_count(len(tasks)),
+                                                initializer=_init_worker,
+                                                initargs=(_arch_std_events, _args.check)) as executor:
+      for path, topic, events in executor.map(_parallel_read_json_events, tasks):
+        if isinstance(events, Exception):
+          if not _args.check:
+            raise RuntimeError(f'Failure processing \'{path}\'') from events
+          failed.append((path, events))
+        else:
+          _json_cache[(path, topic)] = events
+  except (RuntimeError, ValueError, OSError, argparse.ArgumentTypeError) as e:
+    if not _args.check:
+      raise
+    failed.append((_args.starting_dir, e))
+
+  if _args.check:
+    # A table this check does not need is reported, not fatal: the check reads
+    # the whole tree and a caller only asked about one part of it.
+    for path, error in failed:
+      text = str(error)
+      # The architecture standard preprocess names the file and its directory
+      # already, so prefixing the path here only repeats it.
+      if text.startswith('Failure processing'):
+        print(f'jevents: error: {text}', file=sys.stderr)
+      else:
+        print(f'jevents: error: '
+              f'{os.path.relpath(path, _args.starting_dir)}: {text}',
+              file=sys.stderr)
+    reported = _report_mistyped_keys()
+    reported = _report_metrics_without_expr() or reported
+    if failed:
+      sys.exit(2)
+    sys.exit(1 if reported else 0)
 
   for arch in archs:
     arch_path = f'{_args.starting_dir}/{arch}'
@@ -1540,8 +1799,9 @@ model must include the implementor such as "arm/cortex-a34".''',
   assert _bcs is not None
   _bcs.compute()
 
-  # Every table has been read by now, so opening the output here cannot truncate
-  # a file the caller named when a table turns out not to parse.
+  # Everything that reads a table has run by now, so opening the output here
+  # cannot truncate one on a failure: --check leaves the files it was given
+  # alone, and a table that does not parse stops before this point.
   if _args.output_file in (None, '-'):
     _args.output_file = sys.stdout
   else:
@@ -1577,6 +1837,7 @@ struct pmu_table_entry {
 };
 
 """)
+
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
     _args.output_file.write('static const char *const big_c_string =\n')
@@ -1607,5 +1868,27 @@ struct pmu_table_entry {
   if _args.output_string_file:
     _args.output_string_file.close()
 
+def _main() -> None:
+  """Run main(), turning a failed check into exit status 2.
+
+  The try lives in a function, not at module scope: a module-level
+  `except ... as exc` binds a global, and every function here that uses `e`
+  as a local then trips pylint's redefined-outer-name (W0621) in the
+  pylint-pmu-events build target.
+  """
+  try:
+    main()
+  except SystemExit:
+    raise
+  except KeyboardInterrupt:
+    raise
+  except BaseException as exc:
+    # 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: {exc}', file=sys.stderr)
+      sys.exit(2)
+    raise
+
 if __name__ == '__main__':
-  main()
+  _main()
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..2254bc96352c
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Check that jevents.py reports unread json keys and refuses a broken table
+
+shelldir=$(dirname "$0")
+. "${shelldir}"/lib/setup_python.sh
+
+# The check reads the tables and needs no build and no hardware, but an
+# installed perf has no pmu-events/ next to the binary.
+pmudir=$(readlink -f "${shelldir}/../../pmu-events")
+if [ ! -f "${pmudir}/jevents.py" ] || [ ! -f "${pmudir}/metric.py" ]; then
+	echo "Skipping: no pmu-events/jevents.py under ${pmudir}"
+	exit 2
+fi
+
+workdir=$(mktemp -d /tmp/jevents_check.XXXXXX)
+trap 'rm -rf "${workdir}"' EXIT
+
+# A table with one key jevents.py reads and one event, so that a clean run
+# says nothing and exits 0.
+mkdir -p "${workdir}/arch/x86/model"
+cat > "${workdir}/arch/x86/model/events.json" <<'JSON'
+[
+  {
+    "EventName": "CHECK_TEST_EVENT",
+    "EventCode": "0x1",
+    "BriefDescription": "a table the check should accept"
+  }
+]
+JSON
+
+run_check() {
+	"${PYTHON}" "${pmudir}/jevents.py" --check x86 all "${workdir}/arch"
+}
+
+run_check
+if [ $? -ne 0 ]; then
+	echo "pmu-events json keys [Failed: a clean table was reported]"
+	exit 1
+fi
+
+# A near miss for a name jevents.py reads is reported, and the status is 1.
+sed -i 's/"BriefDescription"/"BriefDescript6ion"/' \
+	"${workdir}/arch/x86/model/events.json"
+output=$(run_check 2>&1)
+ret=$?
+case "${ret}" in
+1) ;;
+*)	echo "pmu-events json keys [Failed: a misspelled key exited ${ret}]"
+	exit 1 ;;
+esac
+if ! echo "${output}" | grep -q "BriefDescript6ion"; then
+	echo "pmu-events json keys [Failed: the misspelled key was not reported]"
+	exit 1
+fi
+
+# A table that does not parse is a failure of the check and not a finding:
+# the status is 2, which this script must not pass on as a skip.
+echo '{ "truncated": ' > "${workdir}/arch/x86/model/broken.json"
+run_check >/dev/null 2>&1
+ret=$?
+if [ "${ret}" -ne 2 ]; then
+	echo "pmu-events json keys [Failed: an unreadable table exited ${ret}]"
+	exit 1
+fi
+
+echo "pmu-events json keys [Success]"
+exit 0
-- 
2.53.0

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

[sashiko-bot]

> jevents.py takes the fields it wants out of each JSON object by name:
> jd.get('BriefDescription'), 'EventCode' in jd, and the names in
> event_fields. A key spelled another way is dropped without a word, and
> the object is built without whatever that field held.
> 
> Two of those are in the tree now. "BriefDescript6ion" is in one entry
> each of amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-
> store.json, and since those objects carry no other description,
> ls_inef_sw_pref.all is generated with an empty text field.
> "PublicDescrition" was in 16 of the 28 entries of
> arm64/ampere/ampereonex/mmu.json; nothing a user saw changed there,
> because each of the 16 also had a BriefDescription with the same text,
> which is why the typo could sit there unnoticed.
> 
> Add a --check mode that reads every architecture, not just the one being
> [ ... ]
> perf with it, ran the perf test above, ran the check over the whole
> tree, compared the generated tables with those from an unpatched tree on
> x86, arm64 and all architectures at once, and ran mypy on jevents.py.
> 
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v5 0/3] perf jevents: Check the JSON keys a table may carry

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key spelled differently is dropped without a word
and the object is built without whatever that field held.  Two of those
are in the tree: "PublicDescrition" in 16 entries of an Arm table, where
the lost long description repeated the brief one and so nothing a user
saw changed, and "BriefDescript6ion" in one entry each of three AMD
tables, where the event is generated with no text at all.

Patch 2 makes the model argument optional when the tables are generated by
hand, and patch 1 names the metric that has a name and no MetricExpr
instead of failing on the file.  Patch 3 says which keys a table may carry
and checks the tree against that, instead of reporting the keys a run of
jevents.py did not read.

v5 rewrites patch 3 rather than revising it.  The review asked for
something strict and simple instead of a near-miss heuristic, so the
similarity cut-off, the tracking of which keys were read and the wrapped
objects are gone: the schema is a list of 51 names, 47 of which this
file reads, and a key outside it is reported with the object that
carries it.  A JSON Schema would say the same thing, but perf's build
would then require jsonschema, which it does not use today and which is
not in every builder's python; the list holds the same names either way,
and a vendor that adds a field adds one line here.

Patches 1 and 2 are unchanged.  The check is still something a maintainer
runs - make check-jevents, with JEVENTS_CHECK_ARCH to look at one
architecture - rather than a step of the build, so the generated tables are
byte identical.  An architecture that is not there, or named as a path, is
an error rather than a silent pass, an object where a list belongs is
reported, the name of a metricgroup is not treated as a key, and a small
perf test runs those cases.

The two misspellings the check reports in the tree today were posted
separately in November.

Link: https://lore.kernel.org/linux-perf-users/20261108183846.3804875-1-takano32@gmail.com/
Link: https://lore.kernel.org/linux-perf-users/20261108184600.3810059-1-takano32@gmail.com/

TAKANO Mitsuhiro (3):
  perf jevents: Name the metric when a MetricExpr is missing
  perf jevents: Make the model argument optional
  perf jevents: Check the JSON keys a table may carry

 tools/perf/Makefile                     |  13 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 228 ++++++++++++++++++++----
 tools/perf/tests/shell/jevents_check.sh |  62 +++++++
 4 files changed, 272 insertions(+), 32 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

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

[TAKANO Mitsuhiro]

jevents.py reads each field it wants out of a JSON object by name, so a
MetricExpr key that is missing or spelled another way leaves the object
with a MetricName and no expression. Two different failures followed,
neither of which named the metric: the rewrite in _read_json_events_impl()
raised AttributeError on the None, and a metric that got that far reached
print_pending_metrics(), whose assert fired on it.

Raise in the reader instead, naming the metric, before either of them can
happen. The check is not inside the '-' test that decides whether a metric
takes part in the rewrite: a name with a dash in it can lose its
expression the same way.

Two smaller changes on the same path:

- An architecture standard table that does not parse now carries the
   reason in its message ("Failure processing '<file>' in '<dir>':
   <cause>").  The traceback used to end with the file name alone, and
   only the chained exception said why.

- A table that does not parse is reported on stderr with a "jevents:
   error:" prefix instead of on stdout, where it was mixed with the
   generated output.

And the output files are opened after every table has been read, and not
by argparse, so a build that fails while reading leaves a file the caller
named as it was instead of truncating it to the header.  "Every table"
includes metricgroups.json, whose first pass runs before this point, and
the string table built from all of them: nothing that parses a file runs
after the output is opened.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v5: patch 3 says which keys a table may carry and checks the tree
    against that list, replacing the near-miss report that v4 had.
    The list is 51 names, 47 of which jevents.py reads; an architecture
    that is not there or is named as a path is an error (status 2)
    instead of a silent pass, and an object written without the list
    brackets is reported.  The name of a metricgroup is not treated as
    a key.  jsonschema is not used because perf's build would then
    require it, and the list holds the same names a schema would.
    The target is check-jevents, next to check-headers, and takes
    JEVENTS_CHECK_ARCH; a shell test covers a reported key, a clean
    tree and a missing architecture.

 tools/perf/pmu-events/jevents.py | 78 +++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..61f778dcc1e5 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -460,11 +460,17 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f"Exception processing {path}")
+    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
+    if event.metric_name and event.metric_expr is None:
+      # A MetricName without a MetricExpr leaves nothing to put in the
+      # generated tables, whether or not the name has a dash, so the build
+      # must not go on.  Name the metric instead of letting the table printer
+      # assert on the None.
+      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -493,7 +499,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-        raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\'') from e
+      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1490,33 +1496,13 @@ 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()
 
-  _args.output_file.write(f"""/* SPDX-License-Identifier: GPL-2.0 */
-/* THIS FILE WAS AUTOGENERATED BY `jevents.py arch={_args.arch} model={_args.model}` ! */
-""")
-  _args.output_file.write("""
-#include <pmu-events/pmu-events.h>
-#include "util/header.h"
-#include "util/pmu.h"
-#include <string.h>
-#include <stddef.h>
-
-struct compact_pmu_event {
-\tint offset;
-};
-
-struct pmu_table_entry {
-\tconst struct compact_pmu_event *entries;
-\tuint32_t num_entries;
-\tstruct compact_pmu_event pmu_name;
-};
-
-""")
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
@@ -1553,6 +1539,44 @@ struct pmu_table_entry {
 
   assert _bcs is not None
   _bcs.compute()
+
+  # Every table has been read by now, so opening the output here cannot truncate
+  # a file the caller named when a table turns out not to parse.
+  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}` ! */
+""")
+  _args.output_file.write("""
+#include <pmu-events/pmu-events.h>
+#include "util/header.h"
+#include "util/pmu.h"
+#include <string.h>
+#include <stddef.h>
+
+struct compact_pmu_event {
+\tint offset;
+};
+
+struct pmu_table_entry {
+\tconst struct compact_pmu_event *entries;
+\tuint32_t num_entries;
+\tstruct compact_pmu_event pmu_name;
+};
+
+""")
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
     _args.output_file.write('static const char *const big_c_string =\n')
-- 
2.53.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf jevents: missing file path in _read_json_events_impl()
  MetricExpr error
--

commit e2d62b223b86b6803d9d5c49c56d277f5acb0d3e
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 event defines
MetricName without MetricExpr, improves error reporting on parse failures, and
defers opening the output files until all tables have been parsed.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 080e04b51b8c5..61f778dcc1e56 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -460,11 +460,17 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
>      with open(path, encoding='utf-8') as f:
>        events = json.load(f, object_hook=JsonEvent)
>    except BaseException:
> -    print(f"Exception processing {path}")
> +    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
>      raise
>    metrics: list[Tuple[str, str, metric.Expression]] = []
>    for event in events:
>      event.topic = topic
> +    if event.metric_name and event.metric_expr is None:
> +      # A MetricName without a MetricExpr leaves nothing to put in the
> +      # generated tables, whether or not the name has a dash, so the build
> +      # must not go on.  Name the metric instead of letting the table printer
> +      # assert on the None.
> +      raise RuntimeError(f'Metric {event.metric_name!r} has no MetricExpr')

[Severity: Low]
Does raising RuntimeError outside the try/except block without path in the
message lose the offending file name?

Model and vendor JSON tables are parsed in main() via
executor.map(_parallel_read_json_events, tasks) outside ftw(), so when a
metric is missing MetricExpr, the exception escapes the worker pool without
reporting which JSON file failed.

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

[PATCH v5 2/3] perf jevents: Make the model argument optional

[TAKANO Mitsuhiro]

The model argument carries a default of "all", but a positional argument
with a default and no nargs is still required: "jevents.py x86 <dir>" exits
with "the following arguments are required: model", and so does any caller
that only wants the tables for an architecture.

Make it nargs='?' so the default is reachable and an omitted model means
"all".  Builds pass it explicitly, so the generated tables do not change.

Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v5: patch 3 says which keys a table may carry and checks the tree
    against that list, replacing the near-miss report that v4 had.
    The list is 51 names, 47 of which jevents.py reads; an architecture
    that is not there or is named as a path is an error (status 2)
    instead of a silent pass, and an object written without the list
    brackets is reported.  The name of a metricgroup is not treated as
    a key.  jsonschema is not used because perf's build would then
    require it, and the list holds the same names a schema would.
    The target is check-jevents, next to check-headers, and takes
    JEVENTS_CHECK_ARCH; a shell test covers a reported key, a clean
    tree and a missing architecture.

 tools/perf/pmu-events/jevents.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 61f778dcc1e5..f4dca47eb49b 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -1486,10 +1486,10 @@ def main() -> None:
 
   ap = argparse.ArgumentParser()
   ap.add_argument('arch', help='Architecture name like x86')
-  ap.add_argument('model', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all". For architectures like
-ARM64 with an implementor/model, the model must include the implementor
-such as "arm/cortex-a34".''',
+  ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
+reduce the code size.  Normally set to "all", and that is what an omitted
+model means. For architectures like ARM64 with an implementor/model, the
+model must include the implementor such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
-- 
2.53.0

Re: [PATCH v5 2/3] perf jevents: Make the model argument optional

[sashiko-bot]

> The model argument carries a default of "all", but a positional argument
> with a default and no nargs is still required: "jevents.py x86 <dir>" exits
> with "the following arguments are required: model", and so does any caller
> that only wants the tables for an architecture.
> 
> Make it nargs='?' so the default is reachable and an omitted model means
> "all".  Builds pass it explicitly, so the generated tables do not change.
> 
> Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v5 3/3] perf jevents: Check the JSON keys a table may carry

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name -
jd.get('BriefDescription'), 'EventCode' in jd, jd['ExtSel'], and the
fourteen names in event_fields that it looks for with 'key in jd' - so a
key spelled another way is dropped without a word and the object is built
without whatever that field held.  Nothing in the build and nothing in the
generated table says so.

Two of those are in the tree.  "PublicDescrition" is in 16 of the 28
entries of arm64/ampere/ampereonex/mmu.json, where the long description
each of them lost repeated the BriefDescription, which is why it could sit
there unnoticed.  "BriefDescript6ion" is in one entry each of
amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-store.json,
and those objects carry no other description, so ls_inef_sw_pref.all is
generated with an empty text field on Zen 4, 5 and 6.

Check the tables against the keys they may carry instead of watching which
ones a run reads.  The list is 51 names: 47 of them this file reads, 33 by
name and 14 through event_fields; Experimental is read by metric.py; and
Counter, CountersNumFixed and CountersNumGeneric are read by nothing in
perf although the tables carry them, so a vendor that adds a field changes
one line here by agreement.  A JSON Schema would express the same list,
but perf's build would then require jsonschema, which it does not use
today and which is not in every builder's python, so the list lives in
this file and is the schema.  The name of a metricgroup is not a key: the
file that ends in metricgroups.json maps a name of its own choosing to a
description, and only that description is read.

Add --check, which reads every architecture under starting_dir, or the one
named by the first argument, names each key it does not accept with the
object that carries it on stderr, and exits 0 when the tree is clean, 1
when something was reported and 2 when a table could not be read or there
was nothing to read.  The name of an architecture is a name and not a
path, so ".." is refused rather than walking out of the tree, and an
architecture with no tables of its own is an error rather than a pass: a
name that was typed wrong is the first thing this is meant to catch.  An
object where a list belongs is reported too, because every key of it
would go unchecked.  A build does not run the check, and the generated
tables are byte identical: x86, arm64 and all architectures compared
against the same tree without this patch.

Add a "check-jevents" target next to check-headers, with
JEVENTS_CHECK_ARCH to check one architecture while working on it, and a
perf test for the cases that matter: a clean tree, a key the generator
does not read, and an architecture that is not there.

The two misspellings in the tree are fixed by separate patches; until they
are applied the target reports them, which is what it is for.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v5: patch 3 says which keys a table may carry and checks the tree
    against that list, replacing the near-miss report that v4 had.
    The list is 51 names, 47 of which jevents.py reads; an architecture
    that is not there or is named as a path is an error (status 2)
    instead of a silent pass, and an object written without the list
    brackets is reported.  The name of a metricgroup is not treated as
    a key.  jsonschema is not used because perf's build would then
    require it, and the list holds the same names a schema would.
    The target is check-jevents, next to check-headers, and takes
    JEVENTS_CHECK_ARCH; a shell test covers a reported key, a clean
    tree and a missing architecture.

 tools/perf/Makefile                     |  13 ++-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 148 +++++++++++++++++++++++-
 tools/perf/tests/shell/jevents_check.sh |  62 ++++++++++
 4 files changed, 220 insertions(+), 4 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 00baf90e5bda..a1aac2fdf5e1 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -140,6 +140,17 @@ check-headers:
 sync-headers:
 	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
 
+# The pmu-events tables are data, and a key that no parser reads is invisible
+# until a description goes missing, so the keys a table may carry are a fixed
+# list in jevents.py and this target checks the tree against it.  It reads the
+# source tree and needs no build.  Makefile.perf sets PYTHON for the build and
+# this wrapper does not include it, so default here and let PYTHON= on the
+# command line win when it is given.
+JEVENTS_CHECK_ARCH ?= all
+PYTHON ?= python3
+check-jevents:
+	@$(PYTHON) ./pmu-events/jevents.py --check $(JEVENTS_CHECK_ARCH) all pmu-events/arch
+
 #
 # All other targets get passed through:
 #
@@ -147,4 +158,4 @@ sync-headers:
 	$(print_msg)
 	$(make)
 
-.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers
+.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers check-jevents
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b74b534e1153..ec7dde9ec6e2 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -871,6 +871,7 @@ help:
 	@echo 'Perf maintainer targets:'
 	@echo '  clean			- clean all binary objects and build output'
 	@echo '  check-headers		- check which header copies are out of sync with the kernel'
+	@echo '  check-jevents		- check the pmu-events json tables against the keys they may carry'
 	@echo '  sync-headers		- sync those header copies with the kernel, creating commits'
 	@echo '			  (SYNC_HEADERS_ARGS="..." passes options to the script)'
 
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index f4dca47eb49b..1d114cb8ed29 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -7,7 +7,7 @@ import json
 import metric
 import os
 import sys
-from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
+from typing import (Callable, Dict, List, Optional, Sequence, Set, Tuple)
 import collections
 
 # Global command line arguments.
@@ -1456,6 +1456,136 @@ def _init_worker(std_events: dict) -> None:
   global _arch_std_events
   _arch_std_events = std_events
 
+# The keys a json object under arch/ may carry.  jevents.py reads 47 of them,
+# 33 by name and 14 through event_fields below; Experimental is read by
+# metric.py; Counter, CountersNumFixed and CountersNumGeneric are read by
+# nothing in perf, but the tables carry them, so a vendor that adds a field
+# changes one line here by agreement.  A key outside this set is a misspelling,
+# or a field that has to be agreed on and added here first: "PublicDescrition"
+# sat in 16 entries of an arm64 table and every one of them lost its long
+# description without a word.  This list is the whole schema - it says what may
+# exist in a table, not that the table uses it.
+_TABLE_KEYS: Set[str] = {
+    'AggregationMode', 'AnyThread', 'ArchStdEvent', 'BriefDescription',
+    'Compat', 'ConfigCode', 'Counter', 'CounterMask',
+    'CountersNumFixed', 'CountersNumGeneric', 'Data_LA', 'DefaultMetricgroupName',
+    'DefaultShowEvents', 'Deprecated', 'EdgeDetect', 'EnAllCores',
+    'EnAllSlices', 'Errata', 'EventCode', 'EventidCode',
+    'EventName', 'Experimental', 'ExtSel', 'FCMask',
+    'Filter', 'Invert', 'LegacyCacheCode', 'LegacyConfigCode',
+    'MetricConstraint', 'MetricExpr', 'MetricGroup', 'MetricgroupNoGroup',
+    'MetricName', 'MetricThreshold', 'MSRIndex', 'MSRValue',
+    'NodeType', 'PEBS', 'PerPkg', 'PortMask',
+    'PublicDescription', 'RdWrMask', 'RetirementLatencyMax', 'RetirementLatencyMean',
+    'RetirementLatencyMin', 'SampleAfterValue', 'ScaleUnit', 'SliceId',
+    'ThreadMask', 'UMask', 'Unit',
+}
+
+
+def _check_tables(root: str, arch: str) -> int:
+  """Report the keys in the json tables under root that no reader looks for.
+
+  With arch "all" every architecture is read; otherwise that one is, together
+  with the common and test tables that a build of it reads as well.
+
+  Returns 0 when the tree is clean, 1 when something was reported and 2 when a
+  table could not be read or there was nothing to read, so that a script can
+  gate on the status.
+  """
+  reported = 0
+  unreadable = 0
+  files = 0
+  bad_files: Set[str] = set()
+
+  def report(path: str, what: str) -> None:
+    nonlocal reported
+    bad_files.add(path)
+    print(f'{path}: {what}', file=sys.stderr)
+    reported += 1
+
+  if arch == 'all':
+    roots = [root]
+    arch_root = None
+  else:
+    # arch names a directory under the tables, it is not a path: ".." or "x86/"
+    # would walk out of the tree and report on tables that are not part of it.
+    if not arch or '/' in arch or arch in ('.', '..'):
+      print(f'"{arch}" is not the name of a directory under the tables',
+            file=sys.stderr)
+      return 2
+    arch_root = os.path.join(root, arch)
+    if not os.path.isdir(arch_root):
+      print(f'{root}: there is no architecture "{arch}"', file=sys.stderr)
+      return 2
+    roots = [arch_root]
+    roots += [os.path.join(root, name) for name in ('common', 'test')
+              if name != arch and os.path.isdir(os.path.join(root, name))]
+
+  paths: List[str] = []
+  arch_tables = 0
+  for top in roots:
+    for dirpath, _, names in os.walk(top):
+      for name in sorted(names):
+        if name.endswith('.json'):
+          paths.append(os.path.join(dirpath, name))
+          if top == arch_root:
+            arch_tables += 1
+
+  # The common and test tables exist for every architecture, so an architecture
+  # with tables of its own is what makes a check of it meaningful.
+  if arch_root and not arch_tables:
+    print(f'{arch_root}: the tables of "{arch}" are missing', file=sys.stderr)
+    return 2
+
+  for path in sorted(paths):
+    files += 1
+    try:
+      with open(path, encoding='utf-8') as f:
+        data = json.load(f)
+    except (OSError, ValueError) as e:
+      print(f'{path}: cannot be read: {e}', file=sys.stderr)
+      unreadable += 1
+      continue
+    if path.endswith('metricgroups.json'):
+      # The name of a group is a name of its own choosing and only the
+      # description held under it is read, so there is no key to check here.  A
+      # list where an object belongs means the braces were left out.
+      if not isinstance(data, dict):
+        report(path, f'the table holds a {type(data).__name__}, not an object')
+      continue
+    if isinstance(data, dict):
+      # A list of objects written without its brackets: every key of the one
+      # object in it would go unchecked.
+      report(path, 'the table holds an object, not a list of objects')
+      continue
+    if not isinstance(data, list):
+      report(path, f'the table holds a {type(data).__name__}, '
+                   'not a list of objects')
+      continue
+    for index, obj in enumerate(data):
+      if not isinstance(obj, dict):
+        report(path, f'entry {index} is a {type(obj).__name__}, not an object')
+        continue
+      named = next((obj[k] for k in ('EventName', 'MetricName', 'ArchStdEvent')
+                    if isinstance(obj.get(k), str) and obj[k]), '')
+      what = f'entry {index} ({named})' if named else f'entry {index}'
+      for key in sorted(set(obj) - _TABLE_KEYS):
+        report(path, f'{what}: unknown key "{key}"')
+
+  if not files:
+    print(f'{root}: there are no tables to check', file=sys.stderr)
+    return 2
+  if unreadable:
+    print(f'{unreadable} table(s) could not be read', file=sys.stderr)
+  if reported:
+    print(f'{reported} problem(s) in {len(bad_files)} of {files} tables: a key '
+          'this file does not read is a misspelling, or a field to add to '
+          '_TABLE_KEYS', file=sys.stderr)
+  if unreadable:
+    return 2
+  return 1 if reported else 0
+
+
 def main() -> None:
   global _args
 
@@ -1497,12 +1627,24 @@ model must include the implementor such as "arm/cortex-a34".''',
       help='Root of tree containing architecture directories containing json files'
   )
   ap.add_argument('output_file', nargs='?', default=None,
-                  help='file to write the tables to, default standard output')
+                  help='file to write the tables to, default standard output; '
+                       'not opened with --check')
   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')
+                       'apart from the tables; ignored with --check')
+  ap.add_argument('--check', action='store_true',
+                  help='check the tables under starting_dir against the keys '
+                       'they are allowed to carry and exit without writing '
+                       'anything; arch selects the architecture to read, "all" '
+                       'reads every one, and model is not used.  The exit '
+                       'status is 0 when the tree is clean, 1 when something '
+                       'was reported and 2 when a table could not be read or '
+                       'there was nothing to read')
   _args = ap.parse_args()
 
+  if _args.check:
+    sys.exit(_check_tables(_args.starting_dir, _args.arch))
+
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..311ca5f5ab15
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,62 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# perf jevents tests: --check reports a key the generator does not read, and an
+# architecture that is not there is an error rather than a clean tree.
+
+set -e
+
+shelldir=$(dirname "$0")
+jevents="${shelldir}/../../pmu-events/jevents.py"
+python=${PYTHON:-python3}
+tmpdir=$(mktemp -d "${TMPDIR:-/tmp}/jevents-check.XXXXXX") || exit 2
+trap 'rm -rf "${tmpdir}"' EXIT
+
+# An installed perf has the test but not the generator, and a python that cannot
+# run the generator is not a failure of the check.
+[ -r "${jevents}" ] || exit 2
+"${python}" -c 'import sys; sys.exit(sys.version_info[0] != 3)' 2>/dev/null ||
+	exit 2
+
+mkdir -p "${tmpdir}/x86/model" "${tmpdir}/empty" "${tmpdir}/common" \
+	"${tmpdir}/test"
+
+# The name of a metricgroup is not a key, even when it is spelled like one.
+printf '{"Unit": "a description"}\n' \
+	> "${tmpdir}/x86/model/events-metricgroups.json"
+printf '[{"EventName": "test", "EventCode": "0x1", "BriefDescription": "b"}]\n' \
+	> "${tmpdir}/x86/events.json"
+
+rc=0
+"${python}" "${jevents}" --check x86 all "${tmpdir}" 2>"${tmpdir}/stderr" ||
+	rc=$?
+if [ "${rc}" -ne 0 ]; then
+	cat "${tmpdir}/stderr"
+	echo "a clean tree was reported, status ${rc}"
+	exit 1
+fi
+
+printf '[{"EventName": "test", "BriefDeuxcription": "b"}]\n' \
+	> "${tmpdir}/x86/events.json"
+
+rc=0
+"${python}" "${jevents}" --check x86 all "${tmpdir}" 2>"${tmpdir}/stderr" ||
+	rc=$?
+if [ "${rc}" -ne 1 ]; then
+	cat "${tmpdir}/stderr"
+	echo "expected 1 for an unknown key, got ${rc}"
+	exit 1
+fi
+grep -q 'unknown key "BriefDeuxcription"' "${tmpdir}/stderr" ||
+	{ echo "the unknown key was not named"; exit 1; }
+
+for arch in x86typo .. empty; do
+	rc=0
+	"${python}" "${jevents}" --check "${arch}" all "${tmpdir}" 2>/dev/null ||
+		rc=$?
+	if [ "${rc}" -ne 2 ]; then
+		echo "expected 2 for the architecture '${arch}', got ${rc}"
+		exit 1
+	fi
+done
+
+exit 0
-- 
2.53.0

Re: [PATCH v5 3/3] perf jevents: Check the JSON keys a table may carry

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf tests: jevents_check.sh truncates test description and
  omits (no_mlock)
--

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

perf jevents: Check the JSON keys a table may carry

This commit adds a --check option to jevents.py to validate JSON keys in
pmu-events tables, along with a check-jevents make target and a shell test.

> diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
> new file mode 100755
> index 0000000000000..311ca5f5ab15e
> --- /dev/null
> +++ b/tools/perf/tests/shell/jevents_check.sh
> @@ -0,0 +1,62 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# perf jevents tests: --check reports a key the generator does not read, and an
> +# architecture that is not there is an error rather than a clean tree.

[Severity: Low]
Does wrapping this header comment across two lines truncate the test
description in perf test?

In tools/perf/tests/tests-scripts.c, shell_test__description() reads only
the first non-shebang, non-SPDX comment line and stops, so perf test list
displays "perf jevents tests: --check reports a key the generator does not
read, and an".

Also, append_script() and shell_test__requisites() default needs_mlock to
true unless that single-line description ends with "(no_mlock)". Since this
script only runs jevents.py on temporary JSON files and never maps perf
event ring buffers, could the first comment line be a single-line
description ending with "(no_mlock)"?

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

[PATCH v6 0/3] perf jevents: Check the JSON keys a table may carry

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key spelled differently is dropped without a word
and the object is built without whatever that field held.  Two of those
are in the tree: "PublicDescrition" in 16 entries of an Arm table, where
the lost long description repeated the brief one and so nothing a user saw
changed, and "BriefDescript6ion" in one entry each of three AMD tables,
where the event is generated with no text at all.

Patch 3 says which keys a table may carry and checks the tree against
that, instead of reporting the keys a run of jevents.py did not read.  The
list is 51 names, 47 of which jevents.py reads, one more of which
metric.py reads, and three of which nothing reads although the tables hold
them (Counter, CountersNumFixed, CountersNumGeneric).  A key outside the
list is reported with the object that carries it; there is no cut-off, no
similarity and nothing that depends on what a run of the generator
happened to read.  It is not a JSON schema because perf's build would then
require jsonschema, which it does not use today and which is not in every
builder's python; the list holds the same names that a schema with
"additionalProperties": false would.

The other two patches are small reader fixes that came out of the same
work, and patch 3 stands on its own if you would rather take only the
check:

- patch 2 makes the model argument optional, so "jevents.py x86 <dir>"
  runs by
hand, which is how the check was tried out.
- patch 1 stops the generator from dying with an AttributeError on a
  metric
that has a name and no MetricExpr, which is the same kind of silent JSON
problem, and names the metric and the file when it does.

The single patch posted first (20261110193239) was version 1 of this
check, before those fixes were written; v3 turned it into a series and v5
replaced its body with the list above.

v6 changes two things the review found.  Patch 1 now names the file as
well as the metric, because the tables are read through a worker pool
outside the directory walk and the path was lost on the way out.  The
test's one-line description ends in "(no_mlock)", so perf test lists it
whole and the test does not take a memlock slot.

The check still runs as "make check-jevents" and as a perf test rather
than as part of the build, so the generated tables are byte identical, and
on the tree today it reports exactly the two misspellings above, which are
posted separately.

Link: https://lore.kernel.org/linux-perf-users/20261108183846.3804875-1-takano32@gmail.com/
Link: https://lore.kernel.org/linux-perf-users/20261108184600.3810059-1-takano32@gmail.com/

TAKANO Mitsuhiro (3):
  perf jevents: Name the metric and the file when a MetricExpr is
    missing
  perf jevents: Make the model argument optional
  perf jevents: Check the JSON keys a table may carry

 tools/perf/Makefile                     |  13 +-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 229 ++++++++++++++++++++----
 tools/perf/tests/shell/jevents_check.sh |  61 +++++++
 4 files changed, 272 insertions(+), 32 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

[PATCH v6 1/3] perf jevents: Name the metric and the file when a MetricExpr is missing

[TAKANO Mitsuhiro]

jevents.py reads each field it wants out of a JSON object by name, so a
MetricExpr key that is missing or spelled another way leaves the object
with a MetricName and no expression.  Two different failures followed,
neither of which named the metric: the rewrite in _read_json_events_impl()
raised AttributeError on the None, and a metric that got that far reached
print_pending_metrics(), whose assert fired on it.

Raise in the reader instead, naming the metric and the file it came from,
before either of them can happen.  The file matters because the tables are
read through a worker pool in main(), outside the directory walk, so an
exception there loses the name of the table on its way out.  The check is
not inside the '-' test that decides whether a metric takes part in the
rewrite: a name with a dash in it can lose its expression the same way.

Two smaller changes on the same path:

- An architecture standard table that does not parse now carries the
  reason in its message ("Failure processing '<file>' in '<dir>':
  <cause>").  The traceback used to end with the file name alone, and the
  reader already knows the cause.

- The output files are opened after the tables have been read, so a tree
  that fails to parse leaves no half-written table behind.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v6: patch 1 names the file as well as the metric when a MetricExpr is
    missing (the tables are read through a worker pool, outside the
    walk, so the path was lost).  The perf test's one-line description
    ends in "(no_mlock)" so perf test lists it whole and the test does
    not take a memlock slot.  No other change.

 tools/perf/pmu-events/jevents.py | 79 +++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 27 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..843748915c12 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -460,11 +460,18 @@ def _read_json_events_impl(path: str, topic: str) -> Sequence[JsonEvent]:
     with open(path, encoding='utf-8') as f:
       events = json.load(f, object_hook=JsonEvent)
   except BaseException:
-    print(f"Exception processing {path}")
+    print(f'jevents: error: Exception processing {path}', file=sys.stderr)
     raise
   metrics: list[Tuple[str, str, metric.Expression]] = []
   for event in events:
     event.topic = topic
+    if event.metric_name and event.metric_expr is None:
+      # A MetricName without a MetricExpr leaves nothing to put in the
+      # generated tables, whether or not the name has a dash, so the build
+      # must not go on.  Name the metric instead of letting the table printer
+      # assert on the None.
+      raise RuntimeError(f'{path}: metric {event.metric_name!r} has no '
+                         'MetricExpr')
     if event.metric_name and '-' not in event.metric_name:
       metrics.append((event.pmu, event.metric_name, event.metric_expr))
   updates = metric.RewriteMetricsInTermsOfOthers(metrics)
@@ -493,7 +500,7 @@ def preprocess_arch_std_files(archpath: str) -> None:
         if event.metric_name:
           _arch_std_events[event.metric_name.lower()] = event
     except Exception as e:
-        raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\'') from e
+      raise RuntimeError(f'Failure processing \'{item.name}\' in \'{archpath}\': {e}') from e
 
 
 def add_events_table_entries(item: os.DirEntry, topic: str) -> None:
@@ -1490,33 +1497,13 @@ 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()
 
-  _args.output_file.write(f"""/* SPDX-License-Identifier: GPL-2.0 */
-/* THIS FILE WAS AUTOGENERATED BY `jevents.py arch={_args.arch} model={_args.model}` ! */
-""")
-  _args.output_file.write("""
-#include <pmu-events/pmu-events.h>
-#include "util/header.h"
-#include "util/pmu.h"
-#include <string.h>
-#include <stddef.h>
-
-struct compact_pmu_event {
-\tint offset;
-};
-
-struct pmu_table_entry {
-\tconst struct compact_pmu_event *entries;
-\tuint32_t num_entries;
-\tstruct compact_pmu_event pmu_name;
-};
-
-""")
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
@@ -1553,6 +1540,44 @@ struct pmu_table_entry {
 
   assert _bcs is not None
   _bcs.compute()
+
+  # Every table has been read by now, so opening the output here cannot truncate
+  # a file the caller named when a table turns out not to parse.
+  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}` ! */
+""")
+  _args.output_file.write("""
+#include <pmu-events/pmu-events.h>
+#include "util/header.h"
+#include "util/pmu.h"
+#include <string.h>
+#include <stddef.h>
+
+struct compact_pmu_event {
+\tint offset;
+};
+
+struct pmu_table_entry {
+\tconst struct compact_pmu_event *entries;
+\tuint32_t num_entries;
+\tstruct compact_pmu_event pmu_name;
+};
+
+""")
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
     _args.output_file.write('static const char *const big_c_string =\n')
-- 
2.53.0

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

[sashiko-bot]

> jevents.py reads each field it wants out of a JSON object by name, so a
> MetricExpr key that is missing or spelled another way leaves the object
> with a MetricName and no expression.  Two different failures followed,
> neither of which named the metric: the rewrite in _read_json_events_impl()
> raised AttributeError on the None, and a metric that got that far reached
> print_pending_metrics(), whose assert fired on it.
> 
> Raise in the reader instead, naming the metric and the file it came from,
> before either of them can happen.  The file matters because the tables are
> read through a worker pool in main(), outside the directory walk, so an
> exception there loses the name of the table on its way out.  The check is
> not inside the '-' test that decides whether a metric takes part in the
> rewrite: a name with a dash in it can lose its expression the same way.
> 
> Two smaller changes on the same path:
> 
> - An architecture standard table that does not parse now carries the
>   reason in its message ("Failure processing '<file>' in '<dir>':
>   <cause>").  The traceback used to end with the file name alone, and the
>   reader already knows the cause.
> 
> - The output files are opened after the tables have been read, so a tree
>   that fails to parse leaves no half-written table behind.
> 
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v6 2/3] perf jevents: Make the model argument optional

[TAKANO Mitsuhiro]

The model argument carries a default of "all", but a positional argument
with a default and no nargs is still required: "jevents.py x86 <dir>" exits
with "the following arguments are required: model", and so does any caller
that only wants the tables for an architecture.

Make it nargs='?' so the default is reachable and an omitted model means
"all".  Builds pass it explicitly, so the generated tables do not change.

Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v6: patch 1 names the file as well as the metric when a MetricExpr is
    missing (the tables are read through a worker pool, outside the
    walk, so the path was lost).  The perf test's one-line description
    ends in "(no_mlock)" so perf test lists it whole and the test does
    not take a memlock slot.  No other change.

 tools/perf/pmu-events/jevents.py | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 843748915c12..04ef2f488283 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -1487,10 +1487,10 @@ def main() -> None:
 
   ap = argparse.ArgumentParser()
   ap.add_argument('arch', help='Architecture name like x86')
-  ap.add_argument('model', help='''Select a model such as skylake to
-reduce the code size.  Normally set to "all". For architectures like
-ARM64 with an implementor/model, the model must include the implementor
-such as "arm/cortex-a34".''',
+  ap.add_argument('model', nargs='?', help='''Select a model such as skylake to
+reduce the code size.  Normally set to "all", and that is what an omitted
+model means. For architectures like ARM64 with an implementor/model, the
+model must include the implementor such as "arm/cortex-a34".''',
                   default='all')
   ap.add_argument(
       'starting_dir',
-- 
2.53.0

Re: [PATCH v6 2/3] perf jevents: Make the model argument optional

[sashiko-bot]

> The model argument carries a default of "all", but a positional argument
> with a default and no nargs is still required: "jevents.py x86 <dir>" exits
> with "the following arguments are required: model", and so does any caller
> that only wants the tables for an architecture.
> 
> Make it nargs='?' so the default is reachable and an omitted model means
> "all".  Builds pass it explicitly, so the generated tables do not change.
> 
> Fixes: 5a09b1fd1b1f ("perf jevents: Add model list option")
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v6 3/3] perf jevents: Check the JSON keys a table may carry

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name -
jd.get('BriefDescription'), 'EventCode' in jd, jd['ExtSel'], and the
fourteen names in event_fields that it looks for with 'key in jd' - so a
key spelled another way is dropped without a word and the object is built
without whatever that field held.  Nothing in the build and nothing in the
generated table says so.

Two of those are in the tree.  "PublicDescrition" is in 16 of the 28
entries of arm64/ampere/ampereonex/mmu.json, where the long description
each of them lost repeated the BriefDescription, which is why it could sit
there unnoticed.  "BriefDescript6ion" is in one entry each of
amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-store.json,
and those objects carry no other description, so ls_inef_sw_pref.all is
generated with an empty text field on Zen 4, 5 and 6.

Check the tables against the keys they may carry instead of watching which
ones a run reads.  The list is 51 names: 47 of them this file reads, 33 by
name and 14 through event_fields; Experimental is read by metric.py; and
Counter, CountersNumFixed and CountersNumGeneric are read by nothing in
perf although the tables carry them, so a vendor that adds a field changes
one line here by agreement.  A JSON Schema would express the same list,
but perf's build would then require jsonschema, which it does not use
today and which is not in every builder's python, so the list lives in
this file and is the schema.  The name of a metricgroup is not a key: the
file that ends in metricgroups.json maps a name of its own choosing to a
description, and only that description is read.

Add --check, which reads every architecture under starting_dir, or the one
named by the first argument, names each key it does not accept with the
object that carries it on stderr, and exits 0 when the tree is clean, 1
when something was reported and 2 when a table could not be read or there
was nothing to read.  The name of an architecture is a name and not a
path, so ".." is refused rather than walking out of the tree, and an
architecture with no tables of its own is an error rather than a pass: a
name that was typed wrong is the first thing this is meant to catch.  An
object where a list belongs is reported too, because every key of it
would go unchecked.  A build does not run the check, and the generated
tables are byte identical: x86, arm64 and all architectures compared
against the same tree without this patch.

Add a "check-jevents" target next to check-headers, with
JEVENTS_CHECK_ARCH to check one architecture while working on it, and a
perf test for the cases that matter: a clean tree, a key the generator
does not read, and an architecture that is not there.

The two misspellings in the tree are fixed by separate patches; until they
are applied the target reports them, which is what it is for.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v6: patch 1 names the file as well as the metric when a MetricExpr is
    missing (the tables are read through a worker pool, outside the
    walk, so the path was lost).  The perf test's one-line description
    ends in "(no_mlock)" so perf test lists it whole and the test does
    not take a memlock slot.  No other change.

 tools/perf/Makefile                     |  13 ++-
 tools/perf/Makefile.perf                |   1 +
 tools/perf/pmu-events/jevents.py        | 148 +++++++++++++++++++++++-
 tools/perf/tests/shell/jevents_check.sh |  61 ++++++++++
 4 files changed, 219 insertions(+), 4 deletions(-)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/Makefile b/tools/perf/Makefile
index 00baf90e5bda..a1aac2fdf5e1 100644
--- a/tools/perf/Makefile
+++ b/tools/perf/Makefile
@@ -140,6 +140,17 @@ check-headers:
 sync-headers:
 	@./scripts/sync-headers.sh $(SYNC_HEADERS_ARGS)
 
+# The pmu-events tables are data, and a key that no parser reads is invisible
+# until a description goes missing, so the keys a table may carry are a fixed
+# list in jevents.py and this target checks the tree against it.  It reads the
+# source tree and needs no build.  Makefile.perf sets PYTHON for the build and
+# this wrapper does not include it, so default here and let PYTHON= on the
+# command line win when it is given.
+JEVENTS_CHECK_ARCH ?= all
+PYTHON ?= python3
+check-jevents:
+	@$(PYTHON) ./pmu-events/jevents.py --check $(JEVENTS_CHECK_ARCH) all pmu-events/arch
+
 #
 # All other targets get passed through:
 #
@@ -147,4 +158,4 @@ sync-headers:
 	$(print_msg)
 	$(make)
 
-.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers
+.PHONY: all clean tags TAGS FORCE Makefile build-test build-test-tarball check-headers sync-headers check-jevents
diff --git a/tools/perf/Makefile.perf b/tools/perf/Makefile.perf
index b74b534e1153..ec7dde9ec6e2 100644
--- a/tools/perf/Makefile.perf
+++ b/tools/perf/Makefile.perf
@@ -871,6 +871,7 @@ help:
 	@echo 'Perf maintainer targets:'
 	@echo '  clean			- clean all binary objects and build output'
 	@echo '  check-headers		- check which header copies are out of sync with the kernel'
+	@echo '  check-jevents		- check the pmu-events json tables against the keys they may carry'
 	@echo '  sync-headers		- sync those header copies with the kernel, creating commits'
 	@echo '			  (SYNC_HEADERS_ARGS="..." passes options to the script)'
 
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 04ef2f488283..336fff1f7ad7 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -7,7 +7,7 @@ import json
 import metric
 import os
 import sys
-from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
+from typing import (Callable, Dict, List, Optional, Sequence, Set, Tuple)
 import collections
 
 # Global command line arguments.
@@ -1457,6 +1457,136 @@ def _init_worker(std_events: dict) -> None:
   global _arch_std_events
   _arch_std_events = std_events
 
+# The keys a json object under arch/ may carry.  jevents.py reads 47 of them,
+# 33 by name and 14 through event_fields below; Experimental is read by
+# metric.py; Counter, CountersNumFixed and CountersNumGeneric are read by
+# nothing in perf, but the tables carry them, so a vendor that adds a field
+# changes one line here by agreement.  A key outside this set is a misspelling,
+# or a field that has to be agreed on and added here first: "PublicDescrition"
+# sat in 16 entries of an arm64 table and every one of them lost its long
+# description without a word.  This list is the whole schema - it says what may
+# exist in a table, not that the table uses it.
+_TABLE_KEYS: Set[str] = {
+    'AggregationMode', 'AnyThread', 'ArchStdEvent', 'BriefDescription',
+    'Compat', 'ConfigCode', 'Counter', 'CounterMask',
+    'CountersNumFixed', 'CountersNumGeneric', 'Data_LA', 'DefaultMetricgroupName',
+    'DefaultShowEvents', 'Deprecated', 'EdgeDetect', 'EnAllCores',
+    'EnAllSlices', 'Errata', 'EventCode', 'EventidCode',
+    'EventName', 'Experimental', 'ExtSel', 'FCMask',
+    'Filter', 'Invert', 'LegacyCacheCode', 'LegacyConfigCode',
+    'MetricConstraint', 'MetricExpr', 'MetricGroup', 'MetricgroupNoGroup',
+    'MetricName', 'MetricThreshold', 'MSRIndex', 'MSRValue',
+    'NodeType', 'PEBS', 'PerPkg', 'PortMask',
+    'PublicDescription', 'RdWrMask', 'RetirementLatencyMax', 'RetirementLatencyMean',
+    'RetirementLatencyMin', 'SampleAfterValue', 'ScaleUnit', 'SliceId',
+    'ThreadMask', 'UMask', 'Unit',
+}
+
+
+def _check_tables(root: str, arch: str) -> int:
+  """Report the keys in the json tables under root that no reader looks for.
+
+  With arch "all" every architecture is read; otherwise that one is, together
+  with the common and test tables that a build of it reads as well.
+
+  Returns 0 when the tree is clean, 1 when something was reported and 2 when a
+  table could not be read or there was nothing to read, so that a script can
+  gate on the status.
+  """
+  reported = 0
+  unreadable = 0
+  files = 0
+  bad_files: Set[str] = set()
+
+  def report(path: str, what: str) -> None:
+    nonlocal reported
+    bad_files.add(path)
+    print(f'{path}: {what}', file=sys.stderr)
+    reported += 1
+
+  if arch == 'all':
+    roots = [root]
+    arch_root = None
+  else:
+    # arch names a directory under the tables, it is not a path: ".." or "x86/"
+    # would walk out of the tree and report on tables that are not part of it.
+    if not arch or '/' in arch or arch in ('.', '..'):
+      print(f'"{arch}" is not the name of a directory under the tables',
+            file=sys.stderr)
+      return 2
+    arch_root = os.path.join(root, arch)
+    if not os.path.isdir(arch_root):
+      print(f'{root}: there is no architecture "{arch}"', file=sys.stderr)
+      return 2
+    roots = [arch_root]
+    roots += [os.path.join(root, name) for name in ('common', 'test')
+              if name != arch and os.path.isdir(os.path.join(root, name))]
+
+  paths: List[str] = []
+  arch_tables = 0
+  for top in roots:
+    for dirpath, _, names in os.walk(top):
+      for name in sorted(names):
+        if name.endswith('.json'):
+          paths.append(os.path.join(dirpath, name))
+          if top == arch_root:
+            arch_tables += 1
+
+  # The common and test tables exist for every architecture, so an architecture
+  # with tables of its own is what makes a check of it meaningful.
+  if arch_root and not arch_tables:
+    print(f'{arch_root}: the tables of "{arch}" are missing', file=sys.stderr)
+    return 2
+
+  for path in sorted(paths):
+    files += 1
+    try:
+      with open(path, encoding='utf-8') as f:
+        data = json.load(f)
+    except (OSError, ValueError) as e:
+      print(f'{path}: cannot be read: {e}', file=sys.stderr)
+      unreadable += 1
+      continue
+    if path.endswith('metricgroups.json'):
+      # The name of a group is a name of its own choosing and only the
+      # description held under it is read, so there is no key to check here.  A
+      # list where an object belongs means the braces were left out.
+      if not isinstance(data, dict):
+        report(path, f'the table holds a {type(data).__name__}, not an object')
+      continue
+    if isinstance(data, dict):
+      # A list of objects written without its brackets: every key of the one
+      # object in it would go unchecked.
+      report(path, 'the table holds an object, not a list of objects')
+      continue
+    if not isinstance(data, list):
+      report(path, f'the table holds a {type(data).__name__}, '
+                   'not a list of objects')
+      continue
+    for index, obj in enumerate(data):
+      if not isinstance(obj, dict):
+        report(path, f'entry {index} is a {type(obj).__name__}, not an object')
+        continue
+      named = next((obj[k] for k in ('EventName', 'MetricName', 'ArchStdEvent')
+                    if isinstance(obj.get(k), str) and obj[k]), '')
+      what = f'entry {index} ({named})' if named else f'entry {index}'
+      for key in sorted(set(obj) - _TABLE_KEYS):
+        report(path, f'{what}: unknown key "{key}"')
+
+  if not files:
+    print(f'{root}: there are no tables to check', file=sys.stderr)
+    return 2
+  if unreadable:
+    print(f'{unreadable} table(s) could not be read', file=sys.stderr)
+  if reported:
+    print(f'{reported} problem(s) in {len(bad_files)} of {files} tables: a key '
+          'this file does not read is a misspelling, or a field to add to '
+          '_TABLE_KEYS', file=sys.stderr)
+  if unreadable:
+    return 2
+  return 1 if reported else 0
+
+
 def main() -> None:
   global _args
 
@@ -1498,12 +1628,24 @@ model must include the implementor such as "arm/cortex-a34".''',
       help='Root of tree containing architecture directories containing json files'
   )
   ap.add_argument('output_file', nargs='?', default=None,
-                  help='file to write the tables to, default standard output')
+                  help='file to write the tables to, default standard output; '
+                       'not opened with --check')
   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')
+                       'apart from the tables; ignored with --check')
+  ap.add_argument('--check', action='store_true',
+                  help='check the tables under starting_dir against the keys '
+                       'they are allowed to carry and exit without writing '
+                       'anything; arch selects the architecture to read, "all" '
+                       'reads every one, and model is not used.  The exit '
+                       'status is 0 when the tree is clean, 1 when something '
+                       'was reported and 2 when a table could not be read or '
+                       'there was nothing to read')
   _args = ap.parse_args()
 
+  if _args.check:
+    sys.exit(_check_tables(_args.starting_dir, _args.arch))
+
   archs = []
   for item in os.scandir(_args.starting_dir):
     if not item.is_dir():
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..5e36cb5f035e
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,61 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# check-jevents: an unknown key is reported, a missing architecture is fatal (no_mlock)
+
+set -e
+
+shelldir=$(dirname "$0")
+jevents="${shelldir}/../../pmu-events/jevents.py"
+python=${PYTHON:-python3}
+tmpdir=$(mktemp -d "${TMPDIR:-/tmp}/jevents-check.XXXXXX") || exit 2
+trap 'rm -rf "${tmpdir}"' EXIT
+
+# An installed perf has the test but not the generator, and a python that cannot
+# run the generator is not a failure of the check.
+[ -r "${jevents}" ] || exit 2
+"${python}" -c 'import sys; sys.exit(sys.version_info[0] != 3)' 2>/dev/null ||
+	exit 2
+
+mkdir -p "${tmpdir}/x86/model" "${tmpdir}/empty" "${tmpdir}/common" \
+	"${tmpdir}/test"
+
+# The name of a metricgroup is not a key, even when it is spelled like one.
+printf '{"Unit": "a description"}\n' \
+	> "${tmpdir}/x86/model/events-metricgroups.json"
+printf '[{"EventName": "test", "EventCode": "0x1", "BriefDescription": "b"}]\n' \
+	> "${tmpdir}/x86/events.json"
+
+rc=0
+"${python}" "${jevents}" --check x86 all "${tmpdir}" 2>"${tmpdir}/stderr" ||
+	rc=$?
+if [ "${rc}" -ne 0 ]; then
+	cat "${tmpdir}/stderr"
+	echo "a clean tree was reported, status ${rc}"
+	exit 1
+fi
+
+printf '[{"EventName": "test", "BriefDeuxcription": "b"}]\n' \
+	> "${tmpdir}/x86/events.json"
+
+rc=0
+"${python}" "${jevents}" --check x86 all "${tmpdir}" 2>"${tmpdir}/stderr" ||
+	rc=$?
+if [ "${rc}" -ne 1 ]; then
+	cat "${tmpdir}/stderr"
+	echo "expected 1 for an unknown key, got ${rc}"
+	exit 1
+fi
+grep -q 'unknown key "BriefDeuxcription"' "${tmpdir}/stderr" ||
+	{ echo "the unknown key was not named"; exit 1; }
+
+for arch in x86typo .. empty; do
+	rc=0
+	"${python}" "${jevents}" --check "${arch}" all "${tmpdir}" 2>/dev/null ||
+		rc=$?
+	if [ "${rc}" -ne 2 ]; then
+		echo "expected 2 for the architecture '${arch}', got ${rc}"
+		exit 1
+	fi
+done
+
+exit 0
-- 
2.53.0

Re: [PATCH v6 3/3] perf jevents: Check the JSON keys a table may carry

[sashiko-bot]

> jevents.py takes the fields it wants out of each JSON object by name -
> jd.get('BriefDescription'), 'EventCode' in jd, jd['ExtSel'], and the
> fourteen names in event_fields that it looks for with 'key in jd' - so a
> key spelled another way is dropped without a word and the object is built
> without whatever that field held.  Nothing in the build and nothing in the
> generated table says so.
> 
> Two of those are in the tree.  "PublicDescrition" is in 16 of the 28
> entries of arm64/ampere/ampereonex/mmu.json, where the long description
> each of them lost repeated the BriefDescription, which is why it could sit
> there unnoticed.  "BriefDescript6ion" is in one entry each of
> amdzen4/cache.json, amdzen5/load-store.json and amdzen6/load-store.json,
> and those objects carry no other description, so ls_inef_sw_pref.all is
> generated with an empty text field on Zen 4, 5 and 6.
> 
> [ ... ]
> 
> The two misspellings in the tree are fixed by separate patches; until they
> are applied the target reports them, which is what it is for.
> 
> Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>

Sashiko has reviewed this patch and found no issues.

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

[PATCH v7 0/1] perf jevents: Check the keys of the JSON tables

[TAKANO Mitsuhiro]

This is the check from v6 with everything that was not the check taken out,
as James asked.  What is left is the walk, the set difference against the
fixed list, and the file the object is in:

  - no "make check-jevents" target; it runs as a perf test only
  - no arch filter, no summary line, no handling of a table that holds a
    single object, no special case for files that cannot be read
  - the check is 14 lines and the list of 51 keys is 13, and the two-line
    shell wrapper exists only because perf test finds tests by file name;
    it is not part of the check

Patches 1 and 2 of v6 are dropped, so this is one patch: the check alone.
The command line still takes arch and model the way the generator does, and
the check ignores them.

On today's tree it reports 19 keys in four tables and exits 1:

  arch/arm64/ampere/ampereonex/mmu.json    PublicDescrition   16 objects
  arch/x86/amdzen4/cache.json              BriefDescript6ion   1 object
  arch/x86/amdzen5/load-store.json         BriefDescript6ion   1 object
  arch/x86/amdzen6/load-store.json         BriefDescript6ion   1 object

Both spellings are being fixed by patches already posted, so the check goes
green once those land.  The metricgroups.json files hold a dictionary keyed
by group name and are skipped by file name; without that every character of
every group name is reported.

Changes since v6:

  - one patch instead of three; patches 1 and 2 are dropped
  - the check reports the file and the key and nothing else

TAKANO Mitsuhiro (1):
  perf jevents: Check the keys of the JSON tables

 tools/perf/pmu-events/jevents.py        | 38 +++++++++++++++++++++++++
 tools/perf/tests/shell/jevents_check.sh | 24 ++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

[PATCH v7 1/1] perf jevents: Check the keys of the JSON tables

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key that is spelled differently is dropped without
a word and the object is built without whatever that field held.  Two of
those are in the tree: "PublicDescrition" in 16 entries of an Arm table,
and "BriefDescript6ion" in one entry each of three AMD tables.

A table may carry 51 keys; 47 of them jevents.py reads, one more metric.py
reads, and three are read by nothing although the tables hold them
(Counter, CountersNumFixed, CountersNumGeneric).  A key outside that list
is reported with the file that carries it.  There is no cut-off, no
similarity and nothing that depends on what a run of the generator
happened to read.

The check is not a JSON schema because perf's build would then require
jsonschema, which it does not use today and which is not in every
builder's python.  It runs as a perf test, not as part of the build.

On the tree today it reports 19 keys in four tables.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v7: one patch instead of three.  Patches 1 and 2 are dropped with the
    Makefile target, the arch filter, the summary line and the handling
    of tables that hold a single object; what is left is the walk, the
    set difference against the list, and the file the object is in.

 tools/perf/pmu-events/jevents.py        | 38 +++++++++++++++++++++++++
 tools/perf/tests/shell/jevents_check.sh | 24 ++++++++++++++++
 2 files changed, 62 insertions(+)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..397b4fc97749 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -7,6 +7,7 @@ import json
 import metric
 import os
 import sys
+from pathlib import Path
 from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
 import collections
 
@@ -1450,6 +1451,38 @@ def _init_worker(std_events: dict) -> None:
   global _arch_std_events
   _arch_std_events = std_events
 
+# The keys a json object under arch/ may carry.  jevents.py takes the fields it
+# wants by name and ignores the rest, so a key that is spelled differently is
+# dropped without a word; this is the list a table is allowed to contain.
+_TABLE_KEYS: Set[str] = {
+    'AggregationMode', 'AnyThread', 'ArchStdEvent', 'BriefDescription',
+    'Compat', 'ConfigCode', 'Counter', 'CounterMask',
+    'CountersNumFixed', 'CountersNumGeneric', 'Data_LA', 'DefaultMetricgroupName',
+    'DefaultShowEvents', 'Deprecated', 'EdgeDetect', 'EnAllCores',
+    'EnAllSlices', 'Errata', 'EventCode', 'EventidCode',
+    'EventName', 'Experimental', 'ExtSel', 'FCMask',
+    'Filter', 'Invert', 'LegacyCacheCode', 'LegacyConfigCode',
+    'MetricConstraint', 'MetricExpr', 'MetricGroup', 'MetricgroupNoGroup',
+    'MetricName', 'MetricThreshold', 'MSRIndex', 'MSRValue',
+    'NodeType', 'PEBS', 'PerPkg', 'PortMask',
+    'PublicDescription', 'RdWrMask', 'RetirementLatencyMax', 'RetirementLatencyMean',
+    'RetirementLatencyMin', 'SampleAfterValue', 'ScaleUnit', 'SliceId',
+    'ThreadMask', 'UMask', 'Unit',
+}
+
+def _check_tables(root: str) -> int:
+  """Report the keys in the tables under root that a table may not carry."""
+  bad = 0
+  for path in sorted(Path(root).rglob('*.json')):
+    if path.name.endswith('metricgroups.json'):
+      # A dictionary keyed by metric group name, not a list of events.
+      continue
+    for obj in json.loads(path.read_text(encoding='utf-8')):
+      if extra := set(obj) - _TABLE_KEYS:
+        print(f'{path}: {sorted(extra)}')
+        bad += 1
+  return 1 if bad else 0
+
 def main() -> None:
   global _args
 
@@ -1494,8 +1527,13 @@ such as "arm/cortex-a34".''',
       '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('--check', action='store_true',
+                  help='check the keys of the tables under starting_dir and exit')
   _args = ap.parse_args()
 
+  if _args.check:
+    sys.exit(_check_tables(_args.starting_dir))
+
   _args.output_file.write(f"""/* SPDX-License-Identifier: GPL-2.0 */
 /* THIS FILE WAS AUTOGENERATED BY `jevents.py arch={_args.arch} model={_args.model}` ! */
 """)
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..eb3125e1486a
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# jevents_check: an unknown key in a table is reported (no_mlock)
+#
+# A key that is spelled differently is dropped without a word, so the keys of
+# the tables are checked against the list jevents.py reads.
+
+jevents=$(dirname "$0")/../../pmu-events/jevents.py
+[ -r "${jevents}" ] || exit 2
+
+tmp=$(mktemp -d)
+trap 'rm -rf "${tmp}"' EXIT
+tables="${tmp}/arch/x86/model"
+mkdir -p "${tables}"
+
+# Known keys pass.
+printf '[{"EventName": "a", "EventCode": "0x1"}]\n' > "${tables}/events.json"
+python3 "${jevents}" x86 all "${tmp}/arch" --check > /dev/null || exit 1
+
+# A key the generator does not read is reported.
+printf '[{"EventName": "a", "NotAKey": "x"}]\n' > "${tables}/events.json"
+python3 "${jevents}" x86 all "${tmp}/arch" --check > /dev/null && exit 1
+
+exit 0
-- 
2.53.0

Re: [PATCH v7 1/1] perf jevents: Check the keys of the JSON tables

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] perf test: jevents_check.sh never checks in-tree PMU JSON
  tables
- [Low] perf jevents: _check_tables() ignores mandatory arch and model
  arguments
- [Low] perf test: jevents_check.sh hardcodes python3 without
  availability check
--

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

perf jevents: Check the keys of the JSON tables

This commit adds a --check option to jevents.py to report JSON event table
keys not in _TABLE_KEYS, along with a shell test for the option.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 080e04b51b8c5..397b4fc977497 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> @@ -1450,6 +1451,38 @@ def _init_worker(std_events: dict) -> None:
[ ... ]
> +def _check_tables(root: str) -> int:
> +  """Report the keys in the tables under root that a table may not carry."""
> +  bad = 0
> +  for path in sorted(Path(root).rglob('*.json')):
> +    if path.name.endswith('metricgroups.json'):
> +      # A dictionary keyed by metric group name, not a list of events.
> +      continue
> +    for obj in json.loads(path.read_text(encoding='utf-8')):
> +      if extra := set(obj) - _TABLE_KEYS:
> +        print(f'{path}: {sorted(extra)}')
> +        bad += 1
> +  return 1 if bad else 0
> +
>  def main() -> None:
>    global _args
>  
> @@ -1494,8 +1527,13 @@ such as "arm/cortex-a34".''',
>        '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('--check', action='store_true',
> +                  help='check the keys of the tables under starting_dir and exit')
>    _args = ap.parse_args()
>  
> +  if _args.check:
> +    sys.exit(_check_tables(_args.starting_dir))

[Severity: Low]
Why does --check still require positional arch and model arguments on the
command line if _check_tables() ignores _args.arch and _args.model and walks
every JSON file under starting_dir?  Callers such as jevents_check.sh have
to pass dummy values like "x86 all" before starting_dir that have no effect
on the check.

> diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
> new file mode 100755
> index 0000000000000..eb3125e1486ab
> --- /dev/null
> +++ b/tools/perf/tests/shell/jevents_check.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# jevents_check: an unknown key in a table is reported (no_mlock)
> +#
> +# A key that is spelled differently is dropped without a word, so the keys of
> +# the tables are checked against the list jevents.py reads.
> +
> +jevents=$(dirname "$0")/../../pmu-events/jevents.py
> +[ -r "${jevents}" ] || exit 2
> +
> +tmp=$(mktemp -d)
> +trap 'rm -rf "${tmp}"' EXIT
> +tables="${tmp}/arch/x86/model"
> +mkdir -p "${tables}"
> +
> +# Known keys pass.
> +printf '[{"EventName": "a", "EventCode": "0x1"}]\n' > "${tables}/events.json"
> +python3 "${jevents}" x86 all "${tmp}/arch" --check > /dev/null || exit 1

[Severity: Low]
Will jevents_check.sh fail with exit code 1 instead of skipping with exit
code 2 on systems where python3 is not installed or where a custom
interpreter is configured via PYTHON?  Sourcing lib/setup_python.sh and
invoking $PYTHON would skip cleanly when Python is unavailable.

> +
> +# A key the generator does not read is reported.
> +printf '[{"EventName": "a", "NotAKey": "x"}]\n' > "${tables}/events.json"
> +python3 "${jevents}" x86 all "${tmp}/arch" --check > /dev/null && exit 1

[Severity: Medium]
Does jevents_check.sh ever run jevents.py --check against the in-tree JSON
tables under tools/perf/pmu-events/arch?  The script only tests two
synthetic one-line files in ${tmp}/arch, so neither the build nor perf test
validates the repository's PMU event tables or checks that _TABLE_KEYS stays
in sync with them.

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

[PATCH v8 0/1] perf jevents: Check the keys of the JSON tables

[TAKANO Mitsuhiro]

v8 of the check.  Two things the review found are fixed, and one is left
alone on purpose.

  - --check takes the directory to check as its own argument, so it no
    longer needs the arch and model the generator takes.  The test calls
    "jevents.py --check <dir>" and nothing else.
  - the test takes the interpreter from PYTHON (python3 by default) and
    exits 2 when it is not there, the same way it does when the generator
    is not installed.

The check itself is unchanged: walk, set difference against the list of 51
keys, print the file that carries the bad key, exit 1.  On today's tree it
reports 19 keys in four tables:

  arch/arm64/ampere/ampereonex/mmu.json    PublicDescrition   16 objects
  arch/x86/amdzen4/cache.json              BriefDescript6ion   1 object
  arch/x86/amdzen5/load-store.json         BriefDescript6ion   1 object
  arch/x86/amdzen6/load-store.json         BriefDescript6ion   1 object

The third point was that the test checks a temporary table and never the
tables in the tree, so a misspelling could come back unnoticed.  That is
right, and it cannot be added yet: the tree still carries the two
misspellings above, and a test that fails as soon as it lands is not one
anyone wants.  Once the patches that fix them are in, the test can run
against the tree as well - I would rather add that as a second step than
bake a list of known-bad files into the test now.

Changes since v7:

  - --check takes the directory itself; arch and model are not needed
  - the test uses PYTHON and exits 2 when the interpreter is missing

TAKANO Mitsuhiro (1):
  perf jevents: Check the keys of the JSON tables

 tools/perf/pmu-events/jevents.py        | 41 +++++++++++++++++++++++++
 tools/perf/tests/shell/jevents_check.sh | 29 +++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.53.0

[PATCH v8 1/1] perf jevents: Check the keys of the JSON tables

[TAKANO Mitsuhiro]

jevents.py takes the fields it wants out of each JSON object by name and
ignores the rest, so a key that is spelled differently is dropped without
a word and the object is built without whatever that field held.  Two of
those are in the tree: "PublicDescrition" in 16 entries of an Arm table,
and "BriefDescript6ion" in one entry each of three AMD tables.

A table may carry 51 keys; 47 of them jevents.py reads, one more metric.py
reads, and three are read by nothing although the tables hold them
(Counter, CountersNumFixed, CountersNumGeneric).  A key outside that list
is reported with the file that carries it.  There is no cut-off, no
similarity and nothing that depends on what a run of the generator
happened to read.

The check is not a JSON schema because perf's build would then require
jsonschema, which it does not use today and which is not in every
builder's python.  It runs as a perf test, not as part of the build.

On the tree today it reports 19 keys in four tables.

Signed-off-by: TAKANO Mitsuhiro <takano32@gmail.com>
---
v8: --check takes the directory as its own argument, so it does not need
    the arch and model the generator takes; the test uses PYTHON and exits
    2 when the interpreter is missing.  The check is unchanged.

 tools/perf/pmu-events/jevents.py        | 41 +++++++++++++++++++++++++
 tools/perf/tests/shell/jevents_check.sh | 29 +++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100755 tools/perf/tests/shell/jevents_check.sh

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 080e04b51b8c..5daf92527573 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -7,6 +7,7 @@ import json
 import metric
 import os
 import sys
+from pathlib import Path
 from typing import (Callable, Dict, Optional, Sequence, Set, Tuple)
 import collections
 
@@ -1450,9 +1451,49 @@ def _init_worker(std_events: dict) -> None:
   global _arch_std_events
   _arch_std_events = std_events
 
+# The keys a json object under arch/ may carry.  jevents.py takes the fields it
+# wants by name and ignores the rest, so a key that is spelled differently is
+# dropped without a word; this is the list a table is allowed to contain.
+_TABLE_KEYS: Set[str] = {
+    'AggregationMode', 'AnyThread', 'ArchStdEvent', 'BriefDescription',
+    'Compat', 'ConfigCode', 'Counter', 'CounterMask',
+    'CountersNumFixed', 'CountersNumGeneric', 'Data_LA', 'DefaultMetricgroupName',
+    'DefaultShowEvents', 'Deprecated', 'EdgeDetect', 'EnAllCores',
+    'EnAllSlices', 'Errata', 'EventCode', 'EventidCode',
+    'EventName', 'Experimental', 'ExtSel', 'FCMask',
+    'Filter', 'Invert', 'LegacyCacheCode', 'LegacyConfigCode',
+    'MetricConstraint', 'MetricExpr', 'MetricGroup', 'MetricgroupNoGroup',
+    'MetricName', 'MetricThreshold', 'MSRIndex', 'MSRValue',
+    'NodeType', 'PEBS', 'PerPkg', 'PortMask',
+    'PublicDescription', 'RdWrMask', 'RetirementLatencyMax', 'RetirementLatencyMean',
+    'RetirementLatencyMin', 'SampleAfterValue', 'ScaleUnit', 'SliceId',
+    'ThreadMask', 'UMask', 'Unit',
+}
+
+def _check_tables(root: str) -> int:
+  """Report the keys in the tables under root that a table may not carry."""
+  bad = 0
+  for path in sorted(Path(root).rglob('*.json')):
+    if path.name.endswith('metricgroups.json'):
+      # A dictionary keyed by metric group name, not a list of events.
+      continue
+    for obj in json.loads(path.read_text(encoding='utf-8')):
+      if extra := set(obj) - _TABLE_KEYS:
+        print(f'{path}: {sorted(extra)}')
+        bad += 1
+  return 1 if bad else 0
+
 def main() -> None:
   global _args
 
+  # --check takes the directory to check and needs nothing else, so it is read
+  # before the parser below asks for the three positional arguments.
+  if '--check' in sys.argv:
+    ap = argparse.ArgumentParser()
+    ap.add_argument('--check', metavar='DIR', required=True,
+                    help='report the keys of the tables under DIR and exit')
+    sys.exit(_check_tables(ap.parse_args().check))
+
   def dir_path(path: str) -> str:
     """Validate path is a directory for argparse."""
     if os.path.isdir(path):
diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
new file mode 100755
index 000000000000..62ce70ef5d02
--- /dev/null
+++ b/tools/perf/tests/shell/jevents_check.sh
@@ -0,0 +1,29 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# jevents_check: an unknown key in a table is reported (no_mlock)
+#
+# A key that is spelled differently is dropped without a word, so the keys of
+# the tables are checked against the list jevents.py reads.
+
+jevents=$(dirname "$0")/../../pmu-events/jevents.py
+python=${PYTHON:-python3}
+
+# An installed perf has the test but not the generator, and a python that
+# cannot run the generator is not a failure of the check.
+[ -r "${jevents}" ] || exit 2
+command -v "${python}" > /dev/null || exit 2
+
+tmp=$(mktemp -d)
+trap 'rm -rf "${tmp}"' EXIT
+tables="${tmp}/arch/x86/model"
+mkdir -p "${tables}"
+
+# Known keys pass.
+printf '[{"EventName": "a", "EventCode": "0x1"}]\n' > "${tables}/events.json"
+"${python}" "${jevents}" --check "${tmp}/arch" > /dev/null || exit 1
+
+# A key the generator does not read is reported.
+printf '[{"EventName": "a", "NotAKey": "x"}]\n' > "${tables}/events.json"
+"${python}" "${jevents}" --check "${tmp}/arch" > /dev/null && exit 1
+
+exit 0
-- 
2.53.0

Re: [PATCH v8 1/1] perf jevents: Check the keys of the JSON tables

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] perf test: jevents_check.sh never checks in-tree PMU JSON
  tables
- [Low] perf jevents: --check exits 0 on missing dir and rejects
  --check=DIR
--

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

perf jevents: Check the keys of the JSON tables

This commit adds a --check option to jevents.py to report unrecognized JSON
object keys in PMU event tables and adds a shell test exercising that option.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 080e04b51b8c5..5daf92527573e 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> +def _check_tables(root: str) -> int:
> +  """Report the keys in the tables under root that a table may not carry."""
> +  bad = 0
> +  for path in sorted(Path(root).rglob('*.json')):
> +    if path.name.endswith('metricgroups.json'):
> +      # A dictionary keyed by metric group name, not a list of events.
> +      continue
> +    for obj in json.loads(path.read_text(encoding='utf-8')):
> +      if extra := set(obj) - _TABLE_KEYS:
> +        print(f'{path}: {sorted(extra)}')
> +        bad += 1
> +  return 1 if bad else 0
> +
>  def main() -> None:
>    global _args
>  
> +  # --check takes the directory to check and needs nothing else, so it is read
> +  # before the parser below asks for the three positional arguments.
> +  if '--check' in sys.argv:
> +    ap = argparse.ArgumentParser()
> +    ap.add_argument('--check', metavar='DIR', required=True,
> +                    help='report the keys of the tables under DIR and exit')
> +    sys.exit(_check_tables(ap.parse_args().check))
> +
>    def dir_path(path: str) -> str:
>      """Validate path is a directory for argparse."""
>      if os.path.isdir(path):

[Severity: Low]
Does _check_tables() exit with status 0 when given a nonexistent or
non-directory path?  Unlike starting_dir below, --check does not validate its
argument with type=dir_path, and Path(root).rglob('*.json') yields an empty
iterator when root does not exist, so a misspelled directory silently passes
with zero files checked.

Also, checking "'--check' in sys.argv" misses the single-token "--check=DIR"
form, which falls through to the main parser and fails with missing positional
arguments.

> diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
> new file mode 100755
> index 0000000000000..62ce70ef5d022
> --- /dev/null
> +++ b/tools/perf/tests/shell/jevents_check.sh
> @@ -0,0 +1,29 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# jevents_check: an unknown key in a table is reported (no_mlock)
> +#
> +# A key that is spelled differently is dropped without a word, so the keys of
> +# the tables are checked against the list jevents.py reads.
> +
> +jevents=$(dirname "$0")/../../pmu-events/jevents.py
> +python=${PYTHON:-python3}
> +
> +# An installed perf has the test but not the generator, and a python that
> +# cannot run the generator is not a failure of the check.
> +[ -r "${jevents}" ] || exit 2
> +command -v "${python}" > /dev/null || exit 2
> +
> +tmp=$(mktemp -d)
> +trap 'rm -rf "${tmp}"' EXIT
> +tables="${tmp}/arch/x86/model"
> +mkdir -p "${tables}"
> +
> +# Known keys pass.
> +printf '[{"EventName": "a", "EventCode": "0x1"}]\n' > "${tables}/events.json"
> +"${python}" "${jevents}" --check "${tmp}/arch" > /dev/null || exit 1
> +
> +# A key the generator does not read is reported.
> +printf '[{"EventName": "a", "NotAKey": "x"}]\n' > "${tables}/events.json"
> +"${python}" "${jevents}" --check "${tmp}/arch" > /dev/null && exit 1

[Severity: Medium]
Does jevents_check.sh ever run jevents.py --check on the in-tree JSON tables
under tools/perf/pmu-events/arch?

The commit message and file header state that the table key check runs as a
perf test, and _TABLE_KEYS and _check_tables() include exceptions for unused
keys and metricgroups.json in the tree.  However, this script only tests a
synthetic two-key file in ${tmp}/arch, and nothing in the build or test suite
invokes --check on tools/perf/pmu-events/arch, so neither existing nor future
key typos in the repository's PMU tables are caught by perf test.

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