Skip to content

majit: fuse boxing clusters split across blocks, and build ctypes on Windows - #1135

Merged
youknowone merged 3 commits into
mainfrom
win-work
Aug 10, 2026
Merged

majit: fuse boxing clusters split across blocks, and build ctypes on Windows#1135
youknowone merged 3 commits into
mainfrom
win-work

Conversation

@youknowone

@youknowone youknowone commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Two Windows CI failures: the one main is red on today, and the one waiting behind it.

1. synth/list_pop_append — the perf gate every runner was missing

fuse_boxing_alloc rewrites a malloc_typed(W_XObject { ob_header: PyObject { ob_type: &TYPE, .. }, .. }) cluster into a NewWithVtable plus payload stores, which is what jtransform.py:1012-1046 rewrite_op_malloc produces for a struct heaptracker.py:18-30 get_vtable_for_gcstruct can give a vtable. Upstream does no dataflow to get there — the STRUCT is the malloc's own constant type argument. Here the boxing is spelled as a struct literal handed to malloc_typed, so the type pointer is a value in the graph, and recovering it means following that value.

The pass followed it through operations only. Every boxing constructor is split into blocks by the calls between the &TYPE read and the malloc_typedget_instantiate, and gc_interp::enabled for the numeric boxes — so the aggregate and the header's type pointer reach their uses as block inputargs with no producing operation. The three lookups that key on them all missed, and the pass declined every cluster: 0 of 139 Size descrs in the build carried a vtable.

The fix. Resolve a block inputarg back to its incoming variable when every predecessor passes the same one at that column; stop at a genuine merge, and stop at a link carrying a constant. That is ssa.py:4-10 DataFlowFamilyBuilder's rule — a family holds the one variable a value is stored into plus every variable it is "just passed unmerged into the next block" through — with the constant case matching the separate opportunities_with_const list upstream keeps those in (ssa.py:28-33). Stopping early leaves the cluster unfused, which is the outcome jtransform.py:1039-1041 already gives a struct with no vtable. Same shape as thread_undefined_op_operands further down the file: a hand-rolled walk on majit's lowered graph, because the faithful DataFlowFamilyBuilder port in translator/backendopt/ssa.rs is written against the flowspace graph, not this IR.

Effect. Size descrs carrying a vtable: 0 → 8. w_int_new now emits new_with_vtable + setfield_gc_i + ref_return where it emitted a SyntheticTransparentCtor per allocation — each carrying a symbolic_fnaddr hash the sub-walk cannot record, which rolled the list.pop() fold back on every iteration.

runner dynasm cranelift
windows-latest 137.0x / 281.0x 240.5x
ubuntu-24.04 150.1x 155.2x
macos-latest 44.0x 53.0x

All against a 22x gate. Locally it goes 122.5x → 6.0x dynasm (3.83s → 0.22s), 5.5x cranelift, 36.0x wasm. Probe timings with the empty loop subtracted: pop 208ns/op → 41.6ns/op; the append+pop pair 898ms → 136ms over 5M iterations.

The five existing fuse_boxing_alloc tests all build single-block graphs, which is how the pass could go dead in production without one going red. Added fuse_boxing_alloc_follows_a_cluster_split_across_blocks, which threads the header and the aggregate across two block boundaries; neutralising the walk fails it and leaves the other five green.

2. import ctypes — the failure hidden behind it

Run pyre/extra_tests/parity_tests is the step after Run pyre/check.py in the same job, so while check.py was red it never ran. type_new_metatype_guard.py imports ctypes, and on Windows that raised ImportError: cannot import name '__version__' from '_ctypes'. Fixing check.py would have turned this step red instead.

_ctypes's submodules and its functional registration were gated on all(unix, feature = "host_env"), so Windows got a placeholder namespace in which every name is object. That gate is far wider than the platform-specific code it guards: cdata, metaclass and stginfo carry no libc and no cfg(unix) at all, funcptr has a single libc::wchar_t that libc also spells on Windows, and rustpython_host_env::ctypes already carries the Windows half of the loader (libloading), the ctypes-local last error, and the COM helpers. Gated on any(unix, windows) instead; wasm32 keeps the placeholder, since it has no dynamic loader.

The loader surface is split the way _ctypes.c's #ifdef MS_WIN32 splits it — dlopen/dlsym/dlclose stay posix, Windows gets LoadLibrary, FreeLibrary, FormatError, CopyComPointer, _check_HRESULT, COMError, get_last_error/set_last_error and FUNCFLAG_STDCALL/FUNCFLAG_HRESULT. RTLD_LOCAL/RTLD_GLOBAL go on both platforms (0 where there is no dlfcn.h) because ctypes/__init__.py:14 imports them before it branches on os.name.

The import then reached two more Windows-only gaps: sys.dllhandle, which ctypes/__init__.py:562 builds pythonapi out of with no guard, and _winapi.GetModuleFileName, which sysconfig._init_non_posix calls on it. dllhandle is GetModuleHandleW(NULL) — there is no separate interpreter DLL here, so the module hosting it is the executable, which makes a lookup through pythonapi report the symbol it could not find rather than fail on the handle.

It is a working loader, not just an importable namespace:

>>> ctypes.windll.kernel32.GetCurrentProcessId() == os.getpid()
True
>>> _ctypes.FormatError(2)
'The system cannot find the file specified.'
>>> _ctypes._check_HRESULT(-2147024894)
OSError: [WinError -2147024894] ...

test.test_ctypes now runs 306 cases on Windows (13 failures, 24 errors, 46 skipped) where it previously stopped at the first import. Those remaining failures are ctypes features missing on every platform — the buffer protocol over cdata instances, __pointer_type__/type flags — not Windows gaps, and test.test_ctypes is a non-PASS baseline entry that no gate runs. unix and wasm32 registration is byte-for-byte unchanged.

Verification

Windows host, release, LLBC re-extracted after the last source edit (the interpreter change trips the fingerprint legitimately):

  • pyre/check.py — dynasm 413/413, cranelift 413/413, wasm 409/409; the vendored CPython suite gate skips here, its baseline being darwin-arm64
  • pyre/extra_tests/parity_tests/run.py — every fixture OK on both backends. The one red is builtin_module_loader_spec.py [cpython]: this box's CPython 3.14 has no test package, which the runners' setup-python does
  • cargo test -p majit-translate — 3165 lib + 63 integration, 0 failed
  • cargo check -p pyre-wasm --target wasm32-unknown-unknown clean, confirming wasm32 stayed on the placeholder

The macOS-only CPython suite (gate) reds (test_re, test_pickletools) are untouched and their cause is still unknown.

Summary by CodeRabbit

  • New Features

    • Expanded host-environment support for _ctypes on Windows, including dynamic library loading, symbol lookup, and COM-related functionality.
    • Added _winapi.GetModuleFileName.
    • Added Windows support for process replacement via execv and execve.
    • Added sys.dllhandle on Windows.
  • Bug Fixes

    • Improved Windows argument validation for POSIX-compatible operations.
    • Standardized symbol resolution and error handling for ctypes functions.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The interpreter now supports additional Windows host-environment functionality. Changes cover _ctypes loading and COM helpers, POSIX process and path operations, _winapi.GetModuleFileName, and sys.dllhandle.

Changes

Windows portability

