posix: argument binding across the module surface, plus a lone-surrogate abort - #1113
Conversation
|
Warning Review limit reached
Next review available in: 5 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
WalkthroughThe PR expands POSIX parity coverage and interpreter behavior. It adds argument validation, path-error reporting, platform APIs, CPU-count handling, descriptor and timestamp fixes, EINTR retries, audit and parser validation, surrogate-safe function names, and CI execution after prior check failures. ChangesPOSIX API binding and path conversion
Platform API support
Descriptor and I/O behavior
Runtime validation
Function name preservation
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit c157a14). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
![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
https://github.com/youknowone/pyre/blob/a0d8d952d808e8be1126c64222bf2b532f91f212/pyre-interpreter/src/module/posix/interp_posix.rs#L5463
Convert the PID with a checked pid_t conversion instead of as. On Linux/Android, pid_t is a 32-bit integer, so a call such as os.sched_setaffinity(2**32, mask) wraps the PID to 0 and modifies the current process rather than raising OverflowError; the same unchecked conversion affects the newly added affinity and scheduling-policy functions.
https://github.com/youknowone/pyre/blob/a0d8d952d808e8be1126c64222bf2b532f91f212/pyre-interpreter/src/module/posix/interp_posix.rs#L5366
Return the wrapped syscall result rather than None on success. The referenced PyPy implementations of both sched_setscheduler and sched_setparam return space.newint(res), while rposix.handle_posix_error returns the successful syscall value (0), so direct callers currently observe a deliberate semantic divergence from the required line-by-line port.
AGENTS.md reference: AGENTS.md:L231-L233
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@pyre/extra_tests/parity_tests/os_path_argument_types.py`:
- Around line 82-110: The expectations for integer path arguments in chdir and
scandir currently rely only on sys.platform; update these conditions to also
require the runtime FD support/build-mode signals used by allow_fd, specifically
HAVE_FCHDIR, HAVE_FDOPENDIR, and the applicable HOST_POSIX/host_env/sandbox
configuration. Ensure Unix builds that reject descriptors use the path-only
error messages.
In `@pyre/extra_tests/parity_tests/os_utime_pathconf_truncate.py`:
- Around line 99-107: Update the os.utime descriptor branch to capture a fresh
reference timestamp immediately before os.utime(fd), then assert both st_mtime
and st_atime from os.stat(p) against that reference, matching the existing
path-branch checks.
In `@pyre/extra_tests/parity_tests/str_lone_surrogate_names.py`:
- Around line 46-56: Update the assertions after f.__name__ = SURR and
types.FunctionType construction to verify identity with SURR using is, while
retaining the existing value checks and messages.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 7192-7213: Update the access implementation around bind_path_args
to retain and inspect all bound modifier keywords instead of discarding them.
Reject any supplied dir_fd, effective_ids, or follow_symlinks value with the
established unavailable-platform error pattern before calling access(2), while
preserving normal behavior when none are provided.
- Around line 1139-1156: Gate the O_FSYNC entry in the flag definitions to only
Unix targets where libc::O_FSYNC exists, or provide the project’s established
zero/default fallback for unsupported targets. Keep the existing O_FSYNC value
unchanged on supported platforms and preserve the surrounding O_* entries.
🪄 Autofix
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: 15994c50-5140-4224-ab87-923212dd2cc5
📒 Files selected for processing (13)
.github/workflows/pyre-ci.ymlpyre/extra_tests/parity_tests/os_arg_binding_surface.pypyre/extra_tests/parity_tests/os_cpu_count.pypyre/extra_tests/parity_tests/os_dup2_inheritable.pypyre/extra_tests/parity_tests/os_path_argument_types.pypyre/extra_tests/parity_tests/os_sched_policy_pipe2.pypyre/extra_tests/parity_tests/os_utime_pathconf_truncate.pypyre/extra_tests/parity_tests/str_lone_surrogate_names.pypyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/sys/vm.rs
| f.__name__ = SURR | ||
| check(f.__name__ == SURR, f"__name__ came back as {f.__name__!r}, not the value set") | ||
| check(isinstance(repr(f), str), "repr of a surrogate-named function is not a str") | ||
|
|
||
| # The qualname slot is separate and must not have been clobbered by the name. | ||
| f.__qualname__ = SURR | ||
| check(f.__qualname__ == SURR, f"__qualname__ came back as {f.__qualname__!r}") | ||
|
|
||
| # ── the constructor arm takes the same name ────────────────────────────── | ||
| g = types.FunctionType(f.__code__, {}, SURR) | ||
| check(g.__name__ == SURR, f"FunctionType name came back as {g.__name__!r}") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the supplied name object identity.
Equality does not detect a reconstructed string object. Add is SURR assertions after both __name__ assignment and types.FunctionType construction. This protects the contract that w_name retains the caller-supplied object.
Proposed test change
f.__name__ = SURR
check(f.__name__ == SURR, f"__name__ came back as {f.__name__!r}, not the value set")
+check(f.__name__ is SURR, "__name__ did not retain the assigned string object")
check(isinstance(repr(f), str), "repr of a surrogate-named function is not a str")
...
g = types.FunctionType(f.__code__, {}, SURR)
check(g.__name__ == SURR, f"FunctionType name came back as {g.__name__!r}")
+check(g.__name__ is SURR, "FunctionType did not retain the supplied string object")📝 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.
| f.__name__ = SURR | |
| check(f.__name__ == SURR, f"__name__ came back as {f.__name__!r}, not the value set") | |
| check(isinstance(repr(f), str), "repr of a surrogate-named function is not a str") | |
| # The qualname slot is separate and must not have been clobbered by the name. | |
| f.__qualname__ = SURR | |
| check(f.__qualname__ == SURR, f"__qualname__ came back as {f.__qualname__!r}") | |
| # ── the constructor arm takes the same name ────────────────────────────── | |
| g = types.FunctionType(f.__code__, {}, SURR) | |
| check(g.__name__ == SURR, f"FunctionType name came back as {g.__name__!r}") | |
| f.__name__ = SURR | |
| check(f.__name__ == SURR, f"__name__ came back as {f.__name__!r}, not the value set") | |
| check(f.__name__ is SURR, "__name__ did not retain the assigned string object") | |
| check(isinstance(repr(f), str), "repr of a surrogate-named function is not a str") | |
| # The qualname slot is separate and must not have been clobbered by the name. | |
| f.__qualname__ = SURR | |
| check(f.__qualname__ == SURR, f"__qualname__ came back as {f.__qualname__!r}") | |
| # ── the constructor arm takes the same name ────────────────────────────── | |
| g = types.FunctionType(f.__code__, {}, SURR) | |
| check(g.__name__ == SURR, f"FunctionType name came back as {g.__name__!r}") | |
| check(g.__name__ is SURR, "FunctionType did not retain the supplied string object") |
🤖 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/extra_tests/parity_tests/str_lone_surrogate_names.py` around lines 46 -
56, Update the assertions after f.__name__ = SURR and types.FunctionType
construction to verify identity with SURR using is, while retaining the existing
value checks and messages.
| #[cfg(unix)] | ||
| ("O_CLOEXEC", libc::O_CLOEXEC as i64), | ||
| // The rest of the `<fcntl.h>` set. Each value is the host header's own, | ||
| // and the split below is the hosts' own too: these six are on every | ||
| // Unix, the next two groups are one platform's each. `nt` has none of | ||
| // them and is left with the flags it does have. | ||
| #[cfg(unix)] | ||
| ("O_ACCMODE", libc::O_ACCMODE as i64), | ||
| #[cfg(unix)] | ||
| ("O_ASYNC", libc::O_ASYNC as i64), | ||
| #[cfg(unix)] | ||
| ("O_DIRECTORY", libc::O_DIRECTORY as i64), | ||
| #[cfg(unix)] | ||
| ("O_FSYNC", libc::O_FSYNC as i64), | ||
| #[cfg(unix)] | ||
| ("O_NOCTTY", libc::O_NOCTTY as i64), | ||
| #[cfg(unix)] | ||
| ("O_NOFOLLOW", libc::O_NOFOLLOW as i64), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
libc crate O_ACCMODE O_ASYNC O_FSYNC O_NOFOLLOW_ANY O_SEARCH which unix targets define
💡 Result:
The constants O_ACCMODE, O_ASYNC, O_FSYNC, O_NOFOLLOW_ANY, and O_SEARCH have varying levels of support and definitions across different Unix targets within the Rust libc crate [1][2]. O_ACCMODE: This constant is widely available across Unix targets in the libc crate [3][4]. It serves as a bitmask for file access mode flags (e.g., O_RDONLY, O_WRONLY, O_RDWR) [5][4]. O_ASYNC: This constant is generally available on most Linux-based targets [3][4]. It is typically associated with asynchronous I/O and is commonly found in libc definitions for platforms like Linux (GNU and other environments) [6][4]. It is often explicitly excluded from certain platforms like AIX, Solaris, Haiku, or Cygwin [3]. O_FSYNC: This constant is less universally defined under this specific name in libc [3]. It is often an alias for O_SYNC on many Linux platforms [7]. In the libc crate, it is available on select Unix-like targets, including various BSD and Linux configurations [3]. O_SEARCH: This constant, defined by POSIX to open a directory for searching, has historically been absent from many systems' libc headers because Linux and other systems often lack native support or use different mechanisms (like O_PATH) [8][9]. Following community efforts, it has been added to the libc crate for various supported platforms [10]. Its presence depends on the specific target's libc implementation [10]. O_NOFOLLOW_ANY: This is not a standard POSIX flag and is not broadly supported across Unix-like targets in the libc crate [11]. It is primarily associated with macOS (Darwin) [11][4]. It is used to prevent following symbolic links throughout an entire path [12]. Because it is a platform-specific flag, it is not consistently exposed in the libc crate for non-macOS targets [11][4]. For specific target compatibility, users should consult the libc crate documentation for the target in question, as definitions are generated based on the underlying system's headers [2][13]. Code relying on these constants should generally use conditional compilation (#[cfg(...)]) if cross-platform support is required [11].
Citations:
- 1: https://docs.rs/libc
- 2: https://crates.io/crates/libc
- 3: https://docs.rs/nix/latest/nix/fcntl/struct.OFlag.html
- 4: https://github.com/astral-sh/ruff/blob/77b9cc56/crates/ty_vendored/vendor/typeshed/stdlib/os/__init__.pyi
- 5: https://rust.docs.kernel.org/src/kernel/fs/file.rs.html
- 6: https://doc.servo.org/libc/unix/linux_like/linux/gnu/b64/x86_64/constant.O_ASYNC.html
- 7: https://codebrowser.dev/linux/include/x86_64-linux-gnu/bits/fcntl-linux.h.html
- 8: https://public-inbox.org/libc-alpha/20130805172658.GK221@brightrain.aerifal.cx/
- 9: https://sourceware.org/legacy-ml/libc-alpha/2013-08/msg00032.html
- 10: Add O_SEARCH to all platforms supported by libc crate nix-rust/nix#2374
- 11: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/vm/src/stdlib/posix.rs
- 12: https://leancrew.com/all-this/man/man2/open.html
- 13: https://docs.rs/libc/latest/x86_64-apple-darwin/libc/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo file excerpt:"
sed -n '1100,1195p' pyre/pyre-interpreter/src/module/posix/interp_posix.rs
echo
echo "Cargo libc version/context:"
rg -n '^\[dependencies\]|\blibc\b|^\[dev-dependencies\]|\blibc\b|^\[build-dependencies\]|\blibc\b' Cargo.toml pyre/pyre-interpreter/Cargo.toml 2>/dev/null || true
echo
echo "Search cfg guards around fcntl constants:"
rg -n '#\[cfg|O_CLOEXEC|O_ACCMODE|O_ASYNC|O_DIRECTORY|O_FSYNC|O_NOCTTY|O_NOFOLLOW|O_NOFOLLOW_ANY|O_EXEC|O_SEARCH|O_DIRECT' pyre/pyre-interpreter/src/module/posix/interp_posix.rsRepository: youknowone/pyre
Length of output: 20710
🌐 Web query:
latest libc crate generated docs FSync Haiku O_FSYNC
💡 Result:
In the Rust libc crate, the O_FSYNC constant is not defined for the Haiku target [1]. Haiku's native headers define flags such as O_SYNC, O_RSYNC, and O_DSYNC [2], but O_FSYNC is generally associated with other POSIX-like systems (such as Linux or various BSD variants) where it is often an alias for O_SYNC [3][4]. While the libc crate provides raw FFI bindings for many platforms, it does not expose O_FSYNC for Haiku, reflecting the underlying POSIX implementation details and header definitions of the Haiku operating system [1][2]. High-level crates like nix also explicitly exclude Haiku when defining O_FSYNC to ensure compatibility with the target platform's supported flags [5][6].
Citations:
- 1: https://github.com/rust-lang/libc/blob/main/src%2Funix%2Fhaiku%2Fmod.rs
- 2: https://github.com/haiku/haiku/blob/8f16317a5b6db5c672f331814273e5857555020f/headers/posix/fcntl.h
- 3: https://docs.rs/nix/latest/nix/fcntl/struct.OFlag.html
- 4: https://github.com/nix-rust/nix/blob/ebf75050f2f2726808308f76253f27c97aa6db15/src/fcntl.rs
- 5: https://docs.tvix.dev/rust/src/nix/fcntl.rs.html
- 6: https://codebrowser.dev/tokio/crates/nix-0.27.1/src/fcntl.rs.html
Gate O_FSYNC to targets that define it.
#[cfg(unix)] ("O_FSYNC", libc::O_FSYNC as i64) cannot compile on Unix targets that do not provide libc::O_FSYNC; Haiku is one such target. Add a narrower cfg path for this flag or publish a zero/default fallback like the other non-universal flags.
🤖 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/posix/interp_posix.rs` around lines 1139 -
1156, Gate the O_FSYNC entry in the flag definitions to only Unix targets where
libc::O_FSYNC exists, or provide the project’s established zero/default fallback
for unsupported targets. Keep the existing O_FSYNC value unchanged on supported
platforms and preserve the surrounding O_* entries.
a0d8d95 to
ddad412
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ddad412 to
918e0dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
pyre/extra_tests/parity_tests/os_utime_pathconf_truncate.py (1)
150-158: 🎯 Functional Correctness | 🟡 MinorAdd the missing descriptor access-time assertion.
Line 157 checks only
st_mtimeand reusesbeforefrom the path branch. The test does not detect an implementation that leavesst_atimeatOLD. Capture a descriptor-specific reference immediately beforeos.utime(fd)and assert both timestamps from oneos.stat(p)result. This finding remains unresolved from the previous review.Proposed test fix
fd = os.open(p, os.O_RDWR) try: + descriptor_before = time.time() os.utime(fd) finally: os.close(fd) - check(abs(os.stat(p).st_mtime - before) < 60, f"utime(fd) -> {os.stat(p).st_mtime}") + stat_result = os.stat(p) + check( + abs(stat_result.st_mtime - descriptor_before) < 60, + f"utime(fd) -> {stat_result.st_mtime}", + ) + check( + abs(stat_result.st_atime - descriptor_before) < 60, + "utime(fd) left the access time behind", + )🤖 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/extra_tests/parity_tests/os_utime_pathconf_truncate.py` around lines 150 - 158, Update the os.utime descriptor test to capture a descriptor-specific timestamp immediately before os.utime(fd), then perform one os.stat(p) and assert both st_mtime and st_atime against that reference. Do not reuse the path-branch before value, and preserve the existing descriptor setup and cleanup.pyre/pyre-interpreter/src/module/posix/interp_posix.rs (1)
1158-1198: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
O_FSYNCandO_ASYNCare published under a target-wideunixgate. Both files name these twolibcconstants without a per-target gate, butlibcdoes not define them for every Unix target (Haiku lacksO_FSYNC; AIX, Solaris and Cygwin lackO_ASYNC). The build then fails on those targets.
pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L1158-L1198: replace the#[cfg(unix)]gate on theO_FSYNCandO_ASYNCentries with the narrower target list used by the Linux and Apple groups below them, or omit the name where the header has none.pyre/pyre-interpreter/src/host_seam.rs#L77-L83: moveO_ASYNCandO_FSYNCout of the unconditionalpub uselist into a gated re-export that matches the interp_posix gate.🤖 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/posix/interp_posix.rs` around lines 1158 - 1198, Restrict the O_ASYNC and O_FSYNC entries in interp_posix.rs to the same supported Linux/Apple target gate used by the surrounding platform-specific constants, or omit unsupported names. In host_seam.rs, remove both from the unconditional pub use list and add a matching gated re-export so both files compile consistently on targets where libc lacks these constants.
🤖 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 `@pyre/extra_tests/parity_tests/audit_and_parsercreate_name_argument.py`:
- Around line 167-168: Fix the incomplete comment near the ParserCreate checks
by aligning its wording with the intended sentence form used near line 111,
while preserving the existing meaning about accepting calls and ParserCreate no
longer building parsers.
In `@pyre/extra_tests/parity_tests/os_cpu_count.py`:
- Around line 65-76: Update the CPU-count validation around host_cpu_count to
avoid equating its sysctl-based result with SC_NPROCESSORS_ONLN on Apple/BSD
platforms; import sys for the platform check, use SC_NPROCESSORS_CONF for those
platforms, and retain the online-count comparison elsewhere.
In `@pyre/extra_tests/parity_tests/os_path_argument_types.py`:
- Around line 103-110: Update the parity expectations for truncate, statvfs,
chown, and pathconf to account for host-gated allow_fd behavior: use the
path-only expectation when their corresponding HAVE_FTRUNCATE, HAVE_FSTATVFS,
HAVE_FCHOWN, or HAVE_FPATHCONF feature is unavailable, while retaining WITH_FD
when enabled. Handle utime separately using its HAVE_FUTIMENS/platform behavior,
following the existing listdir/scandir guard pattern where appropriate.
In `@pyre/pyre-interpreter/src/function.rs`:
- Around line 1951-1956: Update name_utf8_mirror and the related
constructor/repr paths to preserve Python-visible surrogate names by reusing the
retained w_name object instead of converting Wtf8 through to_string_lossy().
Keep the existing valid-UTF-8 path unchanged, and ensure lone surrogates remain
represented exactly as stored in w_name.
---
Duplicate comments:
In `@pyre/extra_tests/parity_tests/os_utime_pathconf_truncate.py`:
- Around line 150-158: Update the os.utime descriptor test to capture a
descriptor-specific timestamp immediately before os.utime(fd), then perform one
os.stat(p) and assert both st_mtime and st_atime against that reference. Do not
reuse the path-branch before value, and preserve the existing descriptor setup
and cleanup.
In `@pyre/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 1158-1198: Restrict the O_ASYNC and O_FSYNC entries in
interp_posix.rs to the same supported Linux/Apple target gate used by the
surrounding platform-specific constants, or omit unsupported names. In
host_seam.rs, remove both from the unconditional pub use list and add a matching
gated re-export so both files compile consistently on targets where libc lacks
these constants.
🪄 Autofix
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: f8eeecb7-ce29-495e-a55f-784a4cbad6f9
📒 Files selected for processing (16)
.github/workflows/pyre-ci.ymlpyre/extra_tests/parity_tests/audit_and_parsercreate_name_argument.pypyre/extra_tests/parity_tests/os_access_modifiers.pypyre/extra_tests/parity_tests/os_arg_binding_surface.pypyre/extra_tests/parity_tests/os_cpu_count.pypyre/extra_tests/parity_tests/os_dup2_inheritable.pypyre/extra_tests/parity_tests/os_path_argument_types.pypyre/extra_tests/parity_tests/os_sched_policy_pipe2.pypyre/extra_tests/parity_tests/os_utime_pathconf_truncate.pypyre/extra_tests/parity_tests/str_lone_surrogate_names.pypyre/pyre-interpreter/src/function.rspyre/pyre-interpreter/src/gateway.rspyre/pyre-interpreter/src/host_seam.rspyre/pyre-interpreter/src/module/posix/interp_posix.rspyre/pyre-interpreter/src/module/pyexpat/mod.rspyre/pyre-interpreter/src/module/sys/vm.rs
| # The accepting calls, so the checks above are not passing because | ||
| # ParserCreate stopped building parsers. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the comment sentence.
The sentence is incomplete. Line 111 uses the intended form. Align this one with it.
📝 Proposed wording fix
-# The accepting calls, so the checks above are not passing because
-# ParserCreate stopped building parsers.
+# ...and the accepting calls still accept, so the checks above are not passing
+# because ParserCreate stopped building parsers.📝 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.
| # The accepting calls, so the checks above are not passing because | |
| # ParserCreate stopped building parsers. | |
| # ...and the accepting calls still accept, so the checks above are not passing | |
| # because ParserCreate stopped building parsers. |
🤖 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/extra_tests/parity_tests/audit_and_parsercreate_name_argument.py` around
lines 167 - 168, Fix the incomplete comment near the ParserCreate checks by
aligning its wording with the intended sentence form used near line 111, while
preserving the existing meaning about accepting calls and ParserCreate no longer
building parsers.
| # ── the value, not just its stability ──────────────────────────────────── | ||
| # A constant wrong answer would satisfy everything above. The processor count | ||
| # the host reports through sysconf is the one both sides are built on — the | ||
| # `sysconf(_SC_NPROCESSORS_ONLN)` arm directly, the `sysctl(CTL_HW, HW_NCPU)` | ||
| # arm because a host reports the same processors either way. | ||
| if before is not None and hasattr(os, "sysconf"): | ||
| try: | ||
| onln = os.sysconf("SC_NPROCESSORS_ONLN") | ||
| except (ValueError, OSError): | ||
| onln = None | ||
| if onln is not None and onln > 0: | ||
| check(before == onln, f"cpu_count {before} is not the host's {onln} processors") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
HW_NCPU and _SC_NPROCESSORS_ONLN can disagree.
The Apple and BSD arm of host_cpu_count reads sysctl(CTL_HW, HW_NCPU), which reports the configured processor count. _SC_NPROCESSORS_ONLN reports the online count. On a host with a processor offline the two differ and this equality check fails. Compare against SC_NPROCESSORS_CONF on those platforms, or relax the check to an upper bound.
🔧 Proposed fix
-if before is not None and hasattr(os, "sysconf"):
- try:
- onln = os.sysconf("SC_NPROCESSORS_ONLN")
- except (ValueError, OSError):
- onln = None
- if onln is not None and onln > 0:
- check(before == onln, f"cpu_count {before} is not the host's {onln} processors")
+if before is not None and hasattr(os, "sysconf"):
+ # The sysctl(CTL_HW, HW_NCPU) arm reports the configured count, so the
+ # name to compare against differs from the sysconf(_SC_NPROCESSORS_ONLN)
+ # arm's.
+ name = "SC_NPROCESSORS_CONF" if sys.platform == "darwin" else "SC_NPROCESSORS_ONLN"
+ try:
+ reported = os.sysconf(name)
+ except (ValueError, OSError):
+ reported = None
+ if reported is not None and reported > 0:
+ check(before == reported, f"cpu_count {before} is not the host's {reported} processors")sys needs importing for the platform test.
🤖 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/extra_tests/parity_tests/os_cpu_count.py` around lines 65 - 76, Update
the CPU-count validation around host_cpu_count to avoid equating its
sysctl-based result with SC_NPROCESSORS_ONLN on Apple/BSD platforms; import sys
for the platform check, use SC_NPROCESSORS_CONF for those platforms, and retain
the online-count comparison elsewhere.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 918e0ddac7
ℹ️ 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".
| #[cfg(not(target_env = "musl"))] | ||
| { |
There was a problem hiding this comment.
On Linux/musl, this condition newly publishes sched_getparam, sched_getscheduler, sched_param, and sched_rr_get_interval while omitting the matching setters, even though musl provides the POSIX sched_setparam and sched_setscheduler calls. This also violates the upstream grouping in pypy/module/posix/moduledef.py, where the presence of sched_getparam implies that sched_setparam is exported; applications that use the getter/type as the capability probe will therefore fail only when they attempt the write. Remove the musl exclusion (or gate the entire group consistently if the wrapper genuinely cannot support it).
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
… retry through the signal gate `utime_impl` read the current time out of `SystemTime::now()` when the caller named neither `times` nor `ns`, and passed that pair to `futimens` and `utimensat`. `parse_utime_args` (interp_posix.py:1918-1946) answers a `now` flag beside the pair instead, and `do_utimens` (:1948-1953) writes `rposix.UTIME_NOW` over both nanosecond fields. The flag travels the same way here now: `timespec_of` takes it for the two POSIX calls, and the Windows `SetFileTime` branch, which has no word for "now", reads the clock itself. `ftruncate_retry` compared the errno against EINTR and went straight back to the call. `crt_call!` (builtins.rs:6243-6253) is the raw-call wrapper, not the call gate, so no Python signal handler ran between the retries; interp_posix.py:404-412 retries under `wrap_oserror(..., eintr_retry=True)`. It goes through `builtins::eintr_retry_with` now, as the `open` loop beside it already did, and takes its OSError wrapper from the callers, so `os.truncate` still names the file it opened and `os.ftruncate` still names none. os_utime_pathconf_truncate.py covers the unnamed-time form by name and by descriptor. The comment above `sendfile` said pyre's os wrappers do no manual EINTR retry; three of them do. It states the divergence from interp_posix.py:2958-2974 instead. Assisted-by: Claude
The step carried no condition, so a red check.py skipped it. On this branch the parity suite last ran in run 31131072279; the two runs after it went red on jitstats rows and the step reported nothing on any of the three platforms — including the thirteen Windows failures that earlier run had surfaced. The job still fails on either gate. Assisted-by: Claude
`path_converter` fills `function_name` and `argument_name` from the argument
clinic, so a rejected path reports "{func}: {arg} should be {types}, not {tp}".
`path_or_fd_w` took a function name alone and spelled every argument "path". It
now takes the pair, and `fsencode_path_named_w` gives a path-only boundary both
halves; `fsencode_path_or_fd_w` and its nullable twin pass "path", so the
messages they already emitted are unchanged.
Named: readlink, rename and replace (src/dst, after whichever of the two the
caller reached, since they share one body), symlink, link, access, chroot,
execv, execve, posix_spawn and posix_spawnp (likewise name-derived), and the
Windows chdir, access, link and symlink arms. The Windows twins take the same
strings as their POSIX counterparts; only chdir's allowed-type list differs,
because there is no fchdir there to put "integer" in it.
Left with the caller-less conversion, each with the reason recorded in place:
the Windows-only boundaries (the _get*name family, startfile, listmounts,
_path_splitroot, and the Windows `system`, which declares text rather than a
path and so has a different message shape entirely), whose wording cannot be
measured from a POSIX host; and the values a sequence or mapping is walked for
-- argv entries, execve environment values, posix_spawn file actions -- which
answer the caller-less message under 3.14 as well.
Measured by emitting the TypeError for every (os entry point, argument
position) under CPython 3.14 and under pyre and diffing the two: 48 lines, no
divergence after the change. That measurement is also what found `access`,
`execv` and `execve`, which reach the converter through `extract_path` rather
than through `fsencode_path_w`.
os_path_argument_types.py drops its UNPINNED category and asserts the measured
message at every boundary, including each half of the two-path pairs and the
positions where caller-less is the correct answer.
Assisted-by: Claude
`interp_posix.py:2958-2974` runs both arms inside `while True: ... except OSError: wrap_oserror(..., eintr_retry=True)`, so an interrupted transfer runs the pending Python signal handlers and goes back to the call. The three call sites here had no loop and surfaced InterruptedError instead; they now retry through `builtins::eintr_retry_with`, as `ftruncate`, `truncate`'s `open` and `lockf` already do. The BSD arm keeps its partial-transfer rescue exactly as it was. `rposix.py:3086-3095` returns `sbytes` for `EAGAIN` and `EBUSY` alone; EINTR is not in that set and falls through to `handle_posix_error`, which raises, and the loop above it then re-runs the whole call with the same `offset` and `count` -- both are loop-invariant in `interp_posix.py` and `rposix` never sees the retry. So a partial transfer a signal interrupted is discarded and the range the caller asked for is requested again. The Linux offset arm re-seeds `offset` from the caller's value on every attempt for the same reason: `rposix.sendfile` (`rposix.py:3061-3065`) writes the offset into a fresh cell each call from the argument it was passed, so what a failed call left in the out-parameter is not what the next one starts from. Both Linux arms are `#[cfg(target_os = "linux")]` and do not compile on this host, and `cargo check --target x86_64-unknown-linux-gnu` cannot build them either (cc-rs: failed to find tool "x86_64-linux-gnu-gcc"). They were checked on a real Linux box instead -- `cargo check -p pyre-interpreter --features dynasm` in a Debian trixie container, rc=0, no new warnings. macOS: the same check plus `--features dynasm,sandbox`, both rc=0. Assisted-by: Claude
…them Registers `pipe2`, `sched_rr_get_interval`, `sched_getscheduler`, `sched_setscheduler`, `sched_getparam`, `sched_setparam` and the `posix.sched_param` structseq, each gated to the targets whose libc declares it (`moduledef.py:166-174`, `:216`). The setters are left out under musl, which declares neither. Both setters answer None; `interp_posix.py:3097`/`:3131` hand back the raw `handle_posix_error` result, which is 0 on every success. A `w_param` that is not a `sched_param` is refused with "must have a sched_param object", and a priority the C `int` cannot hold with "sched_priority out of range" (`interp_posix.py:3086-3092`). Adds `O_CLOEXEC` to the constants table. `moduledef.py:264-266` publishes it by name and `pipe2` takes it as its flag; `nt` spells the same intent O_NOINHERIT, so the entry is Unix-only. `host_seam::sys` gains the `sched_param` type and `O_CLOEXEC`. The seam re-exports no syscall function, so `sched_rr_get_interval` — the one call here that reaches libc directly rather than through host_env — is not compiled into a sandbox build; the raising stub serves the name there. Drops "dup3" from the sandbox denial list: neither `moduledef.py` nor `os` publishes such a name on any host. Moves "pipe2" and the sched_* names out of the unconditional list into target-gated loops, since `module_ns_store` writes rather than overwrites and would otherwise publish a name the build never registered. Adds pyre/extra_tests/parity_tests/os_sched_policy_pipe2.py. Assisted-by: Claude
sched_getaffinity/sched_setaffinity are registered on linux and android outside a sandbox build, written against the host header: moduledef.py names neither and rposix wraps neither. The mask is the fixed cpu_set_t, CPU_SETSIZE wide, because the libc crate exposes no CPU_ALLOC; a CPU number at or past that width is refused with EINVAL. Both names are served by sandbox_unavailable in a sandbox build. Eighteen further O_* constants join O_CLOEXEC in the table, in three groups: six on every unix, six on linux/android, seven on the Apple targets. host_seam::sys re-exports all of them, the two platform groups as separate gated blocks. sys._get_cpu_count_config() answers -1. os.py:1180 reads it to decide whether process_cpu_count counts the affinity mask or aliases cpu_count, so publishing sched_getaffinity without it makes `import os` raise. The value is set by -X cpu_count and PYTHON_CPU_COUNT, neither of which this interpreter reads. The sandbox denial list's sched_getscheduler/sched_getparam/ sched_rr_get_interval loop keeps the gate it was written with; the affinity pair is stored after it under its own. os_sched_policy_pipe2.py covers the flag set through a per-platform table and the affinity pair through its mask spellings and refusals. Assisted-by: Claude
… count os.cpu_count and posix._cpu_count answered host_posix::get_number_of_os_threads(), which reads /proc/self/stat's num_threads on linux and the mach task_threads count on the Apple targets. That is the quantity warn_if_multi_threaded uses in the fork path, so both names reported how many threads happened to be alive: 2 on a 9-processor linux host and 2 on an 18-processor macOS host, and the value moved to 8 and back while six threads ran. host_cpu_count ports rpy_cpu_count's two unix arms — sysconf (_SC_NPROCESSORS_ONLN) on linux and android, sysctl(CTL_HW, HW_NCPU) on the Apple and BSD targets, 0 elsewhere — and the registrations keep the count <= 0 answer of None. Both now report 9 and 18 on those hosts. The two registrations carry #[cfg(not(feature = "sandbox"))] because the helper makes a syscall; the sandbox denial list already served both names. The windows registration is unchanged and carries a comment recording that rposix.py:2978-2986 reads GetSystemInfo().dwNumberOfProcessors where it calls available_parallelism, and that no windows oracle is reachable from this host to measure which the surface should report. os_cpu_count.py asserts the count does not move while six threads are started and joined, and that it is the processor count sysconf reports. Assisted-by: Claude
Both dup2 registrations — the unix arm and the nt arm — were make_builtin_function with no Signature, so the trailing __pyre_kw__ marker dict was never split off the argument slice. It occupied the third positional slot and read truthy, so dup2(fd, fd2, inheritable=False) returned an inheritable descriptor; an unknown keyword, a fifth positional, and a keyword duplicating a positional were all accepted, and dup2(fd, fd2=n) reached c_int_w with the dict. Converts both to #[pyre_function] with make_builtin_function_with_arity_and_maybe_sig. The parameters stay PyObjectRef and are unwrapped in the body with c_int_w / is_true: the macro's i32 binding is a w_int_get_value cast that performs no type check. Adds pyre/extra_tests/parity_tests/os_dup2_inheritable.py, asserting get_inheritable after both settings and the default, binding by name, and the exception type of six binding errors. It does not assert message text — dup2 is keyword-bindable upstream, so its arity text belongs to the clinic family, and #[pyre_function] emits the positional-only family until the #[posonly] marker exists. Assisted-by: Claude
access, get_terminal_size, listdir, posix_spawn, posix_spawnp, readlink, scandir, sendfile and symlink took their arguments straight off the argument slice. The trailing __pyre_kw__ marker dict was never split away, so a keyword either vanished or arrived as a positional value, and a surplus positional was dropped rather than refused. Of twenty measured calls, fifteen disagreed with CPython 3.14; symlink(src, dst, target_is_directory, dir_fd) created the link and returned None. Routes them through bind_path_args, which already carries the argument clinic spellings. Adds bind_posonly_args for the two shapes it does not cover: a parameter list wholly before the `/`, which is parsed by _PyArg_CheckPositional and refuses every keyword, and a positional-only prefix with a keyword-only tail, which is parsed by _PyArg_UnpackKeywords and reports its count in the parenthesised form. readlink and symlink now name dir_fd and refuse a descriptor for it, matching what os.supports_dir_fd advertises. access names its three keyword-only modifiers so that an unknown keyword is an error; it still calls access(2) rather than faccessat(2), so none of the three is answered. sendfile names headers, trailers and flags, which neither arm passes to the syscall. Adds pyre/extra_tests/parity_tests/os_arg_binding_surface.py. It asserts message text, not just the exception type, because the two count spellings are not interchangeable, and it checks that symlink refused without creating the link. Assisted-by: Claude
Setting __name__ to a str holding a lone surrogate aborted the process: function_set_func_name mirrored the name into the raw slot through w_str_get_value, which panics at unicodeobject.rs:579 when the backing WTF-8 buffer has no &str view. Such a str is reachable from ordinary code, since every operating-system name is decoded with surrogateescape and sys.argv carries one on this host. Adds name_utf8_mirror, which falls back to the lossy rendering, and uses it at the setter and at both constructor arms. That mirror is a UTF-8 String (NameStorage), so it cannot hold the value exactly, and types.FunctionType(code, globals, name) returned an escaped name once the abort was gone. Both constructor arms now also store the supplied object with function_set_name_obj; __name__ reads w_name, so it returns what was passed, and the mirror serves only repr and the __qualname__ default. Found by running 108 operations against a surrogate supplied through argv and through a non-UTF-8 filename on Linux; this was the only abort. macOS refuses such a filename with EILSEQ, so that producer was exercised in a container. Adds pyre/extra_tests/parity_tests/str_lone_surrogate_names.py, which builds the surrogate from bytes, asserts the payload before using it, and asserts round-trip equality rather than survival. Assisted-by: Claude
`sys.audit` was `|_| Ok(w_none())`, so it accepted anything: no event at all, an `int`, a `str` with no UTF-8 spelling, or the event passed by keyword. Give it the checks `@unwrap_spec(event="text")` performs at vm.py:473 — `audit expected at least 1 argument, got 0`, `audit() argument 1 must be str, not <type>` (`None` for the singleton), `sys.audit() takes no keyword arguments`, and `str_utf8_w` for the encoding. The body past the unwrap still does nothing: `addaudithook` stores no hook. `ParserCreate` checked that `encoding` is `str` or `None` but stored it without asking whether it has a UTF-8 spelling, which the sibling `namespace_separator` arm does ask; add that check. Both arms now report the argument's own type name through `arg_type_name`; `namespace_separator` previously spelled it as the literal `int`. Adds parity test audit_and_parsercreate_name_argument.py, which also pins `ctypes.CDLL`: a surrogate-escaped library name reaches dlopen and fails there, while a surrogate that is not an escape is refused before the call. check.py --backend dynasm,cranelift,wasm: 410/410, 410/410, 406/406. Parity suite: 243 files, no pyre-side failure. Assisted-by: Claude
`access` bound `dir_fd`, `effective_ids` and `follow_symlinks` and then called `access(2)`, which takes none of them: a `dir_fd` was accepted and ignored, so a relative name resolved against the working directory, and `follow_symlinks=False` answered about the file a symlink points at. Neither raised. Port interp_posix.py:747-789. The modified call goes through `faccessat`, with `AT_SYMLINK_NOFOLLOW` and `AT_EACCESS` per rposix.py:2551-2560; the unmodified call stays on `access`. Both answer `error == 0` without `handle_posix_error` upstream, so a refusal is False rather than an OSError. `HAVE_FACCESSAT` joins `_have_functions`, which is what os.py:117,137,158 read to put `access` in `supports_dir_fd`, `supports_effective_ids` and `supports_follow_symlinks`. Where the bit is off — under sandbox — `dir_fd` reports through `DirFD` and the two flags through the new `argument_unavailable`, matching interp_posix.py:771-775 and :298-301. The chmod site now shares that helper. Two further changes fall out of the call swap: * The old body narrowed the mode to `u8` and answered False when it did not fit, citing an EINVAL that this platform's `access(2)` does not raise: `os.access(f, 256)` answered False where the oracle answers True. The mode is now passed as the `c_int` interp_posix.py:744 declares. * The parameters are converted in declaration order, since each can raise and the first one to do so is what the caller sees. Writing the three new conversions ahead of `mode` had made `os.access(f, 2**40, dir_fd="s")` report the dir_fd, and `os.access(f, 2**40, effective_ids=<raising __bool__>)` run `__bool__` at all. Of thirteen measured calls, five disagreed with python3.14 (three capability-set memberships besides). All agree now. Adds parity test os_access_modifiers.py, marked linux,darwin: the assertions are about `faccessat`, which Windows has not, so the reference fails there too. Its discriminating pair is the same relative name answering True with `dir_fd` and False without — a test of the `dir_fd` form alone passes on the old build for any name that also resolves from the working directory. check.py --backend dynasm,cranelift,wasm: 410/410, 410/410, 406/406. Parity suite: 244 files, no pyre-side failure. Assisted-by: Claude
…t None `ParserCreate`'s `intern` parameter defaulted to `w_none()`, so an omitted argument and an explicit `None` arrived identically and neither wrote the parser's `intern` slot -- the dictionary `init_parser_slots` installs stayed in place either way, and a caller asking for no interning got names interned. The default is now the absent marker `PY_NULL`, and `parser_create3` writes the slot only when the argument was supplied. `intern_string` already reads a stored `None` back as "do not intern". Extends the parity test with six rows: the omitted case yields a dictionary and interns two names, the explicit `None` case yields `None` and interns nothing, and a caller-supplied dictionary is the one used and filled. Assisted-by: Claude
`parser_create3` and `sys_audit` both take their name argument through `str_utf8_w`, which refuses a lone surrogate, where `space.text_w` upstream does not. Neither site said why. For pyexpat the reason is a reader: both stored names come back out through `w_str_get_value`, which panics on a lone surrogate -- `declared_or_forced_encoding` for the encoding, `namespace_separator` for the separator. The refusal at the door is what keeps those reads from aborting the process. The `namespace_separator` arm carried no comment at all; the encoding arm's pointed at the separator arm rather than at the reader. For `sys.audit` nothing stores the event, so the encode exists only for the error it raises; the doc comment now says so. Comment-only. Assisted-by: Claude
sched_rr_get_interval, sched_getscheduler, sched_getparam, sched_setscheduler, sched_setparam, sched_getaffinity and sched_setaffinity took the pid with int_w and cast the result to libc::pid_t, truncating instead of raising OverflowError. Assisted-by: Claude
…compare longs by payload try_walker_specialize_unary_negative_int declined an INT_MIN operand and left it to the CallMayForce residual. It now emits GUARD_VALUE on the operand and takes the same _make_ovf2long tail the BINARY_OP overflow arm takes. That tail moved out of try_walker_specialize_binary_op_int into walker_emit_ovf2long_box and both callers share it. The three unary int folds (+, -, ~) gated only on is_exact_builtin_instance, which settles the recorded operand alone: an int subclass keeps the builtin ob_type, so the GUARD_CLASS INT they emit let one enter the trace later and be answered by the fold instead of its own __pos__ / __neg__ / __invert__. They now emit walker_guard_exact_w_class as the long folds already did. walker_unary_int_operand is the shared gate for all three and returns the class; the positive fold's duplicate inline gate is gone. try_walker_specialize_compare_op_long passed the two W_LongObject boxes to jit_w_long_cmp, which read both value payloads inside the callee. It now records the two getfield_gc_r and calls jit_bigint_cmp on the payloads. jit_w_long_cmp had no remaining callers and is deleted. extra_tests/parity_tests/unary_negative_int_min_jit.py covers both directions of the promote split, a bool operand, and a subclass arriving through data after the loop is hot. Assisted-by: Claude
918e0dd to
6f793c2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f793c2c76
ℹ️ 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".
| None => unsafe { pyre_object::w_str_get_wtf8(w_name) } | ||
| .to_string_lossy() | ||
| .into_owned(), |
There was a problem hiding this comment.
When f.__name__ contains a lone surrogate, to_string_lossy() replaces it with U+FFFD in the raw name slot. That slot remains observable: frame_into_generator_for_function passes function_get_name(function) into the generator, so calling a surrogate-named generator function makes generator.__name__ contain replacement characters instead of the assigned name (and other diagnostics reading this slot are similarly corrupted). Preserve the WTF-8 name for these consumers rather than keeping a lossy UTF-8 copy.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
…t event A hook that raises out of the `sys.addaudithook` event means the set refused the new hook. error_is_runtime_error tested for RuntimeError, which is what the C-level PySys_AddAuditHook reads; the sys.addaudithook a Python caller reaches takes any Exception. Measured against 3.14 with a refusing hook already installed, RuntimeError, ValueError and Exception all leave the call returning None and only a BaseException outside Exception comes back out. Renamed to error_is_exception. sys_audit handed the hooks the event object it was given. The `@unwrap_spec(event="text")` round trip re-wraps the unwrapped name, so a str subclass reaches the hooks flattened to a plain str; it now passes w_str_new(event) instead. The addaudithook comment cited vm.py:485; `def addaudithook` is at 486. extra_tests/parity_tests/sys_audit_hooks.py pins the dispatch, the argument tuple, the install order, that a new hook does not see its own installation, the three refusal outcomes, and that an exception from a hook propagates. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c157a14dc2
ℹ️ 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 let Err(err) = audit("sys.addaudithook", &[]) { | ||
| if !error_is_runtime_error(&err) { | ||
| if !error_is_exception(&err) { | ||
| return Err(err); |
There was a problem hiding this comment.
When an existing hook raises ValueError or another non-RuntimeError during sys.addaudithook, this predicate now suppresses the exception and silently refuses the new hook. The ported implementation in pypy/module/sys/vm.py:493-499 suppresses only exceptions matching RuntimeError and re-raises everything else, so widening the check to all Exception subclasses changes observable control flow for hook installers; retain the RuntimeError-specific predicate.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| &[ | ||
| "out_fd", "in_fd", "offset", "count", "headers", "trailers", "flags", | ||
| ], |
There was a problem hiding this comment.
When callers provide headers, trailers, flags, or any fifth through seventh positional argument, this parameter list binds them successfully even though the implementation never reads bound[4..]. Such calls therefore perform a normal transfer while silently discarding requested behavior; both the ported function at pypy/module/posix/interp_posix.py:2946-2957 and the supported Linux surface accept only the four common arguments, so these extras must be rejected until their behavior is implemented.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
| host_posix::sched_setscheduler(pid, policy, ¶m) | ||
| .map_err(|e| io_err(e, ""))?; | ||
| Ok(pyre_object::w_none()) |
There was a problem hiding this comment.
On every successful sched_setscheduler call this returns None, and the matching sched_setparam body does the same, whereas pypy/module/posix/interp_posix.py:3095-3100 and 3128-3133 return space.newint(res) (normally 0). Successful no-op updates using the current policy and priority make this readily observable, so code checking the upstream status receives a different value; return the host result instead of discarding it.
AGENTS.md reference: AGENTS.md:L231-L233
Useful? React with 👍 / 👎.
… jit: pop-fold guard order and recorded-raise roots; typedef: __init_subclass__ keywords (#1204) * jit: read the recorded-raise roots back out of their shadow slots `walker_emit_recorded_builtin_raise` pinned `exc` and each argument and then kept using the locals it had handed to `pin_root`. `RootScope::pin_root` normalizes the address it publishes once a second mutator has existed (`gc_roots.rs`), so past that point the slot and the caller's copy can name different objects, and these values are baked into the trace as `ConstPtr`s that outlive the walk. `args_storage` carried no root at all: it was read off `exc` before the normalization could apply and then indexed once per argument. Take every value back from `shadow_stack_get` after pinning it, and pin `args_storage`, which is the shape the concrete-shadow build for zip/tuple in this file already uses. Assisted-by: Claude * typedef: refuse __init_subclass__ keywords the way parse_obj does The default `__init_subclass__` reported a leftover class-definition keyword as `ArgErr::UnknownKwds`, so `class C(Base, flag=1)` raised "got an unexpected keyword argument 'flag'". `parse_obj` does not report the keyword when the signature has neither `**kwargs` nor a keyword-only argument (`argument.py:377-380`); it collapses every such refusal to "takes no keyword arguments". This signature is `cls` alone, so that branch always applies. CPython 3.14 raises the same sentence. The qualname ahead of the `()` is left as it was: pyre spells it `object.__init_subclass__` and CPython spells it with the subclass, and that choice is not what this changes. The snippet asserted only that the message mentioned `__init_subclass__`, which held for both shapes; it now pins the sentence and that the keyword is not named. Assisted-by: Claude * parity-tests: pin isinstance over a class that overrides __class__ `isinstance` reads `obj.__class__` on an MRO miss, so a `@property __class__` decides the answer and runs on every call: `isinstance(Masked(), int)` is True here, not the False a miss looks like. Nothing in the CPython suite runs that shape hot enough to trace. Counts are printed rather than asserted, so a fold that elided the call, cached its result, or answered False shows up as a diff against CPython. Scope is recorded in the header and was measured, not assumed: this does not reach `observed_replay_safe_isinstance`, whose only consumer is the nested residual abort inside an inline sub-walk. At this call depth the residual is a plain `call_may_force` and that gate no-ops; substituting the weaker predicate it used to carry leaves every line byte-identical. The gate's witness is `bench/synth/foriter_isinstance_class_property_replay.py`. Assisted-by: Claude * parity-tests: end isinstance_class_property with the harness's OK line run.py:172 accepts a case only when the last non-empty stdout line is "OK"; without it all three runners, cpython included, report the file as a failure. Assisted-by: Claude * intobject: correct the claim that gc_interp is off on the native backends `w_int_gc_alloc`'s doc justified keeping its `dont_look_inside` boundary on the grounds that it "costs nothing where the arm is unreachable: `gc_interp::enabled()` is false on the native backends". `enabled_from_env` (`gc_interp.rs`) answers true for every `PYRE_GC_INTERP` value except exactly `0`, so it is on by default on every target, and this arm — not the `malloc_typed` one `fuse_boxing_alloc` rewrites — is the arm `w_int_new` takes. Assisted-by: Claude * jit: separate the pop fold's lock-guard-free label from its trace-guard order `w_list_pop_end`'s doc defines "the descended body must hold no guard" as the absence of a `w_list_lock` acquire/release pair, which is what declines the fold's sub-walk. `try_walker_orthodox_list_pop` and `list_pop_end_jitcode` repeated the word without that qualifier, where it reads as a claim about trace guards. The sub-walk gets no callee frame, so a guard recorded inside it resumes at the caller's CALL boundary and re-executes the whole `pop()`. Every guard the Integer arm can record lands ahead of its `ll_list_int_set_len`: the sole op after that store is the `w_int_new` call, which `dispatch_inline_call_dir_kind` short-circuits into `walker_box_int` (`NewWithVtable` + `SetfieldGc`, no guard recorded) and returns before `run_sub_jitcode_walk`. Nothing asserts the ordering. Assisted-by: Claude * jitcode_dispatch: check the pop fold's guard/store order instead of assuming it `orthodox_list_pop_commit` descends `w_list_pop_end_inner` with no callee frame, so a guard recorded inside resumes at the caller's CALL boundary and re-executes the whole `pop()`. That is sound only while every guard lands before the body's first committed store. Today it does — the Integer arm's `ll_list_int_set_len` is followed only by the `w_int_new` call, which `dispatch_inline_call_dir_kind` short-circuits into `NewWithVtable` + `SetfieldGc` and records no guard — but nothing read the order. `subwalk_guard_follows_store` scans the ops recorded since a `TracePosition` and reports whether a guard follows a `setfield` / `setarrayitem` / `setinteriorfield`. A `start` past the end of the ops vector reports true: the window is gone, so an empty read is not an answer. The commit captures the position before `run_sub_jitcode_walk` and declines with `OrthodoxSubWalkTraceUnsupported` on a positive read, which cuts the tentative IR back to the generic residual. The decline takes the same `w_list_len == len_before` re-read the apply below does: where the arm's store keeps a runtime binding the sub-walk executed it for real, and cutting back to a residual that pops again is the double-apply the append side already had to fix. Unit test covers guard-then-store, store-then-guard, a store recorded before the captured position, `SetarrayitemGc`, and a position past the end. `pyre/bench/synth/list_pop_append.py` still reads 2.2x against its `max-pypy-ratio=22`; it read 73.5x before the fold existed. Assisted-by: Claude * docs: record the spec-vs-implementation ruling the parity review keeps re-deriving Six review findings across PRs #1001, #1079, #1081, #1085 and #1113 are one policy question, not six bugs: pyre follows CPython for what a Python program observes while the review measures every line against PyPy. Nothing in the repo stated the split, so each cycle re-filed them under sections 1/2. The ruling is that pyre's implementation is a port of PyPy and pyre's spec is CPython 3.14. Six of the seven adjudicated cases carry no version delta at all (`sched_setscheduler` has returned None since 3.3, `PyUnicode_FSConverter` has accepted bytes since 3.3, PEP 529 surrogatepass is 3.6, `DirEntry` has cached its `stat_result` since PEP 471), so "3.14" names which CPython to read rather than a lag PyPy is expected to close. - AGENTS.md gains the normative section and the six tests in short form. - The `/parity` skill gains a fourth deviation class, SPEC-DEVIATION, exempt from Principle 6's auto-fix (reverting one re-introduces a known bug), plus the full procedure with its evidence rules and worked examples. - `.github/codex-review-prompt.md` replaces the "Python 3.11 vs 3.14" exception with the four conditions a section-4 entry must carry. Structure — names, module paths, control-flow order, data structures, storage owner, JIT hints — is outside the ruling and follows PyPy unconditionally. A finding where PyPy's shape serves a mechanism pyre also has stops at PyPy: the `DirEntry.stat()` object cache is one, since `interp_posix.py:537-542` states the per-call rebuild is what keeps the allocation virtual. Assisted-by: Claude * _sre: drive an ASCII str subject as bytes and read the stored length `Subject::len()` called `code_points().count()` and `char_to_byte` called `code_point_indices().nth(pos)`, so every match walked the subject before the engine started; `Request::new` and `create_cursor` then walked it again. Add `Subject::AsciiStr` for a `str` whose code points are one byte each, selected by `w_str_is_ascii` where `make_ctx` selects `is_ascii()` (interp_sre.py:246). It drives the WTF-8 payload as bytes, so a character position is already a byte offset -- `UnicodeAsciiMatchContext` (interp_sre.py:52). `StrDrive` is `count` and cursor arithmetic only and every unicode decision keys on the compiled pattern's opcode, which is the property that lets upstream spell that context as a bare `StrMatchContext` subclass. `Subject::Str` now carries the object (`ctx.w_unicode_obj`, interp_sre.py:250) and reads the stored `_len()` and `_index_to_byte` rather than re-deriving them. Its positions remain code point indices that the `Wtf8` driver still resolves by walking; the note on the variant records what converting the reported spans would take. `slice_subject`, `empty_subject` and `finish_output` branch on `is_unicode()`, and `subject_span_bytes` extracts the position mapping and slices once; `char_len` and `char_slice` are gone. On an ASCII subject with n=1.6M, `pat.match(s, pos)` measured 130.7us at pos=0 and 762us at pos=n-10; both are now 0.35us, flat in n and in pos. A differential run over match/search/fullmatch spans, pos/endpos sweeps, findall/finditer/split/sub/subn/expand, bytes/bytearray/memoryview, type mismatches, a str subclass and scanner positions is byte-identical to CPython 3.14.6 on all 608 lines, as it was before the change. check.py --backend dynasm: 425/425. Assisted-by: Claude * BINARY_SLICE: convert str bounds through the index storage `binary_slice_values`'s `str` branch collected the byte offset of every code point in the subject into a `Vec<usize>` to resolve two bounds, so `s[a:b]` cost the whole string. A one-character slice of a 200k subject measured 752us, against 0.42us for the same slice written as a prebuilt slice object, which reaches `w_str_slice_codepoints` and walks only the sliced elements. Read the stored `_length` and convert the two bounds with `_index_to_byte` (unicodeobject.py:1251), which is what the slice-object path already does. The clamping and the `.max(s)` on the stop bound are unchanged, and a bound equal to the count still resolves to the end of the buffer. `binary_slice_values` is shared with the JIT residual (`bh_binary_slice_fn`, call_jit.rs:5765), so both consumers get it. The compiler folds constant bounds to `LOAD_CONST slice` + `BINARY_OP []` and emits `BINARY_SLICE` only for computed ones, so this is the path `json/decoder.py` takes with its per-token `s[end:end + 1]`. Decoding a flat 208 KB ASCII payload with the pure-Python scanner: 25.85s -> 0.089s, with the size sweep going from x3.95/x4.13/x9.39 per doubling to x2.07/x2.62/x1.77. `s[p:p+1]` on a 200k subject: 752us -> 0.54us. A differential run over 12 subjects (ASCII, 2/3/4-byte, lone surrogates, empty, and lengths on the 63/64/65/128 index-storage block boundaries) against 19x19 bound pairs in both spellings, plus list/tuple/bytes slicing and slice assignment, is byte-identical to CPython 3.14.6 on all 4430 lines. check.py --backend dynasm: 425/425. Assisted-by: Claude * _sre: resolve non-ASCII str positions through the stored index storage Subject::Str drove the engine over &Wtf8, whose StrDrive::count counts every code point and whose create_cursor(n) steps over the first n of them. Both run once per match, so a scan that restarts at successive positions walked the subject again on every call. Add Utf8Drive, which carries the W_UnicodeObject next to the payload and answers count with w_str_len and create_cursor with w_str_index_to_byte, minting the cursor at the head of an O(1) suffix reslice. Positions stay code point indices and stepping delegates to the &Wtf8 impl, so the engine's position arithmetic is unchanged. Assisted-by: Claude
1 participant
Ten commits of posix parity work. The through-line for most of them is
argument binding: entry points registered without a
Signaturenever hadthe trailing
__pyre_kw__marker dict split off the argument slice, so akeyword either vanished or arrived as a positional value, and a surplus
positional was dropped rather than refused.
What was actually broken
os.dup2(fd, fd2, inheritable=False)returned an inheritable descriptor.The marker dict landed in the third positional slot and read truthy. Nothing
raised and the return value looked right; only
os.get_inheritableshowed it.os.symlink(src, dst, target_is_directory, dir_fd)created the link andreturned
Nonewhere CPython raises. While probing this, it left a strayb -> asymlink in the repository root.os.access(path, mode, zzz=1)succeeded;os.listdir(d, 1)returned thelisting;
os.listdir(path=d)andos.readlink(path=d)raised, the latterreaching
c_int_wwith the kwargs dict itself.func.__name__ = <str with a lone surrogate>aborted the process atunicodeobject.rs:579. Reachable from ordinary code: every OS-supplied nameis decoded with
surrogateescape, andsys.argvcarries one on macOS.Of twenty measured posix calls, fifteen disagreed with CPython 3.14. All twenty
agree now, message text included.
Approach
interp_posix.rsalready hadbind_path_argsfrom an earlier slice, and itemits the argument-clinic spellings, so most of these get text parity today.
Added
bind_posonly_argsfor the two shapes it did not cover — a parameterlist wholly before the
/(parsed by_PyArg_CheckPositional, which refusesevery keyword and words its count differently), and a positional-only prefix
with a keyword-only tail (
_PyArg_UnpackKeywords, parenthesised count).dup2went through#[pyre_function]instead, since it is registered outsidethat helper's reach. Its guards live in the function body, which matters: the
residual JIT path enters builtins without the keyword binder.
Scope deliberately not taken
os.accessnow bindsdir_fd/effective_ids/follow_symlinksbutstill answers none of them — the body calls
access(2), notfaccessat(2). Making them raise would be consistent with whatos.supports_*advertises but would diverge from CPython on macOS, sotoday's semantics are kept and the capability gap is filed separately.
sendfilebindsheaders/trailers/flags; neither arm passes them on.dup2's arity message is still the positional-only spelling. It iskeyword-bindable upstream, so it belongs to the clinic family, but
#[pyre_function]cannot express that until a#[posonly]marker exists.Verification
Three new parity tests, all green on dynasm and cranelift, with a red control
recorded against the pre-fix binary for each.
os_arg_binding_surface.pyasserts message text rather than just the exception type, because the two count
spellings are not interchangeable; it also checks
symlinkrefused withoutcreating the link, with a positive control that
symlink(dst=)still createsone.
str_lone_surrogate_names.pybuilds its surrogate from bytes and assertsround-trip equality — the first fix turned the abort into silent data loss
(
types.FunctionType(...)returned'bad��name'), which a"did not crash" assertion would have passed.
check.py --backend dynasm,cranelift,wasm: dynasm 404/404, cranelift 404/404,wasm 400/400. The one red,
synth/pypy_type_surface, is a pre-existing basered on all three backends; its figures are byte-identical before and after
these commits (
bridges_compiled 5 -> 102, guard_failures 1011 -> 20497) andthe bench imports only
sys, so it cannot reach any of this. Not re-recorded.The lone-surrogate work was measured from the producers rather than by auditing
the 165
w_str_get_valuecall sites: 108 operations acrosssys.argv(macOS)and a real non-UTF-8 filename (Linux container — macOS refuses to create one,
EILSEQ). That found exactly one abort.— opened by Claude
Summary by CodeRabbit
New Features
pipe2support.Bug Fixes
Tests