From e8ad33bb308276149cf9a56f80c477318b64999a Mon Sep 17 00:00:00 2001 From: Fritz Reese Date: Sat, 3 Oct 2020 17:09:14 -0400 Subject: [PATCH] Fix buffer_info for ctypes buffers (pybind#2502) (#2503) * tests: New test for ctypes buffers (pybind#2502) * fix: fix buffer_info segfault on views with no stride (pybind11#2502) * Explicit conversions in buffer_info to make clang happy (pybind#2502) * Another explicit cast in buffer_info constructor for clang (pybind#2502) * Simpler implementation of buffer_info constructor from Py_buffer. * Move test_ctypes_buffer into test_buffers * Comment on why view->strides may be NULL (and fix some whitespace) * Use c_strides() instead of zero when view->strides is NULL. c_strides and f_strides are moved from numpy.h (py::array) to buffer_info.h (py::detail) so they can be used from the buffer_info Py_buffer constructor. * Increase ctypes buffer test coverage in test_buffers. * Split ctypes tests and skip one which is broken in PyPy2. --- include/pybind11/buffer_info.h | 32 +++++++++++++++++++- include/pybind11/numpy.h | 25 +++------------- tests/test_buffers.cpp | 19 ++++++++++++ tests/test_buffers.py | 53 ++++++++++++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 22 deletions(-) diff --git a/include/pybind11/buffer_info.h b/include/pybind11/buffer_info.h index 308be06a3..d803004a1 100644 --- a/include/pybind11/buffer_info.h +++ b/include/pybind11/buffer_info.h @@ -13,6 +13,29 @@ PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) +PYBIND11_NAMESPACE_BEGIN(detail) + +// Default, C-style strides +inline std::vector c_strides(const std::vector &shape, ssize_t itemsize) { + auto ndim = shape.size(); + std::vector strides(ndim, itemsize); + if (ndim > 0) + for (size_t i = ndim - 1; i > 0; --i) + strides[i - 1] = strides[i] * shape[i]; + return strides; +} + +// F-style strides; default when constructing an array_t with `ExtraFlags & f_style` +inline std::vector f_strides(const std::vector &shape, ssize_t itemsize) { + auto ndim = shape.size(); + std::vector strides(ndim, itemsize); + for (size_t i = 1; i < ndim; ++i) + strides[i] = strides[i - 1] * shape[i - 1]; + return strides; +} + +PYBIND11_NAMESPACE_END(detail) + /// Information record describing a Python buffer object struct buffer_info { void *ptr = nullptr; // Pointer to the underlying storage @@ -53,7 +76,14 @@ struct buffer_info { explicit buffer_info(Py_buffer *view, bool ownview = true) : buffer_info(view->buf, view->itemsize, view->format, view->ndim, - {view->shape, view->shape + view->ndim}, {view->strides, view->strides + view->ndim}, view->readonly) { + {view->shape, view->shape + view->ndim}, + /* Though buffer::request() requests PyBUF_STRIDES, ctypes objects + * ignore this flag and return a view with NULL strides. + * When strides are NULL, build them manually. */ + view->strides + ? std::vector(view->strides, view->strides + view->ndim) + : detail::c_strides({view->shape, view->shape + view->ndim}, view->itemsize), + view->readonly) { this->m_view = view; this->ownview = ownview; } diff --git a/include/pybind11/numpy.h b/include/pybind11/numpy.h index a43119e46..75ec96f01 100644 --- a/include/pybind11/numpy.h +++ b/include/pybind11/numpy.h @@ -564,7 +564,7 @@ public: const void *ptr = nullptr, handle base = handle()) { if (strides->empty()) - *strides = c_strides(*shape, dt.itemsize()); + *strides = detail::c_strides(*shape, dt.itemsize()); auto ndim = shape->size(); if (ndim != strides->size()) @@ -792,25 +792,6 @@ protected: throw std::domain_error("array is not writeable"); } - // Default, C-style strides - static std::vector c_strides(const std::vector &shape, ssize_t itemsize) { - auto ndim = shape.size(); - std::vector strides(ndim, itemsize); - if (ndim > 0) - for (size_t i = ndim - 1; i > 0; --i) - strides[i - 1] = strides[i] * shape[i]; - return strides; - } - - // F-style strides; default when constructing an array_t with `ExtraFlags & f_style` - static std::vector f_strides(const std::vector &shape, ssize_t itemsize) { - auto ndim = shape.size(); - std::vector strides(ndim, itemsize); - for (size_t i = 1; i < ndim; ++i) - strides[i] = strides[i - 1] * shape[i - 1]; - return strides; - } - template void check_dimensions(Ix... index) const { check_dimensions_impl(ssize_t(0), shape(), ssize_t(index)...); } @@ -869,7 +850,9 @@ public: explicit array_t(ShapeContainer shape, const T *ptr = nullptr, handle base = handle()) : array_t(private_ctor{}, std::move(shape), - ExtraFlags & f_style ? f_strides(*shape, itemsize()) : c_strides(*shape, itemsize()), + ExtraFlags & f_style + ? detail::f_strides(*shape, itemsize()) + : detail::c_strides(*shape, itemsize()), ptr, base) { } explicit array_t(ssize_t count, const T *ptr = nullptr, handle base = handle()) diff --git a/tests/test_buffers.cpp b/tests/test_buffers.cpp index 1bc67ff7b..4f90700ac 100644 --- a/tests/test_buffers.cpp +++ b/tests/test_buffers.cpp @@ -9,6 +9,7 @@ #include "pybind11_tests.h" #include "constructor_stats.h" +#include TEST_SUBMODULE(buffers, m) { // test_from_python / test_to_python: @@ -192,4 +193,22 @@ TEST_SUBMODULE(buffers, m) { .def_readwrite("readonly", &BufferReadOnlySelect::readonly) .def_buffer(&BufferReadOnlySelect::get_buffer_info); + // Expose buffer_info for testing. + py::class_(m, "buffer_info") + .def(py::init<>()) + .def_readonly("itemsize", &py::buffer_info::itemsize) + .def_readonly("size", &py::buffer_info::size) + .def_readonly("format", &py::buffer_info::format) + .def_readonly("ndim", &py::buffer_info::ndim) + .def_readonly("shape", &py::buffer_info::shape) + .def_readonly("strides", &py::buffer_info::strides) + .def_readonly("readonly", &py::buffer_info::readonly) + .def("__repr__", [](py::handle self) { + return py::str("itemsize={0.itemsize!r}, size={0.size!r}, format={0.format!r}, ndim={0.ndim!r}, shape={0.shape!r}, strides={0.strides!r}, readonly={0.readonly!r}").format(self); + }) + ; + + m.def("get_buffer_info", [](py::buffer buffer) { + return buffer.request(); + }); } diff --git a/tests/test_buffers.py b/tests/test_buffers.py index d6adaf1f5..d6ca0776b 100644 --- a/tests/test_buffers.py +++ b/tests/test_buffers.py @@ -1,6 +1,7 @@ # -*- coding: utf-8 -*- import io import struct +import ctypes import pytest @@ -107,3 +108,55 @@ def test_selective_readonly_buffer(): memoryview(buf)[0] = b'\0' if env.PY2 else 0 with pytest.raises(TypeError): io.BytesIO(b'1').readinto(buf) + + +def test_ctypes_array_1d(): + char1d = (ctypes.c_char * 10)() + int1d = (ctypes.c_int * 15)() + long1d = (ctypes.c_long * 7)() + + for carray in (char1d, int1d, long1d): + info = m.get_buffer_info(carray) + assert info.itemsize == ctypes.sizeof(carray._type_) + assert info.size == len(carray) + assert info.ndim == 1 + assert info.shape == [info.size] + assert info.strides == [info.itemsize] + assert not info.readonly + + +def test_ctypes_array_2d(): + char2d = ((ctypes.c_char * 10) * 4)() + int2d = ((ctypes.c_int * 15) * 3)() + long2d = ((ctypes.c_long * 7) * 2)() + + for carray in (char2d, int2d, long2d): + info = m.get_buffer_info(carray) + assert info.itemsize == ctypes.sizeof(carray[0]._type_) + assert info.size == len(carray) * len(carray[0]) + assert info.ndim == 2 + assert info.shape == [len(carray), len(carray[0])] + assert info.strides == [info.itemsize * len(carray[0]), info.itemsize] + assert not info.readonly + + +@pytest.mark.skipif( + "env.PYPY and env.PY2", reason="PyPy2 bytes buffer not reported as readonly" +) +def test_ctypes_from_buffer(): + test_pystr = b"0123456789" + for pyarray in (test_pystr, bytearray(test_pystr)): + pyinfo = m.get_buffer_info(pyarray) + + if pyinfo.readonly: + cbytes = (ctypes.c_char * len(pyarray)).from_buffer_copy(pyarray) + cinfo = m.get_buffer_info(cbytes) + else: + cbytes = (ctypes.c_char * len(pyarray)).from_buffer(pyarray) + cinfo = m.get_buffer_info(cbytes) + + assert cinfo.size == pyinfo.size + assert cinfo.ndim == pyinfo.ndim + assert cinfo.shape == pyinfo.shape + assert cinfo.strides == pyinfo.strides + assert not cinfo.readonly