Layer / File(s) Summary
Windows ctypes implementation
pyre/pyre-interpreter/src/builtins.rs, pyre/pyre-interpreter/src/module/_ctypes/*
Host-backed _ctypes support now builds on Unix and Windows. Windows loader, symbol lookup, error, COM, and helper APIs are implemented. Symbol consumers use the interpreter-level lookup routine.
Windows POSIX process and path handling
pyre/pyre-interpreter/src/module/posix/interp_posix.rs
Windows access, execv, execve, and symlink handling now supports platform-specific arguments, validation, and wide-character process APIs.
Windows module and system metadata
pyre/pyre-interpreter/src/module/_winapi/mod.rs, pyre/pyre-interpreter/src/module/sys/vm.rs
_winapi.GetModuleFileName and Windows sys.dllhandle are added.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Poem

A rabbit loads a library bright,
Finds Windows symbols in the night.
Paths and processes hop in line,
sys.dllhandle marks the sign.
Ctypes and WinAPI run just right!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes both main changes: cross-block majit boxing fusion and Windows _ctypes support.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch win-work

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d839b1aaad

ℹ️ 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".

"LoadLibrary: name must be a string or bytes",
));
};
let handle = host_ctypes::open_library(&name).map_err(|e| {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Honor the Windows DLL search flags

On Windows, ctypes.CDLL._load_library passes its computed winmode as the second argument to _ctypes.LoadLibrary (lib-python/3/ctypes/__init__.py:435-451), but this implementation reads only args[0] and calls the flagless loader. Consequently winmode=0, LOAD_LIBRARY_SEARCH_SYSTEM32, and LOAD_LIBRARY_SEARCH_DLL_LOAD_DIR all behave identically, defeating callers that use these flags to constrain DLL and dependency lookup and potentially loading a different DLL than requested. Pass and enforce args[1] through the Windows loader.

Useful? React with 👍 / 👎.

Comment on lines +114 to +115
let mut buffer = [0u16; MAX_PATH];
let length = host_winapi::get_module_file_name(module, &mut buffer);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Retry GetModuleFileName with a larger buffer

When pyre is installed or launched from a Windows path longer than 259 UTF-16 units, this fixed buffer truncates the module path and the code then returns that truncated value as success. Callers such as sysconfig._init_non_posix (lib-python/3/sysconfig/__init__.py:420) use the result to derive the installation prefix, so imports and configuration paths can point at a nonexistent directory. Detect a full buffer and retry with a larger allocation instead of forcibly terminating the truncated result.

Useful? React with 👍 / 👎.

Comment thread majit/majit-translate/src/model.rs Outdated
Comment on lines +3025 to +3026
if incoming.any(|other| other.as_ref() != Some(&source)) || source == current {
break;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Trace equivalent SSA families through joins

When a value crosses a diamond, each arm normally has a distinct block-input variable even if both ultimately originate from the same producer. At the join this direct equality test sees the two arm variables as different and stops, so fuse_boxing_alloc misses an unmerged boxing value despite DataFlowFamilyBuilder.complete() recursively unifying both arm variables with their common origin. Resolve each incoming variable to its family/source before deciding that the join is a genuine merge.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

🤖 Codex parity review

Static analysis of this diff vs the local RPython/PyPy sources (commit 6757955).
Updated: 2026-08-10T12:27:47.475Z

Files in the reviewed diff
pyre/pyre-interpreter/src/builtins.rs
pyre/pyre-interpreter/src/module/_ctypes/cdata.rs
pyre/pyre-interpreter/src/module/_ctypes/funcptr.rs
pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs
pyre/pyre-interpreter/src/module/_ctypes/mod.rs
pyre/pyre-interpreter/src/module/_winapi/mod.rs
pyre/pyre-interpreter/src/module/posix/interp_posix.rs
pyre/pyre-interpreter/src/module/sys/vm.rs

1. Regressions to PyPy parity introduced by this patch

None.

2. Other mismatches introduced by this patch

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:768 ↔ lib_pypy/_ctypes/basics.py:49: COMError.__init__ sets args to (text, details), dropping hresult; PyPy sets self.args = (hresult, text, details).

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:692 ↔ lib_pypy/_ctypes/__init__.py:28: CopyComPointer accepts only Pyre _CArgObject destinations and otherwise returns E_POINTER; PyPy uses the pointer protocol (dst[0] = ...) and therefore accepts compatible pointer-like objects.

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:600 ↔ pypy/module/_rawffi/interp_rawffi.py:655: FormatError() accepts no argument and substitutes the current last-error value; PyPy’s corresponding FormatError(space, code) requires an explicit code.

  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs:9131 ↔ pypy/module/posix/interp_posix.py:1786: Windows execve converts path before validating argv, and accepts any iterable through exec_argv_wide; PyPy first requires argv to be a list or tuple, validates it and env, then converts path. Both accepted inputs and observable exception ordering differ.

  • pyre/pyre-interpreter/src/module/_winapi/mod.rs:129 ↔ pypy/module/sys/initpath.py:311: on a MAX_PATH-sized/truncated GetModuleFileNameW result, Pyre forcibly terminates and returns the truncated path; PyPy treats res >= _MAX_PATH as failure and returns no path.

3. Pre-existing mismatches (already present before this patch)

  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs:9365 ↔ pypy/module/posix/interp_posix.py:1442: target_is_directory is converted via truth-testing (is_true) rather than PyPy’s @unwrap_spec(...=int) conversion. Objects implementing __bool__ but not __index__ are accepted by Pyre; PyPy rejects them. The same truth-test was present before this patch’s argument-binding refactor.

4. Structural adaptations

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:528 ↔ lib_pypy/_ctypes/dll.py:7: Pyre implements CPython-compatible LoadLibrary(name, load_flags=0) with LoadLibraryExW; PyPy exposes LoadLibrary as dlopen(name, mode), whose mode is ignored. This is a CPython-compatibility/API adaptation.

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:492 ↔ pypy/module/_rawffi/moduledef.py:51: Pyre exports CPython _ctypesFUNCFLAG_HRESULT; PyPy’s _rawffi export set has no corresponding flag.

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:68 ↔ rpython/rlib/rdynload.py:224: Pyre exports RTLD_LOCAL and RTLD_GLOBAL as zero on Windows so CPython’s ctypes import path works; PyPy’s Windows dynamic-loader configuration has RTLD_GLOBAL = None and does not export unavailable constants.

  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs:437 ↔ rpython/rlib/rdynload.py:266: Windows symbol lookup uses an HMODULE and direct GetProcAddress, rather than PyPy/RPython’s translated DLLHANDLE abstraction. This is required by Rust’s host-library ownership model and the LoadLibraryExW flag path.

`_ctypes`'s submodules and its functional registration were gated on
`all(unix, feature = "host_env")`, so Windows got a placeholder namespace
in which every name is `object` and `import ctypes` stopped at
`from _ctypes import __version__`. `cdata`, `metaclass` and `stginfo`
carry no `libc` and no `cfg(unix)`, `funcptr` has one `libc::wchar_t`
that `libc` also spells on Windows, and `rustpython_host_env::ctypes`
already carries the Windows half of the loader, the ctypes-local last
error, and the COM helpers. Gate the module on `any(unix, windows)`
instead; wasm32 keeps the placeholder.

Split the loader surface the way `_ctypes.c`'s `#ifdef MS_WIN32` does:
`dlopen`/`dlsym`/`dlclose` stay posix, and Windows gets `LoadLibrary`,
`FreeLibrary`, `FormatError`, `CopyComPointer`, `_check_HRESULT`,
`COMError`, `get_last_error`/`set_last_error` and
`FUNCFLAG_STDCALL`/`FUNCFLAG_HRESULT`. `RTLD_LOCAL`/`RTLD_GLOBAL` are
registered on both platforms — 0 where there is no `dlfcn.h` — because
`ctypes/__init__.py:14` imports them before it branches on `os.name`.
`make_exc_type_with_init` becomes `pub(crate)` for `COMError`.

The import then reached two more Windows-only gaps: `sys.dllhandle`,
which `ctypes/__init__.py:562` builds `pythonapi` out of, and
`_winapi.GetModuleFileName`, which `sysconfig._init_non_posix` calls on
it. `dllhandle` is `GetModuleHandleW(NULL)`, there being no separate
interpreter DLL here.

`extra_tests/parity_tests/type_new_metatype_guard.py` imports `ctypes`,
so it now passes on Windows; `test.test_ctypes` runs 306 cases there
(13 failures, 24 errors, 46 skipped) where it previously stopped at the
first import. unix and wasm32 registration is unchanged.
`LoadLibrary` read only args[0] and called the flagless loader, so the
`winmode` every `CDLL` computes was discarded. It now parses "U|i" and calls
`LoadLibraryExW(name, NULL, load_flags)`, and answers ERROR_MOD_NOT_FOUND
with the FileNotFoundError naming the module rather than a bare winerror
OSError.

Reaching that flags argument means leaving `rustpython_host_env::ctypes`:
its Windows door is `libloading::Library::new`, which has no flags
parameter, and its raw-handle door is unix-only. The Windows handle is now
the HMODULE itself, `FreeLibrary` is the Win32 call on it, and symbol
resolution goes through a new `lookup_symbol` so funcptr.rs and cdata.rs
stay platform-agnostic.

sys.dllhandle is 0, which `get_dllhandle` (vm.py:301) answers without
cpyext; `sysconfig` feeds it to `_winapi.GetModuleFileName`, where handle 0
names the executable.

`audit_and_parsercreate_name_argument` expects OSError from a non-escape
surrogate on win32: `LoadLibraryExW` takes the wide spelling, so a lone
surrogate reaches the loader instead of being refused by an encode.
Four Windows entry points that answered from a placeholder or from a
hand-rolled binder:

- execv/execve were no-op stubs that returned None for any arguments,
  including a bytearray path. They now convert path, argv and env and call
  `_wexecv`/`_wexecve`, which return only on failure.
- access took two positionals and nothing else, so `os.access(path=, mode=)`
  raised. It now binds through bind_path_args and refuses dir_fd,
  effective_ids=True and follow_symlinks=False the way a build without
  faccessat does.
- symlink read args[0..3] straight off the slice, so `symlink(src, dst,
  target_is_directory, dir_fd)` created the link and returned None, and
  `dst=` was an unexpected keyword.
- get_terminal_size refused a keyword against `posix.get_terminal_size`;
  this module answers to `nt` on Windows.

argument_unavailable loses its unix gate; its text names no platform.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6757955239

ℹ️ 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".

Comment on lines +615 to +618
// ── get_last_error / set_last_error — the ctypes-local copy, which is
// separate from the thread's own Win32 last error. The setter answers
// with the value it replaced, the same contract `set_errno` above
// carries and the one the documented signature promises. ──

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Capture last error for flagged foreign calls

On Windows, CDLL(..., use_last_error=True) sets _FUNCFLAG_USE_LASTERROR, but the newly enabled funcptr::cfuncptr_call only translates FUNCFLAG_USE_ERRNO into CallOptions and leaves every other option at its default. Consequently a native call never swaps or captures the Win32 last-error value, so the newly exported get_last_error() returns a stale value precisely for callers that requested this behavior. Honor FUNCFLAG_USE_LASTERROR around the foreign call.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

Comment on lines +650 to +652
crate::module_ns_store(
ns,
"_check_HRESULT",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Run the HRESULT restype checker

On Windows, ctypes.HRESULT installs this function as _check_retval_ (lib-python/3/ctypes/__init__.py:514-525), and OleDLL relies on that hook to turn failing HRESULTs into OSError. However, funcptr::cfuncptr_call decodes every Ret::Code result and returns it directly without invoking the restype's _check_retval_, so failing ctypes.oledll calls silently return negative integers instead of raising. Wire this checker into result conversion rather than merely exporting it.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

Comment on lines +9145 to +9147
for key_obj in keys {
let value_obj = crate::baseobjspace::getitem(args[2], key_obj)?;
let key = extract_path(key_obj)?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Root environment keys across mapping callbacks

When env is a custom mapping whose __getitem__ allocates or otherwise triggers collection, this loop keeps the mapping, all keys, and the returned value only in raw PyObjectRef locals while calling back into Python; a moving collection can therefore leave key_obj, value_obj, and the remaining keys entries stale before extract_path uses them. The analogous _winapi::environment_block path explicitly pins the mapping and keys and re-reads them after each callback (module/_winapi/mod.rs:203-222); the new Windows execve path needs the same rooting discipline.

Useful? React with 👍 / 👎.

Comment on lines +768 to +771
crate::baseobjspace::setattr_str(
w_self,
"args",
pyre_object::tupleobject::w_tuple_new(vec![*text, *details]),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve hresult in COMError.args

For every constructed COMError, this overwrites the base exception's argument tuple with only (text, details), so str(exc), exception serialization, and code inspecting exc.args lose the HRESULT. PyPy's corresponding lib_pypy/_ctypes/basics.py:48-52 stores (hresult, text, details), matching the constructor's three arguments; keep all three values in args.

AGENTS.md reference: AGENTS.md:L231-L233

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/pyre-interpreter/src/module/posix/interp_posix.rs`:
- Around line 9116-9119: Update both exec call sites in
pyre/pyre-interpreter/src/module/posix/interp_posix.rs:9116-9119 and 9167-9170,
covering wexecv and wexecve, to invoke the wide C runtime functions through
crate::builtins::crt_call! and construct failures with
errno_err(crate::builtins::crt_errno(), ""). Replace the current
std::io::Error::last_os_error()/io_err handling at both sites; no other changes
are required.
- Around line 9344-9365: Move the dir_fd_kwarg validation in the symlink entry
point to after both fsencode_path_named_w conversions for src and dst. Preserve
the existing arguments and error handling so path conversions, including
user-defined __fspath__, occur before rejecting dir_fd, matching the ordering
used by access and stat_entry.
🪄 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: b4a3b189-4cbe-473a-95a2-92810c2a30ed

📥 Commits

Reviewing files that changed from the base of the PR and between ab70bc2 and 6757955.

📒 Files selected for processing (8)
  • pyre/pyre-interpreter/src/builtins.rs
  • pyre/pyre-interpreter/src/module/_ctypes/cdata.rs
  • pyre/pyre-interpreter/src/module/_ctypes/funcptr.rs
  • pyre/pyre-interpreter/src/module/_ctypes/interp_ctypes.rs
  • pyre/pyre-interpreter/src/module/_ctypes/mod.rs
  • pyre/pyre-interpreter/src/module/_winapi/mod.rs
  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs
  • pyre/pyre-interpreter/src/module/sys/vm.rs

Comment on lines +9116 to +9119
unsafe { libc::wexecv(command_w.as_ptr(), argv_ptrs.as_ptr()) };
// `wrap_oserror` names no file, so the path stays out of
// the error.
Err(io_err(std::io::Error::last_os_error(), ""))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

execv and execve read the wrong error source after a failed C runtime exec. Both call sites invoke a wide C runtime exec function and then build the OSError from std::io::Error::last_os_error(). On Windows that reads GetLastError(), but _wexecv and _wexecve report failure through the C runtime errno. io_err then maps a Win32 code as though it were a POSIX errno, so a failed exec raises an OSError with an unrelated code. The shared fix is to route both calls through crate::builtins::crt_call! and read crate::builtins::crt_errno(), which is the convention the rest of this file already uses for C runtime calls.

  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L9116-L9119: wrap the libc::wexecv call in crt_call! and build the error with errno_err(crate::builtins::crt_errno(), "").
  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L9167-L9170: wrap the libc::wexecve call in crt_call! and build the error with errno_err(crate::builtins::crt_errno(), "").
📍 Affects 1 file
  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L9116-L9119 (this comment)
  • pyre/pyre-interpreter/src/module/posix/interp_posix.rs#L9167-L9170
🤖 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 9116 -
9119, Update both exec call sites in
pyre/pyre-interpreter/src/module/posix/interp_posix.rs:9116-9119 and 9167-9170,
covering wexecv and wexecve, to invoke the wide C runtime functions through
crate::builtins::crt_call! and construct failures with
errno_err(crate::builtins::crt_errno(), ""). Replace the current
std::io::Error::last_os_error()/io_err handling at both sites; no other changes
are required.

Comment on lines +9344 to +9365
let (bound, kwargs) = bind_path_args(
args,
"symlink",
&["src", "dst", "target_is_directory"],
2,
&["dir_fd"],
)?;
if crate::builtins::kwarg_get(kwargs, "dir_fd")
.is_some_and(|w| !unsafe { pyre_object::is_none(w) })
{
return Err(dir_fd_unavailable());
}
if args.len() < 2 {
return Err(crate::PyError::type_error("symlink() requires 2 arguments"));
}
let src = crate::gateway::fsencode_path_named_w(args[0], "symlink", "src")?;
let dst = crate::gateway::fsencode_path_named_w(args[1], "symlink", "dst")?;
let target_is_directory = match args
.get(2)
.copied()
.or_else(|| crate::builtins::kwarg_get(kwargs, "target_is_directory"))
{
// `symlink` types `dir_fd` as `DirFD(rposix.HAVE_SYMLINKAT)`,
// and `CreateSymbolicLinkW` resolves a relative name against
// the working directory alone.
dir_fd_kwarg(kwargs, false)?;
let src = crate::gateway::fsencode_path_named_w(
bound[0].expect("src is required"),
"symlink",
"src",
)?;
let dst = crate::gateway::fsencode_path_named_w(
bound[1].expect("dst is required"),
"symlink",
"dst",
)?;
let target_is_directory = match bound[2] {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Convert src and dst before you reject dir_fd.

dir_fd is keyword-only and is declared last. The conversions run in declaration order, and each conversion can raise and can run user code through __fspath__. Here dir_fd_kwarg runs first, so os.symlink(bad_path_object, dst, dir_fd=3) reports the dir_fd platform error instead of the src conversion error.

The access entry point added in this same change states this rule at Line 9023 and follows it: it converts path and mode, then calls dir_fd_kwarg. stat_entry follows the same order at Line 3832 and Line 3836. Move the dir_fd_kwarg call after both path conversions so symlink matches.

🐛 Proposed fix for the conversion order
-                // `symlink` types `dir_fd` as `DirFD(rposix.HAVE_SYMLINKAT)`,
-                // and `CreateSymbolicLinkW` resolves a relative name against
-                // the working directory alone.
-                dir_fd_kwarg(kwargs, false)?;
                 let src = crate::gateway::fsencode_path_named_w(
                     bound[0].expect("src is required"),
                     "symlink",
                     "src",
                 )?;
                 let dst = crate::gateway::fsencode_path_named_w(
                     bound[1].expect("dst is required"),
                     "symlink",
                     "dst",
                 )?;
+                // `symlink` types `dir_fd` as `DirFD(rposix.HAVE_SYMLINKAT)`,
+                // and `CreateSymbolicLinkW` resolves a relative name against
+                // the working directory alone.
+                dir_fd_kwarg(kwargs, false)?;
📝 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.

Suggested change
let (bound, kwargs) = bind_path_args(
args,
"symlink",
&["src", "dst", "target_is_directory"],
2,
&["dir_fd"],
)?;
if crate::builtins::kwarg_get(kwargs, "dir_fd")
.is_some_and(|w| !unsafe { pyre_object::is_none(w) })
{
return Err(dir_fd_unavailable());
}
if args.len() < 2 {
return Err(crate::PyError::type_error("symlink() requires 2 arguments"));
}
let src = crate::gateway::fsencode_path_named_w(args[0], "symlink", "src")?;
let dst = crate::gateway::fsencode_path_named_w(args[1], "symlink", "dst")?;
let target_is_directory = match args
.get(2)
.copied()
.or_else(|| crate::builtins::kwarg_get(kwargs, "target_is_directory"))
{
// `symlink` types `dir_fd` as `DirFD(rposix.HAVE_SYMLINKAT)`,
// and `CreateSymbolicLinkW` resolves a relative name against
// the working directory alone.
dir_fd_kwarg(kwargs, false)?;
let src = crate::gateway::fsencode_path_named_w(
bound[0].expect("src is required"),
"symlink",
"src",
)?;
let dst = crate::gateway::fsencode_path_named_w(
bound[1].expect("dst is required"),
"symlink",
"dst",
)?;
let target_is_directory = match bound[2] {
let (bound, kwargs) = bind_path_args(
args,
"symlink",
&["src", "dst", "target_is_directory"],
2,
&["dir_fd"],
)?;
let src = crate::gateway::fsencode_path_named_w(
bound[0].expect("src is required"),
"symlink",
"src",
)?;
let dst = crate::gateway::fsencode_path_named_w(
bound[1].expect("dst is required"),
"symlink",
"dst",
)?;
// `symlink` types `dir_fd` as `DirFD(rposix.HAVE_SYMLINKAT)`,
// and `CreateSymbolicLinkW` resolves a relative name against
// the working directory alone.
dir_fd_kwarg(kwargs, false)?;
let target_is_directory = match bound[2] {
🤖 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 9344 -
9365, Move the dir_fd_kwarg validation in the symlink entry point to after both
fsencode_path_named_w conversions for src and dst. Preserve the existing
arguments and error handling so path conversions, including user-defined
__fspath__, occur before rejecting dir_fd, matching the ordering used by access
and stat_entry.

@youknowone
youknowone merged commit e9d41ee into main Aug 10, 2026
15 of 17 checks passed
@youknowone
youknowone deleted the win-work branch August 10, 2026 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant