diff --git a/majit/majit-metainterp/src/jitprof.rs b/majit/majit-metainterp/src/jitprof.rs index c0d25866eda..56b8ad8d110 100644 --- a/majit/majit-metainterp/src/jitprof.rs +++ b/majit/majit-metainterp/src/jitprof.rs @@ -185,23 +185,25 @@ struct TimingState { /// "a bridge aborted" sends the next reader looking for bridge activity that is /// not there. /// -/// `ABORT_FORCE_QUASIIMMUT` is the one reason nothing on this side can raise, -/// because the quasi-immutable *write* path is unimplemented. `hook_setfield` -/// (`rclass.rs`) genops `jit_force_quasi_immutable`, but no `OpKind` carries it, -/// `jtransform` has no arm for it, it has no jitcode opcode, and there is -/// neither a metainterp opimpl nor a `do_force_quasi_immutable` — so unlike -/// `opimpl_jit_force_quasi_immutable` (`pyjitpl.py:1094-1118`), which raises -/// `SwitchToBlackhole(ABORT_FORCE_QUASIIMMUT)` once the mutate field is -/// non-null, no path here reaches a `SwitchToBlackhole`. The *read* half -/// (`record_quasiimmut_field` → [`majit_ir::OpCode::QuasiimmutField`]) is -/// complete, so the asymmetry is in the port, not in this table. +/// `ABORT_FORCE_QUASIIMMUT` has two producers, both sitting on the residual-call +/// boundary, because that is where a quasi-immutable write lands here: +/// `try_walker_force_quasi_immut_namespace_write` and +/// `try_walker_force_quasi_immut_mapdict_write` +/// (`pyre-jit-trace/src/jitcode_dispatch/residual_call.rs`). Each calls +/// `note_force_quasi_immut_abort`, which stages the reason so the single +/// `aborted_tracing` that follows tallies it here instead of in the `Generic` +/// catch-all — the stand-in for the reason +/// `opimpl_jit_force_quasi_immutable` (`pyjitpl.py:1094-1118`) carries on the +/// `SwitchToBlackhole` instance it raises. /// -/// That tally is therefore a true zero rather than a dead slot, and it stays -/// zero for as long as the write path is missing. Nothing currently declares a -/// `?`-suffixed field, so the gap is unreachable rather than latent — but -/// `AbortReason` gains no variant for it either, so the first `?` field anyone -/// declares gets a setfield hook with nothing downstream and no abort. Keep the -/// slot; do not synthesise a producer for it. +/// The mapdict producer runs only when `PYRE_QMUT_MAPDICT_FORCE` is present in +/// the environment: `mapdict_qmut_force_enabled` asks `var_os(..).is_some()`, +/// so an empty value arms it too, and a `PYRE_*` name set by a caller reaches +/// the child. Nothing in the tree sets it, and with it unset a run reaches +/// this tally only through `try_walker_force_quasi_immut_namespace_write`; +/// that path is live, and — again with the variable unset — +/// `MAJIT_STATS=1 PYRE_MC_DIAG=1` on +/// `bench/synth/trace_too_long_effect_replay.py` prints `abrt_force_qmut=1`. pub const ABORT_COUNTER_KINDS: &[(i32, &str)] = &[ (counters::ABORT_TOO_LONG, "too_long"), (counters::ABORT_BRIDGE, "bridge_or_generic"), diff --git a/pyre/bench/synth/getframe_inline_subwalk_multiframe.py b/pyre/bench/synth/getframe_inline_subwalk_multiframe.py index ee46c88cdb7..367687ed68c 100644 --- a/pyre/bench/synth/getframe_inline_subwalk_multiframe.py +++ b/pyre/bench/synth/getframe_inline_subwalk_multiframe.py @@ -10,8 +10,7 @@ # callee during tracing and prevents this loop from compiling. The specialized # path therefore records the callee frame and creates its `FrameLocalsProxy` # without forcing the outer standard virtualizable; positive-depth lookup stays -# on the established virtual-reference walk until its per-level lowering is -# available. +# on the established virtual-reference walk. # # A real PyPy run compiles one loop with no bridges, forcings, virtualizable # forcings, or aborts. Pyre must print the same value, compile one loop, and @@ -26,14 +25,27 @@ # `bridges_compiled=0`, so the compiled loop leaves for the blackhole before it # ever crosses its back edge. The attribution is a one-run check -- a `leaf` # holding only the depth-zero read records `guard_failures=1`, one holding only -# the `_getframe(2)` read reproduces the full count. The recorded -# `guard_failures` is therefore a known shortfall of the positive-depth walk, -# not a number to preserve; it is expected to fall once that walk gets its -# per-level lowering. `loops_compiled` counts two traces here: `main`'s loop, -# and a linear `Finish` trace for the inlined `mid` -> `leaf` chain, whose own -# four `GuardNotForced`s never fail. The wasm baseline records -# `guard_failures=1` instead because that backend always materializes the -# virtualizable, which makes `GuardNotForced` a no-op there. +# the `_getframe(2)` read reproduces the full count. +# +# The count is a known shortfall, not a number to preserve -- but it will not +# come down from the `sys._getframe` arm. Letting the inline level take that +# arm's own per-level lowering, by deleting the decline in front of it, was +# measured: the fixture still answers correctly, `guard_failures` stays at +# exactly the recorded value, and the `Finish` trace gains three +# `GuardNotForced`s, because the lowering forces once per hop rather than not +# at all. Reaching `main`'s frame object from `leaf` means materializing it +# however the walk is spelled. +# +# What removes the cost is inlining `mid` -> `leaf` into `main`'s loop, where +# the depth-2 lookup lands on the trace's own virtualizable and needs no force: +# `PYPYLOG=jit-summary: pypy3 -P` on this file reports one loop, no +# bridges, `forcings: 0` and no aborts, against the two traces pyre records. +# +# `loops_compiled` counts those two traces: `main`'s loop, and a linear +# `Finish` trace for the inlined `mid` -> `leaf` chain, whose own four +# `GuardNotForced`s never fail. The wasm baseline records `guard_failures=1` +# instead because that backend always materializes the virtualizable, which +# makes `GuardNotForced` a no-op there. import sys diff --git a/pyre/extra_tests/parity_tests/framelocalsproxy_delete_slot.py b/pyre/extra_tests/parity_tests/framelocalsproxy_delete_slot.py new file mode 100644 index 00000000000..81ff0a0e548 --- /dev/null +++ b/pyre/extra_tests/parity_tests/framelocalsproxy_delete_slot.py @@ -0,0 +1,50 @@ +# CPython-suite gap: `test_frame` deletes through the proxy only to check that +# a fast local refuses, so nothing in the suite pins what `del` and `pop` +# report for a key that names no slot, or for a key that cannot be hashed. +# +# `framelocalsproxy_setitem` with no value, and `framelocalsproxy_pop`, both +# resolve the key through the same scan the subscript uses, and that scan +# hashes the key before it looks at any name. Probing a materialized snapshot +# first instead gets the refusal right and everything else wrong: the miss +# arrives as the snapshot's `KeyError`, an unhashable key arrives in the dict's +# terms, and for `pop` the discarded probe turns the hash's `TypeError` into a +# `KeyError` naming the unhashable key. +# +# parity-tests reason: every line below is exception text, which is only worth +# anything next to the runtime it has to agree with. +import sys + + +def report(label, call): + try: + print(label, "->", call()) + except Exception as exc: + print(label, "->", type(exc).__name__, exc.args) + + +def delete_and_pop(): + bound = 1 # noqa: F841 - named through the proxy below + proxy = sys._getframe(0).f_locals + # A key the frame has a slot for is refused whichever way it is asked. + report("del local", lambda: proxy.__delitem__("bound")) + report("pop local", lambda: proxy.pop("bound")) + # The hash runs before the scan, so neither of these reaches the extras + # dict to be described in its terms. + report("del unhashable", lambda: proxy.__delitem__(["unhashable"])) + report("pop unhashable", lambda: proxy.pop(["unhashable"])) + # A frame with no extras dict yet reports the key itself, or the default. + report("del absent", lambda: proxy.__delitem__("absent")) + report("pop absent", lambda: proxy.pop("absent")) + report("pop absent default", lambda: proxy.pop("absent", "fallback")) + # Once the extras dict exists it answers both, and a second delete of the + # same name is a miss again. + proxy["extra"] = 7 + report("pop extra", lambda: proxy.pop("extra")) + proxy["extra"] = 8 + report("del extra", lambda: proxy.__delitem__("extra")) + report("del extra again", lambda: proxy.__delitem__("extra")) + print("bound untouched", bound) + + +delete_and_pop() +print("OK") diff --git a/pyre/extra_tests/parity_tests/framelocalsproxy_getitem_slot.py b/pyre/extra_tests/parity_tests/framelocalsproxy_getitem_slot.py new file mode 100644 index 00000000000..9836c139fc5 --- /dev/null +++ b/pyre/extra_tests/parity_tests/framelocalsproxy_getitem_slot.py @@ -0,0 +1,145 @@ +# CPython-suite gap: `test_frame` reads `frame.f_locals[name]` only for names +# that are bound, so nothing in the suite pins what the proxy reports for an +# unbound slot, for a name it does not carry at all, or for a key that cannot +# be hashed. +# +# `framelocalsproxy_getitem` resolves the key to a locals-plus index and reads +# that one slot. Answering the same lookup by materializing the whole mapping +# and subscripting it gets the value right and everything else wrong: the miss +# arrives as the mapping's own `KeyError(key)`, and an unhashable key arrives +# as the mapping's key-flavoured `TypeError` rather than the one the hash +# raises before any slot is examined. +# +# The same scan answers a write, and it hashes the key before it looks at any +# name: a key that hashes like nothing the frame carries names no slot, however +# it compares. +# +# parity-tests reason: the value a hit returns is identical either way, so a +# snippet that reads a bound local cannot see the difference. What separates +# the two shapes is exception text, which is only worth anything next to the +# runtime it has to agree with. +import sys + + +def scalar_slots(): + bound = 1 + if bound == 0: + unbound = 2 # noqa: F841 - compiled into a slot that is never bound + proxy = sys._getframe(0).f_locals + print("bound", proxy["bound"]) + for name in ("unbound", "absent"): + try: + proxy[name] + except KeyError as exc: + print(name, "KeyError", exc.args) + try: + proxy[["unhashable"]] + except TypeError as exc: + print("unhashable", "TypeError", exc) + # A name the frame has no slot for is stored in, and read back from, the + # frame's separate extras mapping. + proxy["extra"] = 7 + print("extra", proxy["extra"]) + + +class SameHash: + """A non-`str` key that both hashes and compares like the name it holds.""" + + def __init__(self, name): + self.name = name + + def __hash__(self): + return hash(self.name) + + def __eq__(self, other): + return other == self.name + + def __repr__(self): + # The miss below reports the key with `%R`, so the default repr would + # put this object's address in the message. + return f"" + + +class OtherHash(SameHash): + """The same key, hashing like nothing the frame carries.""" + + def __hash__(self): + return hash(self.name) ^ 1 + + +def non_str_keys(): + bound = 1 # noqa: F841 - read through the proxy below + proxy = sys._getframe(0).f_locals + print("same-hash", proxy[SameHash("bound")]) + # The scan compares a name only when its hash matches the key's, so a key + # that claims equality with a name it does not hash like never reaches the + # comparison and reads as absent. + key = OtherHash("bound") + print("hashes differ", hash(key) != hash(key.name)) + try: + proxy[key] + except KeyError as exc: + print("other-hash", "KeyError", exc.args) + + +def non_str_key_writes(): + bound = 1 + proxy = sys._getframe(0).f_locals + # The scan hashes the key before looking at any name, so the key never + # reaches the extras dict to be reported in its terms. + try: + proxy[["unhashable"]] = 1 + except TypeError as exc: + print("write unhashable", "TypeError", exc) + proxy[SameHash("bound")] = 2 + print("same-hash write", bound, proxy["bound"]) + # This one is filtered out by the hash before the comparison, so it names + # no slot and is stored in the extras dict under the key object itself. + proxy[OtherHash("bound")] = 3 + print("other-hash write", bound, proxy["bound"]) + print("extras keys", sorted(repr(key) for key in proxy if not isinstance(key, str))) + + +def hidden_slot(): + # PEP 709 inlines a comprehension into its enclosing scope, and in a class + # body the iteration variable becomes a hidden slot. Hidden is a property + # of the write direction only: the scan skips such a slot when it is + # looking for somewhere to store, so the assignment below goes to the + # extras dict, but the read that follows still reports the live slot. + def probe(): + proxy = sys._getframe(1).f_locals + before = proxy["i"] + proxy["i"] = 99 + return before, proxy["i"] + + class Body: + seen = [probe() for i in range(2)] + + print("hidden", Body.seen, "leaked" if "i" in Body.__dict__ else "not leaked") + + +def cell_and_free_slots(): + captured = "cell" + + def inner(): + own = "own" + proxy = sys._getframe(0).f_locals + # `captured` has to be named in this body for the compiler to make it a + # freevar; reading it only through the proxy would leave it out of the + # locals-plus table entirely. + print("free", proxy["captured"], captured, "local", proxy["own"]) + return own + + # `captured` is a varname of this frame AND a cellvar, so the slot holds + # the cell and a reader has to dereference it; in `inner` the same name is + # a freevar, which lands past every varname in the locals-plus order. + print("cell", sys._getframe(0).f_locals["captured"]) + inner() + + +scalar_slots() +non_str_keys() +non_str_key_writes() +hidden_slot() +cell_and_free_slots() +print("OK") diff --git a/pyre/extra_tests/snippets/compile_universal_newline.py b/pyre/extra_tests/snippets/compile_universal_newline.py new file mode 100644 index 00000000000..6ecc94a0005 --- /dev/null +++ b/pyre/extra_tests/snippets/compile_universal_newline.py @@ -0,0 +1,104 @@ +# pyre-check: gate=1 +"""A source's line terminators are all `\n` by the time the tokenizer sees it. + +`pytokenizer.py:654-662` universal_newline rewrites a line ending in `\r\n` or +in a lone `\r` to one ending in `\n`, and `generate_tokens` calls it on every +line it takes from `splitlines(True)` (`pyparse.py:202`). So the rewrite is not +string syntax and not a property of one entry point: it reaches a file, a +`compile()` argument, `ast.parse` and `-c` alike, and the text of a +triple-quoted literal along with the code around it. + +Neither rewrite moves a line boundary, so the line a statement reports is the +one it reported before. +""" + +import ast +import os +import subprocess +import sys +import tempfile + +CRLF = "x = \"\"\"a\r\nb\"\"\"" +LONE_CR = "x = \"\"\"a\rb\"\"\"" + + +def value_of(source, name="x"): + namespace = {} + exec(compile(source, "", "exec"), namespace) + return namespace[name] + + +# The literal spans the terminator, so its own text carries the rewrite. +assert value_of(CRLF) == "a\nb", repr(value_of(CRLF)) +assert value_of(LONE_CR) == "a\nb", repr(value_of(LONE_CR)) + +# A terminator is not an escape, so being raw or bytes changes nothing. +assert value_of("x = r\"\"\"a\rb\"\"\"") == "a\nb", repr(value_of("x = r\"\"\"a\rb\"\"\"")) +assert value_of("x = b\"\"\"a\r\nb\"\"\"") == b"a\nb", repr(value_of("x = b\"\"\"a\r\nb\"\"\"")) + +# `ast.parse` reads the same rewritten source, so the constant it carries and +# the segment `end_col_offset` describes agree with it. +tree = ast.parse(CRLF) +assert tree.body[0].value.value == "a\nb", repr(tree.body[0].value.value) +assert ast.parse(LONE_CR).body[0].value.value == "a\nb" + +# Statements still land on the lines they were written on. +namespace = {} +exec(compile("x = 1\r\ny = 2\r\nz = 3\r", "", "exec"), namespace) +assert (namespace["x"], namespace["y"], namespace["z"]) == (1, 2, 3), namespace +try: + exec(compile("x = 1\rraise ValueError('boom')", "", "exec"), {}) +except ValueError as exc: + assert exc.__traceback__.tb_next.tb_lineno == 2, exc.__traceback__.tb_next.tb_lineno +else: + raise AssertionError("the raise did not run") + +# A failed compile reports the offending line, and that line came from the +# rewritten source too. How the reference spells the terminator is its own +# business -- CPython drops it and PyPy keeps `\n` -- so what is pinned here is +# only that no carriage return survives into it. +try: + compile("x = 1\r\ny = (\r\nz = 3\r\n", "", "exec") +except SyntaxError as exc: + assert exc.lineno == 2, exc.lineno + assert exc.text is not None and "\r" not in exc.text, repr(exc.text) + assert exc.text.rstrip("\r\n") == "y = (", repr(exc.text) +else: + raise AssertionError("the unclosed paren did not raise") + +# A lone `\r` reaches the same slicing. Before the rewrite ran on the string +# the report is sliced from, this source held no `\n` at all, so the offending +# line came back as `None` rather than as the second line. +try: + compile("a = 1\rb = (\r", "", "exec") +except SyntaxError as exc: + assert exc.lineno == 2, exc.lineno + assert exc.text is not None and "\r" not in exc.text, repr(exc.text) + assert exc.text.rstrip("\r\n") == "b = (", repr(exc.text) +else: + raise AssertionError("the unclosed paren did not raise") + +# The same source reaching the compiler as a `-c` argument, and as a file. +PROGRAM = CRLF + "\nprint(repr(x))\n" + +completed = subprocess.run( + [sys.executable, "-c", PROGRAM], + capture_output=True, + text=True, +) +assert completed.returncode == 0, completed.stderr +assert completed.stdout.strip() == "'a\\nb'", completed.stdout + +# Written as bytes so the carriage returns reach the file itself rather than +# whatever the platform's text mode would spell them as. +handle, path = tempfile.mkstemp(suffix=".py") +os.write(handle, PROGRAM.encode()) +os.close(handle) +try: + completed = subprocess.run([sys.executable, path], capture_output=True, text=True) + assert completed.returncode == 0, completed.stderr + assert completed.stdout.strip() == "'a\\nb'", completed.stdout +finally: + os.unlink(path) + +print("OK") diff --git a/pyre/pyre-interpreter/src/builtins.rs b/pyre/pyre-interpreter/src/builtins.rs index c3e8bdd0c53..35198599be5 100644 --- a/pyre/pyre-interpreter/src/builtins.rs +++ b/pyre/pyre-interpreter/src/builtins.rs @@ -11802,6 +11802,11 @@ fn compile_err_to_syntax_error_maybe_incomplete( source: &str, allow_incomplete: bool, ) -> crate::PyError { + // Every location in `e` indexes the source the compiler was handed, which + // `compile_source_with_opts` had already run through `universal_newline`. + // Slice the same string here, or a `\r\n` input reports a line that still + // carries its carriage return and a lone-`\r` one finds no line at all. + let source = &*crate::compile::universal_newline(source); use rustpython_compiler::parser::{ InterpolatedStringErrorType, LexicalErrorType, ParseErrorType, }; diff --git a/pyre/pyre-interpreter/src/compile.rs b/pyre/pyre-interpreter/src/compile.rs index 2e9dd991368..8e30034f1ad 100644 --- a/pyre/pyre-interpreter/src/compile.rs +++ b/pyre/pyre-interpreter/src/compile.rs @@ -12,6 +12,36 @@ pub use rustpython_compiler_core::bytecode::{ MakeFunctionFlags, OpArg, OpArgState, SpecialMethod, }; +/// Rewrite every `\r\n` and every lone `\r` in a source as `\n`. +/// +/// `pytokenizer.py:654-662` universal_newline does this one line at a time, and +/// `generate_tokens` calls it on every line it takes. The lines come from +/// `splitlines(True)` (`pyparse.py:202`), which ends a line at `\r`, `\r\n` and +/// `\n` alike, so a carriage return can only ever sit at a line's end there — +/// which makes rewriting them all in one pass the same transformation over the +/// whole source. Pyre needs the whole-source form because its tokenizer takes +/// the source rather than a line at a time. +/// +/// A line terminator is not string syntax, so this reaches the text of a +/// triple-quoted literal, raw and bytes ones included, exactly as it does the +/// code around it. Neither rewrite moves a line boundary, so line numbers and +/// the column of anything on a line are what they were. +pub fn universal_newline(source: &str) -> std::borrow::Cow<'_, str> { + if !source.contains('\r') { + return std::borrow::Cow::Borrowed(source); + } + let mut normalized = String::with_capacity(source.len()); + let mut rest = source; + while let Some(carriage_return) = rest.find('\r') { + normalized.push_str(&rest[..carriage_return]); + normalized.push('\n'); + rest = &rest[carriage_return + 1..]; + rest = rest.strip_prefix('\n').unwrap_or(rest); + } + normalized.push_str(rest); + std::borrow::Cow::Owned(normalized) +} + /// Compile Python source code to a RustPython CodeObject. /// /// The filename is the one `exec`/`eval` report for a str source, surfacing @@ -20,8 +50,7 @@ pub use rustpython_compiler_core::bytecode::{ /// The `CompileError` is returned unflattened so the SyntaxError builders /// can read its `python_location` / `python_end_location` / `source_path`. pub fn compile_source(source: &str, mode: Mode) -> Result { - crate::module::thread::ensure_runtime_thread(); - rp_compile(source, mode, "", default_compile_opts()) + compile_source_with_opts(source, mode, "", default_compile_opts()) } /// The `CompileOpts` for an implicit compile (script / `-c` / import): the @@ -56,7 +85,7 @@ pub fn compile_source_with_opts( opts: CompileOpts, ) -> Result { crate::module::thread::ensure_runtime_thread(); - rp_compile(source, mode, filename, opts) + rp_compile(&universal_newline(source), mode, filename, opts) } /// Scan the first two lines of `source` for a PEP 263 coding cookie diff --git a/pyre/pyre-interpreter/src/eval.rs b/pyre/pyre-interpreter/src/eval.rs index d7a48936a67..9ed4a5d1a38 100644 --- a/pyre/pyre-interpreter/src/eval.rs +++ b/pyre/pyre-interpreter/src/eval.rs @@ -1493,7 +1493,7 @@ pub fn get_sys_exception() -> PyObjectRef { if ec.is_null() { return PY_NULL; } - unsafe { (*ec).sys_exc_info(false) } + unsafe { (*ec).sys_exc_info() } } /// Flat TLS write of the per-thread `CURRENT_EXCEPTION` slot — same diff --git a/pyre/pyre-interpreter/src/executioncontext.rs b/pyre/pyre-interpreter/src/executioncontext.rs index 07ceceedda8..5940790fc11 100644 --- a/pyre/pyre-interpreter/src/executioncontext.rs +++ b/pyre/pyre-interpreter/src/executioncontext.rs @@ -591,9 +591,13 @@ impl ExecutionContext { pub fn enter(&mut self, frame: *mut PyFrame) { // pypy/interpreter/executioncontext.py:85-89 enter parity. - if !self.space.is_null() && self.is_tracing > 0 { - self._revdb_enter(frame); - } + // + // `if self.space.reverse_debugging: self._revdb_enter(frame)` folds + // away: the space carries `reverse_debugging = False` + // (baseobjspace.py:429) unless `config.translation.reverse_debugger` + // sets it (baseobjspace.py:444), and reverse debugging is not ported. + // Same fold `bytecode_only_trace` and `side_effects_ok` already carry. + // // `f_backref` is a traced `Type::Ref` field, so the store carries the // generational barrier the JIT's `SetfieldGc` lowering emits for it. // `frame` can be a non-moving old-generation frame while `topframeref` @@ -650,7 +654,8 @@ impl ExecutionContext { // `jit.virtual_ref_finish(frame_vref, frame)` — keepalives only at // interp level (both operands are already live in Rust); the JIT // records VIRTUAL_REF_FINISH during tracing. - self._revdb_leave(got_exception); + // `if self.space.reverse_debugging: self._revdb_leave(got_exception)` + // (executioncontext.py:108-109) folds away with the arm in `enter`. trace_result } @@ -1035,11 +1040,21 @@ impl ExecutionContext { Ok(()) } - pub fn sys_exc_info(&self, _for_hidden: bool) -> PyObjectRef { - let _ = _for_hidden; + /// `executioncontext.py:219-232 sys_exc_info` — the topmost handled + /// exception, or PY_NULL when nothing is being handled. + pub fn sys_exc_info(&self) -> PyObjectRef { if !self.sys_exc_value.is_null() { return self.sys_exc_value; } + if !self.current_gen_or_coroutine.is_null() { + return self._get_topmost_exception(); + } + pyre_object::PY_NULL + } + + /// `executioncontext.py:278-284 _get_topmost_exception` — the first + /// exception parked on the suspended generator/coroutine chain. + pub fn _get_topmost_exception(&self) -> PyObjectRef { let mut generator = self.current_gen_or_coroutine; while !generator.is_null() { let saved = diff --git a/pyre/pyre-interpreter/src/module/_ast/convert.rs b/pyre/pyre-interpreter/src/module/_ast/convert.rs index 1277759ba54..508e427d67d 100644 --- a/pyre/pyre-interpreter/src/module/_ast/convert.rs +++ b/pyre/pyre-interpreter/src/module/_ast/convert.rs @@ -1706,6 +1706,10 @@ pub fn parse_to_object_with_opts( opts: crate::compile::CompileOpts, syntax_check_only: bool, ) -> crate::PyResult { + // The tokenizer sees a source whose line terminators are all `\n` + // (`pytokenizer.py:654-662`), so the same rewrite runs here; the nodes and + // the text `module_to_object` slices segments out of then agree. + let source = &*crate::compile::universal_newline(source); let mut module = match mode { crate::compile::Mode::Eval => parser::parse_expression(source) .map(|parsed| ast::Mod::Expression(parsed.into_syntax())), diff --git a/pyre/pyre-interpreter/src/pyframe.rs b/pyre/pyre-interpreter/src/pyframe.rs index 3409e12b781..e2792a5b60a 100644 --- a/pyre/pyre-interpreter/src/pyframe.rs +++ b/pyre/pyre-interpreter/src/pyframe.rs @@ -120,22 +120,35 @@ pub mod frame_locals_proxy { } fn fast_local_index(&self, key: PyObjectRef) -> Result, crate::PyError> { - // Every candidate name allocates its own string and `eq_w` can run - // a Python `__eq__`, so the caller's key is a pre-allocation copy - // from the first comparison onward. Publish it and reload it for - // each compare; the candidate reuses one slot rather than growing - // the stack once per local. + // Every candidate name allocates its own string and both + // `__hash__` and `__eq__` can run a Python method, so the caller's + // key is a pre-allocation copy from the first of them onward. + // Publish it and reload it for each compare; the candidate reuses + // one slot rather than growing the stack once per local. let roots = pyre_object::gc_roots::push_roots(); let key_slot = roots.base(); roots.pin_root(key); let candidate_slot = key_slot + 1; roots.pin_root(pyre_object::PY_NULL); + // The scan hashes the key first whether it was reached to write or + // to read, so an unhashable key is a `TypeError` here rather than + // whatever the extras dict would go on to say about it. + let key_hash = crate::baseobjspace::hash_w_strict(roots.get(key_slot))?; // `code` addresses the compiler code object, which lives outside // the GC heap and so stays valid across those collections. let code = self.frame().code(); let matches = |name: &str| -> Result { roots.set(candidate_slot, pyre_object::w_str_new(name)); - crate::baseobjspace::eq_w(roots.get(candidate_slot), roots.get(key_slot)) + // A name whose hash differs is never compared, so a key that + // claims equality with a name it does not hash like does not + // reach that name's slot. + Ok( + crate::baseobjspace::hash_w_strict(roots.get(candidate_slot))? == key_hash + && crate::baseobjspace::eq_w( + roots.get(candidate_slot), + roots.get(key_slot), + )?, + ) }; for (index, name) in code.varnames.iter().enumerate() { if hidden_local(code, index) { @@ -196,6 +209,80 @@ pub mod frame_locals_proxy { .map(|_| ()) } + /// `framelocalsproxy_getkeyindex` with `read == true`, followed by + /// `framelocalsproxy_getval`: the value of the first locals-plus slot + /// whose name hashes and compares equal to `key` and which is bound. + /// + /// A name that matches only unbound slots reads as absent, which is + /// what sends `__getitem__` on to `f_extra_locals`. The write + /// direction skips a PEP 709 hidden slot and this one does not, so it + /// cannot share [`Self::fast_local_index`]: a running comprehension's + /// iteration variable is readable through the proxy even though + /// assigning to that name goes to the extras dict instead. + fn locals_plus_value( + &self, + key: PyObjectRef, + ) -> Result, crate::PyError> { + // A candidate name allocates its own string and both `__hash__` + // and `__eq__` can run Python, so the key is rooted before the + // first of them and read back from that root afterwards. + let roots = pyre_object::gc_roots::push_roots(); + let key_slot = roots.base(); + roots.pin_root(key); + let candidate_slot = key_slot + 1; + roots.pin_root(pyre_object::PY_NULL); + // Hashing runs before the scan, so an unhashable key is a + // `TypeError` even for a frame with no locals to compare against. + let key_hash = crate::baseobjspace::hash_w_strict(roots.get(key_slot))?; + let exact_str_key = unsafe { + pyre_object::pyobject::is_exact_type( + roots.get(key_slot), + &pyre_object::pyobject::STR_TYPE, + ) + }; + // `code` addresses the compiler code object, which lives outside + // the GC heap and so stays valid across those collections. + let code = self.frame().code(); + for (index, name, cell_slot) in locals_plus_names(code) { + let same = if exact_str_key { + // The interned-name pointer comparison the scan opens + // with, widened to every equal `str`: comparing WTF-8 + // bytes decides an exact `str` key outright, so it needs + // neither the hash nor a candidate allocation. + unsafe { pyre_object::w_str_get_wtf8(roots.get(key_slot)) }.as_bytes() + == name.as_bytes() + } else { + roots.set(candidate_slot, pyre_object::w_str_new(name)); + // A name whose hash differs is never compared: a key that + // claims equality with a name it does not hash like reads + // as absent rather than as that name's slot. + crate::baseobjspace::hash_w_strict(roots.get(candidate_slot))? == key_hash + && crate::baseobjspace::eq_w( + roots.get(candidate_slot), + roots.get(key_slot), + )? + }; + if !same { + continue; + } + let frame = self.frame(); + if index >= locals_w!(frame).len() { + continue; + } + let slot = locals_w!(frame)[index]; + let value = if cell_slot && !slot.is_null() && unsafe { pyre_object::is_cell(slot) } + { + unsafe { pyre_object::w_cell_get(slot) } + } else { + slot + }; + if !value.is_null() { + return Ok(Some(value)); + } + } + Ok(None) + } + fn key_is_fast_local(&self, key: PyObjectRef) -> Result { Ok(self.fast_local_index(key)?.is_some()) } @@ -233,12 +320,35 @@ pub mod frame_locals_proxy { } fn __getitem__(&self, key: PyObjectRef) -> Result { - // `mapping` allocates the snapshot dict, so reload the key after it. + // `framelocalsproxy_getitem` reads the one slot the key names. + // Materializing the whole mapping to answer a single lookup costs a + // dict plus an entry per bound local, and reports the miss as the + // dict's own `KeyError(key)` rather than the message below. + // + // `locals_plus_value` and the extras lookup both allocate, so + // reload the key from its root between them. let roots = pyre_object::gc_roots::push_roots(); let key_slot = roots.base(); roots.pin_root(key); - let mapping = self.mapping()?; - crate::baseobjspace::getitem(mapping, roots.get(key_slot)) + if let Some(value) = self.locals_plus_value(roots.get(key_slot))? { + return Ok(value); + } + let extra = self.frame().get_extra_locals(); + if !extra.is_null() { + let extra_slot = key_slot + 1; + roots.pin_root(extra); + if let Some(value) = + crate::baseobjspace::finditem(roots.get(extra_slot), roots.get(key_slot))? + { + return Ok(value); + } + } + let key_repr = unsafe { crate::display::py_repr_wtf8(roots.get(key_slot))? }; + Err(crate::PyError::key_error(crate::display::wtf8_format!( + "local variable '", + key_repr, + "' is not defined" + ))) } fn __setitem__( @@ -250,22 +360,29 @@ pub mod frame_locals_proxy { } fn __delitem__(&mut self, key: PyObjectRef) -> Result<(), crate::PyError> { - // The snapshot, the lookup, the fast-local scan and the extras dict - // all allocate, so reload the key between them. + // `framelocalsproxy_setitem` with no value: the scan decides, and + // only a key it places nowhere goes on to the extras dict. Probing + // the snapshot first instead built a whole mapping to reject one + // name, and reported an unhashable key in that dict's terms rather + // than as the hash's own `TypeError`. + // + // The extras lookup allocates, so reload the key from its root. let roots = pyre_object::gc_roots::push_roots(); let key_slot = roots.base(); roots.pin_root(key); - let mapping = self.mapping()?; - // Perform the lookup first so absent/unhashable keys retain the - // underlying mapping's KeyError/TypeError. - crate::baseobjspace::getitem(mapping, roots.get(key_slot))?; if self.key_is_fast_local(roots.get(key_slot))? { return Err(crate::PyError::value_error( "cannot remove local variables from FrameLocalsProxy", )); } - let extra = self.frame().get_or_create_extra_locals(); - crate::baseobjspace::delitem(extra, roots.get(key_slot)) + // A delete does not create the extras dict that a store would. + let extra = self.frame().get_extra_locals(); + if extra.is_null() { + return Err(crate::PyError::key_error_with_key(roots.get(key_slot))); + } + let extra_slot = key_slot + 1; + roots.pin_root(extra); + crate::baseobjspace::delitem(roots.get(extra_slot), roots.get(key_slot)) } fn __len__(&self) -> Result { @@ -413,25 +530,39 @@ pub mod frame_locals_proxy { if args.len() < 2 || args.len() > 3 { return Err(crate::PyError::type_error("pop expected 1 or 2 arguments")); } - // The snapshot, the lookup, the fast-local scan and the extras dict - // all allocate, so publish the incoming arguments and rebuild the - // slice for the delegated call. + // `framelocalsproxy_pop`: the scan decides, and only a key it + // places nowhere reaches the extras dict. Probing the snapshot + // first discarded the lookup's error, so an unhashable key came + // back as a `KeyError` naming that key instead of the `TypeError` + // the hash raises. + // + // The extras lookup allocates, so publish the incoming arguments + // and rebuild the slice for the delegated call. let roots = pyre_object::gc_roots::push_roots(); let args_base = roots.publish(&args[1..]); let nargs = args.len() - 1; roots.normalize(args_base, nargs); - let mapping = self.mapping()?; - if crate::baseobjspace::getitem(mapping, roots.get(args_base)).is_ok() - && self.key_is_fast_local(roots.get(args_base))? - { + if self.key_is_fast_local(roots.get(args_base))? { return Err(crate::PyError::value_error( "cannot remove local variables from FrameLocalsProxy", )); } - let extra = self.frame().get_or_create_extra_locals(); + let extra = self.frame().get_extra_locals(); + if extra.is_null() { + // A frame with no extras dict answers the default if one was + // passed and reports the key itself otherwise; it does not + // gain a dict from a pop that finds nothing. + return if nargs == 2 { + Ok(roots.get(args_base + 1)) + } else { + Err(crate::PyError::key_error_with_key(roots.get(args_base))) + }; + } + let extra_slot = args_base + nargs; + roots.pin_root(extra); let call_args: Vec = (0..nargs).map(|i| roots.get(args_base + i)).collect(); - let result = crate::baseobjspace::call_method(extra, "pop", &call_args); + let result = crate::baseobjspace::call_method(roots.get(extra_slot), "pop", &call_args); if result.is_null() { Err(crate::call::take_call_error() .unwrap_or_else(|| crate::PyError::runtime_error("pop failed"))) @@ -2203,6 +2334,41 @@ pub fn ncells(code: &CodeObject) -> usize { npure_cellvars(code) + code.freevars.len() } +/// True when localsplus slot `idx` is a `varnames` entry that `MAKE_CELL` +/// turned into a cell, so the slot holds the cell and a reader has to +/// dereference it to reach the value. +#[inline] +fn cell_slot(code: &CodeObject, idx: usize) -> bool { + idx < code.localspluskinds.len() + && code.localspluskinds[idx] & crate::bytecode::CO_FAST_CELL != 0 +} + +/// The frame's locals-plus slots in `co_localsplusnames` order — every +/// `varnames` entry, then the `cellvars` that are not already varnames, then +/// the `freevars` — each paired with its slot index and whether the slot holds +/// a cell. +/// +/// `framelocalsproxy_getkeyindex` scans this order and +/// `frame_locals_proxy_snapshot` fills a dict in it, so a name resolves to the +/// same slot on both routes only as long as they walk it the same way. +fn locals_plus_names(code: &CodeObject) -> impl Iterator { + let locals = code + .varnames + .iter() + .enumerate() + .map(|(index, name)| (index, name.as_str(), cell_slot(code, index))); + let pure_cellvars = code + .cellvars + .iter() + .filter(|name| !code.varnames.iter().any(|local| local == *name)); + locals.chain( + pure_cellvars + .chain(code.freevars.iter()) + .enumerate() + .map(|(offset, name)| (code.varnames.len() + offset, name.as_str(), true)), + ) +} + /// True when localsplus slot `idx` carries `CO_FAST_HIDDEN`, i.e. an inlined /// comprehension's iteration variable (PEP 709). The flag records that the /// surrounding binding is saved and restored by the comprehension bytecode; @@ -2772,22 +2938,8 @@ impl PyFrame { setitem_str_object(roots.get(snapshot_slot), name, roots.get(value_slot)) }; - for (index, name) in code.varnames.iter().enumerate() { - let cell_slot = index < code.localspluskinds.len() - && code.localspluskinds[index] & crate::bytecode::CO_FAST_CELL != 0; - insert_slot(index, name.as_ref(), cell_slot)?; - } - let mut index = code.varnames.len(); - for name in code.cellvars.iter() { - if code.varnames.iter().any(|local| local == name) { - continue; - } - insert_slot(index, name.as_ref(), true)?; - index += 1; - } - for name in code.freevars.iter() { - insert_slot(index, name.as_ref(), true)?; - index += 1; + for (index, name, cell_slot) in locals_plus_names(code) { + insert_slot(index, name, cell_slot)?; } Ok(roots.get(snapshot_slot)) } diff --git a/pyre/pyrex/src/lib.rs b/pyre/pyrex/src/lib.rs index 961849faa0f..5871e903a42 100644 --- a/pyre/pyrex/src/lib.rs +++ b/pyre/pyrex/src/lib.rs @@ -95,6 +95,13 @@ fn drain_args(parser: &mut lexopt::Parser) -> Result, le /// and keeps its carriage return, where `" "` alone is blank. That is a /// narrower notion of blank than `textwrap.dedent`'s, which empties every line /// `str.isspace()` answers for. +/// +/// A carriage return does reach this point, and is meant to: the dedent runs on +/// the argument as it was given, and the tokenizer's own `\r\n` / lone-`\r` +/// rewrite (`pytokenizer.py:654-662`) happens after it. Counting `" \r"` as a +/// line that holds something is what the reference does as well -- against a +/// two-space margin a `" \r"` line comes back as `" "`, where a `" "` +/// line comes back empty. fn dedent_command(source: &str) -> std::borrow::Cow<'_, str> { fn split_newline(line: &str) -> (&str, &str) { match line.strip_suffix('\n') { diff --git a/pyre/pyrex/src/repl.rs b/pyre/pyrex/src/repl.rs index bf683714853..0ae9e1cf23c 100644 --- a/pyre/pyrex/src/repl.rs +++ b/pyre/pyrex/src/repl.rs @@ -344,10 +344,11 @@ fn compile_repl_input( empty_line_given: bool, continuing_block: bool, ) -> ShellCompileAction { - #[cfg(windows)] - let normalized = source.replace("\r\n", "\n"); - #[cfg(windows)] - let source = normalized.as_str(); + // `generate_tokens` normalizes every line it takes, on every platform and + // for a lone `\r` as well (`pytokenizer.py:654-662`), so the shell hands the + // compiler the same shape a script would rather than a windows-only + // `\r\n` rewrite. + let source = &*pyre_interpreter::universal_newline(source); match pyre_interpreter::rp_compile(source, Mode::Single, "", Default::default()) { Ok(code) => {