Re: [Python-Dev] Removal of _Py_ForgetReference from public header in 3.9 issue
On 2020-06-14 22:10, cpython@nicwatson.org wrote:
Please excuse if this is the wrong mailing list. I couldn't find one for module maintainers.
This is relevant to capi-sig@python.org; let's continue here.
I maintain an open source Python module in C. I'm trying to verify for the first time that the module still works with cpython 3.9. This module does *not* use the "limited" C API.
In building my module against 3.9b3, I'm getting a missing declaration warning on _Py_ForgetReference. My module builds and passes test fine, this is just a compiler warning issue.
What does the _Py_ForgetReference function do? The documentation says it's only for use in the interpereter core, so I'd assume it's .
I can't follow the reasoning behind the code easily. Why do you use _Py_ForgetReference and PyObject_Del, instead of Py_DECREF(self)?
Should I put an #ifdef Py_TRACE_REFS around the call? Ignore it? What do you think is the proper resolution?
On 15.06.2020 11:02, Petr Viktorin wrote:
This is typically used in error handlers of constructors where you want to avoid having the deallocator run as result of the DECREF, e.g. because the object has not been full initialized yet.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 2020-06-15 11:14, M.-A. Lemburg wrote:
For that I'd recommend marking the object invalid (e.g. by setting some vital pointer in it to NULL), and teaching the deallocator to skip its logic.
If that's not enough we could document and support a "Py_ForgetReference" as public API in CPython. But having only internal API for a use case is not good.
On 15.06.2020 11:35, Petr Viktorin wrote:
Sure, there are often workarounds which can be used, but we do need some way to tell the interpreter "the initialization had a problem, please remove the object from your internal management resources without calling the deallocator".
Perhaps a new public API could be created for this purpose, like you say.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 6/15/20 12:58 PM, M.-A. Lemburg wrote:
I respectfully disagree. There is purposefully a separation between __new__ and __init__. A failure in __new__ should not call __del__, and a failure in __init__ should be gracefully handled by the __del__. It is not the interpreter's duty to provide an interface that breaks this construct. A c-extension class should not get a pass to do things differently from a python-level class. It is important that CPython not provide interfaces that will make other implementation's lives harder (PyPy, GraalPython, MicroPython). Anyone using C-API functions that start with a "_" should be very thoughtful since those are not meant to be public, usually for good reason.
Matti
On 15.06.2020 12:53, Matti Picus wrote:
In many cases, the above will work and you can signal your deallocator which parts of the object have already been initialized, but this is not always possible.
The fact that extension writers turn to APIs marked as internal is usually triggered by lack of public APIs for the purpose. This should be a signal to consider making an API public or creating a better interface to provide similar logic.
FWIW: I don't see problems for other implementations to provide the above API, but perhaps I'm missing something.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 2020-06-15 13:47, M.-A. Lemburg wrote:
Do you have a concrete example where tis is not possible?
"_Py_ForgetReference and PyObject_Del will do normal deallocation of the object *except* calling the user-provided deallocator" strikes me as a very fragile assumption. Where is the assumption is coming from? (That's what I meant to ask when I asked "What does the _Py_ForgetReference function do?" – apologies for wording it in a misleading/loaded way.)
Unfortunately, this doesn't really work as a signal, because nothing is actually signalled back to CPython devs – until the private API in question stops working.
FWIW: I don't see problems for other implementations to provide the above API, but perhaps I'm missing something.
I see no documentation on what the function should do. I assume providing a function with unspecified semantics is always a problem.
On 15.06.2020 14:36, Petr Viktorin wrote:
An error handler in the constructor will know which parts have been initialized and which not, e.g. say an object needs to configure a number of objects in a 3rd party library and one of these steps fails. The constructor can then undo the steps it has taken up to the point where the error occurred.
The deallocator will assume that the complete stack has been completely setup and can be taken down again. Unless there's a way for the deallocator to detect that it is being called on a non-initialized object, it can cause additional errors in the 3rd party library and leave external resources allocated as a result.
The semantics are already defined by the implementation: it will do whatever extra logic the interpreter applies (unlinking the object in debug builds at the moment) when finalizing the object.
The docs would not have to detail what exactly happens, but only state that by calling this API, the object will effectively be taken out of the scope of the interpeter.
True, there's a feedback loop missing for such situations. Perhaps we should create one (this C API ML already serves in part as such a feedback loop).
See above. Being unspecific is intentional, as long as the pre- and post-conditions are made clear in the docs.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
The feedback loop is already there: the issue tracker and various mailing lists. This is an undocumented, private API which should be a clear indication that 3th-party developers are not supposed to use it.
The unspecified part is that there are no documented pre- and post-conditions ;-).
For this particular API I’m far from convinced that exposing it (and another private API used by the OP) is necessary.
Ronald
— Twitter / micro.blog: @ronaldoussoren Blog: https://blog.ronaldoussoren.net/
On 6/15/20 2:47 PM, M.-A. Lemburg wrote:
FWIW: I don't see problems for other implementations to provide the above API, but perhaps I'm missing something.
The problem is that there is no pure python equivalent, and the other implementations have to spend quite a lot of effort mimicking the obscure corners of the C-API. In my opinion, any public C-API function should mirror a python equivalent. Python has no overt language specification, but I would hope one of the underlying principles would be to expose the same paradigms in python and C.
This can be handled in __del__ as well by setting flags to 0 and pointers to NULL values when calling __new__ (or simply calling memset(..0) ), then filling them out in __init__ one at a time. Any 0-ed values indicate that the __del__ function does not have to undo the initialization/allocation of that value.
Matti
On 15.06.2020 16:35, Matti Picus wrote:
That's an odd request. We're talking about the C API here, not the Python API. Of course, the C API will have different requirements than the Python one, simply because it's C.
The call is needed to let the CPython implementation know that it should unlink the object at the moment, but this could be changed in the future. Other implementations could use it for different things.
It's really just a way to tell Python: please remove this object from your management scope.
There is no Python equivalent, since the low level memory management happens entirely in C.
Yes, I know, but think of an object which doesn't otherwise require such a flag. You'd increase the memory footprint just to be able to do proper error handling.
Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 2020-06-15 17:05, M.-A. Lemburg wrote:
The request is to not extend the C-API not only with a new function, but also with a notion of an object that's allocated using PyObject_New but not in the interpreter's "management scope".
Do we have any evidence such objects exist in the real world? In other words, is this enough of a problem that it needs new public API?
On 15.06.2020 17:15, Petr Viktorin wrote:
Perhaps I wasn't clear: the object is already allocated and managed by Python. The error happens in the initializer and the API is meant to signal: please remove this object from the management scope, since I have already dealt with the cleanup.
Well, if you search on Github, you find some 18k hits. Many are from forks of the Python source code, but there are also a number of hits in stackless, nuitka, pyuv, PyPy's cpyext and numpy from a quick inspection.
Most uses call the API to prevent segfaults in debug builds (which use can use object tracing -- not sure whether they always do).
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 17:39, M.-A. Lemburg <mal@egenix.com> a écrit :
Stackless is a fork of Python. Occurrences in Python code base should be ignored.
nuitka, pyuv, PyPy's cpyext and numpy from a quick inspection.
I cannot find _Py_ForgetReference() or _Py_NewReference() in numpy code base (I checked the master branch).
Well, cpyext only implements functions to be compatible with CPython.
nuitka copied/pasted resize_compact() of CPython Objects/unicodeobject.c: https://github.com/Nuitka/Nuitka/blob/82d2da2f639483ae9b74ad7a765a66e45ab367...
... _Py_DEC_REFTOTAL; _Py_ForgetReference(unicode); ...
It's a micro-optimization to resize a Python immutable string in-place. It should be avoided outside CPython code base: use public PyUnicode_Resize() C API function.
pyuv: https://github.com/saghul/pyuv/blob/39342fc2fd688f2fb2120d3092dd9cf52f537de2...
Oh wow, that's ugly code which relies on so many implementation details! It is unlikely to build on the master branch of Python:
/* Taken from http://bugs.python.org/issue8212
- A function to do the necessary adjustments if we find that code
- run during a tp_dealloc or tp_free has resurrected an
- object. It adjusts the total reference count and adds a new
- reference to the type. */ static void resurrect_object(PyObject *self) { /* The object lives again. We must now undo the _Py_ForgetReference
- done in _Py_Dealloc in object.c. */ Py_ssize_t refcnt = Py_REFCNT(self); ASSERT(Py_REFCNT(self) != 0); _Py_NewReference(self); Py_REFCNT(self) = refcnt; /* If Py_REF_DEBUG, _Py_NewReference bumped _Py_RefTotal, so
- we need to undo that. */ #ifdef _Py_DEC_REFTOTAL _Py_DEC_REFTOTAL; #endif /* If Py_TRACE_REFS, _Py_NewReference re-added self to the object
- chain, so no more to do there.
- If COUNT_ALLOCS, the original decref bumped tp_frees, and
- _Py_NewReference bumped tp_allocs: both of those need to be
- undone. */ #ifdef COUNT_ALLOCS --Py_TYPE(self)->tp_frees; --Py_TYPE(self)->tp_allocs; #endif
/* When called from a heap type's dealloc (subtype_dealloc avove), the type will be
- decref'ed on return. This counteracts that. There is no way to otherwise
- let subtype_dealloc know that calling a parent class' tp_dealloc slot caused
- the instance to be resurrected. */ if (PyType_HasFeature(Py_TYPE(self), Py_TPFLAGS_HEAPTYPE)) Py_INCREF(Py_TYPE(self)); return; }
tp_finalize of PEP 442 should avoid the need for such a function, but I didn't check.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 16.06.2020 03:15, Victor Stinner wrote:
While true, the call is in a Stackless util file: https://github.com/swolchok/dspython/blob/8117b6a4a3f3aee7a7586d99aeadbee9ec...
https://github.com/henjo/numpy/blob/cd73a18cdbdf679f2f74b7b21d2951bf124d80fb...
Well, cpyext only implements functions to be compatible with CPython.
Yeah, it looks a lot like a copy of Python's structseq.c.
Not sure why the above code is needed for libuv. It's a library implementing a fast async stack.
Do we even support resurrecting objects in tp_dealloc or tp_free ?
It can be done, of course, but seems like a rather poor design for implementing an object which cannot be deallocated. There are certainly better way of doing this (e.g. keep references to the object alive in a separate dictionary or list).
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 16 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 16.06.2020 10:35, Matti Picus wrote:
Seems so, indeed. This is the master file:
https://github.com/numpy/numpy/blob/master/numpy/core/src/common/ucsnarrow.c
which looks a lot different.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 16 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 16:36, Matti Picus <matti.picus@gmail.com> a écrit :
I concur with Matti here.
Moreover, as I explained in my previous email, it's easy to rewrite a constructor to avoid relying on (removed) private functions of the Python C API.
There is no need to add any new feature here. Just fix your code ;-)
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 15.06.2020 17:19, Victor Stinner wrote:
Hmm, then why do we need PyObject_Del() as a public API ?
Calling it without first calling _Py_ForgetReference() will eventually result in segfaults in Py_TRACE_REFS builds .
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 15.06.2020 17:51, M.-A. Lemburg wrote:
Perhaps the better approach would be to fold the whole trace logic into the PyObject_*() allocation APIs and not expose it at all.
This would then also remove the need to even bother with the cases we discussed here, since PyObject_Del() would take care of undoing the tracing linkage.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 17:51, M.-A. Lemburg <mal@egenix.com> a écrit :
Hmm, then why do we need PyObject_Del() as a public API ?
PyObject_Del() should be called in a deallocator. It is safe since _Py_Dealloc() calls _Py_ForgetReference().
It's even said in its documentation: "This is normally called from the tp_dealloc handler specified in the object’s type." https://docs.python.org/dev/c-api/allocation.html#c.PyObject_Del
For example, xxmodule.c template module has:
static void Xxo_dealloc(XxoObject *self) { Py_XDECREF(self->x_attr); PyObject_Del(self); }
Calling PyObject_Del() is only unsafe outside a deallocator.
--
For the base "object" type, _Py_Dealloc() calls _Py_ForgetReference() and tp_free calls PyObject_Del().
Py_DECREF(op) calls _Py_Dealloc(op) when the reference counter reaches zero:
void _Py_Dealloc(PyObject *op) { destructor dealloc = Py_TYPE(op)->tp_dealloc; #ifdef Py_TRACE_REFS _Py_ForgetReference(op); #endif (*dealloc)(op); }
The default deallocator simply calls tp_free:
static void object_dealloc(PyObject *self) { Py_TYPE(self)->tp_free(self); }
And PyBaseObject.tp_free is set to PyObject_Del.
--
Note: _hashlib_hmac_new_impl() calls PyObject_Del(). IMO it's a bug.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 15.06.2020 18:17, Victor Stinner wrote:
Ok, I forgot. This makes sense for implementing e.g. free lists which keep the memory allocated. So is it true, that when an object's tp_dealloc is called, the tracing links will already have been removed ?
Calling PyObject_Del() is only unsafe outside a deallocator.
But why make/keep it unsafe outside of a deallocator ?
It would be easy to make _Py_ForgetReference() idempotent by testing for NULLness of op->_ob_next and op->_ob_prev and then always calling it as part of PyObject_Del(self) -- just in case.
This only affects tracing builds, so doesn't have any impact on prod builds.
Well, if you look at the dealloc code, this seems intentional. The deallocator assumes that self->ctx is set and allocated.
The code is still buggy in that it doesn't call _Py_ForgetReference(self) (which could be remedied by the above).
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 19:10, M.-A. Lemburg <mal@egenix.com> a écrit :
So is it true, that when an object's tp_dealloc is called, the tracing links will already have been removed ?
Yes, look at _Py_Dealloc().
Calling PyObject_Del() is only unsafe outside a deallocator.
But why make/keep it unsafe outside of a deallocator ?
I suggest only use Py_DECREF() in constructors.
IMO PyObject_Del() or _Py_ForgetReference() must not be called directly.
_Py_ForgetReference() must only be called if Py_TRACE_REFS macro is defined. I'm trying to move C extensions towards a stable ABI: an ABI which is not aware of the Py_TRACE_REFS macro.
The issue with calling PyObject_Del() directly is that _Py_ForgetReference() is not called which breaks the Python linked list of objects of the Py_TRACE_REFS special build.
_Py_ForgetReference() is only defined by Python.h when Py_TRACE_REFS macro is defined. IMHO it would be even better to make the function fully internal.
Exposing _Py_ForgetReference() is causing multiple issues with the stable ABI. I don't see the point of calling it. What is your issue with calling Py_DECREF() in a constructor?
Do you want to be able to call _Py_ForgetReference() for a micro-optimization or something else?
Note: _hashlib_hmac_new_impl() calls PyObject_Del(). IMO it's a bug.
The code is still buggy in that it doesn't call _Py_ForgetReference(self) (...)
Yes, that's my point :-) Sorry if I wasn't explicit enough.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 15.06.2020 20:43, Victor Stinner wrote:
So that's the only official way of calling tp_dealloc ? (just curious)
Well, as you pointed out, PyObject_Del() has to be called in tp_dealloc.
No, perhaps I'm not making myself clear:
If we can have PyObject_Del() always call _Py_ForgetReference() (and making sure that this is safe when the two obj links are NULL), the _Py_ForgetReference() could be made completely internal.
Code which needs to bypass tp_dealloc in tp_init (or other places) would then call PyObject_Del() directly and use this as only public API.
... this bug would then also go away.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
I created https://bugs.python.org/issue40989 to remove _Py_NewReference() and _Py_ForgetReference() from the public C API. I already started the work in Python 3.9 with:
commit f58bd7c1693fe041f7296a5778d0a11287895648 Author: Victor Stinner <vstinner@python.org> Date: Wed Feb 5 13:12:19 2020 +0100
bpo-39542: Make PyObject_INIT() opaque in limited C API (GH-18363)
In the limited C API, PyObject_INIT() and PyObject_INIT_VAR() are now
defined as aliases to PyObject_Init() and PyObject_InitVar() to make
their implementation opaque. It avoids to leak implementation details
in the limited C API.Le lun. 15 juin 2020 à 22:43, M.-A. Lemburg <mal@egenix.com> a écrit :
PyObject_Del() is just an alias to PyObject_Free() memory allocator function. It just frees memory, nothing more:
#define PyObject_Del PyObject_Free
Changing PyObject_Del() behavior is likely to break tons of C extension modules and cause issues with the stable ABI.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 16.06.2020 01:02, Victor Stinner wrote:
You could just as well add the call to _Py_ForgetReference() to PyObject_Free(). The point here is that _Py_ForgetReference() cleans up an internal linked list used for tracing, so not affect any external code.
I don't know where this resistance to make *any* changes in this respect is coming from, when at the same time you are making lots of other breaking changes to other parts of the C API.
This change would make the C API better, safer and remove the need to have to call _Py_ForgetReference() outside the interpreter.
Could you please explain ?
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 16 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le mar. 16 juin 2020 à 10:14, M.-A. Lemburg <mal@egenix.com> a écrit :
In short, it will for sure break (corrupt memory) many C extensions which worked without any issue previously.
As I explained previously, PyObject_Malloc/PyObject_Free are memory allocators which are not aware of the PyObject structure, reference counting, PyGC_Head structure and the garbage collector, etc. It's completely agnostic of Python objects. The functions' name is misleading :-)
For example, PYTHONMALLOC=malloc simply binds these functions to libc malloc/free.
By the way, Modules/gcmodule.c has a similar design: it doesn't use PyObject structure at all, it is not aware of reference counting, etc. Only PyGC_Head structure is used in this file (in the GC code).
PyObject_Malloc/PyObject_Free *are* used to allocate memory for things which are *not* Python objects. A quick search just in the stdlib found one example in Modules/parsermodule.c:
char *strn = (char *)PyObject_MALLOC(len + 1);
...
(void) memcpy(strn, temp_str, len + 1);
...
PyObject_FREE(strn);That's just a raw array of bytes. It's not hard to find other examples, like PyObject_FREE(keys_free_list[--numfreekeys]); in Objects/dictobject.c: it's an array of PyDictKeysObject which are *not* Python objects (yeah again, the structure name is misleading ;-)).
If PyObject_Free() is modified to call _Py_ForgetReference(), it will for sure corrupt memory.
Note: Many aliases to PyObject_Malloc/PyObject_Free functions are provided in the C API, mostly for backward compatibility:
#define PyObject_MALLOC PyObject_Malloc #define PyObject_REALLOC PyObject_Realloc #define PyObject_FREE PyObject_Free #define PyObject_Del PyObject_Free #define PyObject_DEL PyObject_Free
Victor
Night gathers, and now my watch begins. It shall not end until my death.
Hi,
Pseudo code of the issue:
self = <allocate a new object> self->field = <complex code which can fail>; if (<creating field failed> { _Py_ForgetReference((PyObject *)self); PyObject_Del(self); <return an error>; } // ... fill all self members with code which cannot fail ... return self;
In the past, I fixed multiple bugs in such a code pattern: not calling the object deallocator caused multiple bugs, especially depending on whether Py_TRACE_REFS macro was defined or not.
I suggest you use more regular code. Rewrite your code like this:
self = <allocate a new object> self->field = NULL; // or any other marker for "not initialized" or "empty" // ... fill all self members with code which cannot fail ... // at this point, it's safe to call Py_DECREF(self)
self->field = <complex code which can fail>; if (<creating field failed> { Py_DECREF(self); // <=== regular code which calls the deallocator <return an error>; }
return self;
You only have to make sure that your deallocator function handles the case self->field==NULL. Usually, it's trivial to handle such case (do nothing).
It's too easy to misuse _Py_ForgetReference() and PyObject_Del() which create bug in internal Python structures, leak memory, etc.
Victor
Le lun. 15 juin 2020 à 11:36, Petr Viktorin <encukou@gmail.com> a écrit :
-- Night gathers, and now my watch begins. It shall not end until my death.
This has been a rigorous discussion. Thank you for Mr. Lemburg for making all the arguments I was going to make.
My relatively uneducated 2 cents: I think it would be ideal for module writers not to have to deal with trace refs at all. I'm in favor of simplifying the API, in exchange for some loss of flexibility. A failure that only exhibits itself with a special build or configuration of cpython is one that folks are going to miss. Personally, I probably spent 20 hours debugging a missing _Py_ForgetReference. I had to get caught up on how trace refs and cpython memory allocation worked. I don't wish that on anybody.
Thank you for your time.
Nic Watson
On 15.06.2020 11:02, Petr Viktorin wrote:
This is typically used in error handlers of constructors where you want to avoid having the deallocator run as result of the DECREF, e.g. because the object has not been full initialized yet.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 2020-06-15 11:14, M.-A. Lemburg wrote:
For that I'd recommend marking the object invalid (e.g. by setting some vital pointer in it to NULL), and teaching the deallocator to skip its logic.
If that's not enough we could document and support a "Py_ForgetReference" as public API in CPython. But having only internal API for a use case is not good.
On 15.06.2020 11:35, Petr Viktorin wrote:
Sure, there are often workarounds which can be used, but we do need some way to tell the interpreter "the initialization had a problem, please remove the object from your internal management resources without calling the deallocator".
Perhaps a new public API could be created for this purpose, like you say.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 6/15/20 12:58 PM, M.-A. Lemburg wrote:
I respectfully disagree. There is purposefully a separation between __new__ and __init__. A failure in __new__ should not call __del__, and a failure in __init__ should be gracefully handled by the __del__. It is not the interpreter's duty to provide an interface that breaks this construct. A c-extension class should not get a pass to do things differently from a python-level class. It is important that CPython not provide interfaces that will make other implementation's lives harder (PyPy, GraalPython, MicroPython). Anyone using C-API functions that start with a "_" should be very thoughtful since those are not meant to be public, usually for good reason.
Matti
On 15.06.2020 12:53, Matti Picus wrote:
In many cases, the above will work and you can signal your deallocator which parts of the object have already been initialized, but this is not always possible.
The fact that extension writers turn to APIs marked as internal is usually triggered by lack of public APIs for the purpose. This should be a signal to consider making an API public or creating a better interface to provide similar logic.
FWIW: I don't see problems for other implementations to provide the above API, but perhaps I'm missing something.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 2020-06-15 13:47, M.-A. Lemburg wrote:
Do you have a concrete example where tis is not possible?
"_Py_ForgetReference and PyObject_Del will do normal deallocation of the object *except* calling the user-provided deallocator" strikes me as a very fragile assumption. Where is the assumption is coming from? (That's what I meant to ask when I asked "What does the _Py_ForgetReference function do?" – apologies for wording it in a misleading/loaded way.)
Unfortunately, this doesn't really work as a signal, because nothing is actually signalled back to CPython devs – until the private API in question stops working.
FWIW: I don't see problems for other implementations to provide the above API, but perhaps I'm missing something.
I see no documentation on what the function should do. I assume providing a function with unspecified semantics is always a problem.
On 15.06.2020 14:36, Petr Viktorin wrote:
An error handler in the constructor will know which parts have been initialized and which not, e.g. say an object needs to configure a number of objects in a 3rd party library and one of these steps fails. The constructor can then undo the steps it has taken up to the point where the error occurred.
The deallocator will assume that the complete stack has been completely setup and can be taken down again. Unless there's a way for the deallocator to detect that it is being called on a non-initialized object, it can cause additional errors in the 3rd party library and leave external resources allocated as a result.
The semantics are already defined by the implementation: it will do whatever extra logic the interpreter applies (unlinking the object in debug builds at the moment) when finalizing the object.
The docs would not have to detail what exactly happens, but only state that by calling this API, the object will effectively be taken out of the scope of the interpeter.
True, there's a feedback loop missing for such situations. Perhaps we should create one (this C API ML already serves in part as such a feedback loop).
See above. Being unspecific is intentional, as long as the pre- and post-conditions are made clear in the docs.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
The feedback loop is already there: the issue tracker and various mailing lists. This is an undocumented, private API which should be a clear indication that 3th-party developers are not supposed to use it.
The unspecified part is that there are no documented pre- and post-conditions ;-).
For this particular API I’m far from convinced that exposing it (and another private API used by the OP) is necessary.
Ronald
— Twitter / micro.blog: @ronaldoussoren Blog: https://blog.ronaldoussoren.net/
On 6/15/20 2:47 PM, M.-A. Lemburg wrote:
FWIW: I don't see problems for other implementations to provide the above API, but perhaps I'm missing something.
The problem is that there is no pure python equivalent, and the other implementations have to spend quite a lot of effort mimicking the obscure corners of the C-API. In my opinion, any public C-API function should mirror a python equivalent. Python has no overt language specification, but I would hope one of the underlying principles would be to expose the same paradigms in python and C.
This can be handled in __del__ as well by setting flags to 0 and pointers to NULL values when calling __new__ (or simply calling memset(..0) ), then filling them out in __init__ one at a time. Any 0-ed values indicate that the __del__ function does not have to undo the initialization/allocation of that value.
Matti
On 15.06.2020 16:35, Matti Picus wrote:
That's an odd request. We're talking about the C API here, not the Python API. Of course, the C API will have different requirements than the Python one, simply because it's C.
The call is needed to let the CPython implementation know that it should unlink the object at the moment, but this could be changed in the future. Other implementations could use it for different things.
It's really just a way to tell Python: please remove this object from your management scope.
There is no Python equivalent, since the low level memory management happens entirely in C.
Yes, I know, but think of an object which doesn't otherwise require such a flag. You'd increase the memory footprint just to be able to do proper error handling.
Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 2020-06-15 17:05, M.-A. Lemburg wrote:
The request is to not extend the C-API not only with a new function, but also with a notion of an object that's allocated using PyObject_New but not in the interpreter's "management scope".
Do we have any evidence such objects exist in the real world? In other words, is this enough of a problem that it needs new public API?
On 15.06.2020 17:15, Petr Viktorin wrote:
Perhaps I wasn't clear: the object is already allocated and managed by Python. The error happens in the initializer and the API is meant to signal: please remove this object from the management scope, since I have already dealt with the cleanup.
Well, if you search on Github, you find some 18k hits. Many are from forks of the Python source code, but there are also a number of hits in stackless, nuitka, pyuv, PyPy's cpyext and numpy from a quick inspection.
Most uses call the API to prevent segfaults in debug builds (which use can use object tracing -- not sure whether they always do).
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 17:39, M.-A. Lemburg <mal@egenix.com> a écrit :
Stackless is a fork of Python. Occurrences in Python code base should be ignored.
nuitka, pyuv, PyPy's cpyext and numpy from a quick inspection.
I cannot find _Py_ForgetReference() or _Py_NewReference() in numpy code base (I checked the master branch).
Well, cpyext only implements functions to be compatible with CPython.
nuitka copied/pasted resize_compact() of CPython Objects/unicodeobject.c: https://github.com/Nuitka/Nuitka/blob/82d2da2f639483ae9b74ad7a765a66e45ab367...
... _Py_DEC_REFTOTAL; _Py_ForgetReference(unicode); ...
It's a micro-optimization to resize a Python immutable string in-place. It should be avoided outside CPython code base: use public PyUnicode_Resize() C API function.
pyuv: https://github.com/saghul/pyuv/blob/39342fc2fd688f2fb2120d3092dd9cf52f537de2...
Oh wow, that's ugly code which relies on so many implementation details! It is unlikely to build on the master branch of Python:
/* Taken from http://bugs.python.org/issue8212
- A function to do the necessary adjustments if we find that code
- run during a tp_dealloc or tp_free has resurrected an
- object. It adjusts the total reference count and adds a new
- reference to the type. */ static void resurrect_object(PyObject *self) { /* The object lives again. We must now undo the _Py_ForgetReference
- done in _Py_Dealloc in object.c. */ Py_ssize_t refcnt = Py_REFCNT(self); ASSERT(Py_REFCNT(self) != 0); _Py_NewReference(self); Py_REFCNT(self) = refcnt; /* If Py_REF_DEBUG, _Py_NewReference bumped _Py_RefTotal, so
- we need to undo that. */ #ifdef _Py_DEC_REFTOTAL _Py_DEC_REFTOTAL; #endif /* If Py_TRACE_REFS, _Py_NewReference re-added self to the object
- chain, so no more to do there.
- If COUNT_ALLOCS, the original decref bumped tp_frees, and
- _Py_NewReference bumped tp_allocs: both of those need to be
- undone. */ #ifdef COUNT_ALLOCS --Py_TYPE(self)->tp_frees; --Py_TYPE(self)->tp_allocs; #endif
/* When called from a heap type's dealloc (subtype_dealloc avove), the type will be
- decref'ed on return. This counteracts that. There is no way to otherwise
- let subtype_dealloc know that calling a parent class' tp_dealloc slot caused
- the instance to be resurrected. */ if (PyType_HasFeature(Py_TYPE(self), Py_TPFLAGS_HEAPTYPE)) Py_INCREF(Py_TYPE(self)); return; }
tp_finalize of PEP 442 should avoid the need for such a function, but I didn't check.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 16.06.2020 03:15, Victor Stinner wrote:
While true, the call is in a Stackless util file: https://github.com/swolchok/dspython/blob/8117b6a4a3f3aee7a7586d99aeadbee9ec...
https://github.com/henjo/numpy/blob/cd73a18cdbdf679f2f74b7b21d2951bf124d80fb...
Well, cpyext only implements functions to be compatible with CPython.
Yeah, it looks a lot like a copy of Python's structseq.c.
Not sure why the above code is needed for libuv. It's a library implementing a fast async stack.
Do we even support resurrecting objects in tp_dealloc or tp_free ?
It can be done, of course, but seems like a rather poor design for implementing an object which cannot be deallocated. There are certainly better way of doing this (e.g. keep references to the object alive in a separate dictionary or list).
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 16 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 16.06.2020 10:35, Matti Picus wrote:
Seems so, indeed. This is the master file:
https://github.com/numpy/numpy/blob/master/numpy/core/src/common/ucsnarrow.c
which looks a lot different.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 16 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 16:36, Matti Picus <matti.picus@gmail.com> a écrit :
I concur with Matti here.
Moreover, as I explained in my previous email, it's easy to rewrite a constructor to avoid relying on (removed) private functions of the Python C API.
There is no need to add any new feature here. Just fix your code ;-)
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 15.06.2020 17:19, Victor Stinner wrote:
Hmm, then why do we need PyObject_Del() as a public API ?
Calling it without first calling _Py_ForgetReference() will eventually result in segfaults in Py_TRACE_REFS builds .
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
On 15.06.2020 17:51, M.-A. Lemburg wrote:
Perhaps the better approach would be to fold the whole trace logic into the PyObject_*() allocation APIs and not expose it at all.
This would then also remove the need to even bother with the cases we discussed here, since PyObject_Del() would take care of undoing the tracing linkage.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 17:51, M.-A. Lemburg <mal@egenix.com> a écrit :
Hmm, then why do we need PyObject_Del() as a public API ?
PyObject_Del() should be called in a deallocator. It is safe since _Py_Dealloc() calls _Py_ForgetReference().
It's even said in its documentation: "This is normally called from the tp_dealloc handler specified in the object’s type." https://docs.python.org/dev/c-api/allocation.html#c.PyObject_Del
For example, xxmodule.c template module has:
static void Xxo_dealloc(XxoObject *self) { Py_XDECREF(self->x_attr); PyObject_Del(self); }
Calling PyObject_Del() is only unsafe outside a deallocator.
--
For the base "object" type, _Py_Dealloc() calls _Py_ForgetReference() and tp_free calls PyObject_Del().
Py_DECREF(op) calls _Py_Dealloc(op) when the reference counter reaches zero:
void _Py_Dealloc(PyObject *op) { destructor dealloc = Py_TYPE(op)->tp_dealloc; #ifdef Py_TRACE_REFS _Py_ForgetReference(op); #endif (*dealloc)(op); }
The default deallocator simply calls tp_free:
static void object_dealloc(PyObject *self) { Py_TYPE(self)->tp_free(self); }
And PyBaseObject.tp_free is set to PyObject_Del.
--
Note: _hashlib_hmac_new_impl() calls PyObject_Del(). IMO it's a bug.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 15.06.2020 18:17, Victor Stinner wrote:
Ok, I forgot. This makes sense for implementing e.g. free lists which keep the memory allocated. So is it true, that when an object's tp_dealloc is called, the tracing links will already have been removed ?
Calling PyObject_Del() is only unsafe outside a deallocator.
But why make/keep it unsafe outside of a deallocator ?
It would be easy to make _Py_ForgetReference() idempotent by testing for NULLness of op->_ob_next and op->_ob_prev and then always calling it as part of PyObject_Del(self) -- just in case.
This only affects tracing builds, so doesn't have any impact on prod builds.
Well, if you look at the dealloc code, this seems intentional. The deallocator assumes that self->ctx is set and allocated.
The code is still buggy in that it doesn't call _Py_ForgetReference(self) (which could be remedied by the above).
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le lun. 15 juin 2020 à 19:10, M.-A. Lemburg <mal@egenix.com> a écrit :
So is it true, that when an object's tp_dealloc is called, the tracing links will already have been removed ?
Yes, look at _Py_Dealloc().
Calling PyObject_Del() is only unsafe outside a deallocator.
But why make/keep it unsafe outside of a deallocator ?
I suggest only use Py_DECREF() in constructors.
IMO PyObject_Del() or _Py_ForgetReference() must not be called directly.
_Py_ForgetReference() must only be called if Py_TRACE_REFS macro is defined. I'm trying to move C extensions towards a stable ABI: an ABI which is not aware of the Py_TRACE_REFS macro.
The issue with calling PyObject_Del() directly is that _Py_ForgetReference() is not called which breaks the Python linked list of objects of the Py_TRACE_REFS special build.
_Py_ForgetReference() is only defined by Python.h when Py_TRACE_REFS macro is defined. IMHO it would be even better to make the function fully internal.
Exposing _Py_ForgetReference() is causing multiple issues with the stable ABI. I don't see the point of calling it. What is your issue with calling Py_DECREF() in a constructor?
Do you want to be able to call _Py_ForgetReference() for a micro-optimization or something else?
Note: _hashlib_hmac_new_impl() calls PyObject_Del(). IMO it's a bug.
The code is still buggy in that it doesn't call _Py_ForgetReference(self) (...)
Yes, that's my point :-) Sorry if I wasn't explicit enough.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 15.06.2020 20:43, Victor Stinner wrote:
So that's the only official way of calling tp_dealloc ? (just curious)
Well, as you pointed out, PyObject_Del() has to be called in tp_dealloc.
No, perhaps I'm not making myself clear:
If we can have PyObject_Del() always call _Py_ForgetReference() (and making sure that this is safe when the two obj links are NULL), the _Py_ForgetReference() could be made completely internal.
Code which needs to bypass tp_dealloc in tp_init (or other places) would then call PyObject_Del() directly and use this as only public API.
... this bug would then also go away.
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 15 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
I created https://bugs.python.org/issue40989 to remove _Py_NewReference() and _Py_ForgetReference() from the public C API. I already started the work in Python 3.9 with:
commit f58bd7c1693fe041f7296a5778d0a11287895648 Author: Victor Stinner <vstinner@python.org> Date: Wed Feb 5 13:12:19 2020 +0100
bpo-39542: Make PyObject_INIT() opaque in limited C API (GH-18363)
In the limited C API, PyObject_INIT() and PyObject_INIT_VAR() are now
defined as aliases to PyObject_Init() and PyObject_InitVar() to make
their implementation opaque. It avoids to leak implementation details
in the limited C API.Le lun. 15 juin 2020 à 22:43, M.-A. Lemburg <mal@egenix.com> a écrit :
PyObject_Del() is just an alias to PyObject_Free() memory allocator function. It just frees memory, nothing more:
#define PyObject_Del PyObject_Free
Changing PyObject_Del() behavior is likely to break tons of C extension modules and cause issues with the stable ABI.
Victor
Night gathers, and now my watch begins. It shall not end until my death.
On 16.06.2020 01:02, Victor Stinner wrote:
You could just as well add the call to _Py_ForgetReference() to PyObject_Free(). The point here is that _Py_ForgetReference() cleans up an internal linked list used for tracing, so not affect any external code.
I don't know where this resistance to make *any* changes in this respect is coming from, when at the same time you are making lots of other breaking changes to other parts of the C API.
This change would make the C API better, safer and remove the need to have to call _Py_ForgetReference() outside the interpreter.
Could you please explain ?
-- Marc-Andre Lemburg eGenix.com
Professional Python Services directly from the Experts (#1, Jun 16 2020)
Python Projects, Coaching and Support ... https://www.egenix.com/ Python Product Development ... https://consulting.egenix.com/
::: We implement business ideas - efficiently in both time and costs :::
eGenix.com Software, Skills and Services GmbH Pastor-Loeh-Str.48 D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg Registered at Amtsgericht Duesseldorf: HRB 46611 https://www.egenix.com/company/contact/ https://www.malemburg.com/
Le mar. 16 juin 2020 à 10:14, M.-A. Lemburg <mal@egenix.com> a écrit :
In short, it will for sure break (corrupt memory) many C extensions which worked without any issue previously.
As I explained previously, PyObject_Malloc/PyObject_Free are memory allocators which are not aware of the PyObject structure, reference counting, PyGC_Head structure and the garbage collector, etc. It's completely agnostic of Python objects. The functions' name is misleading :-)
For example, PYTHONMALLOC=malloc simply binds these functions to libc malloc/free.
By the way, Modules/gcmodule.c has a similar design: it doesn't use PyObject structure at all, it is not aware of reference counting, etc. Only PyGC_Head structure is used in this file (in the GC code).
PyObject_Malloc/PyObject_Free *are* used to allocate memory for things which are *not* Python objects. A quick search just in the stdlib found one example in Modules/parsermodule.c:
char *strn = (char *)PyObject_MALLOC(len + 1);
...
(void) memcpy(strn, temp_str, len + 1);
...
PyObject_FREE(strn);That's just a raw array of bytes. It's not hard to find other examples, like PyObject_FREE(keys_free_list[--numfreekeys]); in Objects/dictobject.c: it's an array of PyDictKeysObject which are *not* Python objects (yeah again, the structure name is misleading ;-)).
If PyObject_Free() is modified to call _Py_ForgetReference(), it will for sure corrupt memory.
Note: Many aliases to PyObject_Malloc/PyObject_Free functions are provided in the C API, mostly for backward compatibility:
#define PyObject_MALLOC PyObject_Malloc #define PyObject_REALLOC PyObject_Realloc #define PyObject_FREE PyObject_Free #define PyObject_Del PyObject_Free #define PyObject_DEL PyObject_Free
Victor
Night gathers, and now my watch begins. It shall not end until my death.
Hi,
Pseudo code of the issue:
self = <allocate a new object> self->field = <complex code which can fail>; if (<creating field failed> { _Py_ForgetReference((PyObject *)self); PyObject_Del(self); <return an error>; } // ... fill all self members with code which cannot fail ... return self;
In the past, I fixed multiple bugs in such a code pattern: not calling the object deallocator caused multiple bugs, especially depending on whether Py_TRACE_REFS macro was defined or not.
I suggest you use more regular code. Rewrite your code like this:
self = <allocate a new object> self->field = NULL; // or any other marker for "not initialized" or "empty" // ... fill all self members with code which cannot fail ... // at this point, it's safe to call Py_DECREF(self)
self->field = <complex code which can fail>; if (<creating field failed> { Py_DECREF(self); // <=== regular code which calls the deallocator <return an error>; }
return self;
You only have to make sure that your deallocator function handles the case self->field==NULL. Usually, it's trivial to handle such case (do nothing).
It's too easy to misuse _Py_ForgetReference() and PyObject_Del() which create bug in internal Python structures, leak memory, etc.
Victor
Le lun. 15 juin 2020 à 11:36, Petr Viktorin <encukou@gmail.com> a écrit :
-- Night gathers, and now my watch begins. It shall not end until my death.
This has been a rigorous discussion. Thank you for Mr. Lemburg for making all the arguments I was going to make.
My relatively uneducated 2 cents: I think it would be ideal for module writers not to have to deal with trace refs at all. I'm in favor of simplifying the API, in exchange for some loss of flexibility. A failure that only exhibits itself with a special build or configuration of cpython is one that folks are going to miss. Personally, I probably spent 20 hours debugging a missing _Py_ForgetReference. I had to get caught up on how trace refs and cpython memory allocation worked. I don't wish that on anybody.
Thank you for your time.
Nic Watson
participants (6)
-
cpython＠nicwatson.org -
M.-A. Lemburg -
Matti Picus -
Petr Viktorin -
Ronald Oussoren -
Victor Stinner