GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Ctypes implementation by rodrigocam · Pull Request #2364 · RustPython/RustPython Conversation
|
|
I just pushed a commit to get stuff compiling. Excited to review this! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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: class Custom(_SimpleCData): pass ... Traceback (most recent call last): File "<stdin>", line 1, in <module> AttributeError: class must define a '_type_' attribute which here will succeed. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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's middle API in ctypes::function, just to avoid as much unsafe as possible for something like ctypes 🙃 . Now that the functions are copied from cpython, it should be easier to refactor into something more idiomatic.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| #[pyclass(module = "_ctypes", name = "_CData")] | ||
| pub struct PyCData { | ||
| _objects: AtomicCell<Vec<PyObjectRef>>, | ||
| _buffer: PyRwLock<Vec<u8>>, |
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Something like
struct RawBuffer{ inner: *mut u8, size: usize }
?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #[cfg(any(unix, windows, target_os = "wasi"))] | ||
| modules.insert("_ctypes".to_owned(), Box::new(ctypes::make_module)); |
There was a problem 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));
}
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Would libffi work?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| void => { | ||
| ptr::null_mut() | ||
| arg(&ptr::null::<usize>()) |
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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) |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| &self.options | ||
| } | ||
| } | ||
| pub struct RawBuffer { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
@coolreader18 is this what you have in mind?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| } | ||
| pointer => { | ||
| usize::try_from_object(vm, obj).unwrap() as *mut c_void | ||
| arg(&usize::try_from_object(vm, obj).unwrap()) |
There was a problem 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 *mut usize as *mut c_void))
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| buffer | ||
| .obj_bytes() | ||
| .chunks(4) | ||
| .map(|c| NativeEndian::read_u32(c)) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
You can use u32::from_ne_bytes for this; also, there's already a byteorder dependency in vm/Cargo.toml
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
You can use
u32::from_ne_bytesfor this; also, there's already a byteorder dependency in vm/Cargo.toml
Oh boy, I missed that one, I've found the btand le counterparts. I can simply ignore the whole ByteOrder 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 are CharArray_getsets and WCharArray_getsets
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| { | ||
| fn obj_bytes(&self) -> BorrowedValue<[u8]> { | ||
| // @TODO: This is broken | ||
| PyRwLockReadGuard::map(self.data.borrow_value(), |x| unsafe { |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
What are the major blockers for this PR? I think I want to give this a try :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 do |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
3334aa9 to
4725a80
Compare
There was an error while loading. Please reload this page.
|
@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 not I am happy to help with discussion and pointers, but I'm afraid I don't have much time to code :/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
You mean merging as is? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
yes, if we can manage it not to break CI |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I think that we need to do a few things to help future contributions in this code before merging:
Undoubtedly class _CDataMeta(type): ... def __mul__(self, length): return create_array_type(self, length) ... class SimpleType(_CDataMeta): ... class SimpleCData(..., metaclass=SimpleType): ... I couldn't find a way to make Point |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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):
pass
class SimpleCData(object, metaclass=SimpleType):
pass
print(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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
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: #define MOD_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); Here |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| } | ||
| } | ||
|
|
||
| fn slice_adjust_size(length: isize, start: &mut isize, stop: &mut isize, step: isize) -> isize { |
There was a problem 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 of inner_indices in slice.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
They have some resemblance, but are different nonetheless.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
| _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(), |
There was a problem 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 new Vec will be removed right after this line and the given pointer will be a dangling pointer.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| // @TODO: Is this copying? | ||
|
|
||
| let buffered = if copy { | ||
| unsafe { slice::from_raw_parts_mut(buffer.as_mut_ptr(), buffer.len()) } |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
this is not a copy
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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 if copy, or return a view otherwise. Should I use to_contiguous to copy it?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| #[pyimpl] | ||
| pub trait PyCDataFunctions: PyValue { | ||
| #[pymethod] | ||
| fn size_of_instances(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult<PyObjectRef>; |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| fn size_of_instances(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult<PyObjectRef>; | |
| fn size_of_instances(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult; |
PyResult == PyResult<PyObjectRef>
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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) |
There was a problem 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
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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(); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| let size_of = vm.get_attribute(obj, "alignment_of_instances").unwrap(); | |
| let size_of = vm.get_attribute(obj, "alignment_of_instances")?; |
is this unwrap() intended?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if vm.isinstance(&argtypes, &vm.ctx.types.list_type).is_ok() | ||
| || vm.isinstance(&argtypes, &vm.ctx.types.tuple_type).is_ok() |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| 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() | |
| )))? { |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
by watching |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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 have |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I'm not sure why, but I cannot comment on the following reply
The idea of |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I forgot where these two functions are used, but based on the docs: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@youknowone I basically patched all your comments, you wrote:
For this code fn obj_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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
There was a question asking if it does copy. The comment is an answer about it. Creating an slice object doesn't copy anything. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
I added a C file that should be compiled as a shared lib and bundled together with the interpreter. Its import _ctypes_test |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Adding more meta impls.
|
@youknowone I haven't figured out a nice way to implement the data = b'data' ubyte = c_ubyte * len(data) byteslike = ubyte.from_buffer_copy(data) # an instance of ubyte made from data by copying things
My implementation of #[pyclassmethod] fn from_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 now let src_buffer = buffer.obj_bytes(); let empty_instance = ... //creates a new empty instance let dst_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 from |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I don't know well about your requirements, but I guess you can do same way as how |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Not at all :) I'm afraid I can't contribute in the following months. I can help others eventually, tho. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Is this effort still ongoing / has this ctypes implementation been abandoned for another? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Jeong YunWon <[email protected]> Co-authored-by: Rodrigo Oliveira <[email protected]> Co-authored-by: Darley Barreto <[email protected]> Co-authored-by: Noah <[email protected]>
Co-authored-by: Jeong YunWon <[email protected]> Co-authored-by: Rodrigo Oliveira <[email protected]> Co-authored-by: Darley Barreto <[email protected]> Co-authored-by: Noah <[email protected]>
|
Hi folks, I think we should close this, it's too old for any merging attempt. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Agreed, the rebase effort was slowed by refactors that have happened since this pr, I'm planning to start incremental work on this soon. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Note that #5572 is now the tracking issue for further ctypes implementation. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.