diff --git a/pyre/bench/synth/README.md b/pyre/bench/synth/README.md index 83b30e67e1b..20915ebed94 100644 --- a/pyre/bench/synth/README.md +++ b/pyre/bench/synth/README.md @@ -27,8 +27,21 @@ opt into the same directory's self-checking mode: # pyre-check: selfcheck ``` -Such a fixture must exit successfully and print `PASS`. It is still discovered -by both runners; `check_synthetic.py` runs it only when `--pyre` is supplied. -A backend lacking a required mechanism can be scoped out in `check.py` with -`# pyre-check: skip-backends=wasm` followed by a comment explaining the missing -mechanism. +Such a fixture must exit successfully, print `PASS`, and have compiled at +least one loop. The last of those is what stops an interpreted run from +satisfying a guard written about compiled code: the assertion holds either way, +so without it the fixture would keep passing if the shape it guards stopped +reaching the JIT. A fixture whose invariant is not about compiled code says so +with + +```python +# pyre-check: selfcheck-interpreted +``` + +followed by a comment saying why, and is then graded on the exit status and the +marker alone. + +A selfcheck fixture is still discovered by both runners; `check_synthetic.py` +runs it only when `--pyre` is supplied. A backend lacking a required mechanism +can be scoped out in `check.py` with `# pyre-check: skip-backends=wasm` +followed by a comment explaining the missing mechanism. diff --git a/pyre/bench/synth/foriter_operator_index_replay_regression.py b/pyre/bench/synth/foriter_operator_index_replay_regression.py new file mode 100644 index 00000000000..ed5c4700ae3 --- /dev/null +++ b/pyre/bench/synth/foriter_operator_index_replay_regression.py @@ -0,0 +1,40 @@ +# pyre-check: selfcheck +# `operator.index(x)` reaches `space.index`, whose first test is +# `is_int_or_long`: an int is returned as-is, before any `__index__` lookup, so +# that call runs no user code and is replay-safe. Every other argument +# dispatches through `__index__`, which IS user code — so the replay-safe class +# must observe the argument, not merely pin the callable. +# +# `helper` is admitted into the surrounding FOR_ITER body. The trailing opaque +# `id` call makes the first inline sub-walk abort and replay `helper`. The int +# call is the arm being admitted; the object call must stay opaque. Admitting +# the object call too would let the replay run `__index__` a second time, and +# `hits` would read N + 1. + +from operator import index as _index + +N = 5000 +hits = [0] + + +class C: + def __index__(self) -> int: + hits[0] += 1 + return 3 + + +def helper(obj, n): + a = _index(n) + b = _index(obj) + id(obj) + return a + b + + +obj = C() +total = 0 +for _ in range(N): + total += helper(obj, 1) + +assert hits[0] == N, f"__index__ ran {hits[0]} times, expected {N}" +assert total == 4 * N, f"total {total}, expected {4 * N}" +print("PASS") diff --git a/pyre/bench/synth/global_store_plain_dict_globals.wasm.jitstats b/pyre/bench/synth/global_store_plain_dict_globals.wasm.jitstats index 67945696705..6ffd25ce61b 100644 --- a/pyre/bench/synth/global_store_plain_dict_globals.wasm.jitstats +++ b/pyre/bench/synth/global_store_plain_dict_globals.wasm.jitstats @@ -8,8 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1 +guard_failures=18 internal_compile_panics=0 -loops_aborted=0 -loops_compiled=4 +loops_aborted=1 +loops_compiled=5 retraces_compiled=0 diff --git a/pyre/bench/synth/list_append_virtual_payload.cranelift.jitstats b/pyre/bench/synth/list_append_virtual_payload.cranelift.jitstats index 1347a37f02e..4bf8f965d99 100644 --- a/pyre/bench/synth/list_append_virtual_payload.cranelift.jitstats +++ b/pyre/bench/synth/list_append_virtual_payload.cranelift.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=4 +bridges_compiled=7 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1001 +guard_failures=1403 internal_compile_panics=0 loops_aborted=0 loops_compiled=2 diff --git a/pyre/bench/synth/list_append_virtual_payload.dynasm.jitstats b/pyre/bench/synth/list_append_virtual_payload.dynasm.jitstats index 1347a37f02e..4bf8f965d99 100644 --- a/pyre/bench/synth/list_append_virtual_payload.dynasm.jitstats +++ b/pyre/bench/synth/list_append_virtual_payload.dynasm.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=4 +bridges_compiled=7 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1001 +guard_failures=1403 internal_compile_panics=0 loops_aborted=0 loops_compiled=2 diff --git a/pyre/bench/synth/list_append_virtual_payload.wasm.jitstats b/pyre/bench/synth/list_append_virtual_payload.wasm.jitstats index 1347a37f02e..4bf8f965d99 100644 --- a/pyre/bench/synth/list_append_virtual_payload.wasm.jitstats +++ b/pyre/bench/synth/list_append_virtual_payload.wasm.jitstats @@ -1,4 +1,4 @@ -bridges_compiled=4 +bridges_compiled=7 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 @@ -8,7 +8,7 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=1001 +guard_failures=1403 internal_compile_panics=0 loops_aborted=0 loops_compiled=2 diff --git a/pyre/bench/synth/list_to_tuple_star.cranelift.jitstats b/pyre/bench/synth/list_to_tuple_star.cranelift.jitstats index 1434ae6944f..651a3eaf3e9 100644 --- a/pyre/bench/synth/list_to_tuple_star.cranelift.jitstats +++ b/pyre/bench/synth/list_to_tuple_star.cranelift.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=0 +guard_failures=1 internal_compile_panics=0 loops_aborted=0 -loops_compiled=0 +loops_compiled=1 +retraces_compiled=0 diff --git a/pyre/bench/synth/list_to_tuple_star.dynasm.jitstats b/pyre/bench/synth/list_to_tuple_star.dynasm.jitstats index 1434ae6944f..651a3eaf3e9 100644 --- a/pyre/bench/synth/list_to_tuple_star.dynasm.jitstats +++ b/pyre/bench/synth/list_to_tuple_star.dynasm.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=0 +guard_failures=1 internal_compile_panics=0 loops_aborted=0 -loops_compiled=0 +loops_compiled=1 +retraces_compiled=0 diff --git a/pyre/bench/synth/list_to_tuple_star.wasm.jitstats b/pyre/bench/synth/list_to_tuple_star.wasm.jitstats index 1434ae6944f..651a3eaf3e9 100644 --- a/pyre/bench/synth/list_to_tuple_star.wasm.jitstats +++ b/pyre/bench/synth/list_to_tuple_star.wasm.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=0 +guard_failures=1 internal_compile_panics=0 loops_aborted=0 -loops_compiled=0 +loops_compiled=1 +retraces_compiled=0 diff --git a/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats b/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats index 2c65468b921..fa0e7c8b47a 100644 --- a/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats +++ b/pyre/bench/synth/mapdict_frozen_unboxing_fold.cranelift.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=2 +guard_failures=13 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 +retraces_compiled=0 diff --git a/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats b/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats index 2c65468b921..fa0e7c8b47a 100644 --- a/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats +++ b/pyre/bench/synth/mapdict_frozen_unboxing_fold.dynasm.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=2 +guard_failures=13 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 +retraces_compiled=0 diff --git a/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats b/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats index 2c65468b921..fa0e7c8b47a 100644 --- a/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats +++ b/pyre/bench/synth/mapdict_frozen_unboxing_fold.wasm.jitstats @@ -8,7 +8,8 @@ fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=2 +guard_failures=13 internal_compile_panics=0 loops_aborted=0 loops_compiled=3 +retraces_compiled=0 diff --git a/pyre/bench/synth/oserror_errno_fields_regression.py b/pyre/bench/synth/oserror_errno_fields_regression.py index 0ff789690e7..fd3a1b484ca 100644 --- a/pyre/bench/synth/oserror_errno_fields_regression.py +++ b/pyre/bench/synth/oserror_errno_fields_regression.py @@ -1,4 +1,6 @@ # pyre-check: selfcheck +# pyre-check: selfcheck-interpreted +# The invariant is interpreter-level, so the run compiles no loop. # pyre-check: skip-backends=wasm # The wasm guest has no os/filesystem support used by this fixture. # A failed syscall raises the errno-specific OSError subclass with diff --git a/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats b/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats index f517fe99127..dc67c9f95ac 100644 --- a/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats +++ b/pyre/bench/synth/pickle_terminal_raise_resume.wasm.jitstats @@ -10,6 +10,6 @@ field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 guard_failures=319 internal_compile_panics=0 -loops_aborted=8 -loops_compiled=69 +loops_aborted=9 +loops_compiled=70 retraces_compiled=0 diff --git a/pyre/bench/synth/posix_replace_regression.py b/pyre/bench/synth/posix_replace_regression.py index 52336c48a7c..df43cfe9019 100644 --- a/pyre/bench/synth/posix_replace_regression.py +++ b/pyre/bench/synth/posix_replace_regression.py @@ -1,4 +1,6 @@ # pyre-check: selfcheck +# pyre-check: selfcheck-interpreted +# The invariant is interpreter-level, so the run compiles no loop. # pyre-check: skip-backends=wasm # The wasm guest has no filesystem support used by this fixture. # os.replace(src, dst) renames over an existing destination in one step and diff --git a/pyre/bench/synth/surrogate_class_kwargs.cranelift.jitstats b/pyre/bench/synth/surrogate_class_kwargs.cranelift.jitstats index 1d438574725..bc56e8a7e4c 100644 --- a/pyre/bench/synth/surrogate_class_kwargs.cranelift.jitstats +++ b/pyre/bench/synth/surrogate_class_kwargs.cranelift.jitstats @@ -3,12 +3,13 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=0 -fbw_blackhole_adopted_single_frame=0 +fbw_blackhole_adopted_single_frame=14 fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=0 +guard_failures=2159 internal_compile_panics=0 -loops_aborted=0 -loops_compiled=3 +loops_aborted=14 +loops_compiled=4 +retraces_compiled=0 diff --git a/pyre/bench/synth/surrogate_class_kwargs.dynasm.jitstats b/pyre/bench/synth/surrogate_class_kwargs.dynasm.jitstats index 1d438574725..bc56e8a7e4c 100644 --- a/pyre/bench/synth/surrogate_class_kwargs.dynasm.jitstats +++ b/pyre/bench/synth/surrogate_class_kwargs.dynasm.jitstats @@ -3,12 +3,13 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=0 -fbw_blackhole_adopted_single_frame=0 +fbw_blackhole_adopted_single_frame=14 fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=0 +guard_failures=2159 internal_compile_panics=0 -loops_aborted=0 -loops_compiled=3 +loops_aborted=14 +loops_compiled=4 +retraces_compiled=0 diff --git a/pyre/bench/synth/surrogate_class_kwargs.wasm.jitstats b/pyre/bench/synth/surrogate_class_kwargs.wasm.jitstats index 1d438574725..bc56e8a7e4c 100644 --- a/pyre/bench/synth/surrogate_class_kwargs.wasm.jitstats +++ b/pyre/bench/synth/surrogate_class_kwargs.wasm.jitstats @@ -3,12 +3,13 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=0 -fbw_blackhole_adopted_single_frame=0 +fbw_blackhole_adopted_single_frame=14 fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=0 +guard_failures=2159 internal_compile_panics=0 -loops_aborted=0 -loops_compiled=3 +loops_aborted=14 +loops_compiled=4 +retraces_compiled=0 diff --git a/pyre/bench/synth/type_name_surrogate_reject.cranelift.jitstats b/pyre/bench/synth/type_name_surrogate_reject.cranelift.jitstats index 111116de3f9..7d47779afa9 100644 --- a/pyre/bench/synth/type_name_surrogate_reject.cranelift.jitstats +++ b/pyre/bench/synth/type_name_surrogate_reject.cranelift.jitstats @@ -1,14 +1,15 @@ -bridges_compiled=1 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=0 -fbw_blackhole_adopted_single_frame=0 +fbw_blackhole_adopted_single_frame=1 fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=201 +guard_failures=17799 internal_compile_panics=0 -loops_aborted=0 +loops_aborted=1 loops_compiled=2 +retraces_compiled=0 diff --git a/pyre/bench/synth/type_name_surrogate_reject.dynasm.jitstats b/pyre/bench/synth/type_name_surrogate_reject.dynasm.jitstats index 111116de3f9..7d47779afa9 100644 --- a/pyre/bench/synth/type_name_surrogate_reject.dynasm.jitstats +++ b/pyre/bench/synth/type_name_surrogate_reject.dynasm.jitstats @@ -1,14 +1,15 @@ -bridges_compiled=1 +bridges_compiled=0 descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=0 -fbw_blackhole_adopted_single_frame=0 +fbw_blackhole_adopted_single_frame=1 fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 -guard_failures=201 +guard_failures=17799 internal_compile_panics=0 -loops_aborted=0 +loops_aborted=1 loops_compiled=2 +retraces_compiled=0 diff --git a/pyre/bench/synth/type_name_surrogate_reject.wasm.jitstats b/pyre/bench/synth/type_name_surrogate_reject.wasm.jitstats index 111116de3f9..51a6f1a786c 100644 --- a/pyre/bench/synth/type_name_surrogate_reject.wasm.jitstats +++ b/pyre/bench/synth/type_name_surrogate_reject.wasm.jitstats @@ -3,12 +3,13 @@ descr_set_absent=0 descr_set_ambiguous=0 descr_set_stale_absent=0 fbw_blackhole_adopted_multi_frame=0 -fbw_blackhole_adopted_single_frame=0 +fbw_blackhole_adopted_single_frame=1 fbw_rolled_back_with_effects=0 fbw_store_journal_rollback_failed=0 field_pos_attached_misplaced=0 field_pos_spec_misplaced=0 guard_failures=201 internal_compile_panics=0 -loops_aborted=0 +loops_aborted=1 loops_compiled=2 +retraces_compiled=0 diff --git a/pyre/check.py b/pyre/check.py index dab3133957b..1fea117ea8f 100644 --- a/pyre/check.py +++ b/pyre/check.py @@ -6,6 +6,7 @@ import argparse import difflib +import hashlib import math import os import re @@ -865,6 +866,39 @@ def pyre_env(): return env +def effective_wasm_module(): + """The module path the wasm runner will actually load. + + `pyre_env` defaults `PYRE_WASM_MODULE` to the built module but leaves an + inherited one alone, so the default is not what runs whenever the caller + set it. A gate that asks about `WASM_MODULE_PATH` under an override checks + a file the run never opens, and clears the way for the stale module the + override names. + + Both `pyre_env` and the runner read the key by presence, so an empty value + survives into the child and is opened as the empty path; resolving it here + by falsiness would vouch for the built module while every invocation failed. + """ + override = os.environ.get("PYRE_WASM_MODULE") + return WASM_MODULE_PATH if override is None else override + + +def same_file(one, other): + """Whether two paths name one file, tolerating a missing one. + + The override is compared against the built module to decide whether the + build settled the question, and it is written by a caller who may spell it + absolutely, through a symlink, or relative to a different directory. + `os.path.samefile` answers that but raises when either side is absent, + which here is not an error: a missing file is simply not the built one, and + the existence check above has already spoken for the module itself. + """ + try: + return os.path.samefile(one, other) + except OSError: + return False + + def _dump_failed_run(output, stderr, limit=40): """Print the tail of a failed run's captured streams. @@ -1011,6 +1045,27 @@ def _jit_stats_merged(stderr): return fields if seen else None +def _jit_stats_field(stderr, field): + """One integer counter off the merged map, or `None`. + + `None` covers a run that printed no `[jit-stats]` line, one that printed + no such key, and one whose value does not read as an integer. None of + those is the same answer as a printed zero, which is why the caller + distinguishes them. + + The unfiltered map and not the snapshot, for the reason + [`_jit_stats_merged`] gives: a non-vacuity check reads a counter precisely + because it is not part of the recorded surface. + """ + fields = _jit_stats_merged(stderr or "") + if not fields or field not in fields: + return None + try: + return int(fields[field]) + except ValueError: + return None + + def _jit_stats_snapshot(stderr, ungated=()): """Return every jit-stats line merged into normalized key/value text. @@ -1771,6 +1826,29 @@ def synth_selfcheck(path): ) +def synth_selfcheck_interpreted(path): + """Read an optional per-fixture JIT-floor opt-out from its header: + # pyre-check: selfcheck-interpreted + + A selfcheck fixture asserts its own invariant, so it passes whenever the + invariant holds — including on a run where the JIT compiled nothing at all, + which for a guard against a *compiled* mis-admission is a pass that + establishes nothing. `run_selfcheck` therefore requires the run to have + compiled a loop, and this is how a fixture says its invariant is not about + compiled code: the two that carry it (`oserror_errno_fields_regression`, + `posix_replace_regression`) guard interpreter-level behaviour and measure + `loops_compiled=0` on every backend. + + An opt-out rather than an opt-in, so a fixture that quietly stops being + compiled is reported rather than passing on in silence. + """ + return _synth_header_flag( + path, + "# pyre-check: selfcheck-interpreted", + "synthetic selfcheck-interpreted marker takes no value", + ) + + def synth_ungated_jitstats(path): """Read an optional per-fixture jit-stats exemption from its header: # pyre-check: ungated-jitstats=bridges_compiled,guard_failures @@ -1867,6 +1945,480 @@ def default_binary(backend): return f"./target/release/{name}{EXE}" +# Repository-root files every crate is built against. +ROOT_BUILD_INPUTS = ("Cargo.toml", "Cargo.lock", ".cargo/config.toml", "rust-toolchain.toml") + + +def workspace_member_dirs(): + """Directories listed in the root `Cargo.toml` `members` array. + + A `.rs` file only reaches a compiler if it belongs to a member crate, so + this is what separates a build input from a bench fixture or a baseline + sitting elsewhere in the tree. It does not settle the whole input set: one + of those sources can embed a file from anywhere in the tree, which is what + [`embedded_inputs_outside_members`] recovers. `pyre/pyrex/tests/gate_triage_complete.rs` + derives its own search roots the same way, for the same reason — including + the line anchor, which is what keeps `default-members = [` above from + being read as this array. + """ + try: + manifest = Path("Cargo.toml").read_text(encoding="utf-8") + except OSError: + # No readable manifest is no member list, which the caller already + # treats as an unenumerable tree and fails open on. Letting the read + # raise instead would abort the whole run on a traceback. + return [] + listing = re.search(r"^\s*members\s*=\s*\[(.*?)\]", manifest, re.S | re.M) + if not listing: + return [] + members = [] + for member in re.findall(r'"([^"]+)"', listing.group(1)): + # Cargo accepts a glob member. There is none today; expanding here + # keeps a future one from being read as a literal directory name that + # matches nothing and silently narrows the input set. + if any(char in member for char in "*?["): + members.extend(sorted(str(path) for path in Path().glob(member))) + else: + members.append(member) + return members + + +# The package whose build script reads `PYRE_MIR_FRONTEND_LLBC`. `cargo` runs a +# build script with its own package directory as the working directory, so that +# is what a relative entry in the override is relative to — not this script's. +LLBC_OVERRIDE_BASE = "pyre/pyre-jit-trace" + + +def llbc_input_paths(): + """The LLBC artefacts the JIT front end will actually read. + + `majit_translate::build_semantic_program_via_active_frontend` reads them from + `PYRE_MIR_FRONTEND_LLBC` (an OS path-list) before falling back to the + workspace `build/llbc`, so a run under that override is built against + different bytes than the default glob names. + + A relative entry is resolved against `LLBC_OVERRIDE_BASE` rather than taken + as written. The reader is `pyre-jit-trace/build.rs`, and a build script's + working directory is its own package, so `../../build/llbc/pyre-jit.ullbc` + is the workspace artefact to the build and two levels above the repository + to a caller resolving from the root. Taken as written it names a file that + is not there, which the digest records as an unreadable input — the same + value however often the real artefact is rewritten, so `--no-build` would + approve a generated JIT front end built from an LLBC that has since moved. + """ + override = os.environ.get("PYRE_MIR_FRONTEND_LLBC") + if override: + return [ + entry if os.path.isabs(entry) + else os.path.normpath(os.path.join(LLBC_OVERRIDE_BASE, entry)).replace(os.sep, "/") + for entry in override.split(os.pathsep) if entry + ] + return [str(path) for path in Path("build/llbc").glob("*.ullbc")] + + +# `include_str!("x")`, `include_bytes!("x")` and `include!("x")` with a plain +# literal. A path built with `concat!`/`env!` is deliberately unmatched: those +# name `OUT_DIR`, which is under `target/` and so outside the tree this set is +# derived from. +EMBED_MACRO = re.compile(r'include(?:_str|_bytes)?!\s*\(\s*"([^"\\]*)"') + + +def embedded_inputs_outside_members(member_sources, members, listed): + """Files a member crate embeds from outside every member directory. + + `include_str!` and its siblings read their file at compile time, so it is a + build input exactly as the `.rs` around it is, and `rustc` records it in the + depinfo that makes `cargo` rebuild when it changes. The path is written + relative to the source file, so it can climb out of the crate and out of the + workspace entirely: `majit-metainterp/src/ruleopt/mod.rs` embeds + `rpython/jit/metainterp/ruleopt/real.rules`, which the member-directory + filter alone drops. Left out, editing that file rebuilds the artefact + without moving the fingerprint, and a later `--no-build` run approves a + binary built from the previous text. + + Scanned rather than listed by hand, so a second one cannot be added to the + tree without the gate seeing it. + + Conservative in one direction on purpose: an embed under `#[cfg(test)]` + reaches the unit tests and not the measured artefact, and this counts it + anyway rather than track cfgs. The cost is a rebuild nobody needed; the + alternative fails the other way, which is the one that reports a + measurement the binary does not support. + """ + outside = set() + for source in member_sources: + if not source.endswith(".rs"): + continue + try: + text = Path(source).read_text(encoding="utf-8", errors="replace") + except OSError: + # Raced with a delete, or unreadable. The file was in the listing a + # moment ago, so dropping its embeds is no worse than the enclosing + # enumeration already is about a tree changing under it. + continue + if "include" not in text: + continue + for embedded in EMBED_MACRO.findall(text): + resolved = os.path.normpath(os.path.join(os.path.dirname(source), embedded)) + if resolved.startswith("..") or os.path.isabs(resolved): + # Outside the repository, so no digest over the tree can cover + # it; the enumeration says what it saw and nothing more. + continue + resolved = resolved.replace(os.sep, "/") + if resolved.startswith(members) or resolved not in listed: + continue + outside.add(resolved) + return sorted(outside) + + +def build_input_paths(): + """Every file a release artefact is built from, or `None` when the tree + cannot be enumerated — which makes the freshness gate below fail open + rather than block a checkout that has no git. + + Deliberately derived from the tree alone, never from `target/`. Reading the + build scripts' own `cargo:rerun-if-changed=` declarations back out of + `target/*/build/*/output` was tried and removed: the paths there are + written relative to each crate's own directory, so all but one of the 57 + failed to resolve from the repository root and the whole mechanism + contributed nothing but a dependence on which profiles and targets happened + to have been built. + + Untracked files count, which is why the enumeration is not just + `git ls-files`: a new `.rs` under a member crate compiles into the artefact + before it is ever staged. Left out, it would contribute nothing to the + digest, so editing it would not move the fingerprint and a later + `--no-build` run would accept an artefact built from code the digest never + saw. Worse, deleting it again would return the digest to its earlier value + while the artefact still held its code. `--others --exclude-standard` adds + exactly the untracked-and-not-ignored files, so `target/` and the rest of + `.gitignore` stay out. + + One class is knowingly outside the set: an input a build script reads from + outside its crate. The only one in the tree is the `lib-python/3` closure + `pyre-interpreter/build.rs` embeds, and it is guarded by `wasm_vfs`, a + feature no artefact this script measures is built with. Enabling it for the + wasm-host build would mean adding that closure here. A file embedded by the + ordinary sources rather than by a build script is a different matter and is + covered — see [`embedded_inputs_outside_members`]. + """ + try: + listing = subprocess.run( + ["git", "ls-files", "-z", "--cached", "--others", "--exclude-standard"], + capture_output=True, text=True, encoding="utf-8", errors="replace", + ) + except OSError: + return None + if listing.returncode != 0: + return None + members = tuple(member + "/" for member in workspace_member_dirs()) + if not members: + return None + listed = listing.stdout.split("\0") + # Every file under a member crate, whatever its suffix: the CJK codec + # `.c`/`.h` sources and the app-level `.py` bodies reach the binary exactly + # as the `.rs` files do. + paths = [p for p in listed if p.startswith(members) or p in ROOT_BUILD_INPUTS] + # A member crate can embed a file from outside every member directory, and + # the filter above is by path alone, so it cannot see one. + paths += embedded_inputs_outside_members(paths, members, frozenset(listed)) + # The LLBC is a build input the fingerprint gate already tracks by content, + # but it is generated rather than tracked by git. + paths += llbc_input_paths() + return sorted(set(paths)) + + +def build_recipe_digest(): + """A digest of the options an artefact is built *with*. + + The input set above is the files a build reads; this is the command run + over them. `--no-default-features --features dynasm`, the wasm link args, + the toolchain and the build-std flags each select what lands in the + artefact, and none of them is a file under a member directory — this + script is a build input by no path rule, so editing `--growable-table` out + of `WASM_RUSTFLAGS` moved nothing in the fingerprint and a later + `--no-build` run approved a module built with it still in. + + The whole table at once rather than the row for one backend: a stamp + naming only its own row could not be compared without also recording which + row it was, the table drives every artefact, and an edit to it is rare + enough that rebuilding all of them is the cheaper mistake. + + What it does not cover is an option spelled inline in the build functions + rather than held here — `--target wasm32-unknown-unknown` is the one such + literal today. Hashing this script whole would cover those and would also + make every comment edit invalidate every artefact, which in a tree where + the script is edited far more often than the recipe is the worse trade. + """ + recipe = "\0".join(( + repr(CARGO_CONFIG), + WASM_RUSTFLAGS, + repr(WASM_CARGO_TOOLCHAIN), + repr(WASM_BUILD_STD_FLAGS), + WASM_BUILD_OUTPUT, + WASM_MODULE_PATH, + )) + return hashlib.sha256(recipe.encode("utf-8")).hexdigest() + + +# Sentinel distinguishing "not computed yet" from "computed, and there is no +# answer": a `None` result must be cached too, or one unenumerable call would +# leave a later call free to answer differently within the same run. +_FINGERPRINT_UNSET = object() +_BUILD_INPUTS_FINGERPRINT = _FINGERPRINT_UNSET + + +def build_inputs_fingerprint(): + """A digest of every build input's *content*, computed once per run. + + Content and not mtime: a concurrent `git checkout -- .`, a branch + switch, or an editor rewriting a file it did not change re-stamps whole + subtrees, and a gate reading mtimes then blocks a build that is in fact + current. Hashing the roughly one thousand inputs (about 1GB, most of it + the LLBC) costs under a second, against the multi-minute build + `--no-build` exists to skip. + + Computed after a build rather than before it: `cargo` may rewrite + `Cargo.lock`, which is itself an input, so the state that produced the + artefact is the state that exists once the build has finished. Nothing + else a build writes is in the set — that is what keeps the two orders + equivalent for every other input. + + "Once per run" therefore holds only between builds, and + [`invalidate_build_inputs_fingerprint`] is what ends a memoised value's + life. Without that call the rule above would bind the FIRST stamped + artefact alone: a run that builds several backends stamps after each one, + and every stamp past the first would carry a digest read before the build + that produced it. A later `--no-build` run then reads the tree as it + actually is and rejects artefacts that are in fact current. + """ + global _BUILD_INPUTS_FINGERPRINT + if _BUILD_INPUTS_FINGERPRINT is not _FINGERPRINT_UNSET: + return _BUILD_INPUTS_FINGERPRINT + paths = build_input_paths() + if paths is None: + _BUILD_INPUTS_FINGERPRINT = None + return None + _BUILD_INPUTS_FINGERPRINT = digest_input_paths(paths) + return _BUILD_INPUTS_FINGERPRINT + + +def digest_input_paths(paths): + """A digest over the named paths' contents, and over the build recipe.""" + digest = hashlib.sha256() + # The recipe first: an artefact is what its inputs and its build options + # together produce, and neither alone identifies it. + digest.update(build_recipe_digest().encode("ascii")) + digest.update(b"\0") + for path in paths: + # The name goes in before the bytes are read, so a file that is + # tracked but unreadable is distinguishable both from one that is + # absent from the list and from one that is empty. + digest.update(path.encode("utf-8")) + digest.update(b"\0") + try: + handle = open(path, "rb") + except OSError: + digest.update(b"\0") + continue + # Each file contributes a fixed-width value behind a one-byte tag, + # rather than its bytes inline: feeding the content straight in leaves + # the boundary between one file's tail and the next file's name + # unmarked, so a file holding `b"b\0x"` at path `a` hashes to the same + # bytes as an empty `a` beside a `b` holding `x`. Two input sets that + # collide read as one unchanged tree, which is the answer this gate + # exists to refuse. + content = hashlib.sha256() + with handle: + while chunk := handle.read(1 << 20): + content.update(chunk) + digest.update(b"\1") + digest.update(content.digest()) + return digest.hexdigest() + + +# The one input `cargo` itself rewrites during a build. A build window is +# judged with it left out, because a lock file the build resolved is the build +# doing its job rather than the tree moving underneath it. +CARGO_WRITTEN_INPUTS = ("Cargo.lock",) + +# What `inputs` says in a stamp whose artefact was built over a tree that +# changed while it was building. It is not a digest and can equal none, so the +# comparison refuses it; `require_fresh_artefacts` names it to say why. +STAMP_TREE_MOVED = "tree-moved-during-build" + +_BUILD_WINDOW_WITNESS = None + + +def build_window_witness(): + """A digest of the inputs a build is about to read, cargo's own aside. + + Uncached, unlike [`build_inputs_fingerprint`]: the whole point is to be + read twice and compared, so a memoised second read would answer with the + first. + """ + paths = build_input_paths() + if paths is None: + return None + return digest_input_paths([p for p in paths if p not in CARGO_WRITTEN_INPUTS]) + + +def open_build_window(): + """Note the tree state a build is starting from. + + `build_inputs_fingerprint` is read *after* a build, so that it describes + the `Cargo.lock` the build may have resolved. That ordering is what lets an + edit landing mid-build be recorded against an artefact compiled before it: + the stamp would then name a tree the binary does not contain, and a later + `--no-build` run would find them in agreement and measure it. Reading the + tree here as well makes that window observable, which is the only way to + tell the two orders apart. + """ + global _BUILD_WINDOW_WITNESS + _BUILD_WINDOW_WITNESS = build_window_witness() + + +def invalidate_build_inputs_fingerprint(): + """Drop the memoised digest so the next read enumerates the tree again. + + Called once per completed build, before its artefacts are stamped. The + memoisation that survives is the one it exists for: the read-only + `--no-build` path, where nothing writes to the tree between reads. + """ + global _BUILD_INPUTS_FINGERPRINT + _BUILD_INPUTS_FINGERPRINT = _FINGERPRINT_UNSET + + +def artefact_fingerprint_path(artefact): + return Path(str(artefact) + ".inputs") + + +def artefact_content_digest(artefact): + """The artefact's own bytes, or `None` if it cannot be read. + + 35MB of binary hashes in about a tenth of a second, against the + multi-minute build the stamp exists to skip. + """ + digest = hashlib.sha256() + try: + with open(artefact, "rb") as handle: + while chunk := handle.read(1 << 20): + digest.update(chunk) + except OSError: + return None + return digest.hexdigest() + + +def stamp_artefact_inputs(artefact): + """Record the inputs an artefact was just built from, beside it. + + The artefact's own digest goes in beside the input digest, because the + sidecar cannot otherwise tell which binary it is vouching for. A `cargo + build` outside this script overwrites the executable and leaves the + sidecar alone; restoring the tree to the state the sidecar names would + then make the input digests agree while the executable holds different + code, and `--no-build` would record baselines against it. + + A failure here is not worth losing a finished build over, but it is worth + saying: the next `--no-build` run would otherwise report the artefact as + unstamped with no trace of why. + """ + fingerprint = build_inputs_fingerprint() + if fingerprint is None: + return + if _BUILD_WINDOW_WITNESS is not None and _BUILD_WINDOW_WITNESS != build_window_witness(): + # Some input changed while cargo was reading them, so the artefact + # holds neither the tree the build started from nor the one that is + # here now. Stamping the current tree against it would make a later + # `--no-build` run agree with a binary that does not contain it. + print( + f" warning: the tree changed while {artefact} was building, so " + "its inputs are not\n the tree that is here now; it is " + "stamped as unusable for --no-build." + ) + fingerprint = STAMP_TREE_MOVED + content = artefact_content_digest(artefact) + if content is None: + print(f" warning: could not read {artefact} to stamp it") + return + stamp = artefact_fingerprint_path(artefact) + try: + stamp.write_text( + f"inputs {fingerprint}\nartefact {content}\n", encoding="utf-8" + ) + except OSError as exc: + print(f" warning: could not write the build-input stamp {stamp}: {exc}") + + +def read_artefact_stamp(stamp): + """The `(inputs, artefact)` digests a stamp records, or `None`. + + `None` covers both an unreadable stamp and one written in an older format + — either way this script did not write it in a shape it can check, which + is the "built outside this script" case the caller already reports. + """ + try: + text = stamp.read_text(encoding="utf-8") + except OSError: + return None + fields = {} + for line in text.splitlines(): + key, _, value = line.partition(" ") + if value: + fields[key] = value.strip() + if "inputs" not in fields or "artefact" not in fields: + return None + return fields["inputs"], fields["artefact"] + + +def require_fresh_artefacts(artefacts, reason, remedy): + """Refuse to measure an artefact that was built from different sources. + + `--no-build` skips every artefact of a backend, and wasm has two: the + native runner and the wasm module the runner loads. A module left behind by + an earlier build produces a fully green run — and a set of recorded + baselines — for code that is not in it, with nothing in the output saying + so. + + `reason` names the circumstance that put the artefact under question and + `remedy` says what closes it, because skipping the build is not the only + way to arrive here: `PYRE_WASM_MODULE` names the module the runner loads + whether or not a build ran, so a normal run can measure a module this + invocation did not produce. + + Each build stamps its artefact with `build_inputs_fingerprint` and with the + artefact's own content digest; an artefact carrying no stamp was built + outside this script, so its inputs are unknown and the run continues with a + note rather than a refusal. An artefact whose bytes no longer match the + stamp was rebuilt outside this script since, which puts it in the same + unknown-inputs class however well the input digest agrees. + """ + fingerprint = build_inputs_fingerprint() + if fingerprint is None: + return + for artefact in artefacts: + recorded = read_artefact_stamp(artefact_fingerprint_path(artefact)) + if recorded is None: + print( + f" note: {artefact} carries no build-input stamp; " + "its freshness is unchecked" + ) + continue + recorded_inputs, recorded_content = recorded + if recorded_content != artefact_content_digest(artefact): + fault = "has been rebuilt since it was stamped, so what it contains is unknown." + elif recorded_inputs == STAMP_TREE_MOVED: + fault = ("was built while the tree was changing, so which sources " + "it contains was never established.") + elif recorded_inputs != fingerprint: + fault = ("was built from different sources, so it does not contain " + "the current tree.") + else: + continue + print(f"ERROR: {reason}, but {artefact}\n {fault}\n {remedy}") + sys.exit(1) + + # Relative tolerance for wasm float outputs ONLY (see `wasm_outputs_match`). WASM_FLOAT_RTOL = 1e-9 @@ -2424,6 +2976,7 @@ def build_backend(self, backend): if cfg.get("wasm"): return self.build_wasm_backend() print(f"Building {cfg['bin']} (release, backend={backend})...") + open_build_window() cmd = [ "cargo", "build", "--release", "-p", "pyrex", "--bin", cfg["bin"], *cfg["extra"], @@ -2485,6 +3038,10 @@ def build_backend(self, backend): # The wall clock beside cargo's own figure is what makes a build that # recompiled the world distinguishable from a cache hit. print(f" {cargo_finished_line(proc)} — {elapsed:.1f}s wall", flush=True) + # The digest has to be read back after this build, not carried over + # from an earlier one in the same run. + invalidate_build_inputs_fingerprint() + stamp_artefact_inputs(default_binary(backend)) def build_wasm_backend(self): """Build the wasm32 `pyre-wasm` module and the native `pyre-wasm-runner`. @@ -2494,6 +3051,7 @@ def build_wasm_backend(self): produced: the wasm module (needs the export-table / custom-getrandom flags) and the host runner binary. """ + open_build_window() steps = [ ( "pyre-wasm (wasm32, --features wasm-host)", @@ -2549,6 +3107,12 @@ def build_wasm_backend(self): dst = Path(WASM_MODULE_PATH) if not dst.exists() or dst.read_bytes() != src_bytes: dst.write_bytes(src_bytes) + # As in `build_backend`: this build resolves target-specific + # dependencies, so `Cargo.lock` may differ from the state the native + # backends were stamped against. + invalidate_build_inputs_fingerprint() + stamp_artefact_inputs(WASM_MODULE_PATH) + stamp_artefact_inputs(default_binary("wasm")) if WASM_ENGINE == "wasmtime": self._warm_wasm_cache() @@ -3274,7 +3838,8 @@ def run_bench( # ── self-checking regression guard ── - def run_selfcheck(self, name, script, timeout, expect="PASS", skip_backends=()): + def run_selfcheck(self, name, script, timeout, expect="PASS", skip_backends=(), + require_jit=True): """Run a self-checking regression script on each enabled backend. The script asserts its own invariant (exit 0 AND prints *expect*); @@ -3285,6 +3850,14 @@ def run_selfcheck(self, name, script, timeout, expect="PASS", skip_backends=()): *skip_backends* names backends the guard does not apply to (e.g. a `time`-module timing guard cannot run on the wasm guest, which has no `time` module). + + With *require_jit* the run must also have compiled at least one loop. + A self-asserted invariant is satisfied by an interpreted run, so + without this a fixture guarding a compiled mis-admission passes while + establishing nothing — and it would go on passing if the shape it + guards stopped reaching the JIT, which is the change most likely to + make the guard vacuous. `synth_selfcheck_interpreted` turns it off for + a fixture whose invariant is not about compiled code. """ print(f" {name}") for backend in ALL_BACKENDS: @@ -3325,6 +3898,19 @@ def run_selfcheck(self, name, script, timeout, expect="PASS", skip_backends=()): _dump_failed_run(output, stderr) self._append_comparison(backend, name, "-", "-", "FAIL") continue + if require_jit: + compiled = _jit_stats_field(stderr, "loops_compiled") + if compiled is None or compiled < 1: + seen = "no [jit-stats] line" if compiled is None else "loops_compiled=0" + detail = ( + f"the guard ran interpreted ({seen}), so its assertion " + "says nothing about compiled code" + ) + self._record(backend, False, name, detail) + print(f"{red('FAIL')} {detail}") + _dump_failed_run(output, stderr) + self._append_comparison(backend, name, "-", "-", "FAIL") + continue self._record(backend, True, name, "") print(f"{green('PASS')} {elapsed:.2f}s") self._append_comparison(backend, name, "-", "-", f"{elapsed:.2f}s") @@ -3588,6 +4174,7 @@ def run_synthetic_suite(self): str(path), self.args.synthetic_timeout, skip_backends=skip_backends, + require_jit=not synth_selfcheck_interpreted(path), ) else: self.run_synthetic_bench( @@ -3929,12 +4516,41 @@ def main(): f"(missing executable: {pyre_bin})" ) sys.exit(1) - if backend == "wasm" and args.no_build and not Path(WASM_MODULE_PATH).is_file(): + wasm_module = effective_wasm_module() if backend == "wasm" else None + # Asked whether or not a build ran: `PYRE_WASM_MODULE` names the module + # the runner loads, and a build cannot supply one that is not there. + if backend == "wasm" and not Path(wasm_module).is_file(): + skipped = "--no-build requested" if args.no_build else "PYRE_WASM_MODULE set" print( - "ERROR: --no-build requested for backend 'wasm', but the " - f"wasm-host module is missing: {WASM_MODULE_PATH}" + f"ERROR: {skipped} for backend 'wasm', but the " + f"wasm-host module is missing: {wasm_module}" ) sys.exit(1) + if args.no_build: + # `--pyre-path` names a binary from outside this tree, so its age + # says nothing about these sources. The module is a separate + # artefact and is this tree's own unless overridden, so it is asked + # about either way; one from elsewhere carries no stamp and draws + # the unchecked-freshness note rather than a refusal. + artefacts = [] if args.pyre_path else [pyre_bin] + if backend == "wasm": + artefacts.append(wasm_module) + if artefacts: + require_fresh_artefacts( + artefacts, + f"--no-build requested for backend '{backend}'", + "Re-run without --no-build to rebuild it.", + ) + elif backend == "wasm" and not same_file(wasm_module, WASM_MODULE_PATH): + # The build above produced `WASM_MODULE_PATH` and stamped it, but + # the override names something else, so that is what the benchmarks + # will load and what any baseline they record describes. Building a + # module nothing runs is not a freshness check on the one that does. + require_fresh_artefacts( + [wasm_module], + f"PYRE_WASM_MODULE names the module backend '{backend}' will load", + "Unset PYRE_WASM_MODULE to run the module this build produced.", + ) chk._set_pyre(backend, pyre_bin) print() diff --git a/pyre/extra_tests/parity_tests/for_iter_call_bearing_comprehension.py b/pyre/extra_tests/parity_tests/for_iter_call_bearing_comprehension.py index 754cf6472c0..8f32401fae8 100644 --- a/pyre/extra_tests/parity_tests/for_iter_call_bearing_comprehension.py +++ b/pyre/extra_tests/parity_tests/for_iter_call_bearing_comprehension.py @@ -4,10 +4,10 @@ import random # A comprehension whose body calls a user Python function accumulates through -# LIST_APPEND. The call commits body effects, so a mid-body walk abort reaches -# `fbw_foriter_inflight_take` with a non-empty effect journal; that refuses the -# consumed item's delivery and the legacy replay resumes at the next iteration, -# losing the element. Only lengths are asserted, so the check holds whatever +# LIST_APPEND. `randrange` executes `_operator.index` on its bound, and while +# that was booked as a body effect the mid-body walk abort refused the consumed +# item's delivery, the legacy replay resumed at the next iteration, and the +# element was lost. Only lengths are asserted, so the check holds whatever # values the generator produces. random.seed(1234) for trial in range(400): diff --git a/pyre/extra_tests/parity_tests/for_iter_inflight_item_survives_sub_walk_abort.py b/pyre/extra_tests/parity_tests/for_iter_inflight_item_survives_sub_walk_abort.py new file mode 100644 index 00000000000..ae96169033a --- /dev/null +++ b/pyre/extra_tests/parity_tests/for_iter_inflight_item_survives_sub_walk_abort.py @@ -0,0 +1,61 @@ +# CPython-suite gap: the loss is a jit walk-abort path, invisible to an interpreter-level suite. +# parity-tests reason: pins a consumed FOR_ITER item against an abort inside a nested inline callee. + +"""A FOR_ITER item consumed before an aborted sub-walk still reaches the body. + +`step` is a bound method that mutates before it calls, so the walk admits it as +an inline callee and the append commits; `rec` is self-recursive, so descending +into it is refused and the walk aborts with the item already taken off the +iterator. Redelivering it would replay the append, so delivery is refused +whenever a body effect has committed -- which leaves the abort itself owing the +item a carrier. Without one the item is neither delivered nor rolled back, and +the loop body runs for it while its value never lands in `res`: the failure +signature is `len(res) < len(w.seen)`. + +The accumulator is a statement loop, so no LIST_APPEND is involved and what is +pinned is the item rather than the opcode that accumulates it. The trip counts +are the ones the loss was observed at -- the walk has to warm up before the +abort is reached, and a shorter sweep never reaches it. +""" + + +def ck(seq): + h = 7 + for v in seq: + h = (h * 1000003 + v) & 0xFFFFFFFFFF + return h + + +def rec(x): + if x <= 0: + # `id` is opaque, so the descent stops here rather than folding away. + return 1 if id(x) else 0 + return rec(x - 1) + 1 + + +class W: + def __init__(self): + self.seen = [] + + def step(self, x): + # Committed before the call below, and no rollback undoes it. + self.seen.append(x) + return x * 1000003 + rec(x % 4) + + +hh = 7 +bad = [] +for trial in range(1000): + n = trial % 20 + w = W() + res = [] + for i in range(n): + res.append(w.step(i)) + if len(res) != n or len(w.seen) != n: + bad.append((trial, n, len(res), len(w.seen))) + # Consuming both lists every trial is part of the shape the loss needs. + hh = (hh * 1000003 + ck(res) + ck(w.seen) + len(w.seen)) & 0xFFFFFFFFFF + +assert not bad, f"dropped items, (trial, n, len(res), len(seen)): {bad}" +assert hh == 950649862511, hh +print("OK") diff --git a/pyre/extra_tests/parity_tests/for_iter_widened_list_append_never_doubles.py b/pyre/extra_tests/parity_tests/for_iter_widened_list_append_never_doubles.py new file mode 100644 index 00000000000..4e26075a1d5 --- /dev/null +++ b/pyre/extra_tests/parity_tests/for_iter_widened_list_append_never_doubles.py @@ -0,0 +1,38 @@ +# CPython-suite gap: no CPython test observes how often a comprehension body runs. +# parity-tests reason: this pins the widened LIST_APPEND admission as exactly-once. + +# `LIST_APPEND` bodies are admitted whatever the body does. A mid-body walk +# abort must therefore neither drop the consumed item nor re-run the body over +# it. Count the calls as well as the elements: a drop shows up in the length, a +# double-apply only in the counter. The conditional makes half the elements +# take the call arm, so the loop guard-fails on the branch and the abort paths +# are the ones under test. + +calls = [0] + + +def uf(x): + calls[0] += 1 + return x + + +expected = list(range(500)) +for trial in range(200): + calls[0] = 0 + out = [uf(x) if x < 250 else x for x in range(500)] + assert len(out) == 500, (trial, len(out)) + assert calls[0] == 250, (trial, calls[0]) + assert out == expected, trial + +# The same shape where the accumulator is a named local rather than the +# comprehension's own temporary. +for trial in range(200): + calls[0] = 0 + collected = [] + for x in range(500): + collected.append(uf(x) if x < 250 else x) + assert len(collected) == 500, (trial, len(collected)) + assert calls[0] == 250, (trial, calls[0]) + assert collected == expected, trial + +print("OK") diff --git a/pyre/extra_tests/parity_tests/re_jit_call_resume.py b/pyre/extra_tests/parity_tests/re_jit_call_resume.py index 3cd6aaeb383..45733dd794c 100644 --- a/pyre/extra_tests/parity_tests/re_jit_call_resume.py +++ b/pyre/extra_tests/parity_tests/re_jit_call_resume.py @@ -1,7 +1,11 @@ import re -for i in range(10_000): +# 5_000, not more: the dynasm arm of this file spends about 27s of the runner's +# 30s per-fixture budget at 10_000, so it timed out on two different hosts. +# Halving it keeps what the file is for — 16 bridges either way, and 12 of the +# 15 single-frame blackhole adoptions the resume path is measured through. +for i in range(5_000): re.compile(str(i) + "|x") print("OK") diff --git a/pyre/extra_tests/parity_tests/utf8_check_untrusted_bytes.py b/pyre/extra_tests/parity_tests/utf8_check_untrusted_bytes.py new file mode 100644 index 00000000000..ade733ad70c --- /dev/null +++ b/pyre/extra_tests/parity_tests/utf8_check_untrusted_bytes.py @@ -0,0 +1,70 @@ +# CPython-suite gap: no test feeds marshal/pickle a three-byte sequence that +# holds no code point, and none indexes _json past the subject's length. +# parity-tests reason: these reach pyre's own WTF-8 representation, where the +# rejected buffer used to become a str whose length disagrees with its bytes. + +"""Bytes from outside the runtime are checked before they become a str.""" + +import json +import marshal +import pickle +import _json + + +def raises(exc, fn): + try: + fn() + except exc as caught: + return str(caught) + raise AssertionError(f"{exc.__name__} not raised") + + +def loads_both(payload): + """Feed one `TYPE_UNICODE` / `SHORT_BINUNICODE` payload to both readers.""" + size = len(payload) + yield lambda: marshal.loads(b"u" + size.to_bytes(4, "little") + payload) + yield lambda: pickle.loads(b"\x80\x04\x8c" + bytes([size]) + payload + b".") + + +# `ED C0 80` and `ED A0 41` pass a surrogate check that bounds only the first +# byte, and neither encodes a code point. Both readers decode with +# `surrogatepass`, so the position they report is the one that decode stops at +# -- not the first byte a strict scan trips over, which is the surrogate they +# accept. +for payload, reason in ( + (b"\xed\xc0\x80", "byte 0xed in position 0: invalid continuation byte"), + (b"\xed\xa0\x41", "byte 0xed in position 0: invalid continuation byte"), + (b"\xed\xa0\x80\xff", "byte 0xff in position 3: invalid start byte"), + (b"\xed\xa0\x80\xed\xc0\x80", "byte 0xed in position 3: invalid continuation byte"), + (b"\x41\xff", "byte 0xff in position 1: invalid start byte"), + (b"\xed\xa0\x80\xc3", "byte 0xc3 in position 3: unexpected end of data"), +): + for loads in loads_both(payload): + assert raises(UnicodeDecodeError, loads) == f"'utf-8' codec can't decode {reason}" + +# The same encoding of a real lone surrogate stays a one-character string. +assert marshal.loads(b"u\x03\x00\x00\x00\xed\xa0\x80") == "\ud800" +assert pickle.loads(b"\x80\x04\x8c\x03\xed\xa0\x80.") == "\ud800" +# A pair stays two code points, which is what `surrogatepass` decodes. +assert marshal.loads(b"u\x06\x00\x00\x00\xed\xa0\x80\xed\xb0\x80") == "\ud800\udc00" + +# `end` is a code point index into the subject, and both entry points bound it +# before resolving it to a byte offset. +subject = "中" * 100 +assert raises(StopIteration, lambda: json.JSONDecoder().scan_once(subject, 200)) == "200" +assert raises(ValueError, lambda: _json.scanstring(subject, 200)) == "end is out of bounds" +assert _json.scanstring('"ab"', 1) == ("ab", 4) + +# A lone surrogate is a length-1 separator with no UTF-8 spelling. +assert raises(ValueError, lambda: b"ab".hex(chr(0xDC80))) == "sep must be ASCII." +assert b"ab".hex("-") == "61-62" + +# `fromhex` rejects one as an ordinary non-hex character. Everything before +# the first rejected character is ASCII, so its byte offset is its index. +for hex_arg, position in ((chr(0xDC80), 0), ("41" + chr(0xDC80) + "42", 2), ("41\u4e2d", 2)): + reason = f"non-hexadecimal number found in fromhex() arg at position {position}" + assert raises(ValueError, lambda a=hex_arg: bytes.fromhex(a)) == reason + assert raises(ValueError, lambda a=hex_arg: bytearray.fromhex(a)) == reason +assert bytes.fromhex("41 42") == b"AB" + +print("OK") diff --git a/pyre/extra_tests/parity_tests/utf8_surrogatepass_error_span.py b/pyre/extra_tests/parity_tests/utf8_surrogatepass_error_span.py new file mode 100644 index 00000000000..4e9b36b6fad --- /dev/null +++ b/pyre/extra_tests/parity_tests/utf8_surrogatepass_error_span.py @@ -0,0 +1,88 @@ +# CPython-suite gap: test_codeccallbacks exercises surrogatepass round-trips +# but never the span of a truncated surrogate, and neither entry point is +# compared against the other anywhere. +# parity-tests reason: the span is produced by pyre's own state machine, whose +# `allow_surrogates` arm has no counterpart in `unicode_decode_utf8`. + +"""A surrogate the `surrogatepass` decoders cannot complete spans one byte.""" + +import codecs +import _codecs + + +def span(fn): + try: + fn() + except UnicodeDecodeError as e: + return e.start, e.end, e.reason + raise AssertionError("UnicodeDecodeError not raised") + + +# `str_decode_utf8` defaults `allow_surrogates` off, so the state machine stops +# at the second byte and `surrogatepass_errors` is what decodes a complete +# sequence. +assert span(lambda: b"\xed\xa0".decode("utf-8", "surrogatepass")) == ( + 0, 1, "invalid continuation byte") +assert span(lambda: b"\xed\xa0\x41".decode("utf-8", "surrogatepass")) == ( + 0, 1, "invalid continuation byte") +assert span(lambda: b"\xed".decode("utf-8", "surrogatepass")) == ( + 0, 1, "unexpected end of data") +assert span(lambda: b"\xe0\xa0".decode("utf-8", "surrogatepass")) == ( + 0, 2, "unexpected end of data") + +# `interp_codecs.utf_8_decode` turns `allow_surrogates` on, which admits a +# whole `ED A0..BF 80..BF` and nothing less: a pair that does not complete +# reports the byte the allowance was suspending judgement on, not the pair. +for final in (True, False): + assert span(lambda f=final: _codecs.utf_8_decode(b"\xed\xa0\x41", "surrogatepass", f)) == ( + 0, 1, "invalid continuation byte") + assert span(lambda f=final: _codecs.utf_8_decode(b"\xed\xa0\xff", "surrogatepass", f)) == ( + 0, 1, "invalid continuation byte") + assert span(lambda f=final: _codecs.utf_8_decode(b"\x41\xed\xa0\x42", "surrogatepass", f)) == ( + 1, 2, "invalid continuation byte") +assert span(lambda: _codecs.utf_8_decode(b"\xed\xa0", "surrogatepass", True)) == ( + 0, 1, "invalid continuation byte") + +# A lead pair that is not a surrogate keeps the two-byte span, and so does a +# four-byte sequence, so the arm above is the only one that narrowed. +for data, expected in ( + (b"\xe4\xb8", (0, 2, "unexpected end of data")), + (b"\xe4\xb8\x41", (0, 2, "invalid continuation byte")), + (b"\xed\x9f\x41", (0, 2, "invalid continuation byte")), + (b"\xf0\x9f\x98", (0, 3, "unexpected end of data")), + (b"\xf0\x9f\x98\x41", (0, 3, "invalid continuation byte")), + (b"\xe0\x80", (0, 1, "invalid continuation byte")), + (b"\xf0\x8f", (0, 1, "invalid continuation byte")), +): + assert span(lambda d=data: _codecs.utf_8_decode(d, "surrogatepass", True)) == expected, data + +# A truncated pair at the end of a non-final chunk is still retained rather +# than rejected -- the narrowing above applies only once the chunk is final. +assert _codecs.utf_8_decode(b"\xed\xa0", "surrogatepass", False) == ("", 0) +decoder = codecs.getincrementaldecoder("utf-8")("surrogatepass") +assert decoder.decode(b"\xed\xa0", False) == "" +assert decoder.decode(b"\x80", True) == "\ud800" + +# The allowance belongs to `surrogatepass` alone: every other handler sees a +# complete encoded surrogate as the bad continuation byte it is, and answers +# in its own way rather than decoding it. +assert span(lambda: _codecs.utf_8_decode(b"\xed\xa0\x80", "strict", True)) == ( + 0, 1, "invalid continuation byte") +for errors, expected in ( + ("replace", ("�" * 3, 3)), + ("ignore", ("", 3)), + ("backslashreplace", ("\\xed\\xa0\\x80", 3)), + ("surrogateescape", ("\udced\udca0\udc80", 3)), + ("surrogatepass", ("\ud800", 3)), +): + assert _codecs.utf_8_decode(b"\xed\xa0\x80", errors, True) == expected, errors + +# Every complete sequence still round-trips through both entry points. +for subject in ("\ud800", "\udfff", "\U00010000", "a\udc80b", "\ud800" * 50, + "abc", "\xe9中\U00010000", ""): + encoded = subject.encode("utf-8", "surrogatepass") + assert encoded.decode("utf-8", "surrogatepass") == subject + assert _codecs.utf_8_decode(encoded, "surrogatepass", True) == (subject, len(encoded)) + assert str(encoded, "utf-8", "surrogatepass") == subject + +print("OK") diff --git a/pyre/gate-triage.md b/pyre/gate-triage.md index a4dbd65d6c4..f075dae46b3 100644 --- a/pyre/gate-triage.md +++ b/pyre/gate-triage.md @@ -81,10 +81,10 @@ Kept as-is; listed for completeness. when it is unset. It is a measurement probe with no ON behaviour to graduate, so it has no epic — delete it with the demand counter itself once the pool's working set is settled. -- **Default-OFF experiments (1)** — every gate this bucket once held has had - its reader and its ON path deleted, except `PYRE_FORITER_CALL_BODY`, which is - waiting to graduate (§6a2). The other live default-OFF arms are the two wasm - re-emission A/Bs there, kept as the switched-off side of a one-binary +- **Default-OFF experiments (0)** — every gate this bucket once held has had + its reader and its ON path deleted, the last of them when the `LIST_APPEND` + admission it gated became unconditional. The live default-OFF arms left in + §6a2 are wasm A/Bs, kept as the switched-off side of a one-binary comparison rather than as experiments. - **Config / value / master switches (~16)** — tuning, paths, modes; keep: `PYRE_MIR_FRONTEND_LLBC`, `PYRE_WASM_ENGINE`, `_FUEL`, `_MODULE`, `_NO_CACHE`, @@ -179,7 +179,7 @@ Polarity below follows this file's rule, with one correction it needed: an | PYRE_WASM_INLINE_BRIDGE | merging a loop-closing bridge's ops into the module of the loop it guards into, so `guard → bridge → loop` becomes a `br` (`lib.rs inline_bridge_enabled`); `=0`/`false`/`off` restores the separate bridge module | the wasm trace-crossing epic closes; until then it is the one-binary A/B for the crossing shape | | PYRE_WASM_FULL_TEARDOWN | skipping the ~0.2s wasm engine teardown at exit; setting it restores the drops for leak diagnostics | when teardown stops being the dominant fixed startup tax | -### §6a2 — Default-OFF experiments (3) +### §6a2 — Default-OFF experiments (2) Kept as the switched-off arm of a one-binary comparison, not as latent defaults. Bridge inlining reaches module replacement on its own, so @@ -188,7 +188,6 @@ exercises the replacement machinery by itself. | gate | what turning it ON does | retire when | |---|---|---| -| PYRE_FORITER_CALL_BODY | admits a `LIST_APPEND` FOR_ITER body that also carries a CALL (`eval.rs for_iter_call_body_admitted`), so a call-bearing comprehension can trace. The body's item now survives the mid-body abort (the forward resume delivers it), but the loop it compiles residualizes the call it could not inline and is measured SLOWER: `[C(i) for i in range(2000)]` x200 runs 0.478s off / 0.569s on, dynasm | the traced body inlines that call (gh#73/gh#34); until then the ON arm loses and this is the one-binary A/B for it | | PYRE_WASM_REEMIT | re-emits a compiled loop's wasm module into its own table slot once, on the first bridge installed against it | when the replacement path no longer needs an isolated arm | | PYRE_WASM_INLINE_NONHEADER | admits an inlined region whose closing JUMP names a resumable LABEL other than the loop header AND whose source guard is in the LOOP BODY (`lib.rs inline_nonheader_enabled`); `=1`/`true`/`on` arms it. The preamble-sourced half of that class takes a different placement — blocks outside the header `loop`, body past its `end` — and is admitted unconditionally, so this flag now covers only the body-sourced half. Arming it removes 49.4M of the 257.3M cross-module crossings on the 81 fixtures that reach the decline and buys 0.74x/0.67x on two of them, but costs 1.23x on `spectral_norm` | the +18 ops per non-failing iteration it levies on the owner's fall-through is paid back on the fixtures it admits, or an admission rule separates them from `spectral_norm`, which sheds 99.7% of its crossings and still loses 23% | diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index 0a147815095..340b33ba042 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -4462,7 +4462,7 @@ pub fn is_builtin_dir_function(callable: PyObjectRef) -> bool { /// Shared identity test behind the `is_builtin_*_function` predicates: the /// callable is a function whose code is the builtin-code wrapper around /// `expected`. -fn is_builtin_code_function( +pub(crate) fn is_builtin_code_function( callable: PyObjectRef, expected: crate::gateway::BuiltinCodeFn, ) -> bool { @@ -5910,10 +5910,33 @@ fn type_descr_new_with_metaclass( // three-argument type() calls do not pass through __build_class__, so // fill __module__ from the live caller frame when the namespace did // not supply it. + // + // `ensure_module_attr` reaches the caller through + // `getexecutioncontext().gettopframe_nohidden()`, which starts at + // `gettopframe()`, whose `topframeref()` deref forces the frame. The + // `CURRENT_FRAME` thread-local this used to read arrives at the same + // frame while forcing nothing, and the force is the whole point: it is + // what `tracing_after_residual_call` reads back as the callee having + // escaped the virtualizable, so a trace whose body creates a class + // aborts here instead of recording a class object it will then guard + // on. Pyre's `gettopframe_nohidden` leaves the force to its consumers + // (see `force_frame`), and reading `w_globals` below is the consuming + // field read. let class_ns = pyre_object::gc_roots::shadow_stack_get(class_ns_root); if unsafe { pyre_object::w_dict_getitem_str(class_ns, "__module__") }.is_none() { - let frame = crate::eval::CURRENT_FRAME.with(|current| current.get()); + let ec = crate::call::getexecutioncontext() as *mut crate::PyExecutionContext; + let frame = if ec.is_null() { + std::ptr::null_mut() + } else { + unsafe { (*ec).gettopframe_nohidden() } + }; if !frame.is_null() { + // The force reaches the JIT's virtualizable writeback through a + // backend hook whose callee this crate cannot follow, so it is + // judged as able to collect. + let anchor = unsafe { crate::eval::FrameAnchor::from_raw(frame) }; + crate::executioncontext::force_frame(frame); + let frame = anchor.live(); let globals = unsafe { (*frame).get_w_globals() }; if !globals.is_null() && let Some(module) = crate::baseobjspace::finditem_str(globals, "__name__")? diff --git a/pyre/pyre-interpreter/src/module/_codecs/mod.rs b/pyre/pyre-interpreter/src/module/_codecs/mod.rs index ad8cbdee7c9..9b6d3e682a8 100644 --- a/pyre/pyre-interpreter/src/module/_codecs/mod.rs +++ b/pyre/pyre-interpreter/src/module/_codecs/mod.rs @@ -899,10 +899,14 @@ fn utf8_decode_impl( return Err(crate::PyError::type_error("errors must be str or None")); }; let data = decode_input_bytes(w_obj)?; + // `interp_codecs.utf_8_decode`: `surrogatepass` is the one handler that + // decodes a complete ED A0..BF 80..BF sequence in the state machine and + // retains an incomplete one for the next chunk. let (decoded, consumed) = crate::typedef::decode_utf8_with_errors_incremental( &data, errors, crate::baseobjspace::is_true(w_final)?, + errors == "surrogatepass", )?; Ok(w_tuple_new(vec![ w_str_from_wtf8_managed(decoded), diff --git a/pyre/pyre-interpreter/src/module/_json/mod.rs b/pyre/pyre-interpreter/src/module/_json/mod.rs index 0668c548885..8551943a52e 100644 --- a/pyre/pyre-interpreter/src/module/_json/mod.rs +++ b/pyre/pyre-interpreter/src/module/_json/mod.rs @@ -84,7 +84,11 @@ fn encode_basestring_impl(obj: PyObjectRef, ascii_only: bool) -> PyResult { fn scanstring_impl(doc: PyObjectRef, end: i64, strict_obj: PyObjectRef) -> PyResult { let value = require_string(doc)?; - if end < 0 { + // `py_scanstring` bounds `end` against the code point count before using + // it. `w_str_index_to_byte` takes an index in range, so the upper bound + // is the caller's to check; a count equal to `end` is the empty tail, + // which `scan_string` reports as an unterminated string. + if end < 0 || end as usize > unsafe { pyre_object::w_str_len(doc) } { return Err(PyError::value_error("end is out of bounds")); } let strict = crate::baseobjspace::is_true(strict_obj)?; @@ -589,10 +593,12 @@ fn scanner_call_impl(self_obj: PyObjectRef, doc: PyObjectRef, index: i64) -> PyR return Err(PyError::value_error("idx cannot be negative")); } let char_index = index as usize; - let byte_index = unsafe { pyre_object::w_str_index_to_byte(doc, char_index) }; - if byte_index >= unsafe { pyre_object::w_str_get_wtf8(doc) }.len() { + // `scanner_call` compares the index against the code point count, and it + // has to be this way round: `w_str_index_to_byte` takes an index in range. + if char_index >= unsafe { pyre_object::w_str_len(doc) } { return Err(stop_iteration(index)); } + let byte_index = unsafe { pyre_object::w_str_index_to_byte(doc, char_index) }; let _roots = gc_roots::push_roots(); let slot = gc_roots::shadow_stack_len(); for value in [self_obj, doc] { diff --git a/pyre/pyre-interpreter/src/module/_pickle/mod.rs b/pyre/pyre-interpreter/src/module/_pickle/mod.rs index 55a9348d3a1..e22e3a4ad27 100644 --- a/pyre/pyre-interpreter/src/module/_pickle/mod.rs +++ b/pyre/pyre-interpreter/src/module/_pickle/mod.rs @@ -621,34 +621,15 @@ pub(crate) fn read_int_le(data: &[u8]) -> i64 { pub(crate) fn str_from_utf8(data: &[u8]) -> Result { // BINUNICODE is encoded with Python's UTF-8 `surrogatepass`: lone // surrogates are valid pickle payloads and are represented internally as - // WTF-8, while malformed byte sequences must still be rejected. - let s = rustpython_wtf8::Wtf8Buf::from_bytes(data.to_vec()).map_err(utf8_decode_error)?; - Ok(pyre_object::unicodeobject::w_str_from_wtf8_managed(s)) -} - -/// Construct Python's UTF-8 decode error details from Rust's `Utf8Error`. -/// `error_len == None` is a truncated multibyte sequence; otherwise a valid -/// leading byte at `valid_up_to` means the following byte was an invalid -/// continuation, and every other offending byte is an invalid start. -pub(crate) fn utf8_decode_error(bytes: Vec) -> PyError { - let error = std::str::from_utf8(&bytes).unwrap_err(); - let start = error.valid_up_to(); - let reason = match error.error_len() { - None => "unexpected end of data", - Some(_) - if bytes - .get(start) - .is_some_and(|byte| matches!(byte, 0xC2..=0xF4)) => - { - "invalid continuation byte" - } - Some(_) => "invalid start byte", - }; - let end = start - + error - .error_len() - .unwrap_or(bytes.len().saturating_sub(start)); - crate::typedef::unicode_decode_error("utf-8", &bytes, start, end.min(bytes.len()), reason) + // WTF-8, while malformed byte sequences must still be rejected. The + // rejection is `rutf8::wtf8_from_bytes`'s and not `Wtf8Buf::from_bytes`'s, + // which admits sequences that hold no code point at all, and it is + // reported at the position that validator stopped at. + let s = pyre_object::rutf8::wtf8_from_bytes(data, true) + .map_err(|error| crate::typedef::utf8_decode_error_from(data, error.pos))?; + Ok(pyre_object::unicodeobject::w_str_from_wtf8_managed( + s.to_owned(), + )) } crate::py_module! { diff --git a/pyre/pyre-interpreter/src/module/_pickle/unpickler.rs b/pyre/pyre-interpreter/src/module/_pickle/unpickler.rs index f92f7c58aeb..5928c522d18 100644 --- a/pyre/pyre-interpreter/src/module/_pickle/unpickler.rs +++ b/pyre/pyre-interpreter/src/module/_pickle/unpickler.rs @@ -6,7 +6,7 @@ use crate::PyError; use super::{ HIGHEST_PROTOCOL, call_fn, call_meth, decode_long, import_module, op, parse_int_text, - read_int_le, str_from_utf8, unpickling_error, utf8_decode_error, + read_int_le, str_from_utf8, unpickling_error, }; #[derive(Clone, Copy, PartialEq, Eq)] @@ -1620,7 +1620,7 @@ fn read_line_bytes(slot: usize) -> Result, PyError> { fn read_line(slot: usize) -> Result { let bytes = read_line_bytes(slot)?; - String::from_utf8(bytes).map_err(|err| utf8_decode_error(err.into_bytes())) + String::from_utf8(bytes).map_err(|err| crate::typedef::utf8_decode_error(err.as_bytes())) } /// Read a newline-terminated decimal integer argument (GET / PUT in the diff --git a/pyre/pyre-interpreter/src/module/marshal/mod.rs b/pyre/pyre-interpreter/src/module/marshal/mod.rs index 5a058fc4bf6..cd7eee58d14 100644 --- a/pyre/pyre-interpreter/src/module/marshal/mod.rs +++ b/pyre/pyre-interpreter/src/module/marshal/mod.rs @@ -594,7 +594,45 @@ impl FileReader { } } +/// `read_wtf8`'s validation, done with `rutf8::wtf8_from_bytes`. +/// +/// The trait's own body uses `Wtf8::from_bytes`, which accepts three-byte +/// sequences that encode no code point, so a `u`/`a`/`z` payload naming one +/// reaches `w_str_from_wtf8` with a code point count its buffer disagrees with. +/// `r_object` decodes `TYPE_UNICODE` with `surrogatepass`, so surrogates are +/// admitted and a rejection is that call's `UnicodeDecodeError`, reported at +/// the position the validator stopped at, which the sink carries past the +/// wire reader's own error type. +fn strict_wtf8(bytes: &[u8], errors: ErrorSink) -> Result<&Wtf8, wire::MarshalError> { + pyre_object::rutf8::wtf8_from_bytes(bytes, true).map_err(|error| { + errors.remember(crate::typedef::utf8_decode_error_from(bytes, error.pos)); + wire::MarshalError::InvalidUtf8 + }) +} + +/// A marshal byte stream, wrapped so `read_wtf8` is the strict one. +struct BytesReader<'a> { + data: &'a [u8], + errors: ErrorSink, +} + +impl wire::Read for BytesReader<'_> { + fn read_slice(&mut self, n: u32) -> Result<&[u8], wire::MarshalError> { + self.data.read_slice(n) + } + + fn read_wtf8(&mut self, len: u32) -> Result<&Wtf8, wire::MarshalError> { + let errors = self.errors; + strict_wtf8(self.read_slice(len)?, errors) + } +} + impl wire::Read for FileReader { + fn read_wtf8(&mut self, len: u32) -> Result<&Wtf8, wire::MarshalError> { + let errors = self.errors; + strict_wtf8(self.read_slice(len)?, errors) + } + fn read_slice(&mut self, n: u32) -> Result<&[u8], wire::MarshalError> { // A hostile length prefix must not allocate `n` bytes before the file // proves they exist. Fill the reusable result incrementally with a @@ -1030,10 +1068,13 @@ fn marshal_to_bytes( /// false, a decoded code object is rejected (marshal.check_no_code). fn unmarshal_bytes(data: &[u8], allow_code: bool) -> PyResult { let _roots = pyre_object::gc_roots::push_roots(); - let mut reader: &[u8] = data; let mut pending_error = None; - let result = match wire::deserialize_value(&mut reader, PyreMarshalBag::new(&mut pending_error)) - { + let bag = PyreMarshalBag::new(&mut pending_error); + let mut reader = BytesReader { + data, + errors: bag.errors, + }; + let result = match wire::deserialize_value(&mut reader, bag) { Ok(result) => result, Err(error) => return Err(pending_error.unwrap_or_else(|| marshal_error(error))), }; diff --git a/pyre/pyre-interpreter/src/module/operator/mod.rs b/pyre/pyre-interpreter/src/module/operator/mod.rs index 0998bb3684c..8cef3cd1491 100644 --- a/pyre/pyre-interpreter/src/module/operator/mod.rs +++ b/pyre/pyre-interpreter/src/module/operator/mod.rs @@ -7,6 +7,28 @@ fn op_index(args: &[PyObjectRef]) -> Result { unsafe { Ok(range_bigint_to_obj(range_obj_to_bigint(indexed))) } } +/// `index` as it is installed in the module namespace, arity check included. +/// +/// Registered through `interpleveldefs` rather than the `functions:` shorthand +/// because that shorthand wraps the body in an anonymous per-expansion closure: +/// the pointer the `BuiltinCode` then carries has no name any caller can write +/// down, and [`is_operator_index_function`] needs one to pin. +fn op_index_entry(args: &[PyObjectRef]) -> Result { + crate::gateway::check_declared_positional_arity("index", 1, args)?; + op_index(args) +} + +/// True iff `callable` is the canonical `_operator.index` function object. +/// +/// `space_index` answers an `int` (or `long`) by returning the argument +/// itself, before any `__index__` lookup, so that call runs no user code and +/// writes nothing. The JIT's replay-safety classification uses this identity +/// plus an observed int argument to say so; a rebound `operator.index` is a +/// different object and keeps the conservative treatment. +pub fn is_operator_index_function(callable: PyObjectRef) -> bool { + crate::builtins::is_builtin_code_function(callable, op_index_entry) +} + /// Shared body for the binary-arithmetic thunks (`add`/`sub`/`mul`). The /// operand error propagates, matching the `truediv`/`floordiv` thunks. fn op_binary(args: &[PyObjectRef], f: F) -> Result @@ -160,13 +182,20 @@ crate::py_module! { // likewise delegates to `space.sequence_index` (`interp_operator.py`); // `concat` (`op_concat`, `interp_operator.py`) guards both operands for // `__getitem__`. + interpleveldefs: { + // Named registration, so the installed `BuiltinCode` carries a pointer + // `is_operator_index_function` can compare against. Same shape as the + // `functions:` entries otherwise: declared arity 1, own arity check. + "index" => crate::gateway::make_module_builtin_function_with_arity( + "index", op_index_entry, 1, + ), + }, appleveldefs: { "app_operator.py" => [ "itemgetter", "attrgetter", "methodcaller", ], }, functions: { - "index" / 1 = op_index, "add" / 2 = |args| op_binary(args, add), "sub" / 2 = |args| op_binary(args, sub), "mul" / 2 = |args| op_binary(args, mul), diff --git a/pyre/pyre-interpreter/src/module/time/interp_time.rs b/pyre/pyre-interpreter/src/module/time/interp_time.rs index 35a30943b28..ea055d138f3 100644 --- a/pyre/pyre-interpreter/src/module/time/interp_time.rs +++ b/pyre/pyre-interpreter/src/module/time/interp_time.rs @@ -1529,11 +1529,11 @@ pub fn strftime(args: &[PyObjectRef]) -> Result { // Genuinely non-UTF-8 LC_TIME output (from %Z/%a in a non-UTF-8 locale) // is not valid WTF-8; fall back to surrogateescape rather than raising, // mirroring str_decode_locale_surrogateescape. - let result = match rustpython_wtf8::Wtf8Buf::from_bytes(rendered) { + let result = match pyre_object::rutf8::wtf8_from_bytes(&rendered, true) { // interp_time.py returns `space.newutf8(decoded, size)`: // strftime's formatted value is an ordinary runtime string. - Ok(wtf8) => pyre_object::w_str_from_wtf8_managed(wtf8), - Err(bytes) => crate::typedef::charp2uni(&bytes), + Ok(wtf8) => pyre_object::w_str_from_wtf8_managed(wtf8.to_owned()), + Err(_) => crate::typedef::charp2uni(&rendered), }; Ok(result) } diff --git a/pyre/pyre-interpreter/src/typedef.rs b/pyre/pyre-interpreter/src/typedef.rs index 534c075ba7c..f1acaa94d6b 100644 --- a/pyre/pyre-interpreter/src/typedef.rs +++ b/pyre/pyre-interpreter/src/typedef.rs @@ -16,6 +16,9 @@ use std::sync::OnceLock; use majit_rlib::rbigint::RBigInt as BigInt; use pyre_object::pyobject::*; +use pyre_object::rutf8::{ + invalid_byte_2_of_3, invalid_byte_2_of_4, invalid_cont_byte, surrogate_bytes, +}; use pyre_object::*; use rustpython_wtf8::{CodePoint, Wtf8Buf}; @@ -23126,7 +23129,12 @@ fn bytes_maketrans(args: &[PyObjectRef]) -> Result fn parse_hex_string(args: &[PyObjectRef]) -> Result, crate::PyError> { let a = args[0]; if unsafe { pyre_object::is_str(a) } { - return parse_hex_bytes(unsafe { pyre_object::w_str_get_value(a) }.as_bytes()); + // Every character before the first rejected one is a hex digit or + // ASCII whitespace, so the byte offset the scan reports is also the + // code point offset `_PyBytes_FromHex` names. Reading the WTF-8 + // payload rather than demanding a `str` keeps a lone surrogate a + // rejected character instead of an abort. + return parse_hex_bytes(unsafe { pyre_object::w_str_get_wtf8(a) }.as_bytes()); } let Some(buffer) = crate::baseobjspace::simple_buffer_bytes(a)? else { return Err(crate::PyError::type_error(format!( @@ -23429,21 +23437,20 @@ pub(crate) fn bytes_method_hex(args: &[PyObjectRef]) -> Result crate::PyError { + utf8_decode_error_from(bytes, 0) +} + +/// The same, resumed at the position a `surrogatepass` validator stopped at. +/// +/// A strict scan cannot find that position itself: an encoded surrogate is +/// what stops it, and `surrogatepass` accepts one, so scanning `bytes` from +/// the front names the surrogate instead of the byte that was actually +/// rejected. `pos` comes from `rutf8::wtf8_from_bytes`, and the strict scan +/// resumed there stops immediately, because everything WTF-8 rejects at a +/// position UTF-8 rejects there too. +/// +/// # Panics +/// +/// `bytes[pos..]` must not be valid UTF-8, for the reason above. +pub(crate) fn utf8_decode_error_from(bytes: &[u8], pos: usize) -> crate::PyError { + let error = std::str::from_utf8(&bytes[pos..]).unwrap_err(); + let start = pos + error.valid_up_to(); + let reason = match error.error_len() { + None => "unexpected end of data", + Some(_) + if bytes + .get(start) + .is_some_and(|byte| matches!(byte, 0xC2..=0xF4)) => + { + "invalid continuation byte" + } + Some(_) => "invalid start byte", + }; + let end = start + + error + .error_len() + .unwrap_or(bytes.len().saturating_sub(start)); + unicode_decode_error("utf-8", bytes, start, end.min(bytes.len()), reason) +} + pub(crate) fn unicode_decode_error( encoding: &str, data: &[u8], @@ -23753,24 +23806,6 @@ const UTF8_CODE_LENGTH: [u8; 128] = [ 4, 4, 4, 4, 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, // F0-F4 + F5-FF ]; -/// rutf8.py:326-328 -fn invalid_cont_byte(b: u8) -> bool { - (b as i8) >= -0x40 // equivalent: b < 0x80 || b > 0xBF -} - -/// rutf8.py `_invalid_byte_2_of_3`: reject surrogate encodings unless the -/// caller selected the `surrogatepass` path. -fn invalid_byte_2_of_3(ch1: u8, ch2: u8, allow_surrogates: bool) -> bool { - invalid_cont_byte(ch2) - || (ch1 == 0xE0 && ch2 < 0xA0) - || (ch1 == 0xED && ch2 > 0x9F && !allow_surrogates) -} - -/// rutf8.py:345-348 -fn invalid_byte_2_of_4(ch1: u8, ch2: u8) -> bool { - invalid_cont_byte(ch2) || (ch1 == 0xF0 && ch2 < 0x90) || (ch1 == 0xF4 && ch2 > 0x8F) -} - /// interp_locale.py `charp2uni` — decode a C string the way /// `str(bytes, 'utf-8', 'surrogateescape')` does: valid UTF-8 passes /// through and any other byte becomes a lone `0xDC00 + byte` surrogate. @@ -23869,17 +23904,44 @@ pub(crate) fn fsdecode_wtf8_total(data: &[u8]) -> Wtf8Buf { /// Unicode scalar values via `char::from_u32`, or a WTF-8 `CodePoint` for /// the `surrogatepass` path. fn decode_utf8_with_errors(data: &[u8], err_mode: &str) -> Result { - decode_utf8_with_errors_incremental(data, err_mode, true).map(|(decoded, _)| decoded) + decode_utf8_with_errors_incremental(data, err_mode, true, false).map(|(decoded, _)| decoded) } /// PyPy `unicodehelper.str_decode_utf8`: the incremental form additionally /// returns the byte position consumed and leaves a valid but incomplete /// trailing sequence untouched when `final_` is false. +/// +/// `allow_surrogates` is the caller's, as it is upstream: `str_decode_utf8` +/// defaults it to false and only `interp_codecs.utf_8_decode` turns it on. +/// Deriving it from `err_mode` here instead made both entry points take the +/// `_codecs` answer, and the two do not agree — with it off, `ED A0` stops as +/// an invalid continuation byte at 0..1 and `surrogatepass_errors` decodes a +/// complete surrogate itself. +/// +/// `[3.14-spec]` The allowance covers a **complete** `ED A0..BF 80..BF` and +/// nothing less. `_str_decode_utf8_slowpath` reports the whole admitted lead +/// pair when the sequence then fails, so `_codecs.utf_8_decode(b'\xed\xa0A', +/// 'surrogatepass', True)` spans 0..2 there; `unicode_decode_utf8` has no +/// `allow_surrogates` at all and spans 0..1, which is what a caller reads off +/// `UnicodeDecodeError.start`/`.end`. Measured on 3.14.0 and pypy3 over the +/// 42 rows of `utf8_surrogatepass_error_span.py`: they differ on the six +/// where the lead pair is a surrogate and the sequence does not complete, +/// and agree everywhere else. A truncated pair at the end of a non-final +/// chunk is still retained, as both do. pub(crate) fn decode_utf8_with_errors_incremental( data: &[u8], err_mode: &str, final_: bool, + allow_surrogates: bool, ) -> Result<(Wtf8Buf, usize), crate::PyError> { + // `str_decode_utf8` tries the "fast version first": a buffer that is + // already well formed is its own decode, so nothing below runs and no + // error handler is reachable. This is the whole of the common case, and + // it scans a word at a time where the state machine reads a byte at a + // time -- 39 ASCII bytes cost 232 ns through the machine and 34 here. + if let Ok(valid) = pyre_object::rutf8::wtf8_from_bytes(data, allow_surrogates) { + return Ok((valid.to_owned(), data.len())); + } // A custom error handler may replace exc.object; decoding then resumes // from the new bytes (`s`), re-evaluating `size` each iteration. The // common path keeps the borrowed slice (no allocation). @@ -23887,11 +23949,6 @@ pub(crate) fn decode_utf8_with_errors_incremental( let mut size = s.len(); let mut result = Wtf8Buf::new(); let mut pos = 0; - // PyPy `interp_codecs.utf_8_decode` passes - // `allow_surrogates=True` specifically for `surrogatepass`, so a valid - // ED A0..BF 80..BF sequence is decoded directly and an incomplete one is - // retained for the next incremental chunk. - let allow_surrogates = err_mode == "surrogatepass"; // Run a utf-8 error handler and rebind `s`/`size` when it returns // replacement bytes; then advance `pos` to the resume position. macro_rules! run_err { @@ -23938,7 +23995,15 @@ pub(crate) fn decode_utf8_with_errors_incremental( if !final_ { break; } - run_err!(pos, pos + 2, "unexpected end of data"); + // [3.14-spec] A pair only the surrogate allowance admits has + // to complete as a whole surrogate; with the third byte + // missing the answer is the one the allowance suspended. + let (end, reason) = if surrogate_bytes(ordch1, ordch2) { + (pos + 1, "invalid continuation byte") + } else { + (pos + 2, "unexpected end of data") + }; + run_err!(pos, end, reason); continue; } else if n == 4 { // unicodehelper.py:435-459 @@ -23987,7 +24052,15 @@ pub(crate) fn decode_utf8_with_errors_incremental( continue; } if invalid_cont_byte(ordch3) { - run_err!(pos, pos + 2, "invalid continuation byte"); + // [3.14-spec] as above: the allowance covers a whole encoded + // surrogate, so a bad third byte falls back to the strict + // span rather than reporting two bytes as one bad sequence. + let end = if surrogate_bytes(ordch1, ordch2) { + pos + 1 + } else { + pos + 2 + }; + run_err!(pos, end, "invalid continuation byte"); continue; } // 1110xxxx 10yyyyyy 10zzzzzz @@ -24081,19 +24154,17 @@ pub(crate) fn bytes_method_decode(args: &[PyObjectRef]) -> Result { - crate::baseobjspace::str_utf8_w(e)?.to_string() - } - _ => "utf-8".to_string(), + Some(e) if unsafe { pyre_object::is_str(e) } => crate::baseobjspace::str_utf8_w(e)?, + _ => "utf-8", }; let errors = match w_errors { - Some(e) if unsafe { pyre_object::is_str(e) } => { - crate::baseobjspace::str_utf8_w(e)?.to_string() - } - _ => "strict".to_string(), + Some(e) if unsafe { pyre_object::is_str(e) } => crate::baseobjspace::str_utf8_w(e)?, + _ => "strict", }; - let s = decode_bytes_to_wtf8(data, &encoding, errors.as_str())?; + let s = decode_bytes_to_wtf8(data, encoding, errors)?; Ok(pyre_object::w_str_from_wtf8_managed(s)) } @@ -24105,10 +24176,21 @@ pub(crate) fn decode_bytes_to_wtf8( errors: &str, ) -> Result { let err_mode = errors; - let enc_lower = encoding.to_ascii_lowercase().replace('_', "-"); + // The codec name is matched with case folded and `_` read as `-`. Every + // caller inside the runtime, and `bytes.decode`'s own default, already + // spells it that way, so the rewrite is the exception and only it pays + // for a buffer. + let enc_lower: std::borrow::Cow<'_, str> = if encoding + .bytes() + .any(|b| b.is_ascii_uppercase() || b == b'_') + { + std::borrow::Cow::Owned(encoding.to_ascii_lowercase().replace('_', "-")) + } else { + std::borrow::Cow::Borrowed(encoding) + }; if crate::importing::dev_mode_flag() && matches!( - enc_lower.as_str(), + enc_lower.as_ref(), "utf-8" | "utf8" | "u8" @@ -24130,7 +24212,7 @@ pub(crate) fn decode_bytes_to_wtf8( { crate::module::_codecs::validate_error_handler(errors)?; } - let s = match enc_lower.as_str() { + let s = match enc_lower.as_ref() { "utf-8" | "utf8" | "u8" => decode_utf8_with_errors(data, err_mode)?, "ascii" | "us-ascii" | "646" => { let mut out = Wtf8Buf::new(); diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs index 078e211b4ff..90ce1e3027b 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rs @@ -1872,10 +1872,17 @@ pub(crate) fn fbw_abort_nested_unjournaled_residual( // are already represented by their own frame images. This is the // per-frame boundary `convert_and_run_from_pyjitpl` preserves when // it copies every `MIFrame` independently (`blackhole.py:1799-1821`). - let blackhole_required = session - .framestack - .last() - .is_some_and(|frame| fbw_executed_effect_count() != frame.entry_executed_effects); + // An in-flight FOR_ITER item is in no frame image, so the + // per-frame test above cannot see it: a body effect committed in + // an enclosing frame leaves the innermost frame's delta at zero + // while `fbw_foriter_inflight_take` refuses that very item, and + // the legacy path then drops it. Arm the conversion on the same + // signal the refusal reads, so the item is carried forward + // instead of lost. + let blackhole_required = + session.framestack.last().is_some_and(|frame| { + fbw_executed_effect_count() != frame.entry_executed_effects + }) || (fbw_foriter_inflight_active() && fbw_foriter_any_body_effect_signal()); (outer_resume, stack_overrides, blackhole_required) }; FBW_ABORT_OUTER_RESUME.with(|c| c.set(outer_resume)); diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs index c21cea70444..4b9b42008ff 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/mod.rs @@ -5620,6 +5620,22 @@ fn collect_outer_active_boxes( let vstack_mirror: Option<&[OpRef]> = vstack.filter(|_| guard_present); for &idx in &banks.ref_ { let color = idx as usize; + // The semantic slot this color owns AT THE GUARD PC. Where it equals + // the slot being sourced, `regs_r[color]` is upstream's + // `registers_r[index]` (`get_list_of_active_boxes`, `pyjitpl.py`) for + // that slot rather than a merge color the regalloc may have reused; + // both the mirror-hole report and the operand-stack value choice below + // turn on that identity. + let guard_owned_slot = guard_present + .then(|| { + crate::state::semantic_ref_slot_for_reg_color( + nlocals, + guard_stack_only, + &guard_pcdep_entries, + color, + ) + }) + .flatten(); if let Some(mirror) = vstack_mirror { // Resume operand-stack slot for this live Ref color; the mirror // box for that slot is the kept value (bottom-anchored: resume @@ -5665,13 +5681,32 @@ fn collect_outer_active_boxes( // (`synth/list_append_write_barrier_gc`), and declining them // regressed it (loops 12→11, bridges 5→3, aborts 1→4) with // no demonstration that the value encoded today is wrong. - // Whether that NULL is the slot's real value (a `PUSH_NULL` - // sentinel) cannot be settled here: the mirror has several - // writers and only `reseed_vstack_from_shadow` gates a NULL - // behind the live-NULL marker, while the per-op reconcile - // and the vable write-through store one directly. Settling - // that is what closes the remaining case. - if m == OpRef::NONE && !kept_recovered.contains_key(&idx) { + // + // A hole the guard pc's OWN color→slot map resolves is not + // reported at all. `get_list_of_active_boxes` + // (`pyjitpl.py`) snapshots `self.registers_r[index]` and + // never fails, because the register file IS the source; the + // stale-read hazard this report exists for is the case + // where that identity does not hold — the resume merge + // color is unwritten at the guard point, or the regalloc + // reused it for an unrelated SSA temp. When + // `pcdep_color_slots` AT THE GUARD PC maps this color to + // this very semantic slot, the two coincide: the walk + // register read below means exactly `registers_r[index]`, + // and the stack-slot arm consumes it through the same + // `guard_pc_proves_slot` test. That also settles what the + // mirror cannot: a slot whose live value is a NULL — an + // already-cleared operand, a `self_or_null` — is a hole in + // the mirror by construction, because + // `reseed_vstack_from_shadow` reads a dense array where an + // absent slot and a written NULL are the same word and so + // refuses to source either. The register file draws that + // distinction, and upstream keeps the NULL: PyPy's MIFrame + // registers preserve CONST_NULL in snapshots. + if m == OpRef::NONE + && !kept_recovered.contains_key(&idx) + && guard_owned_slot != Some(sem) + { if let Some(first) = unrecovered_kept.as_deref_mut() { first.get_or_insert(idx); } @@ -5803,13 +5838,7 @@ fn collect_outer_active_boxes( let shadow_is_real = vbox.is_some_and(|b| !opref_is_null_const_ptr(b)); let walk_real = walk_box.filter(|&v| v != OpRef::NONE && !opref_is_null_const_ptr(v)); - let guard_pc_proves_slot = guard_present - && crate::state::semantic_ref_slot_for_reg_color( - nlocals, - guard_stack_only, - &guard_pcdep_entries, - color, - ) == Some(s_idx); + let guard_pc_proves_slot = guard_owned_slot == Some(s_idx); if guard_pc_proves_slot { walk_real.or(vbox).unwrap_or_else(fallback) } else if shadow_is_real { diff --git a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs index 377026eb988..6c377d71001 100644 --- a/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs +++ b/pyre/pyre-jit-trace/src/jitcode_dispatch/residual_call.rs @@ -3147,6 +3147,30 @@ pub(crate) fn try_execute_residual_call_via_executor( as usize && args[2] != 0 && unsafe { pyre_object::is_exact_list(args[2] as usize as pyre_object::PyObjectRef) }; + // `operator.index(x)` reaches `space_index`, whose first test is + // `is_int_or_long`: an int (or long) is returned as-is, before any + // `__index__` lookup, so no user code runs and nothing is written. Every + // other argument type dispatches through `__index__` and stays opaque. + // Pin the callable the way the `ord` / `isinstance` / `tuple` arms above + // do — it shares the `CallFn` helper with them and is separated by the + // identity each one pins, so at most one can hold, and a rebound + // `operator.index` is a different object that keeps the conservative + // treatment. Observe the argument too: the callable alone does not bound + // what `space_index` will do. `randrange`'s `_index(start)` is the + // shape that reaches here: it is the ONLY residual the aborted walk of a + // `[randrange(k) for _ in ...]` body executes, and classifying it as a live + // heap write refused the in-flight FOR_ITER delivery and dropped the + // element. + let observed_exact_int_index = helper == majit_ir::PyreHelperKind::CallFn + && args.len() == 3 + && args[1] == 0 + && pyre_interpreter::module::operator::is_operator_index_function( + args[0] as pyre_object::PyObjectRef, + ) + && { + let operand = args[2] as pyre_object::PyObjectRef; + !operand.is_null() && unsafe { pyre_object::pyobject::is_int_or_long(operand) } + }; // `BUILD_TUPLE` / `BUILD_LIST` create a fresh container from their fresh // backing array (`pyopcode.py:1012-1020`). Re-executing either allocation // cannot mutate an object visible before the call. Upstream records list @@ -3171,7 +3195,8 @@ pub(crate) fn try_execute_residual_call_via_executor( || observed_exact_str_ord || observed_replay_safe_isinstance || replay_safe_fresh_allocation - || replay_safe_tuple_from_list; + || replay_safe_tuple_from_list + || observed_exact_int_index; let writes_live_heap = call_descr.result_type() == majit_ir::Type::Void || (helper == majit_ir::PyreHelperKind::CallFn && !replay_safe_tuple_from_list) || matches!( diff --git a/pyre/pyre-jit-trace/src/pyre_cpu.rs b/pyre/pyre-jit-trace/src/pyre_cpu.rs index 034069481d9..80b0c537f99 100644 --- a/pyre/pyre-jit-trace/src/pyre_cpu.rs +++ b/pyre/pyre-jit-trace/src/pyre_cpu.rs @@ -23,9 +23,10 @@ use std::sync::{Arc, OnceLock}; use majit_ir::operand::Operand; use majit_ir::{ArrayDescr, Descr, FieldDescr, GcRef, Type}; use majit_metainterp::cpu::{Cpu, DefaultCpu}; +use pyre_object::rutf8::Utf8IndexStorage; use pyre_object::unicodeobject::{ - UNICODE_BYTE_LEN_OFFSET, UNICODE_LEN_OFFSET, UNICODE_VALUE_OFFSET, W_UNICODE_GC_TYPE_ID, - W_UNICODE_OBJECT_SIZE, + UNICODE_BYTE_LEN_OFFSET, UNICODE_INDEX_STORAGE_OFFSET, UNICODE_LEN_OFFSET, + UNICODE_VALUE_OFFSET, W_UNICODE_GC_TYPE_ID, W_UNICODE_OBJECT_SIZE, }; use rustpython_wtf8::Wtf8Buf; @@ -168,6 +169,14 @@ impl Default for PyreCpu { } } +/// The offset a blackhole item read takes, refused unless it is one: `as +/// usize` wraps a negative operand and truncates one wider than the target's +/// `usize`, and either turns the bounds test that follows into a read of the +/// wrong element. +fn item_index(index: i64) -> Option { + usize::try_from(index).ok() +} + impl Cpu for PyreCpu { fn cls_of_box(&self, box_: &Operand) -> i64 { self.0.cls_of_box(box_) @@ -230,7 +239,7 @@ impl Cpu for PyreCpu { } let s = unsafe { &*value_ptr }; let bytes = s.as_bytes(); - let i = index as usize; + let i = item_index(index)?; if i >= bytes.len() { return None; } @@ -238,10 +247,16 @@ impl Cpu for PyreCpu { } fn bh_unicodegetitem(&self, unicode: GcRef, index: i64) -> Option { - // RPython UNICODE is codepoint-indexed; UNICODEGETITEM returns - // the codepoint value. Pyre's `W_UnicodeObject` stores WTF-8, so - // walk codepoints via `code_points().nth(index)`; `to_u32` - // yields the ordinal (including lone surrogates D800-DFFF). + // RPython UNICODE is codepoint-indexed; UNICODEGETITEM returns the + // codepoint value, `to_u32` (including lone surrogates D800-DFFF). + // Pyre's `W_UnicodeObject` stores WTF-8, where a codepoint index is a + // byte offset only for an ASCII payload, so this resolves it the two + // ways `w_str_codepoint_at` does. + // + // Neither arm builds the index table: a blackhole runs inside a deopt, + // so it reads a table that is already there and otherwise walks. The + // walk is the cost the table exists to remove, and it is the one + // upstream's array read never pays. if unicode.is_null() { return None; } @@ -251,8 +266,23 @@ impl Cpu for PyreCpu { return None; } let s = unsafe { &*value_ptr }; - let i = index as usize; - s.code_points().nth(i).map(|c| c.to_u32() as i64) + let i = item_index(index)?; + let len = unsafe { *((unicode.0 + UNICODE_LEN_OFFSET) as *const usize) }; + if i >= len { + return None; + } + let byte_len = unsafe { *((unicode.0 + UNICODE_BYTE_LEN_OFFSET) as *const usize) }; + // `w_str_is_ascii` — one byte per codepoint, so the index is the offset. + if len == byte_len { + return Some(s.as_bytes()[i] as i64); + } + let storage = unsafe { + *((unicode.0 + UNICODE_INDEX_STORAGE_OFFSET) as *const *const Utf8IndexStorage) + }; + if storage.is_null() { + return s.code_points().nth(i).map(|c| c.to_u32() as i64); + } + Some(pyre_object::rutf8::codepoint_at_index(s, unsafe { &*storage }, i).to_u32() as i64) } } @@ -264,3 +294,91 @@ pub fn shared() -> Arc { CELL.get_or_init(|| Arc::new(PyreCpu::new()) as Arc) .clone() } + +#[cfg(test)] +mod tests { + use super::*; + use rustpython_wtf8::CodePoint; + + /// Every arm of `bh_unicodegetitem` must answer what a codepoint walk + /// answers. The arms are chosen by two facts about the operand — whether + /// it is ASCII, and whether its index table has been built — so each case + /// below puts a string in one of those states and compares the whole + /// index range against the walk. + fn agrees_with_walk(obj: pyre_object::PyObjectRef) { + let cpu = PyreCpu::new(); + let gc = GcRef(obj as usize); + let walk: Vec = unsafe { pyre_object::w_str_get_wtf8(obj) } + .code_points() + .map(|c| c.to_u32() as i64) + .collect(); + for (i, expected) in walk.iter().enumerate() { + assert_eq!( + cpu.bh_unicodegetitem(gc, i as i64), + Some(*expected), + "index {i}" + ); + } + assert_eq!( + cpu.bh_unicodegetitem(gc, walk.len() as i64), + None, + "one past the end" + ); + assert_eq!(cpu.bh_unicodegetitem(gc, -1), None, "negative index"); + } + + /// The offset conversion refuses what it cannot represent, rather than + /// wrapping into range. A wrapped index passes the bounds test that + /// follows it and reads some other element, which is a wrong answer where + /// the refusal is a `None` the caller already handles. + #[test] + fn an_index_that_does_not_fit_the_targets_usize_is_refused() { + assert_eq!(item_index(0), Some(0)); + assert_eq!(item_index(7), Some(7)); + assert_eq!(item_index(-1), None); + assert_eq!(item_index(i64::MIN), None); + // `as usize` truncates this to 0 where `usize` is 32 bits, which is + // the wasm32 target. + const PAST_U32: i64 = 1 << 32; + #[cfg(target_pointer_width = "32")] + assert_eq!(item_index(PAST_U32), None); + #[cfg(target_pointer_width = "64")] + assert_eq!(item_index(PAST_U32), Some(PAST_U32 as usize)); + } + + #[test] + fn bh_unicodegetitem_ascii_reads_the_byte() { + let obj = pyre_object::w_str_new("hello"); + assert!(unsafe { pyre_object::unicodeobject::w_str_is_ascii(obj) }); + agrees_with_walk(obj); + } + + #[test] + fn bh_unicodegetitem_wide_without_a_table_walks() { + let obj = pyre_object::w_str_new("héllo wörld ☃"); + assert!(!unsafe { pyre_object::unicodeobject::w_str_is_ascii(obj) }); + agrees_with_walk(obj); + } + + #[test] + fn bh_unicodegetitem_wide_with_a_table_reads_the_table() { + // Long enough to span more than one 64-codepoint group, so the read + // exercises `baseindex` selection rather than only the first entry. + let obj = pyre_object::w_str_new(&"ábç".repeat(60)); + // Force the lazy build the blackhole arm refuses to do itself. + assert!(unsafe { pyre_object::w_str_codepoint_at(obj, 100) }.is_some()); + agrees_with_walk(obj); + } + + #[test] + fn bh_unicodegetitem_yields_a_lone_surrogate() { + let mut buf = rustpython_wtf8::Wtf8Buf::new(); + buf.push(CodePoint::from_char('a')); + buf.push(CodePoint::from_u32(0xD800).unwrap()); + buf.push(CodePoint::from_char('b')); + let obj = pyre_object::unicodeobject::w_str_from_wtf8(buf); + let cpu = PyreCpu::new(); + assert_eq!(cpu.bh_unicodegetitem(GcRef(obj as usize), 1), Some(0xD800)); + agrees_with_walk(obj); + } +} diff --git a/pyre/pyre-jit/src/eval.rs b/pyre/pyre-jit/src/eval.rs index d6ef0aa1985..40328c8b4e9 100644 --- a/pyre/pyre-jit/src/eval.rs +++ b/pyre/pyre-jit/src/eval.rs @@ -7627,15 +7627,6 @@ fn for_iter_gate_diag_enabled() -> bool { *ENABLED.get_or_init(|| std::env::var_os("PYRE_FOR_ITER_GATE_DIAG").is_some()) } -/// Admit a `LIST_APPEND` FOR_ITER body that also carries a CALL (gh#73/gh#34). -/// Off by default while the mid-body abort's forward resume is being measured: -/// a call commits body effects, and the abort's recovery decides whether the -/// consumed item survives. -fn for_iter_call_body_admitted() -> bool { - static ENABLED: std::sync::OnceLock = std::sync::OnceLock::new(); - *ENABLED.get_or_init(|| std::env::var_os("PYRE_FORITER_CALL_BODY").is_some()) -} - fn for_iter_body_is_jit_safe_at(code: &pyre_interpreter::CodeObject, pc: usize) -> bool { use pyre_interpreter::Instruction as I; let instructions = &code.instructions; @@ -7645,99 +7636,62 @@ fn for_iter_body_is_jit_safe_at(code: &pyre_interpreter::CodeObject, pc: usize) }; let exit = pyre_interpreter::jump_target_forward(instructions, pc + 1, delta.get(op_arg).as_usize()); - // A `LIST_APPEND` (inlined-comprehension accumulator) body is - // admitted only when the body performs no CALL. A compiled loop's - // live-at-exit values stay reachable from the exit state until the - // next JIT activity overwrites it, so the loop variable keeps its - // final binding past the `STORE_FAST` that restores the isolated - // comprehension slot to unbound. A call-free body binds values the - // enclosing frame holds anyway; a per-element call binds a freshly - // constructed object, and `extra_tests/parity_tests/ - // weakref_gc_lifeline.py` then sees the last element survive the - // collection that should have run its weakref callback. Decline - // call-bearing bodies to interpretation until the exit state stops - // rooting what it no longer resumes. + // The `LIST_APPEND` opcode (the inlined-comprehension accumulator) is + // admitted wherever it appears, like `SET_ADD` and `MAP_ADD` beside it. + // That is a statement about the one opcode, not about the body around it: + // the scan below still walks every body instruction and refuses the whole + // FOR_ITER on the first one outside the permitted set. // - // The scan narrows how often the in-flight delivery gap is reached; - // it is not a boundary the gap stays behind. Before #1174 this body, - // which is call-free and which the scan admits, lost a whole OUTER - // iteration — 59 of 60 appends, twice in 400 runs, on dynasm and on - // cranelift, 60 with the JIT off: + // It used to be admitted only for a call-free body, over two hazards. + // Both were re-measured on 2026-08-20 and neither reproduces: // - // for index in items: - // out.append([index for _ in range(1)]) + // * GC liveness — a per-element call binds a freshly constructed object, + // and `extra_tests/parity_tests/weakref_gc_lifeline.py` was said to see + // the last element survive the collection that should have run its + // weakref callback. That fixture is OK 10/10 per backend with the scan + // gone, and the whole parity suite passes under the runner's + // `--gc-poison` (`MAJIT_GC_NURSERY_POISON=1`) as well. + // * The in-flight delivery gap (single-executor tracing, gh#73/#34) — + // the scan only narrowed how often it was reached, never bounded it. + // Its cause here was a misclassification, not an unrecoverable effect: + // `writes_live_heap` holds for EVERY `CallFn`, so `_operator.index` — + // which `space.index` answers by returning an int argument unchanged, + // ahead of any `__index__` lookup — was booked as a body effect, which + // refused the consumed item's delivery AND broke the gh#467 + // CALL-forward odometer. `randrange`'s `_index(start)` is how + // `for_iter_call_bearing_comprehension.py` reached it. That call is now + // a replay-safe observed class (`residual_call.rs`), the walk's + // `effects` at the abort reads 0 rather than 1, and the abort commits a + // forward resume instead of falling back to the legacy replay. // - // The route was TWO in-flight entries and a take that selected on - // recency, not the R1 body-effect refusal and not a mislabelled - // coordinate: `inflight_foriter_body_pc` resolves the outer loop's - // `for_iter_next` (JitCode pc 153) to Python pc 5 and the inner one - // (631) to 34, both correct. The outer `list` loop captured through - // the residual leg and the inner `range` body through the specialised - // leg, so `fbw_foriter_inflight_take` popped the inner entry — whose - // body pc a frame parked at the outer header can never accept — and - // cleared the outer entry it could have delivered. Selecting the - // entry that matches the parked frame fixes that, and is what the - // take now does. + // The stake, same binary, `[uf(x) for x in it]` against the semantically + // identical `for x in it: l.append(uf(x))`: the statement loop runs 45x + // faster than `PYRE_JIT=0` either way, while the comprehension went from + // 0.69x — the JIT costing more than it saved, because a declined caller + // frame makes every callee pay full entry cost while nothing compiles — + // to 9.60x. // - // The shape no longer reaches it here: #1174 made that walk raise - // `callee_inline_abort` with `blackhole_required` set where it raised - // `callee_inline_unsupported`, so it stops aborting — 0 in-flight - // takes and 0 divergences across the whole probe family — which also - // means this base cannot re-witness the fix end to end; the selection - // is pinned by `take_selects_the_entry_the_parked_frame_can_accept_…` - // instead. `SET_ADD` and `MAP_ADD` spell the same shape and carry no - // scan at all. Closing the rest is the in-flight delivery gap - // (single-executor tracing, gh#73/#34). + // Upstream has no counterpart to any of this: `interp_jit.py`'s + // `jit_merge_point` is unconditional, and `pyopcode.py` spells + // `LIST_APPEND` as `space.call_method(v, 'append', w)`, an operation this + // body scan already admits. The scan existed only to protect pyre's + // tracer-level `LIST_APPEND` fold. // // A value-producing but call-free body — arithmetic, subscript, or // an Object-strategy element (`[(i, i) …]`, `[None …]`, `["s" …]`, - // `[{i: i} …]`, `[f"{i}" …]`) — is admitted. The only residual an + // `[{i: i} …]`, `[f"{i}" …]`) — was already admitted. The only residual an // Object-strategy append leaves in the folded body is the idempotent - // `list_write_barrier`, now exempt from the FBW body-effect - // accounting (it is not a body effect, mirroring RPython's - // `COND_CALL_GC_WB`, which pyjitpl never executes and the optimizer - // never treats as a side effect). That exemption keeps the append - // itself out of the body-effect accounting; it does not make the - // enclosing loop exact-resume safe, per the shape above. + // `list_write_barrier`, exempt from the FBW body-effect accounting (it is + // not a body effect, mirroring RPython's `COND_CALL_GC_WB`, which pyjitpl + // never executes and the optimizer never treats as a side effect). // - // A non-empty nested `BUILD_LIST` element (`[[i] …]`) is admitted - // too: the fold virtualizes the inner list, whose separately - // allocated backing block (`NewArray` / `NewArrayClear`) carries no - // jitcode-liveness slot, and with the trace-time single-executor - // forks retired the append body no longer runs under a - // speculative-replay sub-walk, so the block is bound at every - // guard-exit deopt and the shape compiles bit-exact on all backends - // (`bench/synth/nested_list_comprehension_hot.py`). - let body_has_call = { - let mut scan_state = pyre_interpreter::OpArgState::default(); - let mut scan_pc = pc + 1; - let mut has_call = false; - while scan_pc < exit && scan_pc < instructions.len() { - let (scan_instr, scan_arg) = scan_state.get(instructions[scan_pc]); - if let I::ForIter { delta } = scan_instr { - // A nested loop owns its body. It is validated when - // the outer instruction walk reaches that FOR_ITER; - // calls inside it must not taint a LIST_APPEND owned by - // this lexical loop (and vice versa). - scan_pc = pyre_interpreter::jump_target_forward( - instructions, - scan_pc + 1, - delta.get(scan_arg).as_usize(), - ); - scan_state = pyre_interpreter::OpArgState::default(); - continue; - } - if matches!( - scan_instr, - I::Call { .. } | I::CallKw { .. } | I::CallFunctionEx | I::CallIntrinsic1 { .. } - ) { - has_call = true; - break; - } - scan_pc += 1; - } - has_call - }; + // A non-empty nested `BUILD_LIST` element (`[[i] …]`) is admitted too: the + // fold virtualizes the inner list, whose separately allocated backing + // block (`NewArray` / `NewArrayClear`) carries no jitcode-liveness slot, + // and with the trace-time single-executor forks retired the append body no + // longer runs under a speculative-replay sub-walk, so the block is bound + // at every guard-exit deopt and the shape compiles bit-exact on all + // backends (`bench/synth/nested_list_comprehension_hot.py`). let mut body_state = pyre_interpreter::OpArgState::default(); let mut body_pc = pc + 1; while body_pc < exit && body_pc < instructions.len() { @@ -7758,10 +7712,9 @@ fn for_iter_body_is_jit_safe_at(code: &pyre_interpreter::CodeObject, pc: usize) // CALL_INTRINSIC_1 names several unrelated operations. UnaryPositive // and ListToTuple are the two variants codewriter.rs actually lowers: // the first follows the same implicit-dunder exact-resume path as - // UnaryNegative, while the second returns a fresh tuple. Keep counting - // the opcode in body_has_call so a unary-positive user frame still - // taints LIST_APPEND, but do not admit the def-time/import/error-path - // variants whose lowering deliberately aborts permanently. + // UnaryNegative, while the second returns a fresh tuple. Admit those + // two, but not the def-time/import/error-path variants whose lowering + // deliberately aborts permanently. let supported_call_intrinsic_1 = matches!( body_instr, I::CallIntrinsic1 { func } @@ -7822,8 +7775,7 @@ fn for_iter_body_is_jit_safe_at(code: &pyre_interpreter::CodeObject, pc: usize) // aborts all 5 on the force, compiling nothing. | I::LoadBuildClass ) - || ((!body_has_call || for_iter_call_body_admitted()) - && matches!(body_instr, I::ListAppend { .. })); + || matches!(body_instr, I::ListAppend { .. }); if !permitted { if for_iter_gate_diag_enabled() { eprintln!( @@ -14355,11 +14307,12 @@ mod tests { } #[test] - fn for_iter_call_bearing_list_append_comprehension_is_unsafe_for_entry_trace() { - // A per-element CALL binds a freshly constructed object to the - // comprehension's isolated slot, and the compiled loop's exit state - // keeps that last binding reachable, so a call-bearing LIST_APPEND - // body stays interpreter-only. + fn for_iter_call_bearing_list_append_comprehension_body_is_jit_safe() { + // A per-element CALL no longer withholds LIST_APPEND. The element the + // call produces is reachable from the compiled loop's exit state like + // any other body-built value, and a mid-body abort recovers through the + // same Layer 2 defence that already admits `Call` on its own; the two + // opcodes together carry no hazard neither carries alone. use pyre_interpreter::compile_exec; for source in [ "def f(n):\n return [str(i) for i in range(n)]\n", @@ -14367,7 +14320,7 @@ mod tests { ] { let module = compile_exec(source).expect("test code should compile"); let code = function_code_from_module(&module, "f"); - assert!(!function_entry_trace_is_jit_safe(&code)); + assert!(function_entry_trace_is_jit_safe(&code)); assert_eq!(unsupported_jit_shape_of(&code), UnsupportedJitShape::None); } } @@ -14493,10 +14446,13 @@ mod tests { } #[test] - fn unsafe_later_comprehension_does_not_blacklist_an_earlier_loop() { + fn unsafe_later_loop_does_not_blacklist_an_earlier_loop() { + // The trailing loop holds a `with`, whose LOAD_SPECIAL is deliberately + // withheld from the body allow-list, so it is the whole-code scan's + // sole refusal. use pyre_interpreter::{Instruction as I, compile_exec}; let module = compile_exec( - "def run(n):\n escaped = []\n i = 0\n while i < n:\n for value in range(3):\n pass\n escaped.append(range(i, i + 3))\n i += 1\n return [len(item) for item in escaped]\n", + "def run(n, cm):\n escaped = []\n i = 0\n while i < n:\n for value in range(3):\n pass\n escaped.append(range(i, i + 3))\n i += 1\n out = []\n for item in escaped:\n with cm:\n out.append(len(item))\n return out\n", ) .expect("test code should compile"); let code = function_code_from_module(&module, "run"); @@ -14535,9 +14491,13 @@ mod tests { #[test] fn loop_region_includes_out_of_line_handler_rejoining_mid_body() { + // The handler holds a loop of its own whose body opens a `with`, and + // LOAD_SPECIAL is deliberately withheld from the body allow-list. The + // region scan reaches that inner FOR_ITER only if it follows the + // out-of-line handler past the back edge. use pyre_interpreter::{Instruction as I, compile_exec}; let module = compile_exec( - "def run(items, k):\n out = []\n seen = []\n for x in items:\n try:\n items[x + 100]\n except IndexError:\n out.extend([str(y) for y in range(k)])\n seen.append(len(range(x)))\n out.append(x)\n return out, seen\n", + "def run(items, k, cm):\n out = []\n seen = []\n for x in items:\n try:\n items[x + 100]\n except IndexError:\n for y in range(k):\n with cm:\n out.append(y)\n seen.append(len(range(x)))\n out.append(x)\n return out, seen\n", ) .expect("test code should compile"); let code = function_code_from_module(&module, "run"); diff --git a/pyre/pyre-object/src/rutf8.rs b/pyre/pyre-object/src/rutf8.rs index 3b230dbbdc8..a9b3c7690ea 100644 --- a/pyre/pyre-object/src/rutf8.rs +++ b/pyre/pyre-object/src/rutf8.rs @@ -12,16 +12,14 @@ //! `rutf8` function whose whole content is "encode, decode or validate, //! scanning forward" has a checked counterpart there and is *not* re-ported: //! a second, unchecked implementation beside a checked one would be two -//! sources of truth for one invariant. `unichr_as_utf8*` (:40) is -//! `CodePoint::encode_wtf8` / `Wtf8Buf::push`; `check_utf8` (:351), -//! `_check_utf8` (:373) and `get_utf8_length` (:364) are `Wtf8::from_bytes`; -//! `check_ascii` (:242) and `first_non_ascii_char` (:249) are -//! `Wtf8::is_ascii` and a byte scan; `has_surrogates` (:439) and -//! `surrogate_in_utf8` (:489) are `Wtf8::as_str().is_err()`; `islinebreak` -//! (:255), `isspace` (:272) and `utf8_in_chars` (:307) are predicates over a -//! decoded `CodePoint`; `char_escape_helper` (:647), -//! `make_utf8_escape_function` (:660) and `decode_latin_1` (:867) belong to -//! `repr` and `_codecs`. +//! sources of truth for one invariant. `unichr_as_utf8*` is +//! `CodePoint::encode_wtf8` / `Wtf8Buf::push`; `get_utf8_length` is +//! `codepoints_in_utf8`, which upstream also says to prefer; `check_ascii` +//! and `first_non_ascii_char` are `Wtf8::is_ascii` and a byte scan; +//! `has_surrogates` and `surrogate_in_utf8` are `Wtf8::as_str().is_err()`; +//! `islinebreak`, `isspace` and `utf8_in_chars` are predicates over a +//! decoded `CodePoint`; `char_escape_helper`, `make_utf8_escape_function` +//! and `decode_latin_1` belong to `repr` and `_codecs`. //! //! * **This module owns random access**, which the crate deliberately has no //! counterpart for — its iterators are sequential, so resolving the n-th code @@ -37,11 +35,17 @@ //! and holding it lets `codepoint_at_pos` decode through the crate instead of //! carrying a second copy of its decoder. //! -//! Two members of the family are deliberately absent. `null_storage` (:513) -//! has no counterpart: an absent table is a null pointer in the -//! `W_UnicodeObject` slot. `_pos_at_index` (:568), the "Slow!" linear -//! fallback, has no pyre caller — upstream reaches it from `unicodehelper` and -//! `formatting`, neither of which pyre routes this way. +//! `check_utf8` is the one validator that *is* re-ported, because +//! `Wtf8::from_bytes` is not equivalent to it: it accepts sequences upstream +//! rejects (see that function), and it has no way to spell +//! `allow_surrogates=False`. Its three predicates come with it, so the +//! decoders that already share them read them from here. +//! +//! Two members of the family are deliberately absent. `null_storage` has no +//! counterpart: an absent table is a null pointer in the `W_UnicodeObject` +//! slot. `_pos_at_index`, the "Slow!" linear fallback, has no pyre caller — +//! upstream reaches it from `unicodehelper` and `formatting`, neither of +//! which pyre routes this way. //! //! Names, entry layout, group sizes and the build loop follow //! `rpython/rlib/rutf8.py`. The `_is_64bit` branch of @@ -132,6 +136,156 @@ pub fn codepoint_before_pos(code: &Wtf8, pos: usize) -> CodePoint { codepoint_at_pos(code, prev_codepoint_pos(code, pos)) } +/// `CheckError` (`rutf8.py`) — the byte position `check_utf8` stopped at. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct CheckError { + pub pos: usize, +} + +/// `_invalid_cont_byte` (`rutf8.py`) — a byte outside `0x80..=0xBF`, which +/// upstream spells as a signed-char comparison against `-0x40`. +#[inline] +pub fn invalid_cont_byte(b: u8) -> bool { + (b as i8) >= -0x40 +} + +/// `_surrogate_bytes` (`rutf8.py`) — a three-byte lead pair that encodes a +/// surrogate, which is the one pair `invalid_byte_2_of_3` admits only when +/// the caller allows surrogates. +#[inline] +pub fn surrogate_bytes(ch1: u8, ch2: u8) -> bool { + ch1 == 0xED && ch2 > 0x9F +} + +/// `_invalid_byte_2_of_3` (`rutf8.py`) — the second byte of a three-byte +/// sequence, rejecting surrogate encodings unless the caller selected the +/// `surrogatepass` path. +#[inline] +pub fn invalid_byte_2_of_3(ch1: u8, ch2: u8, allow_surrogates: bool) -> bool { + invalid_cont_byte(ch2) + || (ch1 == 0xE0 && ch2 < 0xA0) + || (ch1 == 0xED && ch2 > 0x9F && !allow_surrogates) +} + +/// `_invalid_byte_2_of_4` (`rutf8.py`) — the second byte of a four-byte +/// sequence, which also carries the range bound the leading byte leaves open. +#[inline] +pub fn invalid_byte_2_of_4(ch1: u8, ch2: u8) -> bool { + invalid_cont_byte(ch2) || (ch1 == 0xF0 && ch2 < 0x90) || (ch1 == 0xF4 && ch2 > 0x8F) +} + +/// `check_utf8` (`rutf8.py`) — the code point count of `s`, or the byte +/// position where it stops being well formed. +/// +/// `_check_utf8`'s ones'-complement return and the `CheckError` its caller +/// raises from it are one `Result` here. Upstream's `start`/`stop` window is +/// left out: every pyre caller validates a whole buffer. +pub fn check_utf8(s: &[u8], allow_surrogates: bool) -> Result { + let end = s.len(); + let mut pos = 0; + let mut continuation_bytes = 0; + while pos < end { + let ordch1 = s[pos]; + pos += 1; + // fast path for ASCII + if ordch1 <= 0x7F { + continue; + } + if ordch1 <= 0xC1 { + return Err(CheckError { pos: pos - 1 }); + } + if ordch1 <= 0xDF { + if pos >= end { + return Err(CheckError { pos: pos - 1 }); + } + let ordch2 = s[pos]; + pos += 1; + if invalid_cont_byte(ordch2) { + return Err(CheckError { pos: pos - 2 }); + } + continuation_bytes += 1; + continue; + } + if ordch1 <= 0xEF { + if pos + 2 > end { + return Err(CheckError { pos: pos - 1 }); + } + let ordch2 = s[pos]; + let ordch3 = s[pos + 1]; + pos += 2; + if invalid_byte_2_of_3(ordch1, ordch2, allow_surrogates) || invalid_cont_byte(ordch3) { + return Err(CheckError { pos: pos - 3 }); + } + continuation_bytes += 2; + continue; + } + if ordch1 <= 0xF4 { + if pos + 3 > end { + return Err(CheckError { pos: pos - 1 }); + } + let ordch2 = s[pos]; + let ordch3 = s[pos + 1]; + let ordch4 = s[pos + 2]; + pos += 3; + if invalid_byte_2_of_4(ordch1, ordch2) + || invalid_cont_byte(ordch3) + || invalid_cont_byte(ordch4) + { + return Err(CheckError { pos: pos - 4 }); + } + continuation_bytes += 3; + continue; + } + return Err(CheckError { pos: pos - 1 }); + } + Ok(pos - continuation_bytes) +} + +/// The `Wtf8` view of a buffer `check_utf8` accepts — `str_decode_utf8`'s +/// "fast version first" arm, and the check every untrusted-bytes boundary +/// performs before the buffer becomes a string. +/// +/// `Wtf8::from_bytes` does not stand in for this. Its surrogate arm matches +/// `[0xed, 0xa0.., b3, ..]`, leaving the second byte unbounded above and the +/// third unconstrained, so it accepts `ED C0 80` — three bytes encoding no code +/// point. A buffer admitted that way reaches `W_UnicodeObject` with a code +/// point count that disagrees with what `next_codepoint_pos` steps over, and +/// the two disagree first inside `create_utf8_index_storage`, which reads past +/// the buffer. +/// +/// Its *loop* is kept, because `str::from_utf8` scans a word at a time and +/// `check_utf8` a byte at a time — 0.08 against 0.35 ns per byte, over every +/// name in every code object a marshal load carries. Only the two bounds +/// `_invalid_byte_2_of_3` and `_invalid_byte_3_of_3` impose are restored; +/// `check_utf8_and_wtf8_from_bytes_agree` holds the two to one answer. +pub fn wtf8_from_bytes(s: &[u8], allow_surrogates: bool) -> Result<&Wtf8, CheckError> { + if !allow_surrogates { + // `check_utf8` with the flag off admits exactly well-formed UTF-8, and + // `Utf8Error::valid_up_to` is the same offset it would report: both + // name the start of the sequence the scan stopped on. + return match std::str::from_utf8(s) { + Ok(valid) => Ok(Wtf8::new(valid)), + Err(error) => Err(CheckError { + pos: error.valid_up_to(), + }), + }; + } + let mut pos = 0; + while let Err(error) = std::str::from_utf8(&s[pos..]) { + pos += error.valid_up_to(); + // A strict decode stops at a surrogate's leading byte and nowhere + // else that WTF-8 goes on from, so exactly one three-byte sequence + // may follow before the scan resumes. + match s[pos..] { + [0xED, 0xA0..=0xBF, 0x80..=0xBF, ..] => pos += 3, + _ => return Err(CheckError { pos }), + } + } + // SAFETY: every sequence in `s` is well formed, and admitting the + // surrogates is what separates WTF-8 from UTF-8. + Ok(unsafe { Wtf8::from_bytes_unchecked(s) }) +} + /// `codepoints_in_utf8` (`rutf8.py`) — the number of code points in /// `value[start..end]`. /// @@ -143,7 +297,7 @@ pub fn codepoints_in_utf8(value: &Wtf8, start: usize, end: usize) -> usize { debug_assert!(start <= end); value[start..end] .iter() - .filter(|&&ch| (ch as i8) >= -0x40) + .filter(|&&ch| invalid_cont_byte(ch)) .count() } @@ -294,6 +448,133 @@ mod tests { use super::*; use rustpython_wtf8::Wtf8Buf; + #[test] + fn check_utf8_counts_code_points() { + assert_eq!(check_utf8(b"", true), Ok(0)); + assert_eq!(check_utf8(b"abc", true), Ok(3)); + assert_eq!(check_utf8("é中\u{10000}".as_bytes(), false), Ok(3)); + } + + #[test] + fn check_utf8_rejects_what_wtf8_from_bytes_admits() { + // `Wtf8::from_bytes`'s surrogate arm leaves the second byte unbounded + // above and the third unconstrained, so both of these reach a string. + for bad in [b"\xed\xc0\x80".as_slice(), b"\xed\xa0\x41".as_slice()] { + assert!(Wtf8::from_bytes(bad).is_some()); + assert_eq!(check_utf8(bad, true), Err(CheckError { pos: 0 })); + assert_eq!(wtf8_from_bytes(bad, true), Err(CheckError { pos: 0 })); + } + } + + #[test] + fn surrogate_bytes_names_exactly_what_the_allowance_admits() { + // The decoder's span arms read `surrogate_bytes` after + // `invalid_byte_2_of_3` has already passed, and take it to mean "the + // allowance is why this pair got through". That holds only if the + // predicate names precisely the pairs the two `allow_surrogates` + // answers disagree on. + for ch1 in 0xE0..=0xEFu8 { + for ch2 in 0..=0xFFu8 { + let admitted_by_the_allowance = + invalid_byte_2_of_3(ch1, ch2, false) && !invalid_byte_2_of_3(ch1, ch2, true); + assert_eq!( + admitted_by_the_allowance, + surrogate_bytes(ch1, ch2) && !invalid_cont_byte(ch2), + "{ch1:#04x} {ch2:#04x}" + ); + } + } + } + + #[test] + fn check_utf8_and_wtf8_from_bytes_agree() { + // Every two-byte buffer, every three-byte buffer whose lead byte can + // begin a three-byte sequence, and the four-byte leads around the two + // range bounds. `wtf8_from_bytes` reports where the strict scan + // stopped, which is the sequence start `check_utf8` names. + let mut buf = Vec::new(); + let probe = |buf: &[u8]| { + for allow in [true, false] { + let want = check_utf8(buf, allow); + let got = wtf8_from_bytes(buf, allow).map(|_| ()); + assert_eq!(want.is_ok(), got.is_ok(), "{buf:02x?} allow={allow}"); + if let (Err(a), Err(b)) = (want, got) { + assert_eq!(a, b, "{buf:02x?} allow={allow}"); + } + } + }; + for b1 in 0u16..=0xFF { + for b2 in 0u16..=0xFF { + buf.clear(); + buf.extend_from_slice(&[b1 as u8, b2 as u8]); + probe(&buf); + } + } + for b1 in 0xE0u16..=0xEF { + for b2 in 0u16..=0xFF { + for b3 in 0u16..=0xFF { + buf.clear(); + buf.extend_from_slice(&[b1 as u8, b2 as u8, b3 as u8]); + probe(&buf); + } + } + } + for b1 in [0xF0u8, 0xF3, 0xF4, 0xF5] { + for b2 in 0u16..=0xFF { + buf.clear(); + buf.extend_from_slice(&[b1, b2 as u8, 0x80, 0x80]); + probe(&buf); + buf.clear(); + buf.extend_from_slice(&[b1, 0x90, b2 as u8, 0x80]); + probe(&buf); + } + } + // A surrogate before the offending sequence: the scan must resume + // past it rather than report its own position. + probe(b"\xed\xa0\x80\xed\xc0\x80"); + probe(b"a\xed\xa0\x80\xed\xb0\x80z"); + } + + #[test] + fn check_utf8_admits_surrogates_only_when_asked() { + let lone = b"\xed\xa0\x80"; + assert_eq!(check_utf8(lone, true), Ok(1)); + assert_eq!(check_utf8(lone, false), Err(CheckError { pos: 0 })); + // A pair stays two code points, which is what `surrogatepass` decodes + // `'\\ud800\\udc00'.encode('utf-8', 'surrogatepass')` back to. + assert_eq!(check_utf8(b"\xed\xa0\x80\xed\xb0\x80", true), Ok(2)); + } + + #[test] + fn check_utf8_reports_the_offending_sequences_position() { + assert_eq!(check_utf8(b"ab\x80", true), Err(CheckError { pos: 2 })); + assert_eq!(check_utf8(b"ab\xc2", true), Err(CheckError { pos: 2 })); + assert_eq!( + check_utf8(b"ab\xe0\x80\x80", true), + Err(CheckError { pos: 2 }) + ); + assert_eq!( + check_utf8(b"ab\xf4\x90\x80\x80", true), + Err(CheckError { pos: 2 }) + ); + assert_eq!(check_utf8(b"ab\xf5", true), Err(CheckError { pos: 2 })); + assert_eq!(check_utf8(b"ab\xc1\x81", true), Err(CheckError { pos: 2 })); + } + + #[test] + fn a_checked_buffer_agrees_with_the_index_table() { + // The count `check_utf8` returns is the one `create_utf8_index_storage` + // walks to, which is the disagreement the whole check exists to stop. + let buf = Wtf8Buf::from_string("a\u{e9}\u{4e2d}\u{10000}".repeat(40)); + let len = check_utf8(buf.as_bytes(), true).unwrap(); + assert_eq!(len, buf.code_points().count()); + let storage = create_utf8_index_storage(&buf, len); + assert_eq!( + codepoint_at_index(&buf, &storage, len - 1), + buf.code_points().last().unwrap() + ); + } + fn sample(kind: &str, repeat: usize) -> Wtf8Buf { let mut buf = Wtf8Buf::new(); for i in 0..repeat {