[pypy-commit] pypy cpyext-avoid-roundtrip: (antocuni, arigo)

[arigo]

Author: Armin Rigo <arigo at tunes.org>
Branch: cpyext-avoid-roundtrip
Changeset: r92705:86616775c934
Date: 2017-10-10 16:02 +0200
http://bitbucket.org/pypy/pypy/changeset/86616775c934/

Log:	(antocuni, arigo)

	Test and "fix": PySequence_GetItem() should return a new reference
	to an *existing* object that is kept alive by the original tuple or
	list too. PyArg_ParseTuple() and probably 3rd-party code depends on
	that.

diff --git a/pypy/module/cpyext/sequence.py b/pypy/module/cpyext/sequence.py
--- a/pypy/module/cpyext/sequence.py
+++ b/pypy/module/cpyext/sequence.py
@@ -1,11 +1,13 @@
 
 from rpython.rlib import rerased, jit
+from rpython.rlib.objectmodel import keepalive_until_here
 from pypy.interpreter.error import OperationError, oefmt
 from pypy.objspace.std.listobject import (
     ListStrategy, UNROLL_CUTOFF, W_ListObject, ObjectListStrategy)
 from pypy.module.cpyext.api import (
     cpython_api, CANNOT_FAIL, CONST_STRING, Py_ssize_t, PyObject, PyObjectP)
 from pypy.module.cpyext.pyobject import PyObject, make_ref, from_ref
+from pypy.module.cpyext.pyobject import as_pyobj, incref
 from rpython.rtyper.lltypesystem import rffi, lltype
 from pypy.objspace.std import tupleobject
 
@@ -47,6 +49,9 @@
     converted to a sequence, and raises a TypeError, raise a new TypeError with
     m as the message text. If the conversion otherwise, fails, reraise the
     original exception"""
+    # CCC test and enable these two lines:
+    #if isinstance(w_obj, tupleobject.W_TupleObject):
+    #    return w_obj
     if isinstance(w_obj, W_ListObject):
         # make sure we can return a borrowed obj from PySequence_Fast_GET_ITEM
         w_obj.convert_to_cpy_strategy(space)
@@ -132,7 +137,21 @@
 
     This function used an int type for i. This might require
     changes in your code for properly supporting 64-bit systems."""
-    return space.getitem(w_obj, space.newint(i))
+    if isinstance(w_obj, tupleobject.W_TupleObject):
+        from pypy.module.cpyext.tupleobject import PyTuple_GetItem
+        py_obj = as_pyobj(space, w_obj)
+        py_res = PyTuple_GetItem(space, py_obj, i)
+        incref(space, py_res)
+        keepalive_until_here(w_obj)
+        return py_res
+    if isinstance(w_obj, W_ListObject):
+        from pypy.module.cpyext.listobject import PyList_GET_ITEM
+        py_obj = as_pyobj(space, w_obj)
+        py_res = PyList_GET_ITEM(space, py_obj, i)
+        incref(space, py_res)
+        keepalive_until_here(w_obj)
+        return py_res
+    return make_ref(space, space.getitem(w_obj, space.newint(i)))
 
 @cpython_api([PyObject, Py_ssize_t], PyObject)
 def PySequence_GetItem(space, w_obj, i):
diff --git a/pypy/module/cpyext/test/test_sequence.py b/pypy/module/cpyext/test/test_sequence.py
--- a/pypy/module/cpyext/test/test_sequence.py
+++ b/pypy/module/cpyext/test/test_sequence.py
@@ -156,6 +156,29 @@
         result = api.PySequence_Index(w_gen, w_tofind)
         assert result == 4
 
+    def test_sequence_getitem(self, space, api):
+        # PySequence_GetItem() is defined to return a new reference.
+        # When it happens to be called on a list or tuple, it returns
+        # a new reference that is also kept alive by the fact that it
+        # lives in the list/tuple.  Some code like PyArg_ParseTuple()
+        # relies on this fact: it decrefs the result of
+        # PySequence_GetItem() but then expects it to stay alive.  Meh.
+        # Here, we check that we try hard not to break this kind of
+        # code: if written naively, it could return a fresh PyIntObject,
+        # for example.
+        w1 = space.wrap((41, 42, 43))
+        p1 = api.PySequence_GetItem(w1, 1)
+        p2 = api.PySequence_GetItem(w1, 1)
+        assert p1 == p2
+        assert p1.c_ob_refcnt > 1
+        #
+        w1 = space.wrap([41, 42, 43])
+        p1 = api.PySequence_GetItem(w1, 1)
+        p2 = api.PySequence_GetItem(w1, 1)
+        assert p1 == p2
+        assert p1.c_ob_refcnt > 1
+
+
 class AppTestSetObject(AppTestCpythonExtensionBase):
     def test_sequence_macro_cast(self):
         module = self.import_extension('foo', [