Python object INCREF verification #3023
Conversation
This comment has been minimized.
This comment has been minimized.
ferdonline
changed the title
|
I extracted some of these changes to #3024 so that here we can focus on the INCREF/DECREF verification |
This comment has been minimized.
This comment has been minimized.
ferdonline
force-pushed
the
enh/py_bind/ref_control
branch
from
cfb83ce
to
7c835a0
Compare
This comment has been minimized.
This comment has been minimized.
|
✔️ a0d0d07 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
ferdonline
changed the title
There was a problem hiding this comment.
Seems fine, only minor comments. Please review the part that's a bug/leak fix. If the change is correct, please isolate it in a separate PR. The reason is that this PR mostly doesn't matter. However, hidden in the middle is a bug fix, if we keep it here it's less obvious that the change was intentional; and the commit title wont alert us to the fact that this commit also fixes a leak.
ferdonline
force-pushed
the
enh/py_bind/nb_api_more
branch
from
76305ff
to
c526b7f
Compare
ferdonline
force-pushed
the
enh/py_bind/ref_control
branch
from
a0d0d07
to
9775324
Compare
|
✔️ 9775324 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
|
✔️ aa969ae -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3023 +/- ##
==========================================
- Coverage 67.27% 67.26% -0.01%
==========================================
Files 571 571
Lines 104877 104874 -3
==========================================
- Hits 70554 70543 -11
- Misses 34323 34331 +8 ☔ View full report in Codecov by Sentry. |
|
✔️ 0299c65 -> Azure artifacts URL |
### Context Following #3023, we noticed that there might be extraneous call to INCREF, given it's done on an object returned by `tp_alloc`, which should return a new reference. New references have their ref count to 1 and therefore it should not require any additional reference increment. ### Bug Reproducer ~$ nrniv -python ```python from neuron import h h("create soma") h("objref nc, nil") h('soma nc = new NetCon(&v(0.5), nil)') seg = h.nc.preseg() ``` Exit the interpreter and observe ``` Fatal Python error: bool_dealloc: deallocating True or False: bug likely caused by a refcount error in a C extension ``` ### Scope - Replace the code by the single function `newpysechelp(sec)` which seems to do just the right thing. - Added unit test ### Testing Two python unit tests added: - test_seg_from_sec_x_ref_python - test_seg_from_sec_x_ref_hocpy For debug-ability it might be worth to add the following snippet to `seg_from_sec_x` ``` auto* psec = sec->prop->dparam[PROP_PY_INDEX].get<void*>(); fprintf(stderr, "In seg_from_sec_x. Pysec=%p\n", psec); ``` We will see along the lines of ``` In seg_from_sec_x. Pysec=0x7fa081736150 In seg_from_sec_x. Pysec=0x0 ``` ensuring the we are exercising both cases. After dropping the original section, the pure-python code end up with the same number of references as the hoc-python code.
Objective
Improve bindings: simplify code and improve reference management.
This PR
Py_INCREF. As an idiom, we move it immediately before to the place which utilizes the reference, either an assignment, or a return.X[INC/DEC]REFOriginally
However, Py_INCREF is currently almost exclusively used for an object being assigned to a
PyObject*field of a newly created object. Such fields come non-initialized, possibly non-0, and therefore we can't rely onXDECREF.