GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — ctypes: unify the foreign-call path on a single host_env `call()` API by youknowone · Pull Request #8235 · RustPython/RustPython Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR replaces the host_env ctypes libffi call surface with layout-driven call contracts and migrates vm ctypes argument, layout, and result handling to the new CArgValue/CallValue model. It also adds regression snippets for scalar, pointer, errno, and by-value struct/union calls. Changesctypes foreign-call rewrite
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Python
participant PyCFuncPtr
participant ctypes_callproc
participant host_env_call as host_env::call
Python->>PyCFuncPtr: call(args)
PyCFuncPtr->>ctypes_callproc: addr, arguments, RetSpec, CallOptions
ctypes_callproc->>host_env_call: call(addr, CallArg[], CallRet, CallOptions)
host_env_call->>host_env_call: lower args, validate sizes, swap errno/last_error
host_env_call-->>ctypes_callproc: CallValue or CallError
ctypes_callproc-->>PyCFuncPtr: CallValue / mapped PyErr
PyCFuncPtr->>PyCFuncPtr: build_result(CallValue)
PyCFuncPtr-->>Python: converted Python result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
`call` read `Code` returns through `low::ffi_arg`. The `libffi_sys` binding types `ffi_arg` as `c_ulong`, which is 4 bytes under LLP64 (Windows x64), so `low::call` truncated 8-byte returns (`q`/`Q`/`d`) to the low 4 bytes. `calls_f64_scalar` and `passes_large_struct_by_value` failed on windows-2025. Read a full register (`u64`) instead; `decode_type_code` still slices the leading bytes each type code needs. No change on LP64, where `ffi_arg` is already 8 bytes. Add `typed_two_scalar_args`, which calls the previously-unused `add_i32` ABI helper (two-argument scalar path); the helper tripped `-D warnings` dead-code on clippy. Assisted-by: Claude
`stdlib_ctypes_byvalue.py` reaches libc through `CDLL(None)` and calls `div`/`imaxdiv`/`inet_ntoa`, none of which resolve that way on Windows, so the snippet aborted on windows-2025. Guard it like `stdlib_ctypes_calls.py`: print "OK" and exit before touching `CDLL`. The by-value path is covered on Windows by test_ctypes. Assisted-by: Claude
youknowone
marked this pull request as ready for review
July 7, 2026 17:26
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🤖 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 `@crates/host_env/src/ctypes.rs`:
- Around line 2124-2155: Guard zero-sized by-value aggregates in the call
lowering path: in the aggregate handling inside the function that builds
`lowered` and `ffi_args`, the current `buffer.len() < expected` check still
allows `expected == 0`, which later causes `Arg::new(&buffer[0])` to panic.
Update the `CallArg::Aggregate` branch and/or the `ffi_args` construction to
reject or specially handle zero-sized `Structure`/`Union` arguments before
indexing into the buffer.
In `@crates/vm/src/stdlib/_ctypes/function.rs`:
- Around line 173-180: The default Python int to i32 conversion in the argument
conversion logic is silently truncating overflow to 0; update the conversion in
the function that builds Argument/CArgValue::Int so it checks the bigint-to-i32
result and raises OverflowError on out-of-range values instead of using
unwrap_or(0). Keep the behavior aligned with convert_to_pointer in the same
module by preserving the original error path rather than coercing invalid
inputs.
- Around line 270-285: The by-value struct/union handling in function.rs
currently copies only the raw bytes from the converted PyCData, which can drop
ownership of nested backing references too early. Update the aggregate path in
the conversion logic to keep the converted object alive by returning it
alongside the CArgValue, specifically in the PyCStructure/PyCUnion branch of the
argument conversion code. Preserve the existing byte snapshot and layout
selection, but return Some(converted.clone()) with the aggregate so any
_objects/kept_refs remain valid through the FFI call.
- Around line 211-241: Update ArgumentType for PyTypeRef in convert_object to
recognize PyCArray before the generic _type_ string validation, since array
argtypes carry an element type object in _type_ rather than a ctypes code
string. Adjust the type-check branch around the fast_issubclass and
get_attr("_type_", vm) logic so arrays are accepted and can still be decayed
through from_param into a pointer, instead of failing the PyStr downcast for
cases like c_int * 3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c9d5518f-8def-4b3e-9aa5-6aaff2b2ab1f
📒 Files selected for processing (9)
crates/host_env/src/ctypes.rscrates/vm/src/stdlib/_ctypes.rscrates/vm/src/stdlib/_ctypes/base.rscrates/vm/src/stdlib/_ctypes/function.rscrates/vm/src/stdlib/_ctypes/simple.rscrates/vm/src/stdlib/_ctypes/structure.rscrates/vm/src/stdlib/_ctypes/union.rsextra_tests/snippets/stdlib_ctypes_byvalue.pyextra_tests/snippets/stdlib_ctypes_calls.py
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
173
to
180
| // 10. Python int -> i32 (default integer type) | ||
| if let Ok(int_val) = value.try_int(vm) { | ||
| let val = int_val.as_bigint().to_i32().unwrap_or(0); | ||
| return Ok(Argument { | ||
| ffi_type: ffi_i32_type(), | ||
| keep: None, | ||
| value: FfiArgValue::Scalar(FfiValue::I32(val)), | ||
| value: CArgValue::Int(val), | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
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
Silent truncation on integer overflow instead of raising OverflowError.
to_i32().unwrap_or(0) silently maps an out-of-range Python int to 0 rather than surfacing an error. This is inconsistent with convert_to_pointer in the same file (lines 108-109), which explicitly raises OverflowError when a value doesn't fit. CPython's ConvParam for the no-argtypes default-int case similarly propagates OverflowError on overflow rather than truncating. Silently coercing to 0 produces a wrong value passed to the foreign function without any diagnostic.
🐛 Proposed fix
- if let Ok(int_val) = value.try_int(vm) { - let val = int_val.as_bigint().to_i32().unwrap_or(0); - return Ok(Argument { - keep: None, - value: CArgValue::Int(val), - }); - } + if let Ok(int_val) = value.try_int(vm) { + let val = int_val + .as_bigint() + .to_i32() + .ok_or_else(|| vm.new_overflow_error("int too large to convert"))?; + return Ok(Argument { + keep: None, + value: CArgValue::Int(val), + }); + }
📝 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
| // 10. Python int -> i32 (default integer type) | |
| if let Ok(int_val) = value.try_int(vm) { | |
| let val = int_val.as_bigint().to_i32().unwrap_or(0); | |
| return Ok(Argument { | |
| ffi_type: ffi_i32_type(), | |
| keep: None, | |
| value: FfiArgValue::Scalar(FfiValue::I32(val)), | |
| value: CArgValue::Int(val), | |
| }); | |
| } | |
| // 10. Python int -> i32 (default integer type) | |
| if let Ok(int_val) = value.try_int(vm) { | |
| let val = int_val | |
| .as_bigint() | |
| .to_i32() | |
| .ok_or_else(|| vm.new_overflow_error("int too large to convert"))?; | |
| return Ok(Argument { | |
| keep: None, | |
| value: CArgValue::Int(val), | |
| }); | |
| } |
🤖 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 `@crates/vm/src/stdlib/_ctypes/function.rs` around lines 173 - 180, The default
Python int to i32 conversion in the argument conversion logic is silently
truncating overflow to 0; update the conversion in the function that builds
Argument/CArgValue::Int so it checks the bigint-to-i32 result and raises
OverflowError on out-of-range values instead of using unwrap_or(0). Keep the
behavior aligned with convert_to_pointer in the same module by preserving the
original error path rather than coercing invalid inputs.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Comment on lines
211
to
+241
| impl ArgumentType for PyTypeRef { | ||
| fn to_ffi_type(&self, vm: &VirtualMachine) -> PyResult<FfiType> { | ||
| use super::pointer::PyCPointer; | ||
| use super::structure::PyCStructure; | ||
|
|
||
| // CArgObject (from byref()) should be treated as pointer | ||
| if self.fast_issubclass(CArgObject::static_type()) { | ||
| return Ok(ffi_pointer_type()); | ||
| } | ||
|
|
||
| // Pointer types (POINTER(T)) are always pointer FFI type | ||
| // Check if type is a subclass of _Pointer (PyCPointer) | ||
| if self.fast_issubclass(PyCPointer::static_type()) { | ||
| return Ok(ffi_pointer_type()); | ||
| } | ||
|
|
||
| // Structure types are passed as pointers | ||
| if self.fast_issubclass(PyCStructure::static_type()) { | ||
| return Ok(ffi_pointer_type()); | ||
| } | ||
| fn convert_object( | ||
| &self, | ||
| value: PyObjectRef, | ||
| vm: &VirtualMachine, | ||
| ) -> PyResult<(CArgValue, Option<PyObjectRef>)> { | ||
| // Validate the argument type up front (mirrors the pre-conversion | ||
| // check): pointer-like ctypes types are always acceptable; a simple | ||
| // type must carry a known _type_ code; anything else is unsupported. | ||
| let type_code = if self.fast_issubclass(CArgObject::static_type()) | ||
| || self.fast_issubclass(PyCPointer::static_type()) | ||
| || self.fast_issubclass(PyCStructure::static_type()) | ||
| || self.fast_issubclass(PyCUnion::static_type()) | ||
| { | ||
| None | ||
| } else { | ||
| // Use get_attr to traverse MRO (for subclasses like MyInt(c_int)) | ||
| let typ = self | ||
| .as_object() | ||
| .get_attr(vm.ctx.intern_str("_type_"), vm) | ||
| .ok() | ||
| .ok_or_else(|| vm.new_type_error("Unsupported argument type"))?; | ||
| let typ = typ | ||
| .downcast_ref::<PyStr>() | ||
| .ok_or_else(|| vm.new_type_error("Unsupported argument type"))? | ||
| .to_string(); | ||
| if ffi_type_from_code(&typ).is_none() { | ||
| return Err(vm.new_type_error(format!("Unsupported argument type: {typ}"))); | ||
| } | ||
| Some(typ) | ||
| }; |
Contributor
There was a problem hiding this comment.
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
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and nearby symbols. git ls-files crates/vm/src/stdlib/_ctypes/function.rs crates/vm/src/stdlib/_ctypes/base.rs crates/vm/src/stdlib/_ctypes/*.rs | sed -n '1,120p' printf '\n== outline function.rs ==\n' ast-grep outline crates/vm/src/stdlib/_ctypes/function.rs --view expanded || true printf '\n== outline base.rs ==\n' ast-grep outline crates/vm/src/stdlib/_ctypes/base.rs --view expanded || true printf '\n== search for PyCArray and convert_to_pointer usage ==\n' rg -n "PyCArray|convert_to_pointer|array_paramfunc|call_paramfunc|from_param|Unsupported argument type" crates/vm/src/stdlib/_ctypes -S
Repository: RustPython/RustPython
Length of output: 23436
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read only the relevant slices around the claimed code paths. sed -n '1,220p' crates/vm/src/stdlib/_ctypes/base.rs printf '\n--- function.rs slice ---\n' sed -n '1,320p' crates/vm/src/stdlib/_ctypes/function.rs
Repository: RustPython/RustPython
Length of output: 20130
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the exact type conversion branches in function.rs. nl -ba crates/vm/src/stdlib/_ctypes/function.rs | sed -n '180,280p'
Repository: RustPython/RustPython
Length of output: 199
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read array type definitions and the relevant conversion helpers. nl -ba crates/vm/src/stdlib/_ctypes/base.rs | sed -n '1,260p'
Repository: RustPython/RustPython
Length of output: 199
🏁 Script executed:
#!/bin/bash set -euo pipefail set -o pipefail printf '== function.rs relevant slice ==\n' nl -ba crates/vm/src/stdlib/_ctypes/function.rs | sed -n '200,270p' printf '\n== base.rs relevant slice around PyCArray and helpers ==\n' rg -n "struct PyCArray|impl ArgumentType|array_paramfunc|convert_to_pointer|_type_|from_param|call_paramfunc" crates/vm/src/stdlib/_ctypes/base.rs crates/vm/src/stdlib/_ctypes/function.rs -n -S
Repository: RustPython/RustPython
Length of output: 232
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== array.rs around type construction and from_param ==\n' sed -n '1,220p' crates/vm/src/stdlib/_ctypes/array.rs printf '\n== array.rs around PyCArrayType.from_param ==\n' sed -n '300,360p' crates/vm/src/stdlib/_ctypes/array.rs printf '\n== array.rs around PyCArray definition ==\n' sed -n '390,470p' crates/vm/src/stdlib/_ctypes/array.rs printf '\n== search for _type_ assignment/access in _ctypes ==\n' rg -n '"_type_"|_type_' crates/vm/src/stdlib/_ctypes/array.rs crates/vm/src/stdlib/_ctypes/base.rs crates/vm/src/stdlib/_ctypes/simple.rs crates/vm/src/stdlib/_ctypes/pointer.rs crates/vm/src/stdlib/_ctypes/structure.rs crates/vm/src/stdlib/_ctypes/union.rs -S
Repository: RustPython/RustPython
Length of output: 48584
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read only the relevant array/pointer helper slices with portable tools. sed -n '120,220p' crates/vm/src/stdlib/_ctypes/array.rs printf '\n---\n' sed -n '308,350p' crates/vm/src/stdlib/_ctypes/array.rs printf '\n---\n' sed -n '390,460p' crates/vm/src/stdlib/_ctypes/array.rs printf '\n---\n' rg -n '_type_' crates/vm/src/stdlib/_ctypes/array.rs crates/vm/src/stdlib/_ctypes/base.rs crates/vm/src/stdlib/_ctypes/simple.rs crates/vm/src/stdlib/_ctypes/pointer.rs crates/vm/src/stdlib/_ctypes/structure.rs crates/vm/src/stdlib/_ctypes/union.rs -S
Repository: RustPython/RustPython
Length of output: 43119
Handle array argtypes before _type_ validation. PyCArray types store their element type in _type_, so self.as_object().get_attr("_type_", vm) returns a type object here, not a string code. downcast_ref::<PyStr>() then fails and rejects entries like argtypes = [c_int * 3] before from_param can decay them to a pointer.
🤖 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 `@crates/vm/src/stdlib/_ctypes/function.rs` around lines 211 - 241, Update
ArgumentType for PyTypeRef in convert_object to recognize PyCArray before the
generic _type_ string validation, since array argtypes carry an element type
object in _type_ rather than a ctypes code string. Adjust the type-check branch
around the fast_issubclass and get_attr("_type_", vm) logic so arrays are
accepted and can still be decayed through from_param into a pointer, instead of
failing the PyStr downcast for cases like c_int * 3.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
The by-value struct/union argument path snapshotted the instance bytes and returned `None` for the keep-alive slot, dropping the `from_param` result before the foreign call. When the snapshot embeds pointers into buffers owned by that object's keep-alive set, the call could read freed memory. Return the converted instance as the keep-alive owner. Assisted-by: Claude
A zero-sized `Structure`/`Union` argument reaches the aggregate lowering with `layout.size() == 0` and an empty buffer, which passed the `buffer.len() < expected` check and then panicked on `&buffer[0]`. Use `buffer.first().unwrap_or(&0u8)`; libffi reads nothing for a zero-size type. Assisted-by: Claude
Uh oh!
There was an error while loading. Please reload this page.
1 of 2 tasks
youknowone
added a commit
that referenced
this pull request
Sep 16, 2026
…#8235) * host_env: add unified high-level `call` foreign-call API Add a libffi-hiding foreign-call entry point that takes ctypes type codes and recursive layouts plus raw buffers instead of libffi `Type`/`Arg`, to become the single call path for the VM `_ctypes` (a later change) and other consumers. Also bring in the `callproc_simple` helper so this host_env copy stays byte-identical to the pyre-dev copy during the migration. - `CTypeLayout` (Simple/Pointer/Struct/Union/Array/Opaque) with `size()` and an internal libffi-type lowering built on `ffi_type_for_layout`. - `CallArg` (Typed/Int/Double/Pointer/Aggregate), `CallRet` (Void/Code/Pointer/Aggregate), `CallOptions` (use_errno/use_last_error), `CallValue` (Void/Scalar/Pointer/Aggregate), and `CallError`. - `call(addr, &[CallArg], CallRet, CallOptions) -> Result<CallValue, CallError>`, built on the existing `Cif`/`ffi_*` primitives: the errno / last-error swap wraps only the raw call, and by-value aggregate arguments and returns go through `Cif::call_return_into`. - 16 ABI unit tests over local `extern "C"` functions: scalar parity, by-value struct / nested / array-in-struct / {f32,f32} / large-struct arguments, small / odd / large struct returns, union size, the errno window, and the null-pointer / unknown-code / short-buffer errors. `callproc` and `callproc_simple` are left in place; both are removed once the migration onto `call` completes. Assisted-by: Claude * _ctypes: route the VM call path through host_env call() Replace the VM's direct libffi `Type`/`Arg` marshalling with the unified host_env `call` entry point. Behavior is preserved on the tested paths: structs and unions still pass by pointer and struct returns still use the register-size approximation (passing/returning aggregates by value is a later change). - base.rs: `FfiArgValue` → `CArgValue` (Typed{code,bytes}/Int/Double/Pointer) with `as_call_arg()`; the paramfunc producers emit `CArgValue`, snapshotting buffer bytes at conversion time so no lock is held across the call. - _ctypes.rs: `CArgObject` carries `value: CArgValue` plus a `keep` slot for the from_param z/Z/P null-terminated-copy keepalive that no longer rides in the value; `PyCArg` repr reconstructs the scalar for identical output; byref updated. - simple.rs: `to_ffi_value` → `to_carg_value`; from_param emits `CArgValue` and its keepalive. - function.rs: `Argument{value, keep}`; `ArgumentType` yields a `CArgValue` (with the "unsupported argument type" check moved up front); `CallInfo`'s return fields collapse to `RetSpec` (Void/Pointer/Code) reproducing the exact return type and result dispatch; `ctypes_callproc` builds `CallArg`/`CallRet`/ `CallOptions` and invokes `host_env::ctypes::call`; the errno/last-error swap moves into `CallOptions` (the `with_swapped_*` wrappers are removed); `convert_raw_result` consumes `CallValue`. - extra_tests/snippets/stdlib_ctypes_calls.py: libc calls over the live FFI path (abs/strlen/sqrt, c_char_p/c_void_p returns, a use_errno round-trip); output matches CPython. Two intentional deltas: a NULL function pointer now raises ValueError instead of a debug assertion, and a c_char passed positionally without argtypes now zero-extends via its type code rather than sign-extends via the old tag path, which also makes it agree with the with-argtypes path. The old host_env `callproc`/`ffi_*` and `StgInfo`'s libffi field types remain for now; they are removed when by-value aggregates land. Assisted-by: Claude * _ctypes: pass and return structs and unions by value Flip aggregate arguments and returns from the pointer / register-size approximation to true by-value passing through the host_env `call` entry point, and carry the driving layouts as host_env `CTypeLayout`. - base.rs: `StgInfo.ffi_field_types: Vec<FfiType>` becomes `field_layouts: Vec<CTypeLayout>`, still built incrementally from the base class so struct inheritance is reflected. `StgInfo::to_ffi_type()` is replaced by a `type_layout(ty, &stg, vm)` helper that reads a type's own layout (aggregates from `field_layouts`, arrays recurse into the element type, simple types from their `_type_` code); it takes the already-borrowed `StgInfo` so it never re-locks the type. `CArgValue` gains an `Aggregate { layout, bytes }` variant lowering to `CallArg::Aggregate`, and `struct_union_paramfunc` snapshots the instance bytes into it (tag 'V'). - structure.rs / union.rs: collect each field's `type_layout` into `field_layouts` and store it on the finalized `StgInfo`. - function.rs: `convert_object` passes a struct/union argument by value (snapshot bytes + argtype layout); a `byref()` result is still a pointer. `RetSpec` gains `Aggregate(CTypeLayout)`; `compute_ret_spec` returns it for a struct/union restype; `ctypes_callproc` lowers it to `CallRet::Aggregate`. `convert_raw_result` now drops the restype `StgInfo` read guard before constructing the result instance: instance construction write-locks the type's `StgInfo` to finalize it, which otherwise self-deadlocks against the held read guard (latent since the register-size return path, unreachable until a struct was actually returned by value). - _ctypes.rs: a 'V' cparam now reprs as `<cparam 'V' at 0x..>` (the object-address default), matching PyCArg_repr; the aggregate `CArgValue` routes through that arm. - extra_tests/snippets/stdlib_ctypes_byvalue.py: div/imaxdiv struct returns (8- and 16-byte), inet_ntoa struct and union arguments, with and without argtypes; output matches CPython. Also sorts the imports in the sibling stdlib_ctypes_calls.py snippet (ruff isort). `_pack_` / `_swappedbytes_` layouts stay approximate — `CTypeLayout` carries no explicit field offsets, as the previous libffi field-type path did not either. host_env is untouched, so both `ctypes.rs` copies stay byte-identical. test_ctypes is unchanged at run=322 skipped=58. Assisted-by: Claude * host_env: remove the pre-`call` foreign-call scaffolding With the VM `_ctypes` and pyre both routed through `call`, delete the foreign-call paths and libffi-type builders that no longer have a consumer: - `callproc` (the low-level libffi `Vec<Type>` / `&[Arg]` entry) and its `CallResult` return type plus `call_result_bytes`. - `callproc_simple` and its `SimpleArg` / `SimpleRestype` / `SimpleCallError` types (the pointer-only slice added for pyre's first cut), and the `lookup_function_symbol_addr_str` helper, along with their tests. - `ffi_type_for_layout` + `CTypeParamKind`, `ffi_type_from_format`, `ffi_type_from_tag`, and `ffi_type_for_return_size` — the aggregate/simple libffi-type builders the old marshalling used. `call` and `CTypeLayout` build their own libffi types from `ffi_type_from_code` and the small `ffi_{pointer,byte_struct,repeat,void,i32,f64}_type` helpers, which stay. `simple_type_chars` stays (still used by pyre's simple-type validation). The `call` doc no longer references `callproc_simple`. A stale comment in the VM's result decoder that named `call_result_bytes` is reworded. host_env drops from 4051 to 3535 lines; the `call` ABI unit tests remain (24 tests). test_ctypes is unchanged at run=322 skipped=58. Assisted-by: Claude * host_env: read `call` scalar returns as a full register, use `add_i32` `call` read `Code` returns through `low::ffi_arg`. The `libffi_sys` binding types `ffi_arg` as `c_ulong`, which is 4 bytes under LLP64 (Windows x64), so `low::call` truncated 8-byte returns (`q`/`Q`/`d`) to the low 4 bytes. `calls_f64_scalar` and `passes_large_struct_by_value` failed on windows-2025. Read a full register (`u64`) instead; `decode_type_code` still slices the leading bytes each type code needs. No change on LP64, where `ffi_arg` is already 8 bytes. Add `typed_two_scalar_args`, which calls the previously-unused `add_i32` ABI helper (two-argument scalar path); the helper tripped `-D warnings` dead-code on clippy. Assisted-by: Claude * _ctypes: skip the by-value snippet on Windows `stdlib_ctypes_byvalue.py` reaches libc through `CDLL(None)` and calls `div`/`imaxdiv`/`inet_ntoa`, none of which resolve that way on Windows, so the snippet aborted on windows-2025. Guard it like `stdlib_ctypes_calls.py`: print "OK" and exit before touching `CDLL`. The by-value path is covered on Windows by test_ctypes. Assisted-by: Claude * _ctypes: keep the converted argument alive for by-value aggregates The by-value struct/union argument path snapshotted the instance bytes and returned `None` for the keep-alive slot, dropping the `from_param` result before the foreign call. When the snapshot embeds pointers into buffers owned by that object's keep-alive set, the call could read freed memory. Return the converted instance as the keep-alive owner. Assisted-by: Claude * host_env: avoid indexing an empty by-value aggregate buffer in `call` A zero-sized `Structure`/`Union` argument reaches the aggregate lowering with `layout.size() == 0` and an empty buffer, which passed the `buffer.len() < expected` check and then panicked on `&buffer[0]`. Use `buffer.first().unwrap_or(&0u8)`; libffi reads nothing for a zero-size type. Assisted-by: Claude
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
None yet
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.