BUG: fix improper use of C-API #12524

mattip · 2018-12-10T05:40:52Z

In running tests on PyPy, there were failures due to improper use of the C-API:

Calling InitOperators to initialize the generated ufuncs before calling PyType_Ready(PyUFunc_Type)
Using the PySequence_Fast* functions before calling PySequence_Fast which works on CPython but does not always work on PyPy

We could prevent these mistakes by getting PyPy into our CI testing, but there are still test failures due to

insufficient ctypes and memoryview support in PyPy
late writing docstrings into types and objects via _multiarray_umath.add_docstring

mattip · 2018-12-10T07:04:10Z

Something is wrong with LGTM analysis.

eric-wieser · 2018-12-10T08:21:39Z

numpy/core/src/common/ufunc_override.c

+        if (seq == NULL) {
+            return -1;
+        }
+        Py_DECREF(*out_kwd_obj);


Can use Py_SETREF(*out_kwd_obj, seq) here

Is this now formally supported? I don't see it in https://docs.python.org/3.7/c-api/refcounting.html

Searching those docs, Py_SETREF was mentioned in the 3.6 release notes, it was used extensively in solving bpo-20440. It seems the documentation has yet to catch up.

More relevantly though, I added a backport of it, in npy_compat I beleive

eric-wieser · 2018-12-10T08:21:57Z

numpy/core/src/common/ufunc_override.c

