[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: Added file: http://bugs.python.org/file37954/doc_c-api_buffer.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: Removed file: http://bugs.python.org/file37952/doc_c-api_buffer.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: Added file: http://bugs.python.org/file37953/doc_c-api_buffer.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: Removed file: http://bugs.python.org/file37953/doc_c-api_buffer.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: When I compile and run the above (latest Cython from Git master), I get: () () With Cython version 0.20.1post0 I get: foo.foo() (-1,) (-1,) If you get the correct output from the latest Cython, it looks like this issue has been fixed. Oops, I was running a locally modified version of Cython that contained a patch meant to work around issue #23349. When I run the *actual* upstream master I get the same behavior that you do. But either way, I don't see why it's a problem that it prints (-1,) for the PyBUF_ND case. The consumer didn't request suboffsets information so I would expect it to be OK for the producer to put whatever it wants in the suboffsets field, including a junk pointer that would segfault upon dereference. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: How might an application break with this change? assert(view-suboffsets == NULL); Fair point. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: This leaves me +-0 for the change, with the caveat that applications might break. How might an application break with this change? Compared to the current PyBuffer_IsContiguous(), the patched version is the same except it returns true for a wider range of actually-contiguous buffers. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: Richard Hansen added the comment: Cython doesn't follow the spec though (use Python 3): from _testbuffer import * cpdef foo(): cdef unsigned char[:] v = bytearray(btesting) nd = ndarray(v, getbuf=PyBUF_ND) print(nd.suboffsets) nd = ndarray(v, getbuf=PyBUF_FULL) print(nd.suboffsets) When I compile and run the above (latest Cython from Git master), I get: () () With Cython version 0.20.1post0 I get: foo.foo() (-1,) (-1,) If you get the correct output from the latest Cython, it looks like this issue has been fixed. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: How might an application break with this change? func1: if (!PyBuffer_IsContiguous(view, 'C')) error(); func2(view); func2: assert(view-suboffsets == NULL); ... Yes, I *did* insert sanity checks like this in some places. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Sebastian Berg added the comment: I do not have an opinion either way, but I do think there is a small difference here compared to the other issue. With the other issue, there are cases where we cannot set the strides correctly. If you ask numpy directly whether the array is contiguous (or create it from numpy) and it says yes, and then you get it without asking for a contiguous array it is, without the change, possible to get an array that would *not* be considered contiguous. On the other hand, setting suboffsets to NULL when all are -1 is always possible, if tedious I admit. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: My preference is to apply the patch, of course. There is a legitimate concern that it will break existing code, but I think there are more points in favor of applying the patch: * there exists code that the current behavior is known to break * it's easier to remove assert(view-suboffsets == NULL) than to change producers to not provide an all-negative array (though I admit there may be other more subtle ways the patch would break existing code) * I may be the only one who has spoken up about this, but I doubt I'm the only one who has experienced it -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: To summarize, I think this is different from #22445, which also requests relaxed contiguity tests. #22445 is motivated by the fact that slicing can create a certain class of contiguous buffers that aren't recognized as such. No such reason exists here: All-negative suboffsets are only created if a buffer provider chooses to hand them out instead of NULL. To my knowledge only Cython does this, and it's against the PEP (though strictly speaking not against the docs). So I'm -0.5 on this change in general and -1 for 2.7. I'd reject the issue, but let's wait for Antoine's opinion. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Antoine Pitrou added the comment: I don't have an opinion on this. I've never seen suboffsets in use; but it seems reasonable to detect a dummy suboffsets array and recognize it as contiguous. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: Prepare for a partial reversal of opinion. :) Indexing *can* actually create all-negative suboffsets in a natural way: from _testbuffer import * nd = ndarray([1,2,3,4,5,6,7,8,9,10,11,12], shape=[3,4], flags=ND_PIL) nd.tolist() [[1, 2, 3, 4], [5, 6, 7, 8], [9, 10, 11, 12]] x = nd[1] x.tolist() [5, 6, 7, 8] x.suboffsets (-1,) This leaves me +-0 for the change, with the caveat that applications might break. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: (The following message is mostly off-topic but I think it is relevant to those interested in this issue. This message is about the clarity of the documentation regarding flag semantics, and what I think the flags should mean.) Cython doesn't follow the spec though (use Python 3): from _testbuffer import * cpdef foo(): cdef unsigned char[:] v = bytearray(btesting) nd = ndarray(v, getbuf=PyBUF_ND) print(nd.suboffsets) nd = ndarray(v, getbuf=PyBUF_FULL) print(nd.suboffsets) When I compile and run the above (latest Cython from Git master), I get: () () Looking at the Python source code (Modules/_testbuffer.c ndarray_get_suboffsets() and ssize_array_as_tuple()) the above output is printed if the suboffsets member is NULL. Cython hands out suboffsets even for a PyBUF_ND request, which is definitely wrong. For a PyBUF_FULL request they should only be provided *if needed*. suboffsets appears to be NULL in both cases in your example, which seems acceptable to me given: * the user didn't request suboffsets information in the PyBUF_ND case, and * there are no indirections in the PyBUF_FULL case. Even if suboffsets wasn't NULL, I believe the behavior would still be correct for the PyBUF_ND case. My reading of https://docs.python.org/3/c-api/buffer.html is that flags=PyBUF_ND implies that shape member MUST NOT be NULL, but strides and suboffsets can be NULL or not (it doesn't matter). This interpretation of mine is due to the sentence, Note that each flag contains all bits of the flags below it. If flags=PyBUF_ND meant that strides and suboffsets MUST be NULL, then PyBUF_ND and PyBUF_STRIDES would necessarily be mutually exclusive and flags=(PyBUF_ND|PyBUF_STRIDES) would not make sense (the strides member would have to be both NULL and non-NULL). If (flags PyBUF_INDIRECT) is false, then the consumer is not interested in the suboffsets member so it shouldn't matter what it points to. (And the consumer should not dereference the pointer in case it points to a junk address.) IMHO, if the buffer is C-style contiguous then the producer should be allowed to populate the shape, strides, and suboffsets members regardless of whether or not any of the PyBUF_ND, PyBUF_STRIDES, or PyBUF_INDIRECT flags are set. In other words, for C-style contiguous buffers, the producer should be allowed to act as if PyBUF_INDIRECT was always provided because the consumer will always get an appropriate Py_buffer struct regardless of the actual state of the PyBUF_ND, PyBUF_STRIDES, and PyBUF_INDIRECT flags. It *is* a bug, however, to dereference the strides or suboffsets members with ndarray(v, getbuf=PyBUF_ND) because the consumer didn't ask for strides or suboffsets information and the pointers might be bogus. Stepping back a bit, I believe that the flags should be thought of as imposing requirements on the producer. I think those requirements should be (assuming ndim 0): * PyBUF_ND: If (flags PyBUF_ND) is true: - If (flags PyBUF_STRIDES) is false *and* the producer is unable to produce a block of memory at [buf,buf+len) containing all (len/itemsize) entries in a C-style contiguous chunk, then the producer must raise an exception. - Otherwise, the producer must provide the shape of buffer. If (flags PyBUF_ND) is false: - If the producer is unable to produce a contiguous chunk of (len/itemsize) entries (of any shape) in the block of memory at [buf,buf+len), then the producer must raise an exception. - Otherwise, the producer is permitted to do any of the following: + don't touch the shape member (don't set it to NULL or any other value; just leave it as-is) + set the shape member to NULL + set the shape member to point to an array describing the shape + set the shape member to point to any other location, even if dereferencing the pointer would result in a segfault * PyBUF_STRIDES: If (flags PyBUF_STRIDES) is true: - The producer must provide the appropriate strides information. If (flags PyBUF_STRIDES) is false: - If the producer is unable to produce a block of memory at [buf,buf+len) containing all (len/itemsize) entries, the producer must raise an exception. - Otherwise, the producer is permitted to do any of the following; + don't touch the strides member (don't set it to NULL or any other value; just leave it as-is) + set the strides member to NULL + set the strides member to point to an array describing the strides + set the strides member to point to any other location, even if dereferencing the pointer would result in a segfault * PyBUF_INDIRECT: If (flags PyBUF_INDIRECT) is true:
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: Attached is a documentation patch that adds what I said in msg235141. I doubt everyone will agree with the changes, but maybe it will be a useful starting point. (Despite not having an asterisk next to my username, I have signed the contributor agreement. It just hasn't been processed yet.) -- Added file: http://bugs.python.org/file37952/doc_c-api_buffer.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: Cython doesn't follow the spec though (use Python 3): from _testbuffer import * cpdef foo(): cdef unsigned char[:] v = bytearray(btesting) nd = ndarray(v, getbuf=PyBUF_ND) print(nd.suboffsets) nd = ndarray(v, getbuf=PyBUF_FULL) print(nd.suboffsets) Cython hands out suboffsets even for a PyBUF_ND request, which is definitely wrong. For a PyBUF_FULL request they should only be provided *if needed*. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: People might rely on the fact that contiguous implies suboffsets==NULL. Cython (currently) relies on all-negatives being acceptable and equivalent to suboffsets==NULL. See: https://github.com/cython/cython/pull/367 http://thread.gmane.org/gmane.comp.python.cython.devel/15569 -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Sebastian Berg added the comment: Numpy does not understand suboffsets. The buffers we create will always have them NULL. The other way around To be honest, think it is probably ignoring the whole fact that they might exist at all :/, really needs to be fixed if it is the case. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: For the record: I'm myself guilty of accepting all-negative suboffsets in memoryview.c (see init_slice()) and I think there's even a test for it. However, while it's ok for a specific function to accept this corner case, I'm not sure about is_contiguous(). People might rely on the fact that contiguous implies suboffsets==NULL. Sebastian, did this special case ever come up in the context of NumPy? -- nosy: +seberg ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Stefan Krah added the comment: PEP-3118 says: Py_buffer.suboffsets: If all suboffsets are negative (i.e. no de-referencing is needed, then this must be NULL (the default value). I would be inclined to go with the PEP here and make a doc fix instead. -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Antoine Pitrou added the comment: The patch has an obvious syntax error :-) Other than that, adding a comment would be nice. Bonus points if you can write a test (3.x has infrastructure for that, see Lib/test/test_buffer.py). -- nosy: +pitrou, skrah stage: - patch review versions: -Python 3.2, Python 3.3, Python 3.6 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: -- type: - behavior ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: The patch has an obvious syntax error :-) Doh! Other than that, adding a comment would be nice. Agreed, will do. Bonus points if you can write a test (3.x has infrastructure for that, see Lib/test/test_buffer.py). I'll take a look. Thanks! -- ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
New submission from Richard Hansen: According to https://docs.python.org/2/c-api/buffer.html#the-new-style-py-buffer-struct if the suboffsets member of Py_buffer is non-NULL and all members of the array are negative, the buffer may be contiguous. PyBuffer_IsContiguous() does not behave this way. It assumes that if the suboffsets member is non-NULL then at least one member of the array is non-negative, and thus assumes that the buffer is non-contiguous. This is not always correct. One consequence of this bug is PyBuffer_ToContiguous() (and thus memoryview.tobytes()) falls back to slow (and currently buggy, see issue #23349) memory copying code. Attached is a patch that fixes this bug. The patch is against 2.7 but can be trivially modified to apply to 3.x. -- components: Interpreter Core files: PyBuffer_IsContiguous.patch keywords: patch messages: 235014 nosy: rhansen priority: normal severity: normal status: open title: PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL versions: Python 2.7, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 3.6 Added file: http://bugs.python.org/file37914/PyBuffer_IsContiguous.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: -- versions: -Python 3.2, Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Changes by Richard Hansen rhan...@bbn.com: -- versions: +Python 3.2, Python 3.3 ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue23352] PyBuffer_IsContiguous() sometimes returns wrong result if suboffsets != NULL
Richard Hansen added the comment: I've attached a new version of the patch. Suggestions for simplifying the test code would be appreciated. -- Added file: http://bugs.python.org/file37916/PyBuffer_IsContiguous_v2.patch ___ Python tracker rep...@bugs.python.org http://bugs.python.org/issue23352 ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
