[cpyrt] Only let reshape set dimensions of unknown size

src/cpyrt/LowLevelViews.cxx

@@ -35,6 +35,12 @@ static inline void set_strides(Py_buffer& view, size_t itemsize, bool isfix) {
   }
 }
 
+// The creators mark an outermost dimension of unknown extent with this cap
+// rather than with UNKNOWN_SIZE, keeping byte lengths and loops non-negative.
+static inline Py_ssize_t fake_max(size_t elemsize) {
+  return INT_MAX / (Py_ssize_t)elemsize;
+}
+
 //= cpyrt low level view construction/destruction =========================
 static cpyrt::LowLevelView* ll_new(PyTypeObject* subtype, PyObject*,
                                    PyObject*) {
@@ -685,15 +691,44 @@ static PyObject* ll_shape(cpyrt::LowLevelView* self) {
   Py_buffer& view = self->fBufInfo;
 
   PyObject* shape = PyTuple_New(view.ndim);
-  for (Py_ssize_t idim = 0; idim < view.ndim; ++idim)
-    PyTuple_SET_ITEM(shape, idim, PyInt_FromSsize_t(view.shape[idim]));
+  if (!shape)
+    return nullptr;
+  for (Py_ssize_t idim = 0; idim < view.ndim; ++idim) {
+    PyObject* pydim = PyInt_FromSsize_t(view.shape[idim]);
+    if (!pydim) {
+      Py_DECREF(shape);
+      return nullptr;
+    }
+    PyTuple_SET_ITEM(shape, idim, pydim);
+  }
 
   return shape;
 }
 
+//---------------------------------------------------------------------------
+static PyObject* ll_reshape_error(cpyrt::LowLevelView* self, PyObject* shape,
+                                  const char* why) {
+  PyObject* current = ll_shape(self);
+  if (!current)
+    return nullptr;
+  PyErr_Format(PyExc_ValueError,
+               "cannot reshape array of shape %S into shape %S: %s", current,
+               shape, why);
+  Py_DECREF(current);
+  return nullptr;
+}
+
 //---------------------------------------------------------------------------
 static PyObject* ll_reshape(cpyrt::LowLevelView* self, PyObject* shape) {
-  // Allow the user to fix up the actual (type-strided) size of the buffer.
+  // Fill in the dimensions of the buffer that are not known from its type.
+  //
+  // A view is created with the rank and layout of its C++ type: a flat block
+  // for a rank-1 or fixed-size array, an array of row pointers otherwise. The
+  // strides and the converter projecting sub-views are derived from that and
+  // cannot be re-derived from a shape alone, so the rank of a view never
+  // changes and a dimension, once known, stays what it is. What reshaping can
+  // do is provide the extent of a dimension that the type leaves open, such as
+  // the size of an array behind a pointer.
   if (!PyTuple_Check(shape)) {
     if (shape) {
       PyObject* pystr = PyObject_Str(shape);
@@ -709,58 +744,74 @@ static PyObject* ll_reshape(cpyrt::LowLevelView* self, PyObject* shape) {
   }
 
   Py_buffer& view = self->fBufInfo;
-
-  // verify size match
-  Py_ssize_t oldsz = 0;
-  for (Py_ssize_t idim = 0; idim < view.ndim; ++idim) {
-    Py_ssize_t nlen = view.shape[idim];
-    if (nlen == cpyrt::UNKNOWN_SIZE ||
-        nlen == INT_MAX / view.itemsize /* fake 'max' */) {
-      oldsz = -1; // meaning, unable to check size match
-      break;
-    }
-    oldsz += view.shape[idim];
+  if (view.ndim < 1 || !view.shape || !view.strides) {
+    PyErr_SetString(PyExc_TypeError,
+                    "this low level view has no dimensions to set");
+    return nullptr;
   }
 
-  if (0 < oldsz) {
-    Py_ssize_t newsz = 0;
-    for (Py_ssize_t idim = 0; idim < PyTuple_GET_SIZE(shape); ++idim)
-      newsz += PyInt_AsSsize_t(PyTuple_GET_ITEM(shape, idim));
-    if (oldsz != newsz) {
-      PyObject* tas = PyObject_Str(shape);
-      PyErr_Format(PyExc_ValueError,
-                   "cannot reshape array of size %ld into shape %s",
-                   (long)oldsz, cpyrt_PyText_AsString(tas));
-      Py_DECREF(tas);
+  // An unknown outermost dimension holds the fake max for the element size
+  // (the innermost stride), any other unknown dimension UNKNOWN_SIZE. An
+  // empty dimension of a non-fixed view may also be filled in: it came from
+  // a pointer, with nothing behind it yet that a size could contradict.
+  bool isfix = (intptr_t)view.internal & cpyrt::LowLevelView::kIsFixed;
+  Py_ssize_t elemsize = view.strides[view.ndim - 1];
+  Py_ssize_t fakemax = fake_max(elemsize);
+  Py_ssize_t unit0 = view.ndim == 1 ? elemsize : view.itemsize;
+
+  Py_ssize_t ndim = PyTuple_GET_SIZE(shape);
+  cpyrt::dims_t dims(ndim);
+  for (Py_ssize_t idim = 0; idim < ndim; ++idim) {
+    Py_ssize_t nlen = PyInt_AsSsize_t(PyTuple_GET_ITEM(shape, idim));
+    if (nlen == -1 && PyErr_Occurred())
+      return nullptr;
+    if (nlen < cpyrt::UNKNOWN_SIZE) {
+      PyErr_SetString(PyExc_ValueError, "negative dimensions are not allowed");
       return nullptr;
     }
+    if (nlen == cpyrt::UNKNOWN_SIZE) // store as the creators would
+      nlen = idim == 0 ? fakemax : cpyrt::UNKNOWN_SIZE;
+    else if (PY_SSIZE_T_MAX / (idim == 0 ? unit0 : elemsize) < nlen)
+      return ll_reshape_error(self, shape, "the shape is too large");
+    dims[idim] = nlen;
   }
 
-  // reshape
-  size_t itemsize = view.strides[view.ndim - 1];
-  if (view.ndim != PyTuple_GET_SIZE(shape)) {
-    PyMem_Free(view.shape);
-    PyMem_Free(view.strides);
-
-    view.ndim = (int)PyTuple_GET_SIZE(shape);
-    view.shape = (Py_ssize_t*)PyMem_Malloc(view.ndim * sizeof(Py_ssize_t));
-    view.strides = (Py_ssize_t*)PyMem_Malloc(view.ndim * sizeof(Py_ssize_t));
+  if (ndim != view.ndim)
+    return ll_reshape_error(
+        self, shape,
+        "the number of dimensions of a low level view is fixed by its type");
+
+  for (Py_ssize_t idim = 0; idim < ndim; ++idim) {
+    Py_ssize_t cur = view.shape[idim];
+    bool unknown = cur == cpyrt::UNKNOWN_SIZE ||
+                   (idim == 0 && cur == fakemax) || (cur == 0 && !isfix);
+    if (!unknown && dims[idim] != cur)
+      return ll_reshape_error(self, shape,
+                              "only dimensions of unknown size can be set");
   }
 
-  for (Py_ssize_t idim = 0; idim < PyTuple_GET_SIZE(shape); ++idim) {
-    Py_ssize_t nlen = PyInt_AsSsize_t(PyTuple_GET_ITEM(shape, idim));
-    if (nlen == -1 && PyErr_Occurred())
-      return nullptr;
+  for (Py_ssize_t idim = 0; idim < ndim; ++idim)
+    view.shape[idim] = dims[idim];
 
-    if (idim == 0)
-      view.len = nlen * view.itemsize;
+  // the byte length counts the outermost dimension as the creators set it;
+  // the strides depend only on the layout and stay as they were laid down
+  view.len = dims[0] * unit0;
 
-    view.shape[idim] = nlen;
-  }
+  Py_RETURN_NONE;
+}
 
-  set_strides(view, itemsize, false /* by definition not fixed */);
+//---------------------------------------------------------------------------
+static int ll_setshape(cpyrt::LowLevelView* self, PyObject* value, void*) {
+  if (!value) {
+    PyErr_SetString(PyExc_TypeError, "cannot delete the shape of a view");
+    return -1;
+  }
 
-  Py_RETURN_NONE;
+  PyObject* result = ll_reshape(self, value);
+  if (!result)
+    return -1;
+  Py_DECREF(result);
+  return 0;
 }
 
 //---------------------------------------------------------------------------
@@ -864,7 +915,7 @@ static PyGetSetDef ll_getset[] = {
      (char*)"If true, this array was allocated with C++\'s new[]", nullptr},
     {(char*)"format", (getter)ll_typecode, nullptr, nullptr, nullptr},
     {(char*)"typecode", (getter)ll_typecode, nullptr, nullptr, nullptr},
-    {(char*)"shape", (getter)ll_shape, (setter)ll_reshape, nullptr, nullptr},
+    {(char*)"shape", (getter)ll_shape, (setter)ll_setshape, nullptr, nullptr},
     {(char*)nullptr, nullptr, nullptr, nullptr, nullptr}};
 
 namespace cppjit::cpyrt {
@@ -1038,9 +1089,9 @@ CreateLowLevelViewT(T* address, cpyrt::cdims_t shape,
                     Py_ssize_t itemsize = -1) {
   using namespace cppjit::cpyrt;
   Py_ssize_t nx =
-      (shape.ndim() != UNKNOWN_SIZE) ? shape[0] : INT_MAX / sizeof(T);
+      (shape.ndim() != UNKNOWN_SIZE) ? shape[0] : fake_max(sizeof(T));
   if (nx == UNKNOWN_SIZE)
-    nx = INT_MAX / sizeof(T);
+    nx = fake_max(sizeof(T));
   PyObject* args = PyTuple_New(0);
   LowLevelView* llp = (LowLevelView*)LowLevelView_Type.tp_new(
       &LowLevelView_Type, args, nullptr);

test/test_datatypes.py

@@ -1302,6 +1302,58 @@ def test23_buffer_reshaping(self):
             for i in range(self.N):
                 assert arr[i] == l[i]
 
+        # a fixed-size array accepts only its own shape, and stays intact
+        arr = c.m_int_array
+        assert arr.shape == (self.N,)
+        arr.reshape((self.N,))
+        arr.shape = (self.N,)
+        assert arr.shape == (self.N,)
+        assert list(arr) == list(c.m_int_array)
+
+        # (2, 3) was accepted when sizes were compared as sums (2 + 3 == N)
+        raises(ValueError, arr.reshape, (2, 3))
+        raises(ValueError, arr.reshape, (self.N + 1,))
+        raises(ValueError, arr.reshape, (-1,))
+        assert arr.shape == (self.N,)
+        assert list(arr) == list(c.m_int_array)
+
+        # shapes that do not describe an array (-1 means "unknown" and is fine)
+        arr = c.get_int_array()
+        raises(ValueError, arr.reshape, ())
+        raises(ValueError, arr.reshape, (-2,))
+        raises(ValueError, arr.reshape, (sys.maxsize,))
+        raises(TypeError, arr.reshape, (1.5, 2))
+
+        # sizing to (0,) does not fix the size of an unknown view, others do
+        arr = c.get_int_array()
+        arr.reshape((-1,))
+        assert arr.shape[0] > 0
+        assert len(arr) == arr.shape[0]
+        arr.reshape((0,))
+        assert len(arr) == 0
+        assert list(arr) == []
+        arr.shape = (self.N,)
+        assert arr.shape == (self.N,)
+        assert len(list(arr)) == self.N
+        with raises(ValueError):
+            arr.shape = (2 * self.N,)
+        with raises(TypeError):
+            del arr.shape
+        assert arr.shape == (self.N,)
+
+        # a flexible array member is of unknown size whatever its type; sizing
+        # it counts bytes in element strides, not the (overridden) itemsize
+        cppjit.cppdef("""\
+        namespace ReshapeFlex {
+        struct Names { int n; const char* names[]; };
+        }""")
+
+        names = cppjit.gbl.ReshapeFlex.Names()
+        arr = names.names
+        arr.reshape((3,))
+        assert arr.shape == (3,)
+        assert memoryview(arr).nbytes == 3 * memoryview(arr).strides[0]
+
     def test24_voidp(self):
         """Test usage of void* data"""
 

test/test_lowlevel.py

@@ -866,6 +866,7 @@ def test01_2D_arrays(self):
         data2c = self._data_m("2c")
         for m, tp in data2c:
             arr = getattr(h, m)
+            arr.reshape((3, 5))  # its own shape, the only one it accepts
             assert arr.shape == (3, 5)
             elem_tp = getattr(cppjit.gbl, tp)
             for i in range(3):
@@ -1101,3 +1102,60 @@ def test07_3D_custom_struct(self):
             for j in range(gbl.S + 3):
                 for k in range(gbl.S + 7):
                     assert gbl.consume_klass(gbl.klasses[i][j][k], i, j, k)
+
+    def test08_reshape_sets_unknown_dimensions_only(self):
+        """Reshaping fills in the dimensions the type leaves open"""
+
+        import cppjit
+        import cppjit.ll
+
+        h = cppjit.gbl.MultiDimArrays.DataHolder()
+
+        # a fixed-size array accepts only its own shape, and that must leave
+        # its strides intact (a plain reshape used to corrupt them)
+        arr = h.m_int2c
+        assert arr.shape == (3, 5)
+        strides = memoryview(arr).strides
+        arr.reshape((3, 5))
+        assert arr.shape == (3, 5)
+        assert memoryview(arr).strides == strides
+        for i in range(3):
+            for j in range(5):
+                assert arr[i][j] == 3 * i + j
+                assert arr[i, j] == 3 * i + j
+
+        raises(ValueError, arr.reshape, (5, 3))
+        raises(ValueError, arr.reshape, (15,))
+        assert arr.shape == (3, 5)
+        assert arr[2][4] == 3 * 2 + 4
+
+        # unknown dimensions can be set one at a time and, once set, stay
+        arr = h.m_int2a
+        assert len(arr.shape) == 2
+        assert arr.shape[1] == -1
+        raises(ValueError, arr.reshape, (35,))
+        arr.reshape((5, -1))
+        assert arr.shape[0] == 5 and arr.shape[1] == -1
+        raises(ValueError, arr.reshape, (7, -1))
+        arr.reshape((5, 7))
+        assert arr.shape == (5, 7)
+        for i in range(5):
+            for j in range(7):
+                assert arr[i][j] == h.m_int2a[i, j]
+
+        # a rank-1 pointer view cannot become multi-dimensional either
+        buf = cppjit.ll.malloc["int"](6)
+        assert buf.shape == (6,)
+        raises(ValueError, buf.reshape, (2, 3))
+        assert buf.shape == (6,)
+        cppjit.ll.free(buf)
+
+        # a dimension that would overflow the byte size is rejected, too; the
+        # outermost one counts row pointers here, not ints (used to slip by)
+        arr = cppjit.gbl.MultiDimArrays.DataHolder().m_int2a
+        raises(ValueError, arr.reshape, (5, sys.maxsize))
+        raises(ValueError, arr.reshape, (sys.maxsize // 4 - 1, -1))
+
+        # a freshly constructed view has no dimensions to set
+        v = cppjit._backend.LowLevelView()
+        raises(TypeError, v.reshape, ())