@@ -93,8 +93,16 @@ PyUFuncOverride_GetOutObjects(PyObject *kwds, PyObject **out_kwd_obj, PyObject *
        return 0;
    }
    if (PyTuple_CheckExact(*out_kwd_obj)) {
-        *out_objs = PySequence_Fast_ITEMS(*out_kwd_obj);
-        return PySequence_Fast_GET_SIZE(*out_kwd_obj);
+        PyObject *seq = PySequence_Fast(*out_kwd_obj, "cannot convert");


So on pypy, exact tuples are not also always fast sequences?

They need conversion to a "fast strategy tuple". PyPy will convert a tuple of like items (all int) to an array of unboxed values. In order to support fast access, PyPy must box the values into an array of PyObject*, and build a CPython compatible PyTupleObject. Since this is expensive, it is only done when calling PySequence_Fast()

Note that the PyPy implementation of PySequence_FAST contradicts the CPython docs (emphasis mine):

PyObject* PySequence_Fast(PyObject *o, const char *m)
Return value: New reference.

Return the sequence or iterable o as a list, unless it is already a tuple or list, in which case o is returned. Use PySequence_Fast_GET_ITEM() to access the members of the result. Returns NULL on failure. If the object is not a sequence or iterable, raises TypeError with m as the message text.

So I think we need to emphasize in the comment that the documentation for PySequence_Fast does not apply to PyPy.

It might be worth contacting the CPython devs and recommending they use weaker language in their documentation, to not make this guarantee.

see python/cpython#11140

mhvk · 2018-12-10T16:58:17Z

Be sure to add some comments for the _Fast that this is needed for PyPy!

mhvk · 2018-12-11T21:25:26Z

numpy/core/src/multiarray/multiarraymodule.c

@@ -4570,6 +4570,9 @@ PyMODINIT_FUNC init_multiarray_umath(void) {
     */
    PyArray_Type.tp_hash = PyObject_HashNotImplemented;

+    if (PyType_Ready(&PyUFunc_Type) < 0)


nitpick: I think we never do the if without curly braces.

Except of course we did it in the place where you moved this from. So perhaps my sense of the style guide is wrong...

We've even got if without the regular parentheses in a few rare cases thanks to macros substituting them in

Yes, the convention is always use curly braces. Other case should be fixed when encountered.

eric-wieser · 2018-12-12T04:51:42Z

numpy/core/src/multiarray/multiarraymodule.c

@@ -4580,8 +4584,9 @@ PyMODINIT_FUNC init_multiarray_umath(void) {
    }
    initialize_casting_tables();
    initialize_numeric_types();
-    if(initscalarmath(m) < 0)
+    if(initscalarmath(m) < 0) {


nit: space after if

fixed throughout the file

eric-wieser · 2018-12-12T04:57:52Z

numpy/core/src/multiarray/common.c

+    /*
+     * The C-API recommends calling PySequence_Fast before any of the other
+     * PySequence_Fast* functions. This is required for PyPy
+     */


I think you might want to move this comment and the PySequence_Fast call to before the /* Recursive case ... */ comment

charris · 2018-12-13T17:49:28Z

numpy/core/src/multiarray/methods.c

    PyObject **in_objs, **out_objs;

    /* check inputs */
    nin = PyTuple_Size(args);
    if (nin < 0) {
        return -1;
    }
-    in_objs = PySequence_Fast_ITEMS(args);
+    fast = PySequence_Fast(args, "could not convert args for fast access");


Be nice to make all the error messages the same. Maybe "Cannot convert args for fast access" or some such if length is a problem.

unified the message

Missed one in ufunc_override.c.

sorry, fixed that too

charris · 2018-12-14T06:09:49Z

Some rainy day I'm going to fix all those long lines, but not today. Thanks Matti.

eric-wieser · 2018-12-14T06:11:57Z

numpy/core/src/common/ufunc_override.c

+        }
+        *out_objs = PySequence_Fast_ITEMS(seq);
+        ret = PySequence_Fast_GET_SIZE(seq);
+        Py_SETREF(*out_kwd_obj, seq);


The comment above says that *out_kwd_obj is a borrowed reference, but as a result of PySequence_Fast, it's now a new reference, so the caller needs to decref it.

Similarly, Py_SETREF is incorrect here - it's not safe to decref *out_kwd_obj, as you don't own it. Simply

*out_kwd_obj = seq

is what you want here

Good catch. But now seq may not be a borrowed ref. I changed the interface so that *out_kwd_obj is never a borrowed ref.

The fact that tests pass indicates there may be a leak of kwds['out'] somewhere.

Tests pass because you were swapping a refcount between seq and the original out_kwd_obj. In CPython, those objects happen to be the same, so you got away with it.

eric-wieser · 2018-12-14T06:28:21Z

@charris: Looks like we missed each other here. This patch has refcount bugs (probably only in PyPy), which need fixing.

charris · 2018-12-14T06:30:16Z

@eric-wieser Thanks for checking.

mattip · 2018-12-14T07:40:44Z

xref #12544

BUG: fix refcount issue caused by #12524

…v1.17 Does not work properly on win64 numpy/numpy#12524, merged in 2018

mattip added 2 commits December 10, 2018 07:23

MAINT: call PySequence_Fast() before any other PySequence_Fast* funcs

e64354e

BUG: call PyType_Ready before using type

1c7a2f9

mattip added 00 - Bug 09 - Backport-Candidate labels Dec 10, 2018

eric-wieser reviewed Dec 10, 2018

View reviewed changes

MAINT: comment, fix from review

a3be55e

mattip added this to the 1.16.0 release milestone Dec 11, 2018

mhvk reviewed Dec 11, 2018

View reviewed changes

STY: bracket if blocks with braces

508c0f4

eric-wieser reviewed Dec 12, 2018

View reviewed changes

mattip added 2 commits December 13, 2018 08:02

DOC: rearrange comment sequence from review

7dcee00

STY: add space after "if"

b513156

mattip mentioned this pull request Dec 13, 2018

DOC: emphasize the need to always call PySequence_Fast python/cpython#11140

Merged

charris added the 24 - PyPy label Dec 13, 2018

charris reviewed Dec 13, 2018

View reviewed changes

MAINT: unify error messages

ff32c7f

mattip force-pushed the pypy-cleanup branch from be68888 to ff32c7f Compare December 14, 2018 05:20

charris merged commit 8814211 into numpy:master Dec 14, 2018

eric-wieser reviewed Dec 14, 2018

View reviewed changes

mattip deleted the pypy-cleanup branch December 14, 2018 07:36

mattip added a commit to mattip/numpy that referenced this pull request Dec 14, 2018

BUG: fix refcount issue caused by numpy#12524

e41e97d

mattip mentioned this pull request Dec 14, 2018

BUG: fix refcount issue caused by #12524 #12544

Merged

charris removed the 09 - Backport-Candidate label Dec 16, 2018

charris removed this from the 1.16.0 release milestone Dec 16, 2018

charris added a commit that referenced this pull request Dec 17, 2018

Merge pull request #12544 from mattip/pypy-fixes2

38236fb

BUG: fix refcount issue caused by #12524

charris pushed a commit to charris/numpy that referenced this pull request Dec 17, 2018

BUG: fix refcount issue caused by numpy#12524

0190896

charris mentioned this pull request Dec 17, 2018

BUG: fix refcount issue caused by #12524 #12573

Merged

charris added a commit that referenced this pull request Dec 17, 2018

Merge pull request #12573 from charris/backport-12544

2b0bb38

BUG: fix refcount issue caused by #12524

mozillazg pushed a commit to mozillazg/pypy that referenced this pull request Jul 4, 2021

remove hack to look up function by offset, not needed by numpy since …

6788bd2

…v1.17 Does not work properly on win64 numpy/numpy#12524, merged in 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix improper use of C-API #12524

BUG: fix improper use of C-API #12524

mattip commented Dec 10, 2018

mattip commented Dec 10, 2018

eric-wieser Dec 10, 2018

mhvk Dec 10, 2018

mattip Dec 10, 2018

eric-wieser Dec 12, 2018

eric-wieser Dec 10, 2018

mattip Dec 10, 2018

eric-wieser Dec 12, 2018

mattip Dec 13, 2018

mhvk commented Dec 10, 2018

mhvk Dec 11, 2018

mhvk Dec 11, 2018

tylerjereddy Dec 11, 2018

charris Dec 11, 2018

mattip Dec 12, 2018

eric-wieser Dec 12, 2018

mattip Dec 13, 2018

eric-wieser Dec 12, 2018

mattip Dec 13, 2018

charris Dec 13, 2018

mattip Dec 13, 2018

charris Dec 14, 2018

mattip Dec 14, 2018

charris commented Dec 14, 2018

eric-wieser Dec 14, 2018

eric-wieser Dec 14, 2018

mattip Dec 14, 2018

mattip Dec 14, 2018

eric-wieser Dec 14, 2018

eric-wieser commented Dec 14, 2018 •

edited

Loading

charris commented Dec 14, 2018

mattip commented Dec 14, 2018

BUG: fix improper use of C-API #12524

BUG: fix improper use of C-API #12524

Conversation

mattip commented Dec 10, 2018

mattip commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhvk commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eric-wieser commented Dec 14, 2018 • edited Loading

charris commented Dec 14, 2018

mattip commented Dec 14, 2018

eric-wieser commented Dec 14, 2018 •

edited

Loading