check: report every unusable fixture header at once and ask it in the fmt job, plus two residual bridges that declared a pointer parameter

.github/workflows/pyre-ci.yml

@@ -56,6 +56,13 @@ jobs:
       run: cargo fmt --all -- --check
     - name: Check majit ownership boundary
       run: python3 scripts/check-majit-boundary.py
+    - name: Check synthetic fixture headers
+      # `pyre/bench/synth/` is shared, so a directive `check.py` requires reds
+      # every fixture that arrives without one. Asked here because the pass
+      # reads files and builds nothing (0.2 s over 479 fixtures), while the
+      # synthetic suite cannot answer until the backend build it sits behind
+      # has finished.
+      run: python3 pyre/check.py --check-headers
 
   cpyext-abi:
     name: cpyext ABI

pyre/check.py

@@ -2111,6 +2111,99 @@ def synth_spec_folds(path):
     return names
 
 
+def synth_fixture_headers(path):
+    """Every directive `path` owes, read in one pass.
+
+    Which of the readers above apply depends on the fixture's kind, so that
+    rule lives here rather than being restated at each consumer. Raises
+    `ValueError` on this fixture's first unusable directive.
+    """
+    selfcheck = synth_selfcheck(path)
+    headers = {
+        "selfcheck": selfcheck,
+        "skip_backends": synth_skip_backends(path),
+        "spec_folds": synth_spec_folds(path),
+    }
+    if selfcheck:
+        interpreted = synth_selfcheck_interpreted(path)
+        headers["interpreted"] = interpreted
+        headers["want_compiles"] = (
+            () if interpreted else synth_selfcheck_compiles(path)
+        )
+    else:
+        headers["max_pypy_ratio"] = synth_perf_gate(path)
+        headers["max_rss_mb"] = synth_rss_gate(path)
+        headers["skip_cpython"] = synth_skip_cpython(path)
+        headers["no_cpython"] = synth_no_cpython(path)
+        # `_apply_snapshot_gate` reads these two per backend and nothing there
+        # catches a `ValueError`, so an unusable value in either arrived as a
+        # traceback out of a backend run. Read here for the raise alone; the
+        # gate still reads them itself, once it knows which backend it is.
+        synth_ungated_jitstats(path)
+        synth_jitstats_bands(path)
+    return headers
+
+
+def report_unusable_headers(errors):
+    """Name every fixture whose header could not be read, not just the first.
+
+    `pyre/bench/synth/` is shared, so a directive this script requires reds
+    every fixture that arrives without it, and stopping at the first one costs
+    a CI run per name. Fixtures that fail the same way share a block, so the
+    guidance the reader wrote is printed once instead of once per fixture.
+    """
+    groups = {}
+    for path, message in errors:
+        groups.setdefault(message.replace(str(path), "<fixture>"), []).append(path)
+    print(f"{red('ERROR')}: {len(errors)} fixture(s) with a header this suite cannot read")
+    for message, paths in groups.items():
+        print()
+        for path in paths:
+            print(f"  {path}")
+        for line in message.splitlines():
+            print(f"    {line}")
+
+
+def synthetic_bench_paths(pattern):
+    """The fixtures *pattern* names under [`SYNTHETIC_BENCH_DIR`]."""
+    paths = sorted(Path(SYNTHETIC_BENCH_DIR).glob(pattern))
+    if not paths and not Path(pattern).suffix:
+        paths = sorted(Path(SYNTHETIC_BENCH_DIR).glob(f"{pattern}.py"))
+    return [p for p in paths if p.is_file() and p.suffix == ".py"]
+
+
+def read_synthetic_headers(paths):
+    """`({path: headers}, [(path, message)])` -- the readable and the rest."""
+    headers, unusable = {}, []
+    for path in paths:
+        try:
+            headers[path] = synth_fixture_headers(path)
+        except ValueError as e:
+            unusable.append((path, str(e)))
+    return headers, unusable
+
+
+def check_synthetic_headers(pattern):
+    """`--check-headers`: read every fixture header and report, building nothing.
+
+    This pass touches files and nothing else -- no binary, no reference
+    interpreter, no cargo -- so it belongs beside `cargo fmt --check`, which
+    every expensive job in the workflow already waits on. Run from the suite it
+    cannot answer until the build it sits behind has finished, and an authoring
+    error costs a whole CI run to hear about. Returns a process exit status.
+    """
+    paths = synthetic_bench_paths(pattern)
+    if not paths:
+        print(f"{red('ERROR')}: no synthetic benchmarks matched {pattern!r}")
+        return 1
+    unusable = read_synthetic_headers(paths)[1]
+    if unusable:
+        report_unusable_headers(unusable)
+        return 1
+    print(f"{len(paths)} synthetic fixture header(s) read, pattern={pattern!r}")
+    return 0
+
+
 # `[spec-census] fold=<label> consulted=N fired=N suppressed=N site=... parent=...`
 SPEC_CENSUS_FOLD_RE = re.compile(r"^\[spec-census\] fold=(\S+) .*?\bfired=(\d+)\b", re.M)
 
