Skip to content
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

Merged

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented Aug 5, 2017

@orenmn
Copy link
Contributor

orenmn commented Sep 29, 2017

using PyLong_AsSsize_t() (instead of PyLong_AsLongAndOverflow()) makes it harder to figure out whether _length_ is too big or negative.
Also, ignoring the wrong error messages for negative _length_ (which i hope to fix in bpo-29843), this patch would cause an inconsistency in error messages. for example, on 32-bit Python on Windows, you get:

class MyArray(Array):
    _type_ = c_longlong
    _length_ = (1 << 31) - 1
OverflowError: array too large

class MyArray(Array):
    _type_ = c_longlong
    _length_ = 1 << 31
OverflowError: Python int too large to convert to C ssize_t

Maybe using PyLong_AsLongLongAndOverflow() is an option?

@segevfiner
Copy link
Contributor Author

segevfiner commented Sep 29, 2017

@orenmn Changed it.

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement needs backport to 2.7 type-bug An unexpected behavior, bug, or error and removed type-feature A feature request or enhancement labels Oct 8, 2017
@@ -1413,7 +1413,12 @@ PyCArrayType_new(PyTypeObject *type, PyObject *args, PyObject *kwds)
Py_XDECREF(length_attr);
goto error;
}

#if SIZEOF_SIZE_T <= SIZEOF_LONG
Copy link
Contributor

@orenmn orenmn Oct 8, 2017

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?

Copy link
Contributor Author

@segevfiner segevfiner Oct 8, 2017

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...?)

Copy link
Contributor

@orenmn orenmn Oct 8, 2017

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 :) )

Copy link
Contributor Author

@segevfiner segevfiner Oct 8, 2017

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...

Copy link
Contributor

@orenmn orenmn Oct 8, 2017

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.

Copy link
Member

@serhiy-storchaka serhiy-storchaka Oct 8, 2017

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().

Copy link
Contributor Author

@segevfiner segevfiner Oct 8, 2017

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");

Copy link
Member

@serhiy-storchaka serhiy-storchaka Oct 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Contributor Author

@segevfiner segevfiner Oct 8, 2017

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.

auvipy
auvipy approved these changes Apr 8, 2018
@serhiy-storchaka serhiy-storchaka self-assigned this May 14, 2018
@serhiy-storchaka serhiy-storchaka merged commit 735abad into python:master May 14, 2018
@miss-islington
Copy link
Contributor

miss-islington commented May 14, 2018

Thanks @segevfiner for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 14, 2018
(cherry picked from commit 735abad)

Co-authored-by: Segev Finer <segev208@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented May 14, 2018

GH-6842 is a backport of this pull request to the 3.7 branch.

@miss-islington
Copy link
Contributor

miss-islington commented May 14, 2018

Sorry, @segevfiner and @serhiy-storchaka, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 735abadd5bd91db4a9e6f4311969b0afacca0a1a 2.7

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 14, 2018
(cherry picked from commit 735abad)

Co-authored-by: Segev Finer <segev208@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented May 14, 2018

GH-6843 is a backport of this pull request to the 3.6 branch.

miss-islington added a commit that referenced this pull request May 15, 2018
(cherry picked from commit 735abad)

Co-authored-by: Segev Finer <segev208@gmail.com>
miss-islington added a commit that referenced this pull request May 15, 2018
(cherry picked from commit 735abad)

Co-authored-by: Segev Finer <segev208@gmail.com>
@bedevere-bot
Copy link

bedevere-bot commented Jun 6, 2018

GH-7441 is a backport of this pull request to the 2.7 branch.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request Jun 6, 2018
(cherry picked from commit 735abad)

Co-authored-by: Segev Finer <segev208@gmail.com>
@segevfiner segevfiner deleted the bpo-16865-ctypes-arrays-max-size branch Jul 16, 2018
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
serhiy-storchaka added a commit that referenced this pull request Dec 4, 2018
(cherry picked from commit 735abad)

Co-Authored-By: Segev Finer <segev208@gmail.com>
@serhiy-storchaka serhiy-storchaka removed their assignment Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants