Repository navigation
jit: enforce the recursion-unroll bound once; frame and tuple owner roots - #887
Conversation
WalkthroughThe PR adds persistent GC owner roots and lifetime diagnostics, protects GC-sensitive frame and tuple allocations, adjusts JIT tracing and exception control, fixes platform-specific I/O and faulthandler behavior, initializes standard-stream codecs during bootstrap, and corrects Unicode empty-substring counting. ChangesGC rooting and diagnostics
JIT tracing and entry control
Interpreter runtime behavior
Unicode empty-substring counting
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e74911b57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let raw_slot = if raw.is_null() { | ||
| None | ||
| } else { | ||
| crate::gc_roots::pin_root(raw as PyObjectRef); |
There was a problem hiding this comment.
In a free-threaded run, pin_root itself can park at try_gc_current_object_address, so a foreign major collection may trace raw immediately after this line. The old-gen allocator explicitly returns uninitialized payload memory, and tuple_object_custom_trace reads ob_header.w_class and wrappeditems; tracing this newly rooted tuple before the ptr::write below can therefore interpret garbage as pointers and crash or corrupt the collector. Write a valid tuple with a null wrappeditems field before publishing it, then install the allocated items block afterward.
Useful? React with 👍 / 👎.
| // overflow at a Python call depth of 2. `report_error = 0` for the region | ||
| // is what `stack.h:42-43` is for — a real overflow still surfaces from the | ||
| // interpreter once the region exits. | ||
| let _cc_guard = majit_metainterp::CriticalCodeGuard::enter(); |
There was a problem hiding this comment.
When several already-hot functions first compile while called from one another, every active compile_and_run_once retains its Cranelift stack frames while this guard suppresses native-stack overflow reporting in the generated code it enters. At the documented ~1.2 MB per compiler invocation, only a short nested chain can exhaust a normal thread stack and terminate the process before any interpreter-side check runs; dropping the guard after the compiler finishes, rather than after the compiled Python call returns, preserves overflow detection during user code. The admitted whole-region deviation also conflicts with the repository's strict structural-parity requirement.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
| faulthandler_extract_fd(args.first().copied().unwrap_or(pyre_object::PY_NULL))?; | ||
| #[cfg(all(unix, feature = "host_env"))] | ||
| { | ||
| FAULTHANDLER_FD.store(fd, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
When enable() receives a file object rather than an integer fd, storing only its numeric descriptor lets the object be collected or closed immediately after enable returns. A later fatal signal can then write nothing or, if the descriptor has been reused, write the crash report into an unrelated file. PyPy's Handler.enable deliberately retains fatal_error_w_file until disable; preserve the same interpreter-owned reference instead of keeping only the fd.
AGENTS.md reference: AGENTS.md:L194-L196
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
majit/majit-gc/src/collector.rs (1)
3301-3322: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHard-coded
type_id == 9backtrace + repeated uncached env lookup.Two issues in
remember_young_pointer:
std::env::var_os("MAJIT_GC_LIFETIME_LOG")is re-evaluated on every barrier hit that reaches this function (potentially very hot), even when logging is disabled.if unsafe { (*hdr).type_id() } == 9is an undocumented magic number that unconditionally force-captures a full backtrace for that type. The PR description itself notes this is leftover debug scaffolding.🧹 Proposed cleanup
+static LIFETIME_LOG_ENABLED: std::sync::OnceLock<bool> = std::sync::OnceLock::new(); + fn remember_young_pointer(&mut self, obj: GcRef) { self.remembered_set.push(obj.0); let hdr = unsafe { header_of(obj.0) }; - if std::env::var_os("MAJIT_GC_LIFETIME_LOG").is_some() { + if *LIFETIME_LOG_ENABLED.get_or_init(|| std::env::var_os("MAJIT_GC_LIFETIME_LOG").is_some()) { eprintln!( "[gc][remember] addr={:`#x`} type_id={} source=write_barrier state={:?}", obj.0, unsafe { (*hdr).type_id() }, self.gc_state ); - if unsafe { (*hdr).type_id() } == 9 { - eprintln!("{}", std::backtrace::Backtrace::force_capture()); - } } unsafe { (*hdr).clear_flag(flags::TRACK_YOUNG_PTRS) }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-gc/src/collector.rs` around lines 3301 - 3322, Remove the leftover type_id == 9 backtrace capture from remember_young_pointer, and avoid evaluating MAJIT_GC_LIFETIME_LOG on every barrier invocation by caching the logging-enabled state at an appropriate longer-lived scope. Preserve the existing remember-set update, lifetime log fields, and TRACK_YOUNG_PTRS clearing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 2094-2101: Update the MAJIT_GC_LIFETIME_LOG check in the
major-seed remember logging block to use the same cached environment-variable
mechanism as copy_nursery_object. Reuse the existing cache or helper rather than
calling std::env::var_os directly, while preserving the current logging
behavior.
- Around line 1839-1846: Cache the MAJIT_GC_LIFETIME_LOG enablement once instead
of calling std::env::var_os on every promotion. Update the logging guard in the
promotion path around self.remembered_set.push and apply the same cached flag
used by the other lifetime-log checks, preserving the existing output when
enabled.
In `@majit/majit-gc/src/oldgen.rs`:
- Around line 189-195: Update the MAJIT_GC_LIFETIME_LOG check in the
old-generation free path to use the shared cached environment-variable
configuration established for the collector.rs occurrences, rather than calling
std::env::var_os directly. Preserve the existing lifetime log output and
condition behavior.
- Around line 222-228: Update the MAJIT_GC_LIFETIME_LOG check in the arena free
logging path to use the shared cached environment-variable configuration instead
of calling std::env::var_os directly. Reuse the existing cache or accessor
established by the consolidated change while preserving the current logging
output and condition.
In `@majit/majit-gc/src/shadow_stack.rs`:
- Around line 453-466: Update acquire_owner_root and its corresponding release
logic to maintain a free-index stack alongside OWNER_ROOTS, reusing available
slots in O(1) instead of scanning the vector with position(Option::is_none).
Push released indices onto the free list and pop them during acquisition, while
preserving append behavior when no free index exists and ensuring reused slots
are populated with the new root.
- Around line 484-505: Update OwnerRootGuard to include a PhantomData<*mut ()>
marker field, making the guard non-Send/non-Sync and bound to the acquiring
thread’s OWNER_ROOTS TLS. Initialize this marker in OwnerRootGuard::new while
preserving index acquisition and Drop::drop behavior.
In `@pyre/pyre-interpreter/src/module/faulthandler/handler.rs`:
- Around line 71-75: Move the faulthandler_extract_fd call out of the
unconditional path and into the applicable host_env-gated branches around the fd
setup logic at lines 71–94 and 183. Ensure unsupported configurations return
NotImplementedError before evaluating or invoking file descriptor extraction,
while host_env builds retain the existing extraction and storage behavior.
- Around line 71-75: Validate the descriptor returned by faulthandler_extract_fd
before the FAULTHANDLER_FD.store call, including values produced through
fileno(), and raise a RuntimeError with the expected invalid-file-descriptor
message for negative results. Preserve valid direct-integer and fileno()
behavior, ensuring no invalid descriptor reaches signal-handler registration.
- Around line 71-75: Update the faulthandler setup around
faulthandler_extract_fd to flush the resolved target file object before storing
the descriptor or enabling fatal handlers. Reuse the resolved file object and
its existing flush behavior, ensuring buffered output is written before handler
installation while preserving the current descriptor handling.
- Around line 20-25: Update the faulthandler state around FAULTHANDLER_FD so
enabling the handler retains the file object returned by get_fileno_and_file()
for the handler’s lifetime, or instead duplicates and owns its descriptor.
Ensure the retained resource remains valid until disable() and is released or
closed when the handler is disabled, while preserving the existing default
descriptor behavior.
---
Outside diff comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 3301-3322: Remove the leftover type_id == 9 backtrace capture from
remember_young_pointer, and avoid evaluating MAJIT_GC_LIFETIME_LOG on every
barrier invocation by caching the logging-enabled state at an appropriate
longer-lived scope. Preserve the existing remember-set update, lifetime log
fields, and TRACK_YOUNG_PTRS clearing behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8b23535a-3b6a-41a7-8909-92126d35c16f
📒 Files selected for processing (14)
majit/majit-gc/src/collector.rsmajit/majit-gc/src/oldgen.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-translate/src/codewriter/call.rspypy/objspace/std/test/apptest_unicode.pypypy/objspace/std/unicodeobject.pypyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/faulthandler/handler.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/tupleobject.rs
| /// Acquire a fixed translated-livevar root slot. | ||
| pub fn acquire_owner_root(root: GcRef) -> usize { | ||
| OWNER_ROOTS.with(|roots| { | ||
| let mut roots = roots.borrow_mut(); | ||
| if let Some(index) = roots.iter().position(Option::is_none) { | ||
| roots[index] = Some(root); | ||
| index | ||
| } else { | ||
| let index = roots.len(); | ||
| roots.push(Some(root)); | ||
| index | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
acquire_owner_root's linear free-slot scan can become O(depth) on deep recursion.
FrameBox's persistent owner_root (one per live frame) stays occupied for the frame's whole lifetime, while FrameBox::new's 10 transient field-guards are short-lived. As live frame count grows (e.g. deep non-tail recursion — directly relevant to this PR's fib_recursive discussion), position(Option::is_none) has to skip over all currently-live persistent slots before finding/appending a free one, making each FrameBox::new call (10 acquires) cost O(live-frames) and the whole call stack O(depth²).
Consider tracking free indices in a small stack/free-list alongside the Vec so acquire/release stay O(1).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-gc/src/shadow_stack.rs` around lines 453 - 466, Update
acquire_owner_root and its corresponding release logic to maintain a free-index
stack alongside OWNER_ROOTS, reusing available slots in O(1) instead of scanning
the vector with position(Option::is_none). Push released indices onto the free
list and pop them during acquisition, while preserving append behavior when no
free index exists and ensuring reused slots are populated with the new root.
| pub struct OwnerRootGuard { | ||
| index: usize, | ||
| } | ||
|
|
||
| impl OwnerRootGuard { | ||
| pub fn new(root: GcRef) -> Self { | ||
| Self { | ||
| index: acquire_owner_root(root), | ||
| } | ||
| } | ||
|
|
||
| pub fn get(&self) -> GcRef { | ||
| OWNER_ROOTS.with(|roots| roots.borrow()[self.index].expect("inactive owner-root guard")) | ||
| } | ||
| } | ||
|
|
||
| impl Drop for OwnerRootGuard { | ||
| fn drop(&mut self) { | ||
| release_owner_root(self.index); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)shadow_stack\.rs$|FrameBox|OwnerRootGuard|OWNER_ROOTS|acquire_owner_root|release_owner_root' || true
echo "== outline candidate =="
ast-grep outline majit/majit-gc/src/shadow_stack.rs --view expanded | sed -n '1,220p' || true
echo "== relevant source =="
cat -n majit/majit-gc/src/shadow_stack.rs | sed -n '1,120p;200,340p;440,515p;1260,1300p'
echo "== references around sent/implements =="
rg -n "impl\s+Send|unsafe\s+impl\s+Send|Send|Sync|struct\s+FrameBox|Option<OwnerRootGuard>|pub struct OwnerRootGuard|OWNER_ROOTS|acquire_owner_root|release_owner_root" majit/majit-gc/src/shadow_stack.rsRepository: youknowone/pyre
Length of output: 25027
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== OwnerRootGuard references across repo =="
rg -n "OwnerRootGuard|acquire_owner_root|release_owner_root|Option<OwnerRootGuard>|struct FrameBox|FrameBox" .
echo "== Rust compiler Send check for OwnerRootGuard shape =="
python3 - <<'PY'
from pathlib import Path
p = Path("majit/majit-gc/src/shadow_stack.rs")
text = p.read_text()
start = text.index("pub struct OwnerRootGuard")
end = text.index("\n/// An opaque handle to this thread's", start)
print(text[start:end])
print("contains PhantomData:", "PhantomData" in text[start:end])
PY
echo "== Check Rust availability and run a small Send marker compile check =="
if command -v rustc >/dev/null 2>&1; then
rustc --version
cat >/tmp/owner_root_guard_send_probe.rs <<'RS'
struct GcRef(i64);
struct OwnerRootGuard {
index: usize,
}
impl OwnerRootGuard {
pub fn _new(_root: GcRef) -> Self {
todo!()
}
pub fn _get(&self) -> GcRef {
todo!()
}
}
impl Drop for OwnerRootGuard {
fn drop(&mut self) {
let _index = self.index;
}
}
const _: () = { let _ = |_x: OwnerRootGuard| (std::marker::AssertParamIsSend::<OwnerRootGuard>::is_send(), std::marker::AssertParamIsSync::<OwnerRootGuard>::is_sync()); };
RS
# rustc's AssertParamIsSend is not publicly used this way; try the common static bound;
cat >/tmp/owner_root_guard_send_probe.rs <<'RS'
struct OwnerRootGuard {
index: usize,
}
fn assert_send<T: Send>() {}
fn main() {
assert_send::<OwnerRootGuard>();
}
RS
rustc /tmp/owner_root_guard_send_probe.rs -o /tmp/owner_root_guard_send_probe
/tmp/owner_root_guard_send_probe
else
echo "rustc not available"
fiRepository: youknowone/pyre
Length of output: 11757
Make OwnerRootGuard thread-bound so it cannot be dropped on another thread.
OwnerRootGuard currently stores only index: usize, so it is auto-Send/Sync. If a guard is moved to another thread, its destructor calls release_owner_root via the foreign thread’s OWNER_ROOTS, panicking on an invalid index or releasing a live root from a different GC thread. Add a non-Send PhantomData<*mut ()> field to keep the guard tied to the acquiring thread’s TLS slot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-gc/src/shadow_stack.rs` around lines 484 - 505, Update
OwnerRootGuard to include a PhantomData<*mut ()> marker field, making the guard
non-Send/non-Sync and bound to the acquiring thread’s OWNER_ROOTS TLS.
Initialize this marker in OwnerRootGuard::new while preserving index acquisition
and Drop::drop behavior.
| /// The descriptor `enable` resolved from its `file` argument. `handler.py` | ||
| /// keeps it on the Handler instance, but the signal handler below is a bare | ||
| /// `extern "C" fn` and cannot capture, so it is handed over through a static. | ||
| /// Defaults to 2, which is what `get_fileno_and_file` answers for None. | ||
| #[cfg(all(unix, feature = "host_env"))] | ||
| static FAULTHANDLER_FD: std::sync::atomic::AtomicI32 = std::sync::atomic::AtomicI32::new(2); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)handler\.rs$|faulthandler|handler\.py' || true
echo "== handler.rs outline =="
ast-grep outline pyre/pyre-interpreter/src/module/faulthandler/handler.rs --view expanded || true
echo "== handler.rs relevant lines =="
wc -l pyre/pyre-interpreter/src/module/faulthandler/handler.rs
sed -n '1,240p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs
echo "== search usages =="
rg -n "FAULTHANDLER_FD|faulthandler|Handler|enable\\(|disable\\(|get_fileno_and_file|handler" pyre/pyre-interpreter/src/module/faulthandler -SRepository: youknowone/pyre
Length of output: 18258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pypy/faulthandler handler.py relevant methods =="
sed -n '1,180p' pypy/module/faulthandler/handler.py
echo "== CPython faulthandler.c relevant object retention =="
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/python/cpython/main/Modules/faulthandler.c"
try:
text = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
except Exception as e:
print(f"FETCH_FAILED: {e}")
raise SystemExit(0)
for needle in ("fatal_file_obj", "fatal_fileno", "PyDict_SetItemString", "Py_DECREF"):
print(f"\n/// matches {needle}")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start=max(1,i-8); end=min(len(text.splitlines()),i+12)
lines=text.splitlines()
for j in range(start,end+1):
print(f"{j:5d}: {lines[j-1]}")
break
PY
echo "== pypy module cintf =="
sed -n '1,220p' pypy/module/faulthandler/cintf.pyRepository: youknowone/pyre
Length of output: 10978
🌐 Web query:
CPython Modules faulthandler.c fatal_file_obj fatal_fileno Py_DECREF
💡 Result:
In CPython's faulthandler.c, the symbols fatal_file_obj, fatal_fileno, and Py_DECREF relate to the management of file-like objects used for dumping diagnostic information (such as tracebacks) during fatal errors [1][2][3]. Key technical details regarding these components: 1. Handling of File Objects (fatal_file_obj and fatal_fileno): The faulthandler module allows users to specify a file-like object to receive diagnostic output [4][5]. Because fault handlers must be signal-safe—meaning they cannot reliably perform complex operations like memory allocation or object method calls—the module pre-resolves the file descriptor (fd) of the provided file object when faulthandler is enabled or registered [1][2]. Historically, internal structures or logic involved managing these file objects to ensure the file descriptor remained valid [3]. Specifically, the code must track the file object (PyObject) to prevent it from being garbage collected while the handler is active, and store the associated integer file descriptor (fd) for use during a signal crash [1][2]. 2. Role of Py_DECREF: When the faulthandler module is enabled or updated with a new file object, it increments the reference count of the file-like object to keep it alive [1]. When the module is disabled, reconfigured, or when the Python process shuts down, Py_DECREF is used to decrement the reference count of the stored file object, allowing Python's garbage collector to reclaim the object if its reference count drops to zero [1]. 3. Signal Safety Constraints: Because faulthandler must function during a crash (e.g., SIGSEGV), it operates under strict constraints [6]. It cannot invoke high-level Python methods like.fileno or.flush during the signal handler execution because these are not signal-safe [1]. Consequently, the module resolves the file descriptor (via the logic associated with these symbols) in advance, so that during a crash, it can perform direct, signal-safe writes to the integer file descriptor [1][2]. Developers using faulthandler are advised to keep the file object open and alive for the duration of the handler's activation [4][7]. If the file object is closed or the file descriptor is reassigned (e.g., via os.dup2), the handler might inadvertently write data to the wrong destination [5][8][9].
Citations:
- 1: https://github.com/python/cpython/blob/master/Modules/faulthandler.c
- 2: https://chromium.googlesource.com/external/github.com/python/cpython/+/refs/tags/v3.6.10rc1/Modules/faulthandler.c
- 3: https://bugs.python.org/file38436/issue23566_update.patch
- 4: https://runebook.dev/ko/docs/python/library/faulthandler/issue-with-file-descriptors
- 5: https://docs.python.org/es/3.5/library/faulthandler.html
- 6: https://docs.python.org/3/library/faulthandler.html
- 7: https://runebook.dev/ko/docs/python/library/faulthandler/faulthandler.dump_c_stack
- 8: https://github.com/python/python-docs-ja/blob/3.14/library/faulthandler.po
- 9: https://github.com/python/python-docs-es/blob/0ac89529/library/faulthandler.po
Retain the enabled file alongside its descriptor.
PyPy and CPython keep the file object returned by get_fileno_and_file() alive for the lifetime of the fatal handler, while Rust now stores only FAULTHANDLER_FD. If the passed file object becomes unreachable without an explicit owner, its descriptor can close or be reused before disable(), redirecting fatal diagnostics. Keep a rooted file reference or take ownership of a duplicated descriptor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/faulthandler/handler.rs` around lines 20 -
25, Update the faulthandler state around FAULTHANDLER_FD so enabling the handler
retains the file object returned by get_fileno_and_file() for the handler’s
lifetime, or instead duplicates and owns its descriptor. Ensure the retained
resource remains valid until disable() and is released or closed when the
handler is disabled, while preserving the existing default descriptor behavior.
| let fd = | ||
| faulthandler_extract_fd(args.first().copied().unwrap_or(pyre_object::PY_NULL))?; | ||
| #[cfg(all(unix, feature = "host_env"))] | ||
| { | ||
| FAULTHANDLER_FD.store(fd, std::sync::atomic::Ordering::Relaxed); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="pyre/pyre-interpreter/src/module/faulthandler/handler.rs"
if [ -f "$file" ]; then
echo "== file size =="
wc -l "$file"
echo "== relevant region =="
sed -n '1,130p' "$file" | cat -n
else
echo "file not found: $file"
echo "candidate files:"
fd -i 'handler.rs|faulthandler' .
fi
echo "== faulthandler_extract_fd references =="
rg -n "faulthandler_extract_fd|FAULTHANDLER_FD|NotImplementedError|fileno" pyre/pyre-interpreter/src/module/faulthandler handler.rs 2>/dev/null || trueRepository: youknowone/pyre
Length of output: 8632
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lines 160-190 =="
sed -n '160,195p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs | cat -n
echo "== compare references nearby =="
sed -n '150,190p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs | cat -n
echo "== simple cfg-order model for object argument =="
python3 - <<'PY'
from pathlib import Path
src = Path("pyre/pyre-interpreter/src/module/faulthandler/handler.rs").read_text()
enable = src[src.index('pub fn register_module').index('pub fn register_module')+src.index("fn faulthandler_extract_fd")]
print("fd extraction before all cfg branches",
src.index("faulthandler_extract_fd(args.first().copied().unwrap_or(pyre_object::PY_NULL))?") <
src.index("#[cfg(all(unix, feature = \"host_env\"))]"))
print("second fd extraction before cfg branches nearby",
"faulthandler_extract_fd(args.get(1).copied().unwrap_or(pyre_object::PY_NULL))?" in src)
PYRepository: youknowone/pyre
Length of output: 4750
🌐 Web query:
CPython faulthandler enable register file descriptor fileno NotImplementedError unsupported platform
💡 Result:
The NotImplementedError or other errors like RuntimeErrors encountered when using faulthandler.enable or faulthandler.register generally arise from platform-specific limitations or issues with the provided file object, rather than a lack of support for file descriptors themselves [1][2][3]. Since Python 3.5, both faulthandler.enable and faulthandler.register explicitly support passing an integer file descriptor (fd) directly as the file argument, in addition to file-like objects that provide a.fileno method [1][3][4]. Key points regarding these errors include: 1. Platform Availability: The faulthandler.register function is not available on Windows [1][2]. Attempting to call it on Windows will result in an AttributeError (as it is not present in the module) or potentially other errors depending on how it is accessed. 2. Invalid File Descriptor or.fileno: When a file-like object is provided, CPython attempts to call its.fileno method to retrieve the underlying integer file descriptor [5][6]. If the object does not have a.fileno method, or if that method does not return a valid integer file descriptor (e.g., it raises an OSError or returns an invalid value), CPython will raise a RuntimeError: "file.fileno is not a valid file descriptor" [5][7][6]. This often happens in environments where standard streams have been replaced by objects that do not wrap a real OS-level file descriptor (e.g., certain interactive notebook environments or virtualized streams) [7]. 3. Invalid Integer FD: If you pass an integer directly and it is negative or otherwise invalid, CPython will raise a ValueError: "file is not a valid file descriptor" [5][6]. To resolve these issues, ensure that the object passed to faulthandler is either a valid integer file descriptor or a proper file-like object that successfully returns a valid file descriptor via.fileno [5][3]. In restricted environments, you may need to pass a fallback stream, such as sys.stderr [7].
Citations:
- 1: https://docs.python.org/3/library/faulthandler.html
- 2: https://github.com/python/cpython/blob/main/Doc/library/faulthandler.rst
- 3: https://docs.python.org/3.11/library/faulthandler.html
- 4: https://github.com/python/cpython/blob/3.11/Doc/library/faulthandler.rst
- 5: https://github.com/python/cpython/blob/main/Modules/faulthandler.c
- 6: https://github.com/python/cpython/blob/master/Modules/faulthandler.c
- 7: faulthandler.enable() fails with IOError ipython/ipykernel#91
Keep descriptor extraction behind the host_env configuration gate.
faulthandler_extract_fd(...) runs before unsupported builds can return the intended NotImplementedError. For non-integer, non-None file arguments it can execute a user-provided fileno() and surface its exception or side effects. Move the extraction into each host_env branch for lines 71-94 and 183.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/faulthandler/handler.rs` around lines 71 -
75, Move the faulthandler_extract_fd call out of the unconditional path and into
the applicable host_env-gated branches around the fd setup logic at lines 71–94
and 183. Ensure unsupported configurations return NotImplementedError before
evaluating or invoking file descriptor extraction, while host_env builds retain
the existing extraction and storage behavior.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate handler.rs =="
fd -a 'handler\.rs$' . | sed 's#^\./##'
echo "== relevant source slices =="
for f in $(fd 'handler\.rs$' .); do
echo "--- $f ($(wc -l < "$f") lines) ---"
sed -n '1,140p' "$f" | nl -ba
done
echo "== search faulthandler_extract_fd definitions/usages =="
rg -n "faulthandler_extract_fd|FAULTHANDLER_FD|enable\\(-|fileno|PY_NEW" -S .Repository: youknowone/pyre
Length of output: 402
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== find upstream cpython faulthandler references in repo =="
rg -n "Modules/faulthandler|faulthandler\\.c|enable\\(-|fileno|invalid_fd|FD_SET|sig*.*signal|write\\(" -S . || true
echo "== git tracked files around likely cpython source copies =="
git ls-files | rg -i 'faulthandler|cpython|handler' || trueRepository: youknowone/pyre
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== handler.rs lines 1-120 =="
sed -n '1,135p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs
echo "== handler.rs exact symbols context =="
rg -n "fn faulthandler_extract_fd|pub fn faulthandler_extract_fd|FAULTHANDLER_FD|enable|fileno|first\\(\\)" pyre/pyre-interpreter/src/module/faulthandler/handler.rs
echo "== pyre faulthandler module references =="
rg -n "faulthandler_extract_fd|FaultHandler|enable|fileno|PY_NEW" lib-python pyre -S --glob '*.py' --glob '*.rs' || true
echo "== cpython faulthandler.c enable excerpt if present =="
if [ -f pypy/module/faulthandler/faulthandler.c ]; then
sed -n '1,230p' pypy/module/faulthandler/faulthandler.c
fiRepository: youknowone/pyre
Length of output: 50372
🌐 Web query:
CPython faulthandler.c enable invalid file descriptor fd < 0 fileno() returns invalid
💡 Result:
In CPython's faulthandler module, the function faulthandler_get_fileno in Modules/faulthandler.c is responsible for resolving file-like objects or integers to a valid file descriptor (fd) [1][2]. It enforces strict validation to ensure that any fd provided—either directly as an integer or returned by a fileno method—is a valid, non-negative file descriptor [1][2]. When a file descriptor is passed or resolved, the implementation checks if the value is less than 0 [1][2]. If this condition is met, it explicitly raises a Python exception to prevent the use of an invalid descriptor [1][2]: 1. If an integer is passed directly to functions like faulthandler.enable, and it is negative (fd < 0), the module raises a ValueError: "file is not a valid file descriptor" [1][2]. 2. If the module calls the.fileno method of a file-like object and that method returns a negative value (fd < 0), it raises a RuntimeError: "file.fileno is not a valid file descriptor" [1][2]. These checks were implemented to ensure the reliability of the fault handler, which must operate in signal-safe conditions where it cannot safely recover from I/O errors caused by invalid descriptors [1][3][4]. The module also includes specific handling to ensure that if sys.stderr is None (which can occur in certain environments like pythonw.exe), it raises a RuntimeError rather than attempting to access a non-existent fileno method, ensuring a clear error path instead of a crash [5][6].
Citations:
- 1: https://github.com/python/cpython/blob/main/Modules/faulthandler.c
- 2: https://github.com/python/cpython/blob/master/Modules/faulthandler.c
- 3: https://docs.python.org/3/library/faulthandler.html
- 4: https://github.com/vstinner/faulthandler
- 5: faulthandler should handle sys.stderr being None gracefully python/cpython#65696
- 6: https://bugs.python.org/issue21497
Validate fileno() results before publishing them.
The direct-integer path rejects negative file descriptors, but the fileno() case returns the cast value unchanged. A custom fileno() returning -1 lets enable() store FAULTHANDLER_FD and register the signal handler with an invalid descriptor. Preserve upstream semantics and raise before FAULTHANDLER_FD.store; for fileno() results this is a RuntimeError like file.fileno is not a valid file descriptor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/faulthandler/handler.rs` around lines 71 -
75, Validate the descriptor returned by faulthandler_extract_fd before the
FAULTHANDLER_FD.store call, including values produced through fileno(), and
raise a RuntimeError with the expected invalid-file-descriptor message for
negative results. Preserve valid direct-integer and fileno() behavior, ensuring
no invalid descriptor reaches signal-handler registration.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipecd
# Locate the file and inspect the relevant function plus surrounding context.
if [ -f pyre/pyre-interpreter/src/module/faulthandler/handler.rs ]; then
wc -l pyre/pyre-interpreter/src/module/faulthandler/handler.rs
sed -n '1,140p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs
else
echo "target file not found"
fd -a handler.rs .
fi
# Search for faulthandler-related code and helper definitions/usages.
rg -n "faulthandler_extract_fd|FAULTHANDLER_FD|register|dump|install|fileno|flush" pyre/pyre-interpreter/src/module/faulthandler || trueRepository: youknowone/pyre
Length of output: 200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the file and inspect the relevant function plus surrounding context.
if [ -f pyre/pyre-interpreter/src/module/faulthandler/handler.rs ]; then
echo "== handler.rs line count and relevant section =="
wc -l pyre/pyre-interpreter/src/module/faulthandler/handler.rs
sed -n '1,140p' pyre/pyre-interpreter/src/module/faulthandler/handler.rs
else
echo "target file not found"
fd -a handler.rs . || true
fi
# Search for faulthandler-related code and helper definitions/usages.
rg -n "faulthandler_extract_fd|FAULTHANDLER_FD|register|dump|install|fileno|flush" pyre/pyre-interpreter/src/module/faulthandler || trueRepository: youknowone/pyre
Length of output: 11207
🌐 Web query:
CPython faulthandler.c fatal handler setup flush file object before installing fatal signal handlers
💡 Result:
In CPython, the faulthandler module is designed to dump Python tracebacks upon receiving fatal signals (such as SIGSEGV, SIGFPE, SIGABRT, SIGBUS, and SIGILL) [1][2]. The implementation in Modules/faulthandler.c includes a helper function, faulthandler_get_fileno, which is used to retrieve the file descriptor for the output file [3][4]. Before installing the fatal signal handlers, faulthandler_get_fileno performs the following steps: 1. It retrieves the target file object (defaulting to sys.stderr if None or NULL is provided) [3][5]. 2. It validates that the file has a valid file descriptor by calling its fileno method [3][4]. 3. Critically, it calls _PyFile_Flush(file) to ensure that any pending output in the file's buffer is written out [3][4]. This step is important because if a fatal crash occurs, the process might be terminated immediately or enter an inconsistent state, and unflushed buffers could be lost [2]. By flushing the file object during the setup phase—specifically when enabling the handler via faulthandler.enable or registering a signal—CPython attempts to ensure that the file state is consistent before the signal handlers take over and potentially perform raw writes (using low-level file descriptors) to dump tracebacks [3][4][2]. The module warns that users should keep the file open until the fault handler is disabled, as the module stores and uses the file descriptor directly [1][2].
Citations:
- 1: https://github.com/python/cpython/blob/main/Doc/library/faulthandler.rst
- 2: https://docs.python.org/3/library/faulthandler.html
- 3: https://github.com/python/cpython/blob/main/Modules/faulthandler.c
- 4: https://github.com/python/cpython/blob/36e4ffc1/Modules/faulthandler.c
- 5: https://github.com/python/cpython/blob/b35c3791/Modules/faulthandler.c
🌐 Web query:
CPython Modules/faulthandler.c faulthandler_get_fileno flush stderr _PyFile_Flush
💡 Result:
In CPython's faulthandler module (located in Modules/faulthandler.c), the function faulthandler_get_fileno is a static helper used to retrieve a file descriptor from a provided file-like object [1][2]. The function's primary responsibilities and logic are as follows: 1. Input Handling: It accepts a pointer to a PyObject (file_ptr). If the provided file is NULL or Py_None, it defaults to using sys.stderr [1][3]. If an integer is passed, it treats it directly as a file descriptor [1][2]. 2. Fileno Retrieval: If a file object is provided, it calls the object's fileno method (via PyObject_CallMethodNoArgs) to obtain the underlying integer file descriptor [1][2]. 3. Flushing: After successfully obtaining the file descriptor, it attempts to flush the file using the internal function _PyFile_Flush(file) [1][2]. 4. Error Handling: - If _PyFile_Flush fails, the function ignores the error, clears the Python error indicator using PyErr_Clear, and proceeds to return the file descriptor [1][2]. This ensures that the faulthandler can continue to dump information even if the flush operation fails. - If the fileno call fails or returns an invalid descriptor, the function sets an appropriate exception and returns -1 [1][2]. The internal function _PyFile_Flush(file) is defined in CPython's internal file utilities (Include/internal/pycore_fileutils.h) and implemented in Objects/fileobject.c [4][5]. It provides a convenient, standardized C API to call the flush method of a file object, returning -1 on error and 0 on success [4][6]. This utility was introduced to replace repetitive boilerplate code used throughout the CPython codebase to flush files [6].
Citations:
- 1: https://github.com/python/cpython/blob/main/Modules/faulthandler.c
- 2: https://github.com/python/cpython/blob/b35c3791/Modules/faulthandler.c
- 3: https://github.com/python/cpython/blob/36e4ffc1/Modules/faulthandler.c
- 4: https://github.com/python/cpython/blob/main/Objects/fileobject.c
- 5: https://github.com/python/cpython/blob/master/Include/internal/pycore_fileutils.h
- 6: Add convenient C API for flushing a file python/cpython#109611
Flush the target file before enabling fatal handlers.
faulthandler_extract_fd() resolves the file descriptor, but buffered output may still remain unwritten when fatal handlers install before normal cleanup. Flush the resolved file object before installing the handlers, matching the upstream fatal-handler setup semantics.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/faulthandler/handler.rs` around lines 71 -
75, Update the faulthandler setup around faulthandler_extract_fd to flush the
resolved target file object before storing the descriptor or enabling fatal
handlers. Reuse the resolved file object and its existing flush behavior,
ensuring buffered output is written before handler installation while preserving
the current descriptor handling.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit d1c5c97). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
2e74911 to
6879f66
Compare
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/6879f6682c6e54bf979ad612c31efe9a80a07dcb/pyre-interpreter/src/module/faulthandler/handler.rs#L75
When enable(file=...) receives an integer such as 2**32 + 1, faulthandler_extract_fd casts the i64 to i32 before validating it, so the value wraps to fd 1 and is now persisted here; a fatal report can consequently be written to an unrelated descriptor instead of raising OverflowError. PyPy's Handler.get_fileno_and_file uses space.c_int_w, and this repository already provides baseobjspace::c_int_w with the required 32-bit range check, so use that conversion before storing the fd.
AGENTS.md reference: AGENTS.md:L194-L196
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The empty-needle assertions in apptest_unicode.py:815 all use ASCII
payloads, where a byte count and a code point count coincide. Add the
non-ASCII cases, plus the identities count('') == len(s) + 1 and
count('') == rfind('') - find('') + 1.
pypy 7.3.22 fails the first added assertion: '\xe9\xe8\xe9\xe8\xe9'.count('')
is 11 there, the byte length plus one, because descr_count
(unicodeobject.py:1116) passes the byte bounds from
_unwrap_and_compute_idx_params into rstring.py:836, which answers
end - start + 1 for an empty needle. CPython 3.14 answers 6.
Assisted-by: Claude
descr_count passed the byte bounds from _unwrap_and_compute_idx_params
straight to rstring's search, which answers end - start + 1 for an empty
needle, so '\xe9\xe8\xe9\xe8\xe9'.count('') was 11 rather than 6. Answer
it from _codepoints_in_utf8 instead, with start_index > end_index giving
0 as rstring.py:827 does, and the whole-string case reading self._len()
so the call stays O(1) -- rutf8.codepoints_in_utf8 is a linear scan.
end_index is clipped to len(value) first. Here it never exceeds that, but
the same code on pypy main reaches descr_count with end_index one past
the end, which is how that branch signals an out of range start; without
the clip 'aaa'.count('', 10) answers 1 instead of 0.
Assisted-by: Claude
…on stack-critical opimpl_recursive_call (pyjitpl.py:1390-1416) bounds recursive inlining with a single check: count the portal frames already on the framestack, and inline while that count is below max_unroll_recursion (default 7). pyre applied that check via fbw_inline_recursion_count / FBW_MAX_INLINE_RECURSION and then added two conditions upstream does not have -- a framestack.len() bound against a separate fbw_max_rec_unroll_depth knob (default 1), and a framestack.is_empty() requirement on bridge admission. Together they made max_unroll_recursion unreachable for a bridge-served recursion. Drop both, along with fbw_max_rec_unroll_depth and PYRE_FBW_REC_UNROLL_DEPTH. compile_and_run_once (the warmstate.py:425-444 bound_reached analogue) traces, runs the backend compiler, and enters the code it just produced on one native stack. Cranelift's compiler measures ~1.2 MB against the 768 KB MAX_STACK_SIZE budget, so the entered code's prologue probe reported an overflow at a Python call depth of 2 (synth/wasm_ca_trampoline_decline). Mark the region with CriticalCodeGuard, the report_error = 0 window of stack.h:42-43 that compile.py:976, pyjitpl.py:3305 and resume.py:1318 already use. Assisted-by: Claude
… lifetime logging - pyframe: FrameBox holds an OwnerRootGuard so the frame stays a shadow-stack root between FrameBox::new and execute_frame installing it on the ExecutionContext chain. - shadow_stack: add OWNER_ROOTS, a fixed-slot root vector with acquire/release and an RAII OwnerRootGuard, registered per mutator for the STW root walk. - tupleobject: pin the freshly allocated tuple header across the items-block allocation and the write barrier, reading it back from the shadow stack. - _io/textio: POSIX stdio takes newline='\n' (app_main.py create_stdio:494), so readuniversal / readtranslate are Windows-only. - faulthandler: write the fatal-error line to the descriptor enable was given, carried to the bare extern "C" handler through a static. - jitcode_dispatch: a non-root header that requested a retrace no longer returns before merge-point registration (compile.py:1079 retrace_needed). - majit-gc: MAJIT_GC_LIFETIME_LOG-gated logging of remembered-set adds and old-gen frees; trace_and_update_object takes the call site name. - codewriter/call: expand the comment on indirect-call family resolution. Assisted-by: Claude
The five gates on MAJIT_GC_LIFETIME_LOG called std::env::var_os directly. One of them sits in the write barrier and two more in the old-gen sweep, so every remembered-set add and every free took the environment lock and scanned it linearly whether or not the variable was set. Add gc_lifetime_log_enabled(), a LazyLock<bool> in the shape of majit_metainterp::majit_log_enabled and gc_stress_enabled, and route all five sites through it. Interleaved A/B, min of 5 rounds, on check.py's exec metric: synth/const_arg_call_resume 0.171s -> 0.103s synth/calls_closures 0.758s -> 0.635s synth/class_attrs_methods 0.908s -> 0.774s check.py --backend dynasm goes from 341/342 to 342/342; const_arg_call_resume was the one failure at 34.5x against the 30x gate and now measures 20.9x. Assisted-by: Claude
allocate_stdio filled the TextIOWrapper payload field by field and never ran the codec half of __init__, so sys.stdin/stdout/stderr carried a null w_decoder. make_std_stream covers that with instance-dict overrides for write, readline and flush, so sys.stdin.readline() worked while sys.stdin.read(), sys.stdin.readlines() and iteration reached the real methods and raised io.UnsupportedOperation: not readable. A hand built _io.TextIOWrapper(sys.stdin.buffer) read the same descriptor fine. Move the tail of __init__ into attach_buffer and run it from allocate_stdio too, so a standard stream reaches the state a constructed wrapper does rather than the fields a struct literal can spell -- readuniversal and readtranslate were being derived by hand from the newline the literal chose. The codec cannot be looked up there: it imports encodings, whose own import sys re-enters the sys module creation that allocate_stdio runs inside and hands the import system a second, half built module, which breaks the importlib bootstrap with KeyError: 'importlib._bootstrap'. pylifecycle.c runs init_sys_streams after init_importlib for that reason, so add init_stream_codecs at the same point and have it attach the codec to the __std*__ aliases, skipping anything that is not a stream allocate_stdio built. read(), readlines() and iteration over sys.stdin now agree with CPython 3.14 and PyPy 3.11, including '\r\n' left untranslated with newlines None. A differential probe over _io.TextIOWrapper construction, newline modes, seek and reinitialization reports no behavior change. Assisted-by: Claude
The tracer executes each residual concretely, so the `next()` that ends
the drain raises for real and the shared `bh_*` helper publishes it into
both `BH_LAST_EXC_VALUE` and the backend `_store_exception` cells
(`publish_residual_call_exception`). `drive_unpack_iterable_trace`
consumed that exception on the metainterp side only — the walk's
`TraceAction::Finish { exit_with_exception }` arm drops the loop-exit
`StopIteration` so the caller re-derives its own, and parks anything
else — leaving the backend cells set when the hook returned.
The next compiled trace's `must_save_exception` guard stages those cells
into `jf_guard_exc`, so `grab_exc_value` handed the stale value to the
blackhole, which unwound it to `ExitFrameWithExceptionRef`: the call
returned NULL with no error raised, surfacing later as a `StopIteration`
out of an unrelated operator. Under `PYRE_JD1=1`, a `list.sort` keyed
by a fresh `functools.cmp_to_key` class failed on round 48 with
`StopIteration` from `(x > y)` on two `str`, and `test_io.py` died at
load.
Call `drain_backend_jit_exc()` in that finish arm and in the live-enter
path's compiled-side-slot clear, matching `blackhole.rs route_to_catch`,
which drains after catching in frame.
Assisted-by: Claude
`opimpl_recursive_call` (`pyjitpl.py:1390-1416`) makes the inline decision from the portal-frame count alone, which is why the `framestack.len()` and `framestack.is_empty()` conditions on `root_bridge` were dropped. The `inline_subwalk` condition dropped with them has no upstream counterpart for a different reason: `perform_call` pushes onto `MetaInterp.framestack` and returns to the single `interpret()` loop, so upstream is never inside one walk while starting another and cannot seed a multiframe snapshot from a sub-walk. Seeding one there makes the wasm bridge replay re-execute the sub-walk's body: `synth/ca_bridge_multiframe_resume_double_call` and `synth/recursion_memo_branch` produced wrong output and `fib_recursive` timed out, while dynasm and cranelift stayed green. Restore the condition. check.py: dynasm 343/343, wasm 340/340. Assisted-by: Claude
6879f66 to
d1c5c97
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1c5c975a3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if unsafe { (*hdr).type_id() } == 9 { | ||
| eprintln!("{}", std::backtrace::Backtrace::force_capture()); | ||
| } |
There was a problem hiding this comment.
When MAJIT_GC_LIFETIME_LOG is enabled and type-9 arrays receive write barriers—as frame-locals arrays routinely do—this captures and synchronously prints a full stack trace for every event. That can produce enormous logs and heavily perturb or stall the GC/JIT experiments the lifetime logger is intended to diagnose; remove this leftover type-specific scaffold or place it behind a separate explicit backtrace option.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
pyre/pyre-interpreter/src/module/_io/textio.rs (1)
958-970: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConvert boolean arguments before codec lookup.
lookup_text_codec()may importencodings, while Lines 969-970 may execute user-defined__bool__and raise. A failing boolean conversion therefore performs an observable import before argument conversion completes. Move the truth tests before Line 963.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/module/_io/textio.rs` around lines 958 - 970, In the constructor flow, move the is_true conversions for line_buffering and write_through before lookup_text_codec, while preserving their existing error propagation and Argument Clinic truth-testing behavior. Ensure boolean argument conversion completes before any codec lookup or encodings import occurs; keep the encoding, errors, and newline validation semantics unchanged.pyre/pyre-jit/src/eval.rs (1)
8598-8639: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
CriticalCodeGuardstays armed far past the intended compile step — can mask a real stack overflow.
_cc_guard(line 8608) is a local oftry_function_entry_jit, and thematch compile_and_run_once(...) { ... }block that follows is the function's tail expression. Rust drops locals only after the tail expression finishes evaluating, so_cc_guard— which suppresses stack-overflow reporting per its documented Drop contract — stays alive through the entireContinueRunningNormally => Some(handle_jitexception(frame_root.frame()))arm as well.handle_jitexceptionruns the ordinary interpreter loop (eval_loop_jitin a loop) until the whole call completes, which can be an arbitrarily large amount of subsequent bytecode/recursion — well beyond the "compile_and_run_once native-compiler-frame" window the adjacent comment describes. A genuine deep recursion occurring during that extended window would have its stack-overflow check suppressed until the guard finally drops, risking a hard crash instead of a gracefulRecursionError.Scope the guard tightly around just the
compile_and_run_oncecall so it is dropped before falling intohandle_jitexception.🛑 Proposed fix: narrow the critical-code region
- let _cc_guard = majit_metainterp::CriticalCodeGuard::enter(); let env = PyreEnv; if majit_metainterp::majit_log_enabled() { eprintln!( "[jit][func-entry] start tracing key={} arg0={:?}", green_key, debug_first_arg_int(frame_root.frame()), ); } let next_instr = frame_root.frame().next_instr(); - match compile_and_run_once( - frame_root.frame(), - green_key, - next_instr, - CompileOnceStart::FunctionEntry, - driver, - info, - &env, - ) { + let compile_outcome = { + let _cc_guard = majit_metainterp::CriticalCodeGuard::enter(); + compile_and_run_once( + frame_root.frame(), + green_key, + next_instr, + CompileOnceStart::FunctionEntry, + driver, + info, + &env, + ) + }; + match compile_outcome { Some(LoopResult::Done(result)) => Some(result), Some(LoopResult::ExitFrameWithException(err)) => Some(Err(err)), Some(LoopResult::ContinueRunningNormally) => { Some(handle_jitexception(frame_root.frame())) } None => None, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit/src/eval.rs` around lines 8598 - 8639, Limit the lifetime of _cc_guard in try_function_entry_jit to only the compile_and_run_once invocation by placing the guard and call inside a nested scope or equivalent temporary expression. Ensure the guard is dropped before the ContinueRunningNormally arm invokes handle_jitexception, while preserving the existing match result handling.pyre/pyre-interpreter/src/pyframe.rs (1)
3795-3839: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
_locals_owner_rootis created but never re-read;locals_cells_stack_w/arrstay pointed at the pre-guard address.The comment states the guard exists because
w_cell_new, the barrier, "or any other GC operation" can park this free-threaded mutator while a foreign collector runs. But unlike every other new owner-root site in this PR (FrameBox::new's 10 field guards,tupleobject.rs'sraw_slot), this code never calls_locals_owner_root.get()afterward:
let arr = unsafe { &mut *locals_cells_stack_w };is built from the pre-guard pointer, then written to across thew_cell_new/w_tuple_getitemloop.remember_frame_locals_array(locals_cells_stack_w)(line 3839) and thePyFrame { locals_cells_stack_w, .. }construction (line 3854) both reuse the same stale local.If
alloc_frame_locals_arraycan ever place its result in the movable nursery, a foreign-thread minor collection during this window would relocate the array whilearr/locals_cells_stack_wkeep pointing at the old (soon-reset/reused) nursery bytes — corrupting whatever new allocation later reuses that memory, and registering a dangling address into the remembered set viaremember_frame_locals_array.Please confirm
alloc_frame_locals_arrayunder everyFrameLocalsArrayAllocationvariant used here is guaranteed non-moving (old-gen/std-alloc only); if any variant can be nursery-resident, re-derive the pointer from_locals_owner_root.get()after the population block (mirroring theraw_slotpattern intupleobject.rs) beforeremember_frame_locals_arrayand thePyFrameconstruction.#!/bin/bash set -euo pipefail echo "== FrameLocalsArrayAllocation definition ==" rg -n "enum FrameLocalsArrayAllocation" -A 15 pyre/pyre-interpreter/src/pyframe.rs echo "== alloc_frame_locals_array definition ==" ast-grep run --pattern 'fn alloc_frame_locals_array($$$) { $$$ }' --lang rust pyre/pyre-interpreter/src/pyframe.rs echo "== all usages/variants ==" rg -n "alloc_frame_locals_array|FrameLocalsArrayAllocation::" pyre/pyre-interpreter/src/pyframe.rs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-interpreter/src/pyframe.rs` around lines 3795 - 3839, Verify that every FrameLocalsArrayAllocation path used by alloc_frame_locals_array is guaranteed non-moving; if any can return nursery memory, re-derive locals_cells_stack_w from _locals_owner_root.get() after the population block and use the refreshed pointer for remember_frame_locals_array and PyFrame construction. Also ensure arr is only used before any potentially moving GC operation, preserving the existing owner-root protection.majit/majit-translate/src/codewriter/call.rs (1)
3351-3357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAn empty
graphsfamily now resolves to the builtin-wrapper set regardless of the call site.
Some(graphs) if graphs.is_empty()keys on "family resolved to nothing", not on "this is theBuiltinCode.funcPBC site", so any post-loweringIndirectCallthat ended up with an empty family pulls every__pyre_wrap_*graph into the BFS. That only widenscandidate_graphs, but it also makes the seeding unfalsifiable. Consider carrying an explicit marker on the lowered op (or asserting the site's descr/trait root) instead of overloading emptiness.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-translate/src/codewriter/call.rs` around lines 3351 - 3357, Update the OpKind::IndirectCall handling in the call-graph traversal so builtin-wrapper graphs are selected only for the specific BuiltinCode.func PBC site, not merely when graphs is empty. Carry an explicit lowered-op marker or validate the site’s descriptor/trait root, while preserving normal cloning for nonempty families and skipping unresolved None values.pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs (1)
2600-2611: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
foriter_dirty_boundis unreachable, and the PopJumpIfNone bypass is a tautology.In
try_walker_inline_user_call(Lines 1613‑1627)bound_methodis only built when!method_form, and it setsmethod_form = truein the same branch; every other caller passesbound_method: None. Sobound_method.is_some() && !method_form(Line 2605) can never hold — theDirtyFOR_ITER admission for stored bound methods is dead, andif foriter_dirty_bound && !try_multiframe(Line 2752) never fires. By the same reasoningbound_method.is_none() || method_format Line 3087 is alwaystrue, so the new PopJumpIfNone gate never skips the scan.If the intent is "reached as a stored bound method rather than through LOAD_METHOD", that needs a distinct flag (e.g. keep
method_formfalse for the bound-method path, or trackvia_stored_method: bool) instead of thebound_method/method_formpair.Also applies to: 3087-3103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs` around lines 2600 - 2611, The stored-bound-method path is indistinguishable from the LOAD_METHOD path, making foriter_dirty_bound unreachable and the PopJumpIfNone gate in try_walker_inline_user_call a tautology. Introduce or preserve a distinct indicator for calls reached through a stored bound method, then use it in the CalleeReplaySafety::Dirty admission and the PopJumpIfNone scan condition so both paths can behave differently as intended.
♻️ Duplicate comments (2)
majit/majit-gc/src/shadow_stack.rs (2)
484-505: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
OwnerRootGuardstill auto-Send/Sync; unresolved from prior review.
OwnerRootGuard { index: usize }has noPhantomData<*mut ()>marker, so it can still be moved to a different thread. ItsDropreleasesindexvia that other thread'sOWNER_ROOTS, which either panics on an out-of-range/inactive index or — if that index happens to be occupied on the foreign thread — silently clears a different live root, letting the GC reclaim an unrelated, still-referenced object.FrameBox(pyre-interpreter/src/pyframe.rs) now storesOption<OwnerRootGuard>on every frame, making this the primary consumer of the unguarded type in a codebase that is explicitly moving toward free-threaded execution.🔒️ Suggested fix
pub struct OwnerRootGuard { index: usize, + _not_send: std::marker::PhantomData<*mut ()>, } impl OwnerRootGuard { pub fn new(root: GcRef) -> Self { Self { index: acquire_owner_root(root), + _not_send: std::marker::PhantomData, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-gc/src/shadow_stack.rs` around lines 484 - 505, Make OwnerRootGuard explicitly !Send and !Sync by adding a PhantomData<*mut ()> marker to the struct and initializing it in OwnerRootGuard::new. Preserve the existing index acquisition, get, and Drop behavior while ensuring the guard cannot cross thread boundaries.
453-466: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winFree-slot scan is still O(depth) per acquire, and
FrameBox::newnow amplifies it.
acquire_owner_rootstill linearly scansroots.iter().position(Option::is_none). This was already flagged as a performance concern; it is now materially worse becauseFrameBox::new(pyre-interpreter/src/pyframe.rs) acquires 10 owner-root slots per call, so deep non-tail recursion (e.g.fib_recursive, explicitly called out in the PR's verification notes as having "No performance claim") pays an O(live-frames × 10) scan per frame, i.e. quadratic in recursion depth overall.♻️ Suggested free-list fix
+thread_local! { + static OWNER_ROOTS_FREE: RefCell<Vec<usize>> = const { RefCell::new(Vec::new()) }; +} + pub fn acquire_owner_root(root: GcRef) -> usize { OWNER_ROOTS.with(|roots| { let mut roots = roots.borrow_mut(); - if let Some(index) = roots.iter().position(Option::is_none) { - roots[index] = Some(root); - index - } else { - let index = roots.len(); - roots.push(Some(root)); - index - } + OWNER_ROOTS_FREE.with(|free| { + if let Some(index) = free.borrow_mut().pop() { + roots[index] = Some(root); + index + } else { + let index = roots.len(); + roots.push(Some(root)); + index + } + }) }) } pub fn release_owner_root(index: usize) { let _ = OWNER_ROOTS.try_with(|roots| { let mut roots = roots.borrow_mut(); assert!(index < roots.len() && roots[index].is_some(), "releasing an inactive owner-root slot"); roots[index] = None; - while roots.last().is_some_and(Option::is_none) { - roots.pop(); - } + if index + 1 == roots.len() { + roots.pop(); + } else { + let _ = OWNER_ROOTS_FREE.try_with(|free| free.borrow_mut().push(index)); + } }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@majit/majit-gc/src/shadow_stack.rs` around lines 453 - 466, Replace the linear free-slot search in acquire_owner_root with an explicit free-list or equivalent O(1) reusable-slot structure, updating it whenever owner-root slots are acquired and released. Preserve existing slot reuse and growth behavior, and ensure FrameBox::new’s repeated acquisitions remain constant-time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@majit/majit-gc/src/collector.rs`:
- Around line 3398-3408: Remove the conditional block that checks
(*hdr).type_id() == 9 and calls Backtrace::force_capture() within the GC
lifetime logging path. Keep the general “[gc][remember]” eprintln diagnostic and
its existing fields unchanged.
In `@pyre/pyre-interpreter/src/module/_io/textio.rs`:
- Around line 821-841: Update the initialization flow around the buffer
assignment to invoke the GC write barrier immediately after storing buffer,
before any allocations or callbacks. Also update set_encoder_decoder so
encoder/decoder assignments are published with a write barrier before subsequent
GC-capable work, preserving the existing initialization behavior.
---
Outside diff comments:
In `@majit/majit-translate/src/codewriter/call.rs`:
- Around line 3351-3357: Update the OpKind::IndirectCall handling in the
call-graph traversal so builtin-wrapper graphs are selected only for the
specific BuiltinCode.func PBC site, not merely when graphs is empty. Carry an
explicit lowered-op marker or validate the site’s descriptor/trait root, while
preserving normal cloning for nonempty families and skipping unresolved None
values.
In `@pyre/pyre-interpreter/src/module/_io/textio.rs`:
- Around line 958-970: In the constructor flow, move the is_true conversions for
line_buffering and write_through before lookup_text_codec, while preserving
their existing error propagation and Argument Clinic truth-testing behavior.
Ensure boolean argument conversion completes before any codec lookup or
encodings import occurs; keep the encoding, errors, and newline validation
semantics unchanged.
In `@pyre/pyre-interpreter/src/pyframe.rs`:
- Around line 3795-3839: Verify that every FrameLocalsArrayAllocation path used
by alloc_frame_locals_array is guaranteed non-moving; if any can return nursery
memory, re-derive locals_cells_stack_w from _locals_owner_root.get() after the
population block and use the refreshed pointer for remember_frame_locals_array
and PyFrame construction. Also ensure arr is only used before any potentially
moving GC operation, preserving the existing owner-root protection.
In `@pyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rs`:
- Around line 2600-2611: The stored-bound-method path is indistinguishable from
the LOAD_METHOD path, making foriter_dirty_bound unreachable and the
PopJumpIfNone gate in try_walker_inline_user_call a tautology. Introduce or
preserve a distinct indicator for calls reached through a stored bound method,
then use it in the CalleeReplaySafety::Dirty admission and the PopJumpIfNone
scan condition so both paths can behave differently as intended.
In `@pyre/pyre-jit/src/eval.rs`:
- Around line 8598-8639: Limit the lifetime of _cc_guard in
try_function_entry_jit to only the compile_and_run_once invocation by placing
the guard and call inside a nested scope or equivalent temporary expression.
Ensure the guard is dropped before the ContinueRunningNormally arm invokes
handle_jitexception, while preserving the existing match result handling.
---
Duplicate comments:
In `@majit/majit-gc/src/shadow_stack.rs`:
- Around line 484-505: Make OwnerRootGuard explicitly !Send and !Sync by adding
a PhantomData<*mut ()> marker to the struct and initializing it in
OwnerRootGuard::new. Preserve the existing index acquisition, get, and Drop
behavior while ensuring the guard cannot cross thread boundaries.
- Around line 453-466: Replace the linear free-slot search in acquire_owner_root
with an explicit free-list or equivalent O(1) reusable-slot structure, updating
it whenever owner-root slots are acquired and released. Preserve existing slot
reuse and growth behavior, and ensure FrameBox::new’s repeated acquisitions
remain constant-time.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 50b327f0-67c1-49af-b615-d3bc58a00d02
📒 Files selected for processing (17)
majit/majit-gc/src/collector.rsmajit/majit-gc/src/lib.rsmajit/majit-gc/src/oldgen.rsmajit/majit-gc/src/shadow_stack.rsmajit/majit-translate/src/codewriter/call.rspypy/objspace/std/test/apptest_unicode.pypypy/objspace/std/unicodeobject.pypyre/pyre-interpreter/src/module/_io/textio.rspyre/pyre-interpreter/src/module/faulthandler/handler.rspyre/pyre-interpreter/src/module/sys/vm.rspyre/pyre-interpreter/src/pyframe.rspyre/pyre-jit-trace/src/jitcode_dispatch/fbw_state.rspyre/pyre-jit-trace/src/jitcode_dispatch/inline_call.rspyre/pyre-jit-trace/src/jitcode_dispatch/mod.rspyre/pyre-jit/src/eval.rspyre/pyre-object/src/tupleobject.rspyre/pyrex/src/lib.rs
| if crate::gc_lifetime_log_enabled() { | ||
| eprintln!( | ||
| "[gc][remember] addr={:#x} type_id={} source=write_barrier state={:?}", | ||
| obj.0, | ||
| unsafe { (*hdr).type_id() }, | ||
| self.gc_state | ||
| ); | ||
| if unsafe { (*hdr).type_id() } == 9 { | ||
| eprintln!("{}", std::backtrace::Backtrace::force_capture()); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Remove the hard-coded type_id == 9 debug backtrace before merge.
The PR's own verification notes flag this as leftover debug scaffolding. Hard-coding a bare 9 with no named constant or comment explaining what type that ID refers to makes this unmaintainable, and Backtrace::force_capture() is expensive if anyone flips MAJIT_GC_LIFETIME_LOG on for a different purpose and unexpectedly eats this cost on every remembered-set insertion for that type.
🧹 Suggested cleanup
- if unsafe { (*hdr).type_id() } == 9 {
- eprintln!("{}", std::backtrace::Backtrace::force_capture());
- }
+ // TODO: drop once the investigation this was added for is closed.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if crate::gc_lifetime_log_enabled() { | |
| eprintln!( | |
| "[gc][remember] addr={:#x} type_id={} source=write_barrier state={:?}", | |
| obj.0, | |
| unsafe { (*hdr).type_id() }, | |
| self.gc_state | |
| ); | |
| if unsafe { (*hdr).type_id() } == 9 { | |
| eprintln!("{}", std::backtrace::Backtrace::force_capture()); | |
| } | |
| } | |
| if crate::gc_lifetime_log_enabled() { | |
| eprintln!( | |
| "[gc][remember] addr={:`#x`} type_id={} source=write_barrier state={:?}", | |
| obj.0, | |
| unsafe { (*hdr).type_id() }, | |
| self.gc_state | |
| ); | |
| // TODO: drop once the investigation this was added for is closed. | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@majit/majit-gc/src/collector.rs` around lines 3398 - 3408, Remove the
conditional block that checks (*hdr).type_id() == 9 and calls
Backtrace::force_capture() within the GC lifetime logging path. Keep the general
“[gc][remember]” eprintln diagnostic and its existing fields unchanged.
| self.w_buffer = buffer; | ||
| self.w_encoding = w_str_new(encoding); | ||
| self.w_errors = w_str_new(errors); | ||
| self.w_newline = newline; | ||
| self.line_buffering = line_buffering; | ||
| self.write_through = write_through; | ||
| self.decoded.reset(); | ||
| self.snapshot = None; | ||
| self.pending_bytes = None; | ||
| self.pending_bytes_count = 0; | ||
| self.chunk_size = 8192; | ||
| self.b2cratio = 0.0; | ||
| self.set_newline(newline_value); | ||
| self.has_read1 = crate::baseobjspace::getattr_str(buffer, "read1").is_ok(); | ||
| self.set_encoder_decoder(codec)?; | ||
| self.seekable_flag = | ||
| crate::baseobjspace::is_true(super::call_method_result(buffer, "seekable", &[])?)?; | ||
| self.telling = self.seekable_flag; | ||
| self.state = STATE_OK; | ||
| self.reset_encoder_state(); | ||
| pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Publish the wrapper before further GC-capable work.
Line 821 stores buffer, then Lines 822-840 allocate and invoke callbacks before the first barrier at Line 841. A collection in that window can miss the new old→young edges. Publish immediately after the initial store; likewise ensure encoder/decoder assignments are published before subsequent callbacks in set_encoder_decoder.
Proposed fix
self.w_buffer = buffer;
+ pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8);
self.w_encoding = w_str_new(encoding);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.w_buffer = buffer; | |
| self.w_encoding = w_str_new(encoding); | |
| self.w_errors = w_str_new(errors); | |
| self.w_newline = newline; | |
| self.line_buffering = line_buffering; | |
| self.write_through = write_through; | |
| self.decoded.reset(); | |
| self.snapshot = None; | |
| self.pending_bytes = None; | |
| self.pending_bytes_count = 0; | |
| self.chunk_size = 8192; | |
| self.b2cratio = 0.0; | |
| self.set_newline(newline_value); | |
| self.has_read1 = crate::baseobjspace::getattr_str(buffer, "read1").is_ok(); | |
| self.set_encoder_decoder(codec)?; | |
| self.seekable_flag = | |
| crate::baseobjspace::is_true(super::call_method_result(buffer, "seekable", &[])?)?; | |
| self.telling = self.seekable_flag; | |
| self.state = STATE_OK; | |
| self.reset_encoder_state(); | |
| pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8); | |
| self.w_buffer = buffer; | |
| pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8); | |
| self.w_encoding = w_str_new(encoding); | |
| self.w_errors = w_str_new(errors); | |
| self.w_newline = newline; | |
| self.line_buffering = line_buffering; | |
| self.write_through = write_through; | |
| self.decoded.reset(); | |
| self.snapshot = None; | |
| self.pending_bytes = None; | |
| self.pending_bytes_count = 0; | |
| self.chunk_size = 8192; | |
| self.b2cratio = 0.0; | |
| self.set_newline(newline_value); | |
| self.has_read1 = crate::baseobjspace::getattr_str(buffer, "read1").is_ok(); | |
| self.set_encoder_decoder(codec)?; | |
| self.seekable_flag = | |
| crate::baseobjspace::is_true(super::call_method_result(buffer, "seekable", &[])?)?; | |
| self.telling = self.seekable_flag; | |
| self.state = STATE_OK; | |
| self.reset_encoder_state(); | |
| pyre_object::gc_hook::try_gc_write_barrier(self as *mut Self as *mut u8); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyre/pyre-interpreter/src/module/_io/textio.rs` around lines 821 - 841,
Update the initialization flow around the buffer assignment to invoke the GC
write barrier immediately after storing buffer, before any allocations or
callbacks. Also update set_encoder_decoder so encoder/decoder assignments are
published with a write barrier before subsequent GC-capable work, preserving the
existing initialization behavior.
`W_TextIOWrapper` published its reference fields with one write barrier at the end of `attach_buffer`, but every `w_str_new` in between allocates and `set_newline` / `getattr_str` / `set_encoder_decoder` run Python, so a collection could land after a store and before the barrier that would have told it to trace the wrapper. Barrier after each reference store instead, through a `publish_refs` helper, in `attach_buffer` and in `set_encoder_decoder`. `attach_stdio_codec` dropped both the codec lookup and the incremental encoder/decoder construction errors, leaving a stream that reports itself unreadable however readable its buffer is. Propagate them: `init_stream_codecs` and `init_importlib_bootstrap` now return the failure, with a bootstrap failure still winning as the more fundamental one. `allocate_stdio` still cannot propagate — it runs inside `sys` module creation, before there is an interpreter to raise into — but it no longer discards the result blind: with a null codec the only fallible step `attach_buffer` reaches is `buffer.seekable()`, so finish the two fields that early return skipped rather than leaving them at `Default` and forcing `STATE_OK` over a half-applied init. `faulthandler.enable` resolved `file=None` to a hard-coded fd 2 and dropped the file object. `get_fileno_and_file` (`handler.py:35-49`) resolves the CURRENT `sys.stderr`, asks it for `fileno()`, and flushes it ignoring an ordinary flush error; `enable` then parks it as `fatal_error_w_file` (`handler.py:145`) and `disable` clears it (`:150`), because the descriptor the installed handler writes to belongs to that object. pyre has no Handler instance, so the owner is a process-global slot walked as a GC root alongside the other interpreter globals — not a module attribute, which would show up in `dir(faulthandler)` where upstream has none. `acquire_owner_root` scanned `OWNER_ROOTS` from 0 for a free slot, so rooting the frames of a recursion `depth` deep cost O(depth²). Released interior slots now go on a free stack; a released tail still shrinks the walked range, and the shrink drops the queued indices it took out of range. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) with and without this commit, and passes on the Linux CI runner. cargo test: pyre-interpreter 432, majit-gc 215. Assisted-by: Claude
Drop the `CriticalCodeGuard` from the `bound_reached` compile path (`eval.rs`). It spanned the whole trace-compile-and-dispatch region, so `report_error = 0` covered the walk's residual calls into user code — `stack.c:111` returns that flag in place of the overflow verdict, turning a RecursionError into an unreported native overflow. Upstream marks only regions that run no user code (`compile.py:976` `cpu.force` + `handle_async_forcing`, `pyjitpl.py:3305` `rebuild_state_after_failure`, `resume.py:1318`). Measured on the fixture it was added for, `synth/wasm_ca_trampoline_decline`: with the guard suppressed the overflow no longer occurs at all, so the region is inert. Record why `fbw_inline_recursion_count` matching `w_code` alone is the element-wise greenkey compare of `pyjitpl.py:1396-1401`: `MIFrame.setup` (`pyjitpl.py:74-80`) assigns `greenkey` once, so a frame's greenkey stays its entry greens, and every `InlineFrame` is pushed at a callee entry with the `(w_code, 0)` identity `reconstruct_inline_recipe` also stamps. Write the tuple header before publishing its root (`tupleobject.rs`). `try_gc_alloc_stable_raw` returns uninitialized payload and `tuple_object_custom_trace` reads `ob_header.w_class` and `wrappeditems`, so a collection between `pin_root` and the items-block allocation read garbage. The header now goes in first with a null items block — the state the trace hook returns early on — and the block is stored after. Restore `FAULTHANDLER_FD` when `enable_fatal_handlers` fails; a failed re-enable was redirecting the handlers already installed. Give `OwnerRootGuard` a `PhantomData<*mut ()>`: its `index` names a slot in the acquiring thread's `OWNER_ROOTS`, and a bare `usize` left the guard auto-`Send`. Delete the `type_id == 9` backtrace capture from `remember_young_pointer`. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) 3/3 both with and without this commit, and passes on the Linux CI runner. Assisted-by: Claude
`W_TextIOWrapper` published its reference fields with one write barrier at the end of `attach_buffer`, but every `w_str_new` in between allocates and `set_newline` / `getattr_str` / `set_encoder_decoder` run Python, so a collection could land after a store and before the barrier that would have told it to trace the wrapper. Barrier after each reference store instead, through a `publish_refs` helper, in `attach_buffer` and in `set_encoder_decoder`. `attach_stdio_codec` dropped both the codec lookup and the incremental encoder/decoder construction errors, leaving a stream that reports itself unreadable however readable its buffer is. Propagate them: `init_stream_codecs` and `init_importlib_bootstrap` now return the failure, with a bootstrap failure still winning as the more fundamental one. `allocate_stdio` still cannot propagate — it runs inside `sys` module creation, before there is an interpreter to raise into — but it no longer discards the result blind: with a null codec the only fallible step `attach_buffer` reaches is `buffer.seekable()`, so finish the two fields that early return skipped rather than leaving them at `Default` and forcing `STATE_OK` over a half-applied init. `faulthandler.enable` resolved `file=None` to a hard-coded fd 2 and dropped the file object. `get_fileno_and_file` (`handler.py:35-49`) resolves the CURRENT `sys.stderr`, asks it for `fileno()`, and flushes it ignoring an ordinary flush error; `enable` then parks it as `fatal_error_w_file` (`handler.py:145`) and `disable` clears it (`:150`), because the descriptor the installed handler writes to belongs to that object. pyre has no Handler instance, so the owner is a process-global slot walked as a GC root alongside the other interpreter globals — not a module attribute, which would show up in `dir(faulthandler)` where upstream has none. `acquire_owner_root` scanned `OWNER_ROOTS` from 0 for a free slot, so rooting the frames of a recursion `depth` deep cost O(depth²). Released interior slots now go on a free stack; a released tail still shrinks the walked range, and the shrink drops the queued indices it took out of range. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) with and without this commit, and passes on the Linux CI runner. cargo test: pyre-interpreter 432, majit-gc 215. Assisted-by: Claude
* review: act on the Codex and CodeRabbit findings on #887 Drop the `CriticalCodeGuard` from the `bound_reached` compile path (`eval.rs`). It spanned the whole trace-compile-and-dispatch region, so `report_error = 0` covered the walk's residual calls into user code — `stack.c:111` returns that flag in place of the overflow verdict, turning a RecursionError into an unreported native overflow. Upstream marks only regions that run no user code (`compile.py:976` `cpu.force` + `handle_async_forcing`, `pyjitpl.py:3305` `rebuild_state_after_failure`, `resume.py:1318`). Measured on the fixture it was added for, `synth/wasm_ca_trampoline_decline`: with the guard suppressed the overflow no longer occurs at all, so the region is inert. Record why `fbw_inline_recursion_count` matching `w_code` alone is the element-wise greenkey compare of `pyjitpl.py:1396-1401`: `MIFrame.setup` (`pyjitpl.py:74-80`) assigns `greenkey` once, so a frame's greenkey stays its entry greens, and every `InlineFrame` is pushed at a callee entry with the `(w_code, 0)` identity `reconstruct_inline_recipe` also stamps. Write the tuple header before publishing its root (`tupleobject.rs`). `try_gc_alloc_stable_raw` returns uninitialized payload and `tuple_object_custom_trace` reads `ob_header.w_class` and `wrappeditems`, so a collection between `pin_root` and the items-block allocation read garbage. The header now goes in first with a null items block — the state the trace hook returns early on — and the block is stored after. Restore `FAULTHANDLER_FD` when `enable_fatal_handlers` fails; a failed re-enable was redirecting the handlers already installed. Give `OwnerRootGuard` a `PhantomData<*mut ()>`: its `index` names a slot in the acquiring thread's `OWNER_ROOTS`, and a bare `usize` left the guard auto-`Send`. Delete the `type_id == 9` backtrace capture from `remember_young_pointer`. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) 3/3 both with and without this commit, and passes on the Linux CI runner. Assisted-by: Claude * review: act on the remaining #887 findings `W_TextIOWrapper` published its reference fields with one write barrier at the end of `attach_buffer`, but every `w_str_new` in between allocates and `set_newline` / `getattr_str` / `set_encoder_decoder` run Python, so a collection could land after a store and before the barrier that would have told it to trace the wrapper. Barrier after each reference store instead, through a `publish_refs` helper, in `attach_buffer` and in `set_encoder_decoder`. `attach_stdio_codec` dropped both the codec lookup and the incremental encoder/decoder construction errors, leaving a stream that reports itself unreadable however readable its buffer is. Propagate them: `init_stream_codecs` and `init_importlib_bootstrap` now return the failure, with a bootstrap failure still winning as the more fundamental one. `allocate_stdio` still cannot propagate — it runs inside `sys` module creation, before there is an interpreter to raise into — but it no longer discards the result blind: with a null codec the only fallible step `attach_buffer` reaches is `buffer.seekable()`, so finish the two fields that early return skipped rather than leaving them at `Default` and forcing `STATE_OK` over a half-applied init. `faulthandler.enable` resolved `file=None` to a hard-coded fd 2 and dropped the file object. `get_fileno_and_file` (`handler.py:35-49`) resolves the CURRENT `sys.stderr`, asks it for `fileno()`, and flushes it ignoring an ordinary flush error; `enable` then parks it as `fatal_error_w_file` (`handler.py:145`) and `disable` clears it (`:150`), because the descriptor the installed handler writes to belongs to that object. pyre has no Handler instance, so the owner is a process-global slot walked as a GC root alongside the other interpreter globals — not a module attribute, which would show up in `dir(faulthandler)` where upstream has none. `acquire_owner_root` scanned `OWNER_ROOTS` from 0 for a free slot, so rooting the frames of a recursion `depth` deep cost O(depth²). Released interior slots now go on a free stack; a released tail still shrinks the walked range, and the shrink drops the queued indices it took out of range. check.py: dynasm 345/345, wasm 341/341. cranelift 344/345 — `synth/mutate_then_raise_caught` panics in `collector.rs` (`minor_custom_trace_target`, invalid type_id) with and without this commit, and passes on the Linux CI runner. cargo test: pyre-interpreter 432, majit-gc 215. Assisted-by: Claude * faulthandler: per-signal file owners, invalid-fileno rejection, gated resolution `interp_return_log_enabled` was defined twice in `pyre-interpreter/src/eval.rs` — #874 and #907 each added a copy under the same `cfg`, and neither PR's CI saw the other's, so `main` does not compile (E0428). Delete the later duplicate. `register` discarded the file `get_fileno_and_file` resolved while the installed handler kept only its descriptor, so a `register(sig, f)` whose `f` was later collected left the handler writing through a closed or reused fd. `handler.py:125` keeps `user_w_files[signum] = w_file` and `:132` pops it on `unregister`; hold the same per-signal owners in a process-global table walked by the faulthandler root walker, which now covers both it and the fatal-error file. A `fileno()` returning a negative value reached `FAULTHANDLER_FD` and the handler registration unchecked. Measured: PyPy accepts it (`enable()` succeeds), 3.14 raises `RuntimeError: file.fileno() is not a valid file descriptor` — distinct from the direct-int arm's `ValueError: file is not a valid file descriptor`. Follow 3.14. Resolving the file argument runs a user `fileno()` and `flush()`; `enable` and `register` ran it before the support gate, so a build that can only answer `NotImplementedError` executed those side effects first. Move both inside the gate — which also puts `register`'s integer coercions ahead of the file resolution, as its argument parsing does. Not changed, after measuring: a `SystemExit` raised out of `flush` does NOT abort `enable`. `handler.py:44-48` re-raises asynchronous errors (`error.py:62-65`) and PyPy does propagate it, but 3.14 clears the flush error unconditionally and swallows it; 3.14 is the behaviour target. check.py: dynasm 345/345, cranelift 345/345, wasm 341/341 — all three green; `synth/mutate_then_raise_caught`, which panicked in `collector.rs` (`minor_custom_trace_target`) 3/3 before, now passes 5/5. cargo test pyre-interpreter 438. faulthandler probes (enable/register file retention across GC, unregister, negative fileno, missing fileno, flush errors) match python3.14 line for line. Assisted-by: Claude
Four commits on top of
main.jit: enforce the recursion-unroll bound once, and mark loop compilation stack-criticalopimpl_recursive_call(pyjitpl.py:1390-1416) bounds recursive inlining with a single check: count the portal frames already on the framestack, and inline while that count is belowmax_unroll_recursion(default 7). pyre applied that check viafbw_inline_recursion_count/FBW_MAX_INLINE_RECURSIONand then added two conditions upstream does not have:framestack.len()bound against a separatefbw_max_rec_unroll_depthknob (default 1), andframestack.is_empty()requirement on bridge admission.Together they made
max_unroll_recursionunreachable for a bridge-served recursion, so the knob was inert at every value. Both are dropped, along withfbw_max_rec_unroll_depthandPYRE_FBW_REC_UNROLL_DEPTH.Removing them exposed a second, independent defect.
compile_and_run_once(thewarmstate.py:425-444bound_reachedanalogue) traces, runs the backend compiler, and enters the code it just produced on one native stack. Upstream's backend is a few RPython frames and stays well inside the budget; Cranelift's is an optimizing compiler, measured at ~1.2 MB against the 768 KBMAX_STACK_SIZE, so the entered code's prologue probe reported an overflow at a Python call depth of 2 — a spuriousRecursionErrorinsynth/wasm_ca_trampoline_decline. The region is now marked withCriticalCodeGuard, thereport_error = 0window ofstack.h:42-43thatcompile.py:976,pyjitpl.py:3305andresume.py:1318already use; a real overflow still surfaces once the region exits.Diagnosis notes: the failure is not a leak (capacity is flat at 441 before and after 190 hot-loop iterations, both backends) and not a bad probe (Cranelift's
stack_addrproxy differs from a realcurrent_sp()read by 80 bytes). Guards placed on the walker's inline-push site, on the CA-bridge compile, and around all ofhandle_failchanged nothing, which is what narrowed it to the primary-loop compile path.frame and tuple owner roots, POSIX stdio newline, faulthandler fd, GC lifetime loggingWorking-tree changes that were in flight, collected into one commit at the author's request:
pyframe:FrameBoxholds anOwnerRootGuardso the frame stays a shadow-stack root betweenFrameBox::newandexecute_frameinstalling it on the ExecutionContext chain.shadow_stack:OWNER_ROOTS, a fixed-slot root vector with acquire/release and an RAIIOwnerRootGuard, registered per mutator for the STW root walk.tupleobject: pin the freshly allocated tuple header across the items-block allocation and the write barrier._io/textio: POSIX stdio takesnewline='\n'(app_main.py create_stdio:494), soreaduniversal/readtranslateare Windows-only.faulthandler: write the fatal-error line to the descriptorenablewas given.jitcode_dispatch: a non-root header that requested a retrace no longer returns before merge-point registration (compile.py:1079 retrace_needed).majit-gc:MAJIT_GC_LIFETIME_LOG-gated logging of remembered-set adds and old-gen frees.This commit carries debug scaffolding — the
MAJIT_GC_LIFETIME_LOGlogging includes a hard-codedtype_id == 9backtrace print. Worth stripping before merge.Verification
cargo fmt --all -- --checkclean.cargo test -p pyre-jit-trace -p pyre-jit --features dynasm— 650 passed, 0 failed.check.py: dynasm 340/342, cranelift 340/342.synth/wasm_ca_trampoline_declinepasses on both (it is the fixture the second half of the first commit fixes).synth/str_search_index_boundspanics (compile.py:458 assert i == len(inputargs), 16 != 26) — pre-existing onmain: verified by buildingmain's tree unmodified and reproducing it there. Also reproduced with this branch's JIT commit reverted and with the whole WIP commit reverted.synth/const_arg_call_resumetrips the 30x perf gate at ~40x against a 0.01s pypy denominator, on a machine that was concurrently building.Perf numbers for
fib_recursiveare not being claimed here: the earlier 2.20x → 1.98x-vs-pypy reading was taken on a previous base, and a re-measure on this base landed at 2.26x with pypy and CPython both 13-15% slower in the same run, i.e. under load. It needs a quiet machine before it means anything.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
count('')to count code points (including astral characters) and handle ranged slices properly.enable(file=...)target.Documentation
Tests