@@ -4499,20 +4592,16 @@ def _check_spec_folds(self, name, path, spec_folds, timeout, t_cpython, t_pypy):
                 self._append_comparison(b, name, t_cpython, t_pypy, "FAIL")
         return False
 
-    def run_synthetic_bench(self, path, timeout):
+    def run_synthetic_bench(self, path, timeout, headers):
         self.maybe_refresh_startups()
         name = f"synth/{Path(path).stem}"
         effective_timeout = scaled_timeout(timeout, self.args.timeout_scale)
-        try:
-            max_pypy_ratio = synth_perf_gate(path)
-            max_rss_mb = synth_rss_gate(path)
-            skip_backends = synth_skip_backends(path)
-            skip_cpython = synth_skip_cpython(path)
-            no_cpython = synth_no_cpython(path)
-            spec_folds = synth_spec_folds(path)
-        except ValueError as e:
-            print(f"{red('ERROR')}: {e}")
-            sys.exit(1)
+        max_pypy_ratio = headers["max_pypy_ratio"]
+        max_rss_mb = headers["max_rss_mb"]
+        skip_backends = headers["skip_backends"]
+        skip_cpython = headers["skip_cpython"]
+        no_cpython = headers["no_cpython"]
+        spec_folds = headers["spec_folds"]
 
         print(f"  {name}")
 
@@ -4612,46 +4701,38 @@ def run_synthetic_bench(self, path, timeout):
 
     def run_synthetic_suite(self):
         pattern = self.args.synthetic_pattern
-        paths = sorted(Path(SYNTHETIC_BENCH_DIR).glob(pattern))
-        if not paths and not Path(pattern).suffix:
-            paths = sorted(Path(SYNTHETIC_BENCH_DIR).glob(f"{pattern}.py"))
-        paths = [p for p in paths if p.is_file() and p.suffix == ".py"]
+        paths = synthetic_bench_paths(pattern)
         if not paths:
             print(f"{red('ERROR')}: no synthetic benchmarks matched {pattern!r}")
             sys.exit(1)
 
         print(bold("synthetic parity suite"))
         print(dim(f"{len(paths)} benchmark(s), pattern={pattern!r}"))
+        # Read every fixture's header before running any of them: a missing or
+        # malformed directive is an authoring error, and it is reported with
+        # the others of its kind rather than one per run. `--check-headers`
+        # runs this same pass with nothing built, so ordinarily it has already
+        # answered by the time the suite reaches here.
+        headers, unusable = read_synthetic_headers(paths)
+        if unusable:
+            report_unusable_headers(unusable)
+            sys.exit(1)
+
         for path in paths:
-            try:
-                selfcheck = synth_selfcheck(path)
-                skip_backends = synth_skip_backends(path) if selfcheck else ()
-                # Every header a selfcheck fixture owes, read inside the same
-                # try: a missing or malformed directive is an authoring error
-                # to report by name, not a traceback out of the suite loop.
-                selfcheck_folds = synth_spec_folds(path) if selfcheck else ()
-                interpreted = selfcheck and synth_selfcheck_interpreted(path)
-                want_compiles = (
-                    synth_selfcheck_compiles(path)
-                    if selfcheck and not interpreted
-                    else ()
-                )
-            except ValueError as e:
-                print(f"{red('ERROR')}: {e}")
-                sys.exit(1)
-            if selfcheck:
+            header = headers[path]
+            if header["selfcheck"]:
                 self.run_selfcheck(
                     f"synth/{path.stem}",
                     str(path),
                     self.args.synthetic_timeout,
-                    skip_backends=skip_backends,
-                    require_jit=not interpreted,
-                    spec_folds=selfcheck_folds,
-                    want_compiles=want_compiles,
+                    skip_backends=header["skip_backends"],
+                    require_jit=not header["interpreted"],
+                    spec_folds=header["spec_folds"],
+                    want_compiles=header["want_compiles"],
                 )
             else:
                 self.run_synthetic_bench(
-                    str(path), self.args.synthetic_timeout,
+                    str(path), self.args.synthetic_timeout, header,
                 )
         # A fixture that loses its cpython reference also loses the
         # cpython/pypy output cross-check, so the count belongs in the summary
@@ -4960,8 +5041,18 @@ def parse_backend_specs(specs):
         default=20.0,
         help="per-script timeout in seconds for synthetic benchmarks",
     )
+    parser.add_argument(
+        "--check-headers",
+        action="store_true",
+        help="read every synthetic fixture's `# pyre-check:` header, report the "
+             "unusable ones and exit; builds and runs nothing",
+    )
     parser.add_argument("pyre_path", nargs="?", default="")
     args = parser.parse_args()
+    # Answered here, ahead of the backend resolution below: the check is over
+    # the fixture files, and nothing about it needs a backend to exist.
+    if args.check_headers:
+        sys.exit(check_synthetic_headers(args.synthetic_pattern))
     try:
         args.backends = parse_backend_specs(args.backend)
     except argparse.ArgumentTypeError as e:

pyre/pyre-interpreter/src/opcode_ops.rs

@@ -1149,20 +1149,23 @@ pub extern "C" fn bh_lookup_exc_class_for_kind(kind_disc: i64) -> i64 {
 }
 
 /// C-ABI residual bridge for `exc_kind_discriminant`: the caught exception
-/// value rides in as a `PyObjectRef`; its `kind` discriminant rides back as `i64`.
-#[allow(improper_ctypes_definitions)]
-pub extern "C" fn bh_w_exception_get_kind(evalue: pyre_object::PyObjectRef) -> i64 {
-    pyre_object::interp_exceptions::exc_kind_discriminant(evalue)
+/// value rides in through the integer arg slot a residual call supplies, so
+/// take it as `i64` here; its `kind` discriminant rides back as `i64`.
+///
+/// `PyObjectRef` is `*mut PyObject`, which is four bytes on wasm32 and eight
+/// on the native targets. Spelling the parameter as the pointer would declare
+/// an `i32` argument there while the emitted `call_indirect` supplies a word,
+/// and the mismatch traps.
+pub extern "C" fn bh_w_exception_get_kind(evalue: i64) -> i64 {
+    pyre_object::interp_exceptions::exc_kind_discriminant(evalue as pyre_object::PyObjectRef)
 }
 
 /// C-ABI residual bridge for `exception_object_matches_stop_iteration`: the
-/// caught exception value rides in as a `PyObjectRef`; its boolean result rides
-/// back in the integer result slot.
-#[allow(improper_ctypes_definitions)]
-pub extern "C" fn bh_exception_object_matches_stop_iteration(
-    evalue: pyre_object::PyObjectRef,
-) -> i64 {
-    crate::error::exception_object_matches_stop_iteration(evalue) as i64
+/// caught exception value rides in through the integer arg slot, the
+/// [`bh_w_exception_get_kind`] twin; its boolean result rides back in the
+/// integer result slot.
+pub extern "C" fn bh_exception_object_matches_stop_iteration(evalue: i64) -> i64 {
+    crate::error::exception_object_matches_stop_iteration(evalue as pyre_object::PyObjectRef) as i64
 }
 
 #[cfg(test)]