Enable ctypes #6457
Enable ctypes #6457
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughMajor refactoring of the ctypes module introducing new FFI calling machinery (Argument/OutBuffers abstractions), buffer protocol support across pointer/array types, enhanced error handling for errno/last-error, library loading improvements, field descriptor removal, and type system restructuring with platform-specific behaviors for Unix/Windows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin ctypes |
youknowone
force-pushed
the
ctypes
branch
8 times, most recently
from
bb11608 to
d28fddf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/vm/src/stdlib/ctypes/pointer.rs (1)
687-777: Integer writes viawrite_value_at_addresscan panic on overflow; returnOverflowErrorinstead.The branches for integer sizes use
expect("int too large"):let i = int_val.as_bigint(); match size { 1 => *ptr = i.to_u8().expect("int too large"); 2 => write_unaligned(ptr as *mut i16, i.to_i16().expect("int too large")); 4 => write_unaligned(ptr as *mut i32, i.to_i32().expect("int too large")); 8 => write_unaligned(ptr as *mut i64, i.to_i64().expect("int too large")); … }For a large
ctypes.c_intassigned through a pointer (e.g.ptr[0] = 1 << 1000), this will panic the Rust VM instead of raising anOverflowErrorlike CPython.Recommend converting these to checked conversions that map failure to a Python exception, e.g.:
- if let Ok(int_val) = value.try_int(vm) { - let i = int_val.as_bigint(); - match size { - 1 => { - *ptr = i.to_u8().expect("int too large"); - } + if let Ok(int_val) = value.try_int(vm) { + let i = int_val.as_bigint(); + match size { + 1 => { + let v = i.to_u8().ok_or_else(|| vm.new_overflow_error("int too large for 1‑byte field"))?; + *ptr = v; + } 2 => { - std::ptr::write_unaligned(ptr as *mut i16, i.to_i16().expect("int too large")); + let v = i.to_i16().ok_or_else(|| vm.new_overflow_error("int too large for 2‑byte field"))?; + std::ptr::write_unaligned(ptr as *mut i16, v); } 4 => { - std::ptr::write_unaligned(ptr as *mut i32, i.to_i32().expect("int too large")); + let v = i.to_i32().ok_or_else(|| vm.new_overflow_error("int too large for 4‑byte field"))?; + std::ptr::write_unaligned(ptr as *mut i32, v); } 8 => { - std::ptr::write_unaligned(ptr as *mut i64, i.to_i64().expect("int too large")); + let v = i.to_i64().ok_or_else(|| vm.new_overflow_error("int too large for 8‑byte field"))?; + std::ptr::write_unaligned(ptr as *mut i64, v); }Same concern applies to the
P/z/Zpointer cases usingto_usize().expect(...). This is user‑controlled input, so avoiding VM panics is important.crates/vm/src/stdlib/ctypes/simple.rs (1)
722-976: Large integer conversions invalue_to_bytes_endiancan panic instead of raisingOverflowError.For many ctypes integer and pointer codes, you now do:
let v = int_val.as_bigint().to_i128().expect("int too large") as i16; // or let v = int_val.as_bigint().to_usize().expect("int too large for pointer");If a user passes a value outside the target type’s range (e.g.
c_short(1 << 1000)orc_void_p(1 << 200)), this will panic the Rust VM instead of raising a PythonOverflowErroras CPython does.Because
value_to_bytes_endian()is used fromPyCSimple::slot_new,set_value, andfrom_param, this is a user‑reachable crash.To fix this, consider:
- Changing
value_to_bytes_endianto returnPyResult<Vec<u8>>so you can propagateOverflowErrorfrom here; or- Pre‑checking ranges in callers before calling into this helper.
Pattern sketch for one case:
- "h" => { - if let Ok(int_val) = value.try_index(vm) { - let v = int_val.as_bigint().to_i128().expect("int too large") as i16; - return to_bytes!(v); - } + "h" => { + if let Ok(int_val) = value.try_index(vm) { + if let Some(v) = int_val.as_bigint().to_i16() { + return to_bytes!(v); + } + // Caller can turn this into vm.new_overflow_error("...") if you propagate Result + } vec![0; 2] }Similar guarded conversions are needed for all the
to_i128().expect(...)/to_usize().expect(...)paths (integer and pointer types). Right now this is a correctness and robustness issue that should be addressed before relying on these paths heavily.crates/vm/src/stdlib/ctypes.rs (1)
574-616:dlopennow returns a cache ID, butdlclose/dlsymcall libc with that ID as a pointer (likely UB).On Unix:
dlopen(...)useslibrary::libcache().get_or_insert_lib_with_mode/insert_raw_handleand returns ausizehandle that is an index/key intoExternalLibs, not necessarily the raw*mut c_voidfromlibc::dlopen.free_library(handle)correctly passes that ID todrop_lib(handle).However, the new functions:
fn dlclose(handle: usize, vm: &VirtualMachine) -> PyResult<()> { let result = unsafe { libc::dlclose(handle as *mut libc::c_void) }; … } fn dlsym(handle: usize, name: PyStrRef, vm: &VirtualMachine) -> PyResult<usize> { let ptr = unsafe { libc::dlsym(handle as *mut libc::c_void, symbol_name.as_ptr()) }; … }treat the same
usizeas a rawvoid*and pass it straight todlclose/dlsym. If Python code calls_ctypes.dlopenand then_ctypes.dlsym/_ctypes.dlclosewith that returned value (which matches CPython’s API shape), you’ll end up calling libc with an invalid pointer, which is undefined behavior and may segfault.You probably want one of:
- Make
dlopenreturn the raw*mut c_voidand keep the cache keyed separately, or- Keep returning the cache ID but have
dlclose/dlsymlook up the real OS handle vialibcache().get_lib(id)and then calllibc::dlclose/dlsymon that underlying handle (or delegate toExternalLibsmethods and avoid calling libc here entirely).As it stands, these APIs are inconsistent and unsafe if used together.
Also applies to: 626-665
crates/vm/src/stdlib/ctypes/function.rs (1)
1946-1976: Incomplete errno swapping implementation.The errno save/restore logic has TODO comments indicating incomplete implementation. The current code:
- Saves the current errno before the call
- Restores it after the call
But it doesn't implement the full CPython behavior which requires:
- Swap the OS errno with the ctypes thread-local stored errno before the call
- After the call, save the new OS errno to ctypes storage and restore the previous OS errno
This means
ctypes.get_errno()andctypes.set_errno()won't work correctly with callbacks.Would you like me to help design the thread-local errno storage mechanism, or should this be tracked as a follow-up issue?
🧹 Nitpick comments (5)
crates/vm/src/stdlib/ctypes/array.rs (1)
17-28: PEP 3118 itemsize derivation looks correct; consider minor robustness tweaks.The new
get_size_from_format()+AsBufferlogic correctly recoversitemsizefrom the format string for empty/zero-dim arrays and usesshapeto compute strides, matching CPython’s “element-size, not 0” behavior for empty arrays and zero strides when any dim is 0.Two small robustness points you may want to consider:
shape.iter().product()onusizewill wrap on overflow. Givenshapeultimately comes from validatedStgInfo, it’s probably fine in practice, but if you expect user‑supplied huge dimensions, a checked product with a gracefulOverflowErrorwould be safer.get_size_from_format()defaultsitemsizeto1iftype_inforeturnsNone. With the currenttype_infocoverage this shouldn’t trigger, but if new complex format codes are introduced later, it might be safer to pass a default (e.g.stg_info.element_size) instead of silently using 1.Functionally this is fine to merge; the above are defensive improvements you can apply later if needed.
Also applies to: 1081-1112
crates/vm/src/stdlib/ctypes/pointer.rs (1)
781-803: PointerAsBufferimplementation is reasonable, minor format default nit.Exposing
_Pointeras a scalar buffer of lengthsizeof(void*)with the storedstg_info.formatandndim=0is consistent with the rest of the ctypes buffer protocol; usingCDATA_BUFFER_METHODSensures writes go through the existing CData machinery.The only minor nit is the default
formatof"&B"when noStgInfo.formatis present;"&P"(pointer) might be a slightly more descriptive fallback, but this doesn’t affect correctness and can be adjusted later if desired.crates/vm/src/stdlib/ctypes/library.rs (3)
73-77: RedundantDropimplementation.The explicit
close()call inDropis unnecessary. WhenSharedLibraryis dropped, thePyMutex<Option<Library>>field is automatically dropped, which will drop the containedLibraryif present.The current implementation adds lock contention overhead without benefit.
🔎 Consider removing the explicit Drop impl
-impl Drop for SharedLibrary { - fn drop(&mut self) { - self.close(); - } -}
94-115: Minor inefficiency: Library loaded before checking cache.The library is loaded via
SharedLibrary::new()before checking if an open copy already exists in the cache. If the library is already cached and open, the newly loaded handle is immediately dropped.Consider checking the cache first to avoid unnecessary library loads.
🔎 Proposed optimization
pub fn get_or_insert_lib( &mut self, library_path: impl AsRef<OsStr>, _vm: &VirtualMachine, ) -> Result<(usize, &SharedLibrary), libloading::Error> { - let new_lib = SharedLibrary::new(library_path)?; - let key = new_lib.get_pointer(); - - match self.libraries.get(&key) { - Some(l) => { - if l.is_closed() { - self.libraries.insert(key, new_lib); - } - } - _ => { - self.libraries.insert(key, new_lib); - } - }; - - Ok((key, self.libraries.get(&key).expect("just inserted"))) + let new_lib = SharedLibrary::new(&library_path)?; + let key = new_lib.get_pointer(); + + if let Some(existing) = self.libraries.get(&key) { + if !existing.is_closed() { + // Return existing open library, drop new_lib + return Ok((key, existing)); + } + } + self.libraries.insert(key, new_lib); + Ok((key, self.libraries.get(&key).expect("just inserted"))) }
138-138: Inconsistent error handling:unwrap()vsexpect().Line 138 uses
.unwrap()while the Windows equivalent (line 114) uses.expect("just inserted"). For consistency and better panic messages, preferexpect()with a descriptive message.🔎 Proposed fix
- Ok((key, self.libraries.get(&key).unwrap())) + Ok((key, self.libraries.get(&key).expect("just inserted")))
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (70)
Lib/ctypes/__init__.pyis excluded by!Lib/**Lib/ctypes/_endian.pyis excluded by!Lib/**Lib/ctypes/test/__init__.pyis excluded by!Lib/**Lib/ctypes/test/__main__.pyis excluded by!Lib/**Lib/ctypes/test/test_delattr.pyis excluded by!Lib/**Lib/ctypes/test/test_simplesubclasses.pyis excluded by!Lib/**Lib/ctypes/util.pyis excluded by!Lib/**Lib/test/test_ctypes.pyis excluded by!Lib/**Lib/test/test_ctypes/__init__.pyis excluded by!Lib/**Lib/test/test_ctypes/__main__.pyis excluded by!Lib/**Lib/test/test_ctypes/_support.pyis excluded by!Lib/**Lib/test/test_ctypes/test_aligned_structures.pyis excluded by!Lib/**Lib/test/test_ctypes/test_anon.pyis excluded by!Lib/**Lib/test/test_ctypes/test_array_in_pointer.pyis excluded by!Lib/**Lib/test/test_ctypes/test_arrays.pyis excluded by!Lib/**Lib/test/test_ctypes/test_as_parameter.pyis excluded by!Lib/**Lib/test/test_ctypes/test_bitfields.pyis excluded by!Lib/**Lib/test/test_ctypes/test_buffers.pyis excluded by!Lib/**Lib/test/test_ctypes/test_bytes.pyis excluded by!Lib/**Lib/test/test_ctypes/test_byteswap.pyis excluded by!Lib/**Lib/test/test_ctypes/test_c_simple_type_meta.pyis excluded by!Lib/**Lib/test/test_ctypes/test_callbacks.pyis excluded by!Lib/**Lib/test/test_ctypes/test_cast.pyis excluded by!Lib/**Lib/test/test_ctypes/test_cfuncs.pyis excluded by!Lib/**Lib/test/test_ctypes/test_checkretval.pyis excluded by!Lib/**Lib/test/test_ctypes/test_delattr.pyis excluded by!Lib/**Lib/test/test_ctypes/test_dlerror.pyis excluded by!Lib/**Lib/test/test_ctypes/test_errno.pyis excluded by!Lib/**Lib/test/test_ctypes/test_find.pyis excluded by!Lib/**Lib/test/test_ctypes/test_frombuffer.pyis excluded by!Lib/**Lib/test/test_ctypes/test_funcptr.pyis excluded by!Lib/**Lib/test/test_ctypes/test_functions.pyis excluded by!Lib/**Lib/test/test_ctypes/test_incomplete.pyis excluded by!Lib/**Lib/test/test_ctypes/test_init.pyis excluded by!Lib/**Lib/test/test_ctypes/test_internals.pyis excluded by!Lib/**Lib/test/test_ctypes/test_keeprefs.pyis excluded by!Lib/**Lib/test/test_ctypes/test_libc.pyis excluded by!Lib/**Lib/test/test_ctypes/test_loading.pyis excluded by!Lib/**Lib/test/test_ctypes/test_macholib.pyis excluded by!Lib/**Lib/test/test_ctypes/test_memfunctions.pyis excluded by!Lib/**Lib/test/test_ctypes/test_numbers.pyis excluded by!Lib/**Lib/test/test_ctypes/test_objects.pyis excluded by!Lib/**Lib/test/test_ctypes/test_parameters.pyis excluded by!Lib/**Lib/test/test_ctypes/test_pep3118.pyis excluded by!Lib/**Lib/test/test_ctypes/test_pickling.pyis excluded by!Lib/**Lib/test/test_ctypes/test_pointers.pyis excluded by!Lib/**Lib/test/test_ctypes/test_prototypes.pyis excluded by!Lib/**Lib/test/test_ctypes/test_python_api.pyis excluded by!Lib/**Lib/test/test_ctypes/test_random_things.pyis excluded by!Lib/**Lib/test/test_ctypes/test_refcounts.pyis excluded by!Lib/**Lib/test/test_ctypes/test_repr.pyis excluded by!Lib/**Lib/test/test_ctypes/test_returnfuncptrs.pyis excluded by!Lib/**Lib/test/test_ctypes/test_simplesubclasses.pyis excluded by!Lib/**Lib/test/test_ctypes/test_sizes.pyis excluded by!Lib/**Lib/test/test_ctypes/test_slicing.pyis excluded by!Lib/**Lib/test/test_ctypes/test_stringptr.pyis excluded by!Lib/**Lib/test/test_ctypes/test_strings.pyis excluded by!Lib/**Lib/test/test_ctypes/test_struct_fields.pyis excluded by!Lib/**Lib/test/test_ctypes/test_structures.pyis excluded by!Lib/**Lib/test/test_ctypes/test_unaligned_structures.pyis excluded by!Lib/**Lib/test/test_ctypes/test_unicode.pyis excluded by!Lib/**Lib/test/test_ctypes/test_unions.pyis excluded by!Lib/**Lib/test/test_ctypes/test_values.pyis excluded by!Lib/**Lib/test/test_ctypes/test_varsize_struct.pyis excluded by!Lib/**Lib/test/test_ctypes/test_win32.pyis excluded by!Lib/**Lib/test/test_ctypes/test_win32_com_foreign_func.pyis excluded by!Lib/**Lib/test/test_ctypes/test_wintypes.pyis excluded by!Lib/**Lib/test/test_exceptions.pyis excluded by!Lib/**Lib/test/test_os.pyis excluded by!Lib/**Lib/test/test_threading.pyis excluded by!Lib/**
📒 Files selected for processing (10)
crates/vm/src/stdlib/ctypes.rs(6 hunks)crates/vm/src/stdlib/ctypes/array.rs(3 hunks)crates/vm/src/stdlib/ctypes/base.rs(6 hunks)crates/vm/src/stdlib/ctypes/field.rs(0 hunks)crates/vm/src/stdlib/ctypes/function.rs(26 hunks)crates/vm/src/stdlib/ctypes/library.rs(4 hunks)crates/vm/src/stdlib/ctypes/pointer.rs(9 hunks)crates/vm/src/stdlib/ctypes/simple.rs(12 hunks)crates/vm/src/stdlib/ctypes/structure.rs(0 hunks)crates/vm/src/version.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/vm/src/stdlib/ctypes/structure.rs
- crates/vm/src/stdlib/ctypes/field.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/version.rscrates/vm/src/stdlib/ctypes/array.rscrates/vm/src/stdlib/ctypes/library.rscrates/vm/src/stdlib/ctypes/simple.rscrates/vm/src/stdlib/ctypes/pointer.rscrates/vm/src/stdlib/ctypes.rscrates/vm/src/stdlib/ctypes/base.rscrates/vm/src/stdlib/ctypes/function.rs
🧠 Learnings (4)
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
crates/vm/src/stdlib/ctypes/library.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/vm/src/stdlib/ctypes.rscrates/vm/src/stdlib/ctypes/function.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/ctypes.rs
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/ctypes.rs
🧬 Code graph analysis (4)
crates/vm/src/stdlib/ctypes/array.rs (3)
crates/vm/src/stdlib/ctypes.rs (3)
type_info(144-238)t(55-55)cls(696-696)crates/vm/src/builtins/list.rs (1)
__class_getitem__(340-342)crates/vm/src/builtins/genericalias.rs (2)
args(102-102)from_args(101-108)
crates/vm/src/stdlib/ctypes/pointer.rs (5)
crates/vm/src/stdlib/ctypes/union.rs (2)
base(114-114)as_buffer(554-577)crates/vm/src/stdlib/ctypes/base.rs (28)
std(376-376)std(377-377)std(2149-2149)std(2242-2242)proto(291-293)proto(928-928)proto(993-993)proto(1540-1540)proto(1557-1557)new(149-165)new(1246-1254)value(862-863)value(871-871)value(871-871)value(893-893)value(907-907)value(922-922)value(932-932)value(1361-1361)value(1369-1369)value(1377-1377)value(1426-1426)value(1441-1441)value(1455-1455)value(1469-1469)value(1494-1494)size(680-682)size(1671-1673)crates/vm/src/stdlib/ctypes/function.rs (8)
std(54-54)std(61-61)std(73-73)std(126-126)new(2032-2099)value(87-87)f(898-898)as_buffer(1656-1680)crates/vm/src/stdlib/ctypes/simple.rs (2)
new(239-245)as_buffer(1405-1426)crates/vm/src/stdlib/ctypes/array.rs (1)
as_buffer(1070-1122)
crates/vm/src/stdlib/ctypes.rs (1)
crates/vm/src/stdlib/ctypes/library.rs (3)
libcache(155-160)new(23-27)new(84-88)
crates/vm/src/stdlib/ctypes/base.rs (2)
crates/vm/src/stdlib/ctypes.rs (1)
stg_info(39-42)crates/vm/src/stdlib/ctypes/library.rs (1)
libcache(155-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
🔇 Additional comments (33)
crates/vm/src/version.rs (4)
18-33: LGTM - good compatibility workaround with clear documentation.The hardcoded MSC version for ctypes compatibility is well-documented. Minor consideration: on ARM64 Windows,
target_pointer_width = "64"would still report "AMD64" rather than "ARM64". This is likely acceptable since the goal is CPython compatibility forfind_msvcrt(), but worth noting if ARM Windows support becomes important.
35-40: LGTM!The non-Windows branch correctly uses the
COMPILERconstant with consistent formatting.
42-47: LGTM!The format string structure is clear, and the PyPy convention for the newline separator is documented.
58-59: LGTM!The
#[cfg(not(windows))]attribute correctly prevents unused constant warnings on Windows builds since the constant is only used in the non-Windows code path.crates/vm/src/stdlib/ctypes/array.rs (1)
463-466:__class_getitem__implementation is consistent with other generics.Using
PyGenericAlias::from_args(cls, args, vm)matches the pattern inlist.__class_getitem__and keeps Array typing behavior aligned with other builtins.crates/vm/src/stdlib/ctypes/base.rs (2)
296-341: Endian-awareget_field_formatlogic matches ctypes expectations.The updated
get_field_formatcleanly separates:
- Complex formats (
T{…},(…)…,&…,X{…}) which are returned as-is, preserving their internal endianness/shape encoding.- Simple formats where you strip any existing endian prefix and re‑apply
>/<based on thebig_endianflag.- A fallback path based on
_type_whenStgInfo.formatis absent.This is a sensible mirror of CPython’s behavior and should work well with the new
StgInfo.formatgeneration insimple.rs.
1182-1208: Library symbol resolution + helper tweaks look good.
- The new
in_dllpath that goes vialibrary::libcache().get_lib(handle)and thenlib.get::<*const u8>(symbol)improves error reporting (“Library not found”, “symbol … not found in library”) and centralizes handle management.FfiArgValue::OwnedPointer(usize, PyObjectRef)now uses the crate-wide alias, which simplifies typing and matches other code in this module.- The
is_simple_instance()helper correctly uses the metaclass to detectPyCSimpleTypesubclasses and will be handy to keep logic in one place.No functional issues spotted here.
Also applies to: 1816-1817, 2157-2165
crates/vm/src/stdlib/ctypes/simple.rs (5)
24-57: PEP 3118 format helpers and swapped-type setup are well factored.
ctypes_code_to_pep3118()centralizes mapping from ctypes codes (i,l,q,?, etc.) to fixed-size PEP 3118 codes, which simplifies keepingStgInfo.formatconsistent across platforms.alloc_format_string_for_type()+ its use inPyCSimpleType::initandcreate_swapped_types()gives native and swapped types explicit</>prefixes without sprinkling format-string construction everywhere.- In
create_swapped_types(), usingis_little_endianand passing it as thebig_endianflag toalloc_format_string_for_type()correctly generates the opposite endian prefix for the swapped type.The overall design here is clean and should make buffer/FFI format handling much easier to reason about.
Also applies to: 568-577, 601-677
203-216:"X"(BSTR) handling inset_primitivematches"Z"semantics.Treating
"X"as “BSTR-like” with the same acceptance rules as"Z"(None, int address, or unicode string) is consistent with the rest of the pointer-like simple types and matches how COM BSTRs are typically exposed. No issues here.
1000-1019:is_cwchar_array_or_pointerchange toPyResult<bool>is an improvement.Switching to
stg_info(vm)?and returningPyResult<bool>lets you propagate “abstract class” and other type errors instead of silently treating them as “not a wchar array/pointer”. The updated call site infrom_paramcorrectly uses?to bubble up failures.
1166-1187: Improvedc_wchar_pvalue decoding across platforms.The new
Zbranch inPyCSimple::value:
- Uses
Wtf8Buf::from_wideon Windows to handle UTF‑16 surrogate pairs correctly.- Falls back to simple
char::from_u32for UTF‑32wchar_ton non‑Windows platforms.This is a significant improvement over treating all
wchar_tas 32‑bit, and aligns with how Windows error messages and wide strings behave.
1404-1425: AsBuffer forPyCSimpleis consistent with the newStgInfo.format.Using:
stg_info.sizeas bothlenanditemsize, andstg_info.format(with"B"as a fallback),to build a scalar (
ndim=0)BufferDescriptoris in line with the rest of the ctypes buffer implementations (arrays, unions, pointers). This should makememoryview(c_int())etc. behave predictably under the new PEP 3118 format scheme.crates/vm/src/stdlib/ctypes.rs (4)
90-111: Module init now registersPyCFuncPtrTypecorrectly.Adding
function::PyCFuncPtrType::make_class(ctx);alongside the other ctypes metaclasses ensures the function pointer metatype is initialized before you exportCFuncPtrand related helpers. This matches how the other ctypes types are wired into_ctypes.
389-391:SIZEOF_TIME_Tbased onlibc::time_tis appropriate.Using
std::mem::size_of::<libc::time_t>()forSIZEOF_TIME_Ttracks the actual C ABI on the host and is preferable to hardcoding per‑platform constants.
961-1008: Errno and Windows last-error helpers look correct and match ctypes expectations.
set_errnonow returns the previous value while updatingerrno, which matches Python’sctypes.set_errno.- On Windows,
CTYPES_LOCAL_LAST_ERRORprovides thread-local storage for_ctypes’ “last error”, andget_last_error/set_last_erroroperate solely on that TLS value.save_and_restore_last_errorcorrectly restores the thread-local value into the OSLastErrorbefore the call and saves the newLastErrorback into TLS afterwards.This is the expected pattern for
use_last_error=Truesemantics and should integrate cleanly with the updated FFI call paths.
1124-1173: Default FFI return type changed toc_int; behavior looks intentional.Switching
call_function_internalto:let cif = Cif::new(arg_types, Type::c_int()); let result: i32 = unsafe { cif.call(code_ptr, &ffi_args) }; Ok(vm.ctx.new_int(result).into())makes the default return type
c_int(32‑bit) instead ofisize, aligning with CPython’s_ctypes.call_function/_ctypes.call_cdeclfunctionbehavior. The argument marshalling is unchanged, so this looks like a deliberate compatibility improvement.crates/vm/src/stdlib/ctypes/library.rs (4)
11-13: LGTM!The visibility change to
pub struct SharedLibrarywithpub(crate) libfield provides appropriate encapsulation while allowing necessary access within the crate.
141-148: LGTM!The
insert_raw_handlemethod correctly uses the handle value as the cache key, consistent withget_pointer()behavior.
49-61: Thetransmute_copyapproach is appropriate for this use case, but document the design decision clearly.Using
transmute_copyto extract the OS handle is the correct pattern here—libloading::Library::into_raw()consumes ownership and is incompatible with keeping the library alive in a Mutex. However, clarify in comments that this is intentional to avoid ownership transfer and that the library's internal layout (storing the handle pointer as the first field) is relied upon. Consider adding a version constraint onlibloadingin Cargo.toml if not already present to guard against breaking changes.
41-47: The code correctly handlesdlopen(NULL)handles. WhenSharedLibraryis dropped, the underlyinglibloading::Librarycallsdlclose, which safely decrements the reference count on the main program handle without unloading it. This is standard behavior protected by reference counting and poses no risk.Likely an incorrect or invalid review comment.
crates/vm/src/stdlib/ctypes/function.rs (13)
138-151: LGTM!The integer-to-pointer conversion correctly handles negative values (allowing
-1as0xFFFF...) and appropriately raisesOverflowErrorfor out-of-range values, matching CPython'sPyLong_AsVoidPtrbehavior.
196-212: LGTM!The string-to-wide-string conversion correctly uses native byte order (
to_ne_bytes()) which matches the platform'swchar_trepresentation. Thekeepobject properly maintains buffer lifetime during the FFI call.
1161-1170: LGTM!The
Argumentstruct correctly bundles FFI type, value, and akeepobject to maintain buffer lifetime during FFI calls. The#[allow(dead_code)]onkeepis appropriate since the field's purpose is to prevent premature deallocation rather than direct access.
1358-1374: LGTM!The
ctypes_callprocfunction cleanly constructs the FFI call from theArgumentslice, properly separating type and value extraction. The return type dispatch handles void, pointer, and scalar returns appropriately.
1619-1637: LGTM!The platform-specific
use_last_errorhandling correctly wraps the FFI call on Windows while cleanly suppressing the unused warning on other platforms.
1654-1681: LGTM!The
AsBufferimplementation correctly provides buffer access to function pointer data, with appropriate fallbacks whenStgInfois unavailable.
1770-1779: LGTM!Adding the
flagsfield toThunkUserDataenables proper errno handling during callback execution, matching CPython's callback behavior.
1828-1845: LGTM!The platform-specific
wchar_thandling correctly addresses the size difference (16-bit on Windows, 32-bit on Unix) and uses appropriate string conversion methods for each platform.
2032-2082: LGTM!The
PyCThunk::newfunction cleanly integrates theflagsparameter, enabling per-callback errno handling and other flag-controlled behaviors.
1507-1527: LGTM!The
pycdata_from_ffi_resultfunction safely copies FFI result data with proper bounds checking to prevent buffer overflows.
1310-1349: LGTM!The
build_callargsdispatcher cleanly routes to the appropriate argument builder based on the presence ofargtypes,paramflags, and COM method context.
1743-1758: LGTM!The
_flags_getter correctly prioritizes class-level attribute overStgInfo, enabling dynamic flag configuration via CDLL's_FuncPtrclass pattern.
420-426: The format string"X{}"is intentional and correctly documented.The review comment incorrectly assumes this code should follow Python's standard struct module conventions. However, the code explicitly documents this as RustPython's custom default: the comment on line 1658 states "Use default values for function pointers: format="X{}", size=sizeof(pointer)". This is RustPython's internal format representation, not standard Python ctypes. Throughout the ctypes implementation, custom format strings are used (e.g., "B" for unions/structures, "&{}{}" for pointers), indicating that "X{}" is an intentional design choice for function pointers in this implementation.
Likely an incorrect or invalid review comment.
| // Default: return as int | ||
| let val = match raw_result { | ||
| RawResult::Pointer(p) => *p as i32, | ||
| RawResult::Value(v) => *v as i32, | ||
| RawResult::Void => return None, | ||
| }; | ||
| return Some(vm.ctx.new_int(val).into()); | ||
| } |
There was a problem hiding this comment.
Pointer truncation when returning as default int.
When restype is None or not a recognized type, pointer values are cast to i32 (lines 1439, 1459, 1473), which truncates 64-bit addresses. While this may match CPython's legacy behavior, it could cause subtle bugs on 64-bit systems.
Consider returning as isize or the full pointer value to preserve address fidelity, or document this limitation.
🔎 Proposed fix for default return
None => {
// Default: return as int
let val = match raw_result {
- RawResult::Pointer(p) => *p as i32,
- RawResult::Value(v) => *v as i32,
+ RawResult::Pointer(p) => *p as isize,
+ RawResult::Value(v) => *v as i32,
RawResult::Void => return None,
};
return Some(vm.ctx.new_int(val).into());
}Committable suggestion skipped: line range outside the PR's diff.
| // Set format string: "&<element_format>" or "&(shape)<element_format>" for arrays | ||
| if let Some(ref proto) = stg_info.proto { | ||
| let item_info = proto.stg_info_opt().expect("proto has StgInfo"); | ||
| let current_format = item_info.format.as_deref().unwrap_or("B"); | ||
| stg_info.format = Some(format!("&{}", current_format)); | ||
| // Include shape for array types in the pointer format | ||
| let shape_str = if !item_info.shape.is_empty() { | ||
| let dims: Vec<String> = item_info.shape.iter().map(|d| d.to_string()).collect(); | ||
| format!("({})", dims.join(",")) | ||
| } else { | ||
| String::new() | ||
| }; | ||
| stg_info.format = Some(format!("&{}{}", shape_str, current_format)); | ||
| } |
There was a problem hiding this comment.
Avoid panicking when _type_ has no StgInfo in PyCPointerType::init.
Here:
if let Some(ref proto) = stg_info.proto {
let item_info = proto.stg_info_opt().expect("proto has StgInfo");
…
}_type_ can legitimately be an abstract/incomplete ctypes type without StgInfo yet, in which case this will panic instead of raising a Python exception. Better to skip format initialisation if there is no StgInfo:
- if let Some(ref proto) = stg_info.proto {
- let item_info = proto.stg_info_opt().expect("proto has StgInfo");
+ if let Some(ref proto) = stg_info.proto {
+ if let Some(item_info) = proto.stg_info_opt() {
…
- stg_info.format = Some(format!("&{}{}", shape_str, current_format));
+ stg_info.format = Some(format!("&{}{}", shape_str, current_format));
+ }
}You already validate typ_type.stg_info_opt().is_some() in set_type, so this change just makes the default init path tolerant of incomplete types.
🤖 Prompt for AI Agents
In crates/vm/src/stdlib/ctypes/pointer.rs around lines 47 to 59, avoid
unwrapping/expect on proto.stg_info_opt() which can be None for
abstract/incomplete ctypes types; instead check for presence and skip format
initialization when there is no StgInfo. Concretely, replace the expect with a
guarded branch (e.g., if let Some(item_info) = proto.stg_info_opt() { … } ) and
only compute shape_str and set stg_info.format inside that branch so the init
path does not panic and remains tolerant of incomplete types.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.