ctypes: unify the foreign-call path on a single host_env call() API#8235
Open
youknowone wants to merge 6 commits into
Open
ctypes: unify the foreign-call path on a single host_env call() API#8235youknowone wants to merge 6 commits into
call() API#8235youknowone wants to merge 6 commits into
Conversation
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
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
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
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
Contributor
📝 WalkthroughWalkthroughThis PR replaces the libffi-based ChangesForeign-call API rewrite (host_env + vm ctypes)
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Python
participant PyCFuncPtr
participant ctypes_callproc
participant HostCall as host_env::call
Python->>PyCFuncPtr: call(args)
PyCFuncPtr->>ctypes_callproc: addr, arguments, RetSpec, CallOptions
ctypes_callproc->>HostCall: call(addr, CallArg[], CallRet, CallOptions)
HostCall->>HostCall: lower args, validate aggregate size, swap errno
HostCall-->>ctypes_callproc: CallValue or CallError
ctypes_callproc-->>PyCFuncPtr: CallValue / mapped PyErr
PyCFuncPtr->>PyCFuncPtr: build_result(CallValue)
PyCFuncPtr-->>Python: converted Python result
Suggested reviewers: 🚥 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 |
`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
Contributor
There was a problem hiding this comment.
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
Comment on lines
+2124
to
+2155
| CallArg::Aggregate { layout, buffer } => { | ||
| let expected = layout.size(); | ||
| if buffer.len() < expected { | ||
| return Err(CallError::BufferTooSmall { | ||
| expected, | ||
| got: buffer.len(), | ||
| }); | ||
| } | ||
| ffi_arg_types.push(layout.to_ffi_type()?); | ||
| lowered.push(Lowered::Aggregate(buffer)); | ||
| } | ||
| } | ||
| CallResult::Value(val) => { | ||
| let bytes = val.to_ne_bytes(); | ||
| Some((bytes.to_vec(), core::mem::size_of_val(val))) | ||
| } | ||
|
|
||
| let ffi_return_type = match ret { | ||
| CallRet::Void => ffi_void_type(), | ||
| CallRet::Code(code) => { | ||
| ffi_type_from_code(code).ok_or_else(|| CallError::UnknownTypeCode(code.to_string()))? | ||
| } | ||
| CallRet::Pointer => ffi_pointer_type(), | ||
| CallRet::Aggregate(layout) => layout.to_ffi_type()?, | ||
| }; | ||
|
|
||
| // Pass 2: borrow the completed `lowered` as libffi Args. No reallocation can | ||
| // now invalidate the scalar borrows; aggregate Args point at caller buffers. | ||
| let ffi_args: Vec<Arg<'_>> = lowered | ||
| .iter() | ||
| .map(|arg| match arg { | ||
| Lowered::Scalar(value) => ffi_arg_from_value(value), | ||
| Lowered::Aggregate(buffer) => Arg::new(&buffer[0]), | ||
| }) | ||
| .collect(); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and locate the lowering / conversion paths.
git ls-files crates/host_env/src/ctypes.rs
ast-grep outline crates/host_env/src/ctypes.rs --view expanded || true
# Show the relevant line ranges around the reported area.
sed -n '2085,2175p' crates/host_env/src/ctypes.rs
# Find where aggregate buffers are created and whether zero-sized aggregates are handled.
rg -n "CallArg::Aggregate|BufferTooSmall|layout\.size\(\)|Arg::new\(&buffer\[0\]\)|convert_object|Structure|Union|_fields_" crates/host_env/src/ctypes.rsRepository: RustPython/RustPython
Length of output: 19690
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect layout sizing and any object-to-aggregate conversion paths.
sed -n '1888,1970p' crates/host_env/src/ctypes.rs
printf '\n---\n'
sed -n '3000,3535p' crates/host_env/src/ctypes.rs
# Search for other zero-size aggregate handling or construction sites.
rg -n "size\(\) == 0|layout\.size\(\)|empty|_fields_|Structure|Union|CallArg::Aggregate|CallRet::Aggregate" crates/host_env/src/ctypes.rsRepository: RustPython/RustPython
Length of output: 22491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where CTypeLayout values are constructed from Python-side ctypes objects.
rg -n "CTypeLayout::Struct|CTypeLayout::Union|CTypeLayout::Opaque|CTypeLayout::Array|CTypeLayout::Pointer|CTypeLayout::Simple|_fields_|_pack_|_align_|sizeof|offset" crates/host_env/src/ctypes.rs
# Inspect the construction / conversion logic if found.
sed -n '1040,1888p' crates/host_env/src/ctypes.rsRepository: RustPython/RustPython
Length of output: 32776
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find every construction / use site of CTypeLayout outside ctypes.rs.
rg -n "CTypeLayout::|CTypeLayout\b" cratesRepository: RustPython/RustPython
Length of output: 5590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the ctypes layout construction and aggregate conversion paths.
sed -n '220,290p' crates/vm/src/stdlib/_ctypes/base.rs
printf '\n---\n'
sed -n '1890,1990p' crates/vm/src/stdlib/_ctypes/base.rs
printf '\n---\n'
sed -n '260,320p' crates/vm/src/stdlib/_ctypes/function.rsRepository: RustPython/RustPython
Length of output: 8826
Guard zero-sized aggregate arguments here.
buffer.len() < expected still lets expected == 0 through, so an empty Structure/Union can reach Arg::new(&buffer[0]) and panic in pass 2. Reject or special-case zero-sized by-value aggregates instead. crates/host_env/src/ctypes.rs:2124-2153
🤖 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/host_env/src/ctypes.rs` around lines 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.
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.
🎯 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.
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.
🎯 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 -SRepository: 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.rsRepository: 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 -SRepository: 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 -SRepository: 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 -SRepository: 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.
Comment on lines
+270
to
285
| // For structure/union types, pass the aggregate by value: snapshot the | ||
| // instance bytes and build its call layout from the argtype. A byref() | ||
| // result is a CArgObject and was already handled above (stays a pointer). | ||
| if self.fast_issubclass(PyCStructure::static_type()) | ||
| || self.fast_issubclass(PyCUnion::static_type()) | ||
| { | ||
| if let Some(cdata) = converted.downcast_ref::<PyCData>() { | ||
| let bytes = cdata.buffer.read().to_vec(); | ||
| let layout = self.stg_info_opt().map_or_else( | ||
| || CTypeLayout::Opaque { size: bytes.len() }, | ||
| |stg| super::base::type_layout(self, &stg, vm), | ||
| ); | ||
| return Ok((CArgValue::aggregate(layout, bytes), None)); | ||
| } | ||
| return Ok((convert_to_pointer(&converted, vm)?, None)); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section and nearby helpers.
FILE='crates/vm/src/stdlib/_ctypes/function.rs'
wc -l "$FILE"
sed -n '220,340p' "$FILE"
# Find how the return value's keep slot is consumed in this file.
rg -n "keep|CArgValue::aggregate|from_param|kept_refs|objects|CArgObject" "$FILE"
# Map the file structure to spot related helpers.
ast-grep outline "$FILE" --view expandedRepository: RustPython/RustPython
Length of output: 14312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE='crates/vm/src/stdlib/_ctypes/function.rs'
# Inspect the call path that builds and uses Argument.keep.
sed -n '1180,1325p' "$FILE"
sed -n '1325,1405p' "$FILE"
# Inspect PyCData ownership fields and any code that populates _objects/objects.
rg -n "struct PyCData|objects.write|_objects|kept_refs|from_param|keep:" crates/vm/src/stdlib/_ctypes -n
sed -n '520,640p' crates/vm/src/stdlib/_ctypes/base.rs
sed -n '640,720p' crates/vm/src/stdlib/_ctypes/base.rs
# Check whether structure/union from_param implementations can return new objects.
rg -n "from_param" crates/vm/src/stdlib/_ctypes -g '!**/target/**'Repository: RustPython/RustPython
Length of output: 30030
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '120,210p' crates/vm/src/stdlib/_ctypes/structure.rs
sed -n '360,430p' crates/vm/src/stdlib/_ctypes/union.rs
sed -n '220,360p' crates/vm/src/stdlib/_ctypes/simple.rs
# Look for constructors that create new CData instances with owned/buffered refs.
rg -n "from_bytes_with_length|from_buffer_copy_impl|from_buffer_impl|from_param\\(" crates/vm/src/stdlib/_ctypes/{structure.rs,union.rs,simple.rs,array.rs,base.rs}Repository: RustPython/RustPython
Length of output: 13702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,540p' crates/vm/src/stdlib/_ctypes/base.rs
sed -n '715,870p' crates/vm/src/stdlib/_ctypes/base.rs
sed -n '870,930p' crates/vm/src/stdlib/_ctypes/base.rsRepository: RustPython/RustPython
Length of output: 12974
Keep the converted PyCData alive for by-value struct/union args. This branch copies only the raw bytes; if from_param returns a fresh ctypes object with nested _objects/kept_refs, dropping it here can free backing memory before the FFI call uses the copied pointer values. Return Some(converted.clone()) with the aggregate.
🤖 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 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Introduce a single libffi-hiding foreign-call entry point in
host_env(
ctypes::call) and route the VM_ctypesmodule through it, replacing theper-call libffi
Type/Argmarshalling. On top of that, pass and returnStructure/Unionby value, and delete the now-unused pre-callscaffolding.Commits
callforeign-call API — acall(addr, &[CallArg], CallRet, CallOptions) -> Result<CallValue, CallError>entry point that takes ctypes type codes and recursive
CTypeLayouts plusraw buffers instead of libffi
Type/Arg. Built on the existingCif/ffi_*primitives; aggregate returns go throughCif::call_return_into.Adds 16 ABI unit tests over local
extern "C"functions.call()— abehavior-preserving migration of
ctypes_callprocand the argument/returnmarshalling onto
call; the errno / last-error swap moves intoCallOptions.arguments lower to
CallArg::Aggregateand struct/union returns toCallRet::Aggregate.StgInfocarries per-fieldCTypeLayouts builtincrementally from the base class, so struct inheritance is reflected. Also
fixes a latent self-deadlock in the result path (a restype
StgInforeadguard was held across result-instance construction, which write-locks the
same
StgInfoto finalize it — unreachable until a struct was actuallyreturned by value).
callforeign-call scaffolding — delete theold
callproc/callproc_simplepaths and the libffi-type builders(
ffi_type_for_layout,CTypeParamKind,ffi_type_from_tag, …) that nolonger have a consumer.
Testing
python -m test test_ctypes:run=322 skipped=58, unchanged across all fourcommits.
extra_tests/snippets/stdlib_ctypes_calls.py(libc calls over the liveFFI path) and
stdlib_ctypes_byvalue.py(div/imaxdivstruct returns,inet_ntoastruct and union arguments); output matches CPython.pass.
Known limitation carried over from the previous libffi field-type path:
_pack_/_swappedbytes_layouts stay approximate, sinceCTypeLayoutcarries no explicit field offsets.
Opening as a draft for early visibility / CI.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
errnoand platform-specific last-error behavior during ctypes calls.