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

@@ -13,6 +13,29 @@
 
 PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)
 
+PYBIND11_NAMESPACE_BEGIN(detail)
+
+// Default, C-style strides
+inline std::vector<ssize_t> c_strides(const std::vector<ssize_t> &shape, ssize_t itemsize) {
+    auto ndim = shape.size();
+    std::vector<ssize_t> 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<ssize_t> f_strides(const std::vector<ssize_t> &shape, ssize_t itemsize) {
+    auto ndim = shape.size();
+    std::vector<ssize_t> 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<ssize_t>(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;
     }

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<ssize_t> c_strides(const std::vector<ssize_t> &shape, ssize_t itemsize) {
-        auto ndim = shape.size();
-        std::vector<ssize_t> 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<ssize_t> f_strides(const std::vector<ssize_t> &shape, ssize_t itemsize) {
-        auto ndim = shape.size();
-        std::vector<ssize_t> strides(ndim, itemsize);
-        for (size_t i = 1; i < ndim; ++i)
-            strides[i] = strides[i - 1] * shape[i - 1];
-        return strides;
-    }
-
     template<typename... Ix> 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())

tests/test_buffers.cpp

@@ -9,6 +9,7 @@
 
 #include "pybind11_tests.h"
 #include "constructor_stats.h"
+#include <pybind11/stl.h>
 
 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_<py::buffer_info>(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();
+    });
 }

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