(I've sent this also as a Bitbucket pull request. Sending it here too
because I'm not sure whose attention that reaches, and also for the
sake of developing in the open.)
We were increfing an object on the first time we borrow a reference to it,
then decrefing it on the first time we destroy a borrowed-from container.
So if we borrow from two containers, then destroy one, we'd take the
refcount back where it started -- possibly zero -- even though the C code
still has a legitimate reference. This is demonstrated by test_borrow_destroy
in test_borrow.py, added last night.
Change the logic to incref the object the first time we borrow it from
each container, and decref when any borrowed-from container is destroyed.
Also fix a NameError in some code that runs when DEBUG_REFCOUNT is set.
pypy/module/cpyext/pyobject.py | 32 +++++++++-----------------------
pypy/module/cpyext/test/test_borrow.py | 17 ++++++++++++++++-
2 files changed, 25 insertions(+), 24 deletions(-)
diff --git a/pypy/module/cpyext/pyobject.py b/pypy/module/cpyext/pyobject.py
index 99aabfb..9e42153 100644
--- a/pypy/module/cpyext/pyobject.py
+++ b/pypy/module/cpyext/pyobject.py
@@ -144,14 +144,11 @@ class RefcountState:
# { w_container -> { w_containee -> None } }
# the None entry manages references borrowed during a call to
# generic_cpy_call()
- self.borrowed_objects = {}
- # { addr of containee -> None }
# For tests
self.non_heaptypes_w = []
def _freeze_(self):
- assert not self.borrowed_objects
assert self.borrow_mapping == {None: {}}
self.py_objects_r2w.clear() # is not valid anymore after translation
return False
@@ -187,22 +184,19 @@ class RefcountState:
"""
ref = make_ref(self.space, w_borrowed)
obj_ptr = rffi.cast(ADDR, ref)
- if obj_ptr not in self.borrowed_objects:
- # borrowed_objects owns the reference
- self.borrowed_objects[obj_ptr] = None
- else:
- Py_DecRef(self.space, ref) # already in borrowed list
borrowees = self.borrow_mapping.setdefault(w_container, {})
- borrowees[w_borrowed] = None
+ if w_borrowed in borrowees:
+ Py_DecRef(self.space, w_borrowed) # cancel incref from make_ref()
+ else:
+ borrowees[w_borrowed] = None
+
return ref
def reset_borrowed_references(self):
"Used in tests"
- while self.borrowed_objects:
- addr, _ = self.borrowed_objects.popitem()
- w_obj = self.py_objects_r2w[addr]
- Py_DecRef(self.space, w_obj)
+ for w_container, w_borrowed in self.borrow_mapping.items():
+ Py_DecRef(self.space, w_borrowed)
self.borrow_mapping = {None: {}}
def delete_borrower(self, w_obj):
@@ -232,17 +226,10 @@ class RefcountState:
ref = self.py_objects_w2r.get(w_obj, lltype.nullptr(PyObject.TO))
if not ref:
if DEBUG_REFCOUNT:
- print >>sys.stderr, "Borrowed object is already gone:", \
- hex(containee)
+ print >>sys.stderr, "Borrowed object is already gone!"
return
- containee_ptr = rffi.cast(ADDR, ref)
- try:
- del self.borrowed_objects[containee_ptr]
- except KeyError:
- pass
- else:
- Py_DecRef(self.space, ref)
+ Py_DecRef(self.space, ref)
class InvalidPointerException(Exception):
pass
@@ -290,7 +277,6 @@ def track_reference(space, py_obj, w_obj, replace=False):
if not replace:
assert w_obj not in state.py_objects_w2r
assert ptr not in state.py_objects_r2w
- assert ptr not in state.borrowed_objects
state.py_objects_w2r[w_obj] = py_obj
if ptr: # init_typeobject() bootstraps with NULL references
state.py_objects_r2w[ptr] = w_obj
diff --git a/pypy/module/cpyext/test/test_borrow.py
b/pypy/module/cpyext/test/test_borrow.py
index 25f5101..c903271 100644
--- a/pypy/module/cpyext/test/test_borrow.py
+++ b/pypy/module/cpyext/test/test_borrow.py
@@ -39,7 +39,6 @@ class AppTestBorrow(AppTestCpythonExtensionBase):
assert module.test_borrowing() # the test should not leak
def test_borrow_destroy(self):
- skip("FIXME")
module = self.import_extension('foo', [
("test_borrow_destroy", "METH_NOARGS",
"""
@@ -59,3 +58,19 @@ class AppTestBorrow(AppTestCpythonExtensionBase):
"""),
])
assert module.test_borrow_destroy() == 42
+
+ def test_double_borrow(self):
+ module = self.import_extension('foo', [
+ ("run", "METH_NOARGS",
+ """
+ PyObject *t = PyTuple_New(1);
+ PyObject *i = PyInt_FromLong(42);
+ PyTuple_SetItem(t, 0, i);
+ i = PyTuple_GetItem(t, 0);
+ PyTuple_GetItem(t, 0);
+ Py_DECREF(t);
+ return PyInt_FromLong(PyInt_AsLong(i));
+ """),
+ ])
+ # i.e., there was an InvalidPointerException
+ assert module.run() == 0
_______________________________________________
pypy-dev@codespeak.net
http://codespeak.net/mailman/listinfo/pypy-dev
