Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Ctypes implementation#2364

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
rodrigocam wants to merge57 commits intoRustPython:main
base:main
Choose a base branch
Loading
fromrodrigocam:ctypes
Open

Conversation

rodrigocam
Copy link
Contributor

@rodrigocamrodrigocam commentedDec 9, 2020
edited
Loading

@darleybarreto and I are implementing thectypes module. This an initial implementation for review. At this stage, we are focusing on Linux platforms to in the future extend to other platforms likectypes fromcpython does.

darleybarreto, youknowone, vicky5124, jopemachine, RickCao-2017, and matan-h reacted with thumbs up emojicoolreader18, youknowone, domdfcoding, ShadowJonathan, DimitrisJim, vicky5124, Jaakkonen, and redfave reacted with rocket emojifanninpm, Snowapril, and RickCao-2017 reacted with eyes emoji
@coolreader18
Copy link
Member

I just pushed a commit to get stuff compiling. Excited to review this!

rodrigocam reacted with heart emojidarleybarreto reacted with rocket emoji

@darleybarreto
Copy link

One thing to note is that some things are directly translation from PyPy, others from CPython's. Also there's that difference I said on Gitter, regarding declaring a class like:

classCustom(_SimpleCData):pass...Traceback (mostrecentcalllast):File"<stdin>",line1,in<module>AttributeError:classmustdefinea'_type_'attribute

which here will succeed.
Besides this, I've tried to stay as close as CPython's behavior as possible.

Copy link
Member

@coolreader18coolreader18 left a comment

Choose a reason for hiding this comment

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

Here's what I have so far; I think it would be better to use more of libffi'smiddle API inctypes::function, just to avoid as muchunsafe as possible for something like ctypes 🙃 . Now that the functions are copied from cpython, it should be easier to refactor into something more idiomatic.

darleybarreto reacted with thumbs up emoji
#[pyclass(module = "_ctypes", name = "_CData")]
pub struct PyCData {
_objects: AtomicCell<Vec<PyObjectRef>>,
_buffer: PyRwLock<Vec<u8>>,
Copy link
Member

Choose a reason for hiding this comment

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

I think the buffer is supposed to be a raw pointer and a length, so that you can mutate the original data it if you want to.

Choose a reason for hiding this comment

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

Something like

structRawBuffer{inner:*mutu8,size:usize}

?

@coolreader18coolreader18 mentioned this pull requestDec 14, 2020
Comment on lines 127 to 128
#[cfg(any(unix, windows, target_os = "wasi"))]
modules.insert("_ctypes".to_owned(), Box::new(ctypes::make_module));
Copy link
Member

Choose a reason for hiding this comment

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

instead of duplicating same cfg, it can be grouped like:

#[cfg(any(unix, windows, target_os ="wasi"))]{        modules.insert(os::MODULE_NAME.to_owned(),Box::new(os::make_module));        modules.insert("_ctypes".to_owned(),Box::new(ctypes::make_module));}

Copy link
Member

Choose a reason for hiding this comment

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

Although I don't know if ctypes would be available on wasi, I don't think it has dll functionality yet

Choose a reason for hiding this comment

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

Would libffi work?

Copy link
Member

Choose a reason for hiding this comment

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

No, wasm doesn't really have any sort of support for loading another wasm module at runtime

darleybarreto reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe interesting, Blazor does load DLLs at runtime from what I can see(from this blog). How feasible is something similar for Rust?

}
void => {
ptr::null_mut()
arg(&ptr::null::<usize>())
Copy link

@darleybarretodarleybarretoDec 20, 2020
edited
Loading

Choose a reason for hiding this comment

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

I've replace this for

ptr::null::<c_void>()

since it seemed more "correct", which I'changed after committing this. So in a next change batch it will be replaced.

coolreader18 reacted with thumbs up emoji
@@ -252,11 +242,17 @@ struct PyCDataBuffer {
// This trait will be used by all types
impl Buffer for PyCDataBuffer {
fn obj_bytes(&self) -> BorrowedValue<[u8]> {
PyRwLockReadGuard::map(self.data.borrow_value(), |x| x.as_slice()).into()
PyRwLockReadGuard::map(self.data.borrow_value(), |x| unsafe {
slice::from_raw_parts(x.inner, x.size)

Choose a reason for hiding this comment

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

I'm not sure if this is copying things

Copy link
Member

Choose a reason for hiding this comment

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

no, it isn't. slice is a sort of view

@@ -265,14 +261,21 @@ impl Buffer for PyCDataBuffer {
&self.options
}
}
pub struct RawBuffer {

Choose a reason for hiding this comment

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

@coolreader18 is this what you have in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it is.

Copy link
Member

Choose a reason for hiding this comment

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

The safe expression of this type in Rust isBox<[u8]> which is exactly equivalent to this struct. Is this intended work to avoid somethiing like ownership check? I think there must be a better way.

}
pointer => {
usize::try_from_object(vm, obj).unwrap() as *mut c_void
arg(&usize::try_from_object(vm, obj).unwrap())

Choose a reason for hiding this comment

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

Also replace this with

arg(&(usize::try_from_object(vm, obj)?as*mutusizeas*mutc_void))

buffer
.obj_bytes()
.chunks(4)
.map(|c| NativeEndian::read_u32(c))
Copy link
Member

Choose a reason for hiding this comment

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

You can useu32::from_ne_bytes for this; also, there's already a byteorder dependency in vm/Cargo.toml

Copy link
Member

Choose a reason for hiding this comment

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

Also, what function is this supposed to be? CArray doesn't have a value property from what I can see

Choose a reason for hiding this comment

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

You can useu32::from_ne_bytes for this; also, there's already a byteorder dependency in vm/Cargo.toml

Oh boy, I missed that one, I've found thebtandle counterparts. I can simply ignore the wholeByteOrder crate, then.

Also, what function is this supposed to be? CArray doesn't have a value property from what I can see

These properties come from it's metaclass (PyCArrayType_Type), which areCharArray_getsets andWCharArray_getsets

fn obj_bytes(&self) -> BorrowedValue<[u8]> {
// @TODO: This is broken
PyRwLockReadGuard::map(self.data.borrow_value(), |x| unsafe {

Choose a reason for hiding this comment

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

This need to be fixed, "casting" the associated type somehow

@Pluriscient
Copy link
Contributor

What are the major blockers for this PR? I think I want to give this a try :)

@darleybarreto
Copy link

What are the major blockers for this PR? I think I want to give this a try :)

Unfortunately there are several problems with the state of things and I haven't been able to continue working on this (and will not be able to do so for a while). The main problem is how it behaves differently from CPython's and PyPy's, a meta class should be added for Struct, CSimpleType, Pointer, ... (all but CFuntion), so that you can doc_int * 10 and return a proper array. In the current impltype(CSimpleType) == type, which does not has a__mul__ as required for ac_int, for instance.

@youknowoneyouknowoneforce-pushed thectypes branch 5 times, most recently from3334aa9 to4725a80CompareAugust 6, 2021 16:10
@darleybarreto
Copy link

darleybarreto commentedAug 6, 2021
edited
Loading

@youknowone I tried to sync with master a while ago, but there was a problem and I got too busy to look at that again, so thanks for that! Are you planning into work on this? If so, I think there are some tests I ported from CPython that I altered just to see if the implementation was working-ish.

To be fully compatible to CPython and PyPy, the types must have a metaclass that's notPyType. I tried to work something there, but I couldn't find a solution. This happens because a base type has to implement some ops at type level, eg,c_int * 8 should call__mul__ from its meta (a yet non-existing classPrimitiveTypeMeta which is a subclass ofPyType) and return ac_array.

I am happy to help with discussion and pointers, but I'm afraid I don't have much time to code :/

@youknowone
Copy link
Member

@darleybarreto Hi, thank you for the comment. I am not planning for working on ctypes at the moment, but looking for a way to merge it as it is for future contributors - because you already worked really a lot of it - if it doesn't break things too much. How do you think about it?

@darleybarreto
Copy link

You mean merging as is?

@youknowone
Copy link
Member

yes, if we can manage it not to break CI

@darleybarreto
Copy link

I think that we need to do a few things to help future contributions in this code before merging:

  1. Refactor types structure.
  2. Documentation.
  3. A thorough review to improve code quality.

Undoubtedly1 is the major blocker here. Since I don't know much of the internals of RustPython, I wasn't able to solve this problem. Perhaps you can give me pointers and I can work it out. PyPy does is basically this way:

class_CDataMeta(type):    ...def__mul__(self,length):returncreate_array_type(self,length)    ...classSimpleType(_CDataMeta):    ...classSimpleCData(...,metaclass=SimpleType):    ...

I couldn't find a way to maketype(SimpleCData) beSimpleType instead oftype.

Point2 is extremely important since this code is basically too experimental and not that much well written. I could gladly do it based on PyPy, CPython and documentations.
Point3 is somewhat important because I am not a very experienced Rust programmer, so if anyone could help me writing idiomatic andperformant Rust, I would appreciate!

@youknowone
Copy link
Member

Thank you for detailed explanation. The metaclass issue is recently solved. Here is the test:

$ cat t.py class _CDataMeta(type):    def __mul__(self, length):return create_array_type(self, length)class SimpleType(_CDataMeta):    passclass SimpleCData(object, metaclass=SimpleType):    passprint(type(SimpleCData))assert type(SimpleCData) == SimpleType$ cargo run t.py     Finished dev [unoptimized + debuginfo] target(s)in 0.06s     Running`target/debug/rustpython t.py`<class'__main__.SimpleType'>

I will look in the code for 3

darleybarreto reacted with thumbs up emoji

@darleybarreto
Copy link

darleybarreto commentedAug 7, 2021
edited
Loading

Oh, I'm sorry, I meant to do the same thing in the Rust side.CPython does it in the C side of things by manually filling the type:

#defineMOD_ADD_TYPE(TYPE_EXPR,TP_TYPE,TP_BASE) \    do { \        PyTypeObject *type = (TYPE_EXPR); \        Py_SET_TYPE(type, TP_TYPE); \        type->tp_base = TP_BASE; \        if (PyModule_AddType(mod, type) < 0) { \            return -1; \        } \    } while (0)MOD_ADD_TYPE(&Simple_Type,&PyCSimpleType_Type,&PyCData_Type);

HerePyCSimpleType_Type is the meta andPyCData_Type is the base class.

}
}

fn slice_adjust_size(length: isize, start: &mut isize, stop: &mut isize, step: isize) -> isize {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't compare precisely, but this function might be a duplication ofinner_indices inslice.rs

Choose a reason for hiding this comment

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

They have some resemblance, but are different nonetheless.

youknowone reacted with thumbs up emoji
_type_: new_simple_type(Either::A(&outer_type), vm)?.into_ref(vm),
_length_: length,
_buffer: PyRwLock::new(RawBuffer {
inner: Vec::with_capacity(length * itemsize).as_mut_ptr(),
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this line is safe. The newVec will be removed right after this line and the given pointer will be a dangling pointer.

darleybarreto reacted with thumbs up emoji
// @TODO: Is this copying?

let buffered = if copy {
unsafe { slice::from_raw_parts_mut(buffer.as_mut_ptr(), buffer.len()) }
Copy link
Member

Choose a reason for hiding this comment

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

this is not a copy

darleybarreto reacted with thumbs up emoji

Choose a reason for hiding this comment

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

Here

let buffered =if copy{unsafe{ slice::from_raw_parts_mut(buffer.as_mut_ptr(), buffer.len())}.as_mut_ptr()}else{    buffer.as_mut_ptr()};

The idea is to copy the bytes from the buffer ifcopy, or return a view otherwise. Should I useto_contiguous to copy it?

#[pyimpl]
pub trait PyCDataFunctions: PyValue {
#[pymethod]
fn size_of_instances(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult<PyObjectRef>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fnsize_of_instances(zelf:PyRef<Self>,vm:&VirtualMachine) ->PyResult<PyObjectRef>;
fnsize_of_instances(zelf:PyRef<Self>,vm:&VirtualMachine) ->PyResult;

PyResult ==PyResult<PyObjectRef>

darleybarreto reacted with thumbs up emoji
@@ -252,11 +242,17 @@ struct PyCDataBuffer {
// This trait will be used by all types
impl Buffer for PyCDataBuffer {
fn obj_bytes(&self) -> BorrowedValue<[u8]> {
PyRwLockReadGuard::map(self.data.borrow_value(), |x| x.as_slice()).into()
PyRwLockReadGuard::map(self.data.borrow_value(), |x| unsafe {
slice::from_raw_parts(x.inner, x.size)
Copy link
Member

Choose a reason for hiding this comment

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

no, it isn't. slice is a sort of view

PyCDataFunctions::alignment_of_instances(zelf.into_ref(vm), vm)
}
Either::B(obj) if obj.has_class_attr("alignment_of_instances") => {
let size_of = vm.get_attribute(obj, "alignment_of_instances").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let size_of = vm.get_attribute(obj,"alignment_of_instances").unwrap();
let size_of = vm.get_attribute(obj,"alignment_of_instances")?;

is thisunwrap() intended?

darleybarreto reacted with thumbs up emoji
Comment on lines 281 to 282
if vm.isinstance(&argtypes, &vm.ctx.types.list_type).is_ok()
|| vm.isinstance(&argtypes, &vm.ctx.types.tuple_type).is_ok()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if vm.isinstance(&argtypes,&vm.ctx.types.list_type).is_ok()
|| vm.isinstance(&argtypes,&vm.ctx.types.tuple_type).is_ok()
if vm.isinstance(&argtypes,&vm.ctx.types.list_type).and_then(|_| vm.isinstance(&argtypes,&vm.ctx.types.tuple_type)).map_err(|e| vm.new_type_error(format!(
"_argtypes_ must be a sequence of types, {} found.",
argtypes.to_string()
)))?{

darleybarreto reacted with thumbs up emoji
@youknowone
Copy link
Member

youknowone commentedAug 7, 2021
edited
Loading

by watchingrustpython_vm::builtins::pytype::new, I feel like that'd be simply done by replacingtyp field inPyObject, but not sure it actually will be that easy.
Because I don't understand this patch that much, would you make specific place in this PR that you need those type stuff? Then I will fill the fitting code there.

@darleybarreto
Copy link

would you make specific place in this PR that you need those type stuff?

Let me get the CPython definitions as example

MOD_ADD_TYPE(&Struct_Type,&PyCStructType_Type,&PyCData_Type);MOD_ADD_TYPE(&Union_Type,&UnionType_Type,&PyCData_Type);MOD_ADD_TYPE(&PyCPointer_Type,&PyCPointerType_Type,&PyCData_Type);MOD_ADD_TYPE(&PyCArray_Type,&PyCArrayType_Type,&PyCData_Type);MOD_ADD_TYPE(&Simple_Type,&PyCSimpleType_Type,&PyCData_Type);MOD_ADD_TYPE(&PyCFuncPtr_Type,&PyCFuncPtrType_Type,&PyCData_Type);

Here we haveMOD_ADD_TYPE(&Class, &MetaClass, &BaseClass) right? Looking atSimple_Type (in RustPython this isPySimpleType inprimitive.rs), we see that its metaclass implements the__mul__, so we can havec_int * 8 just fine, wherec_int is subclass ofSimple_Type. I used a traitPyCDataSequenceMethods inbasics.rs to implement the__mul__, but doingimpl PyCDataSequenceMethods for PySimpleTypeMeta {} inprimitive.rs doesn't work becausePySimpleTypeMeta is not the metaclass ofPySimpleType.

@darleybarreto
Copy link

I'm not sure why, but I cannot comment on the following reply

The safe expression of this type in Rust is Box<[u8]> which is exactly equivalent to this struct. Is this intended work to avoid somethiing like ownership check? I think there must be a better way.

The idea ofRawBuffer is to be a "view" of the external buffer, so when needed, we copy the bytes from that buffer. I think there's one case we need to copy data and another where we need a view.

@youknowone
Copy link
Member

What happens when the buffer is destroyed? Does it deletes only the pointer(as a reference) or also deletes the data it contains (as an owner)? If it does both, then does it need a flag to distinguish them?

@darleybarreto
Copy link

I forgot where these two functions are used, but based on the docs:
from_buffer_copy:C.from_buffer_copy(object, offset=0) -> C instance\ncreate a C instance from a readable buffer
from_buffer:C.from_buffer(object, offset=0) -> C instance\ncreate a C instance from a writeable buffer

youknowone reacted with thumbs up emoji

@youknowone
Copy link
Member

youknowone commentedAug 7, 2021
edited
Loading

Yes, that's possible to adapt to all files. It is up to you for your convenience.dll module looked good to use it. And I am pretty sure every file can use it. But you don't need to use it if you don't feel like to do.

darleybarreto reacted with thumbs up emoji

@darleybarreto
Copy link

@youknowone I basically patched all your comments, you wrote:

no, it isn't. slice is a sort of view

For this code

fnobj_bytes(&self) ->BorrowedValue<[u8]>{PyRwLockReadGuard::map(self.data.borrow_value(), |x|unsafe{        slice::from_raw_parts(x.inner, x.size)}).into()}

What did you meant there?

@youknowone
Copy link
Member

There was a question asking if it does copy. The comment is an answer about it. Creating an slice object doesn't copy anything.

darleybarreto reacted with thumbs up emoji

@darleybarreto
Copy link

darleybarreto commentedAug 8, 2021
edited
Loading

I added a C file that should be compiled as a shared lib and bundled together with the interpreter. Its_ctypes_test.c. How would one make this happen? Would it be some sort of build script usingcc? And we should be able to do this:

import_ctypes_test

@youknowone
Copy link
Member

build.rs file can contain any build script. Usingcc crate will be helpful.

@darleybarreto
Copy link

@youknowone I haven't figured out a nice way to implement thefrom_buffer.from_buffer_copy is straight forward, you simply create a new instance (default args for both tp_new and init) and copy the bytes from the value to create an instance. For example:

data=b'data'ubyte=c_ubyte*len(data)byteslike=ubyte.from_buffer_copy(data)# an instance of ubyte made from data by copying things

from_buffer does a similar thing, but it doesn't copy things, it points to the source. So if you changebyteslike, you changedata.

My implementation offrom_buffer_copy is something in these lines

#[pyclassmethod]fnfrom_buffer_copy(cls:PyRef<Self>,obj:PyObjectRef,offset:OptionalArg,vm:&VirtualMachine,) ->PyResult{let buffer =try_buffer_from_object(vm,&obj)?;let opts = buffer.get_options().clone();let(size, offset) =Self::buffer_check(cls, opts, offset, vm);//ignore this for nowlet src_buffer = buffer.obj_bytes();let empty_instance = ...//creates a new empty instance    letdst_buffer = empty_instance.obj_bytes_mut();    dst_buffer.copy_from_slice(&src_buffer[offset..offset+size]);Ok(empty_instance.as_object().clone())}

So the idea is simply get the bytes fromobj (it should implement the buffer protocol) and point to them. How would I point to the data inobj instead of copying it? Should I use some refcounted type to avoid pointing to dangling things?obj should always be a buffer with write access.

@youknowone
Copy link
Member

I don't know well about your requirements, but I guess you can do same way as howPyMemoryView does. Or even just use it. it just holds aPyBufferRef and copy the data only when it needs to be. It also locksPyBufferRef when the lock is required.

@youknowone
Copy link
Member

I think this PR is (practically) not possible to rebase anymore in this state. Do you mind if we squash the commits into single commit for rebase?

darleybarreto reacted with thumbs up emoji

@darleybarreto
Copy link

I think this PR is (practically) not possible to rebase anymore in this state. Do you mind if we squash the commits into single commit for rebase?

Not at all :)

I'm afraid I can't contribute in the following months. I can help others eventually, tho.

@akbog
Copy link

Is this effort still ongoing / has this ctypes implementation been abandoned for another?

arihant2math added a commit to arihant2math/RustPython that referenced this pull requestJan 20, 2025
Co-authored-by: Jeong YunWon <jeong@youknowone.org>Co-authored-by: Rodrigo Oliveira <rodrigo.redcode@gmail.com>Co-authored-by: Darley Barreto <darleybarreto@gmail.com>Co-authored-by: Noah <33094578+coolreader18@users.noreply.github.com>
@arihant2matharihant2math mentioned this pull requestJan 20, 2025
arihant2math added a commit to arihant2math/RustPython that referenced this pull requestJan 23, 2025
Co-authored-by: Jeong YunWon <jeong@youknowone.org>Co-authored-by: Rodrigo Oliveira <rodrigo.redcode@gmail.com>Co-authored-by: Darley Barreto <darleybarreto@gmail.com>Co-authored-by: Noah <33094578+coolreader18@users.noreply.github.com>
@darleybarreto
Copy link

Hi folks, I think we should close this, it's too old for any merging attempt.
Happy to see@arihant2math tackling ctypes. I was actually look at this again (starting from scratch).
I'd like to help on designing the new implementation, but haven't got much spare time to code.

@youknowone
Copy link
Member

This is still a best trial of ctypes now. I wouldn't close it unless we actually merge any ctypes implementation.

For anyone who will try another ctypes implementation, I hope it can be gradually mergable, not to be suffered by huge size of rebase.

darleybarreto reacted with thumbs up emoji

@arihant2math
Copy link
Collaborator

Agreed, the rebase effort was slowed by refactors that have happened since this pr, I'm planning to start incremental work on this soon.

@arihant2math
Copy link
Collaborator

Note that#5572 is now the tracking issue for further ctypes implementation.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@youknowoneyouknowoneyouknowone left review comments

@darleybarretodarleybarretodarleybarreto left review comments

@PluriscientPluriscientPluriscient left review comments

@coolreader18coolreader18coolreader18 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@rodrigocam@coolreader18@darleybarreto@Pluriscient@youknowone@akbog@arihant2math

[8]ページ先頭

©2009-2025 Movatter.jp