New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-16865: Support arrays >=2GB in ctypes #3006
bpo-16865: Support arrays >=2GB in ctypes #3006
Conversation
using
Maybe using |
@orenmn Changed it. |
Modules/_ctypes/_ctypes.c
Outdated
@@ -1413,7 +1413,12 @@ PyCArrayType_new(PyTypeObject *type, PyObject *args, PyObject *kwds) | |||
Py_XDECREF(length_attr); | |||
goto error; | |||
} | |||
|
|||
#if SIZEOF_SIZE_T <= SIZEOF_LONG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless i am missing something, this #if
is potentially problematic. In case sizeof(size_t) < sizeof(long)
, there could be a silent truncation here.
Maybe change to #if SIZEOF_SIZE_T == SIZEOF_LONG
, and then later #elif SIZEOF_SIZE_T == SIZEOF_LONG_LONG
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will of course make the code fail to build in cases where sizeof(size_t) != sizeof(long) && sizeof(size_t) != sizeof(long long)
. Is that desirable @serhiy-storchaka ? It used to previously truncate if sizeof(size_t) < sizeof(long)
. (Does Python even support such systems...?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could raise an OverflowError in cases where sizeof(size_t) != sizeof(long) && sizeof(size_t) != sizeof(long long)
and _length_
is too large.
IIUC, without your patch, there is no silent truncation, but an OverflowError.
(Of course, this is only my opinion, so take it with a grain of salt :) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was previously just: Modules/_ctypes/_ctypes.c:1416, so it used to silently truncate if sizeof(size_t) < sizeof(long)
. When sizeof(size_t) > sizeof(long)
, it raised OverflowError
when _length_
was too big. With the patch, the behavior when sizeof(size_t) < sizeof(long)
is preserved, while when it's bigger, it will use PyLong_AsLongLongAndOverflow
which allows to pass larger lengths.
I'm reluctant to add code to handle sizeof(size_t) < sizeof(long)
. Especially if CPython itself doesn't support such a platform, since it will never be tested. Even more so since it requires to write a custom variation of PyLong_As{Type}AndOverflow
to handle the smaller types, and which types should that be...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad with regard to the silent truncation: it seems that you are right, as later on we have stgdict->length = length;
, and stgdict->length
is Py_ssize_t
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think that it is better to restore the initial variant of this PR, with using PyLong_AsSsize_t()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@serhiy-storchaka @orenmn Maybe if I use PyLong_AsSsize_t
and do something like:
if (PyErr_ExceptionMatches(PyExc_OverflowError))
PyErr_SetString(PyExc_OverflowError,
"The '_length_' attribute is too large");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, it seems we will only reach the code here when declaring an array via inheritance, it will fail sooner in Objects/abstract.c:953-967 due to the call to PyNumber_AsSsize_t
, when declaring an array via the *
operator (Obviously with a different error). This happens because the *
operator in ctypes
is implemented via PySequenceMethods.sq_repeat
.
40ad59e
to
ad24633
Compare
Thanks @segevfiner for the PR, and @serhiy-storchaka for merging it |
(cherry picked from commit 735abad) Co-authored-by: Segev Finer <segev208@gmail.com>
GH-6842 is a backport of this pull request to the 3.7 branch. |
Sorry, @segevfiner and @serhiy-storchaka, I could not cleanly backport this to |
(cherry picked from commit 735abad) Co-authored-by: Segev Finer <segev208@gmail.com>
GH-6843 is a backport of this pull request to the 3.6 branch. |
GH-7441 is a backport of this pull request to the 2.7 branch. |
(cherry picked from commit 735abad) Co-authored-by: Segev Finer <segev208@gmail.com>
https://bugs.python.org/issue16865