[C API] Add _Py_Borrow() private function: call Py_XDECREF() and return the object

[vstinner]

BPO	42522
Nosy	@ronaldoussoren, @vstinner, @lazka, @shihai1991
PRs	bpo-42522: Add Py_Borrow() and Py_XBorrow() functions #23591

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-12-01.23:47:46.226>
created_at = <Date 2020-12-01.11:21:37.282>
labels = ['expert-C-API', '3.10']
title = '[C API] Add _Py_Borrow() private function: call Py_XDECREF() and return the object'
updated_at = <Date 2020-12-01.23:47:46.224>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2020-12-01.23:47:46.224>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-12-01.23:47:46.226>
closer = 'vstinner'
components = ['C API']
creation = <Date 2020-12-01.11:21:37.282>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 42522
keywords = ['patch']
message_count = 11.0
messages = ['382237', '382243', '382245', '382247', '382249', '382250', '382252', '382255', '382258', '382270', '382284']
nosy_count = 4.0
nosy_names = ['ronaldoussoren', 'vstinner', 'lazka', 'shihai1991']
pr_nums = ['23591']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue42522'
versions = ['Python 3.10']

[vstinner]

I'm working on a script to migrate old C extension modules to the latest flavor of the Python C API:
https://github.com/pythoncapi/upgrade_pythoncapi

I would like to replace frame->f_code with PyFrame_GetCode(frame). The problem is that frame->f_code is a borrowed reference, whereas PyFrame_GetCode() returns a strong reference. Having a Py_Borrow() function would help to automate migration to PyFrame_GetCode():

static inline PyObject* Py_Borrow(PyObject *obj)
{
    Py_XDECREF(obj);
    return obj;
}

So frame->f_code can be replaced with Py_Borrow(PyFrame_GetCode(frame)).

Py_Borrow() is similar to Py_XDECREF() but can be used as an expression:

    PyObject *code = Py_Borrow(PyFrame_GetCode(frame));

Py_Borrow() is the opposite of Py_XNewRef(). For example, Py_Borrow(Py_XNewRef(obj)) leaves the reference count unchanged (+1 and then -1).

My pratical problem is that it's not easy not add Py_XDECREF() call when converting C code to the new C API.

Example 1:

    PyObject *code = frame->f_code;

This one is easy and can be written as:

PyObject *code = PyFrame_GetCode(frame); Py_XDECREF(code);

Example 2:

func(frame->f_code);

This one is more tricky. For example, the following code using a macro is wrong:

    #define Py_BORROW(obj) (Py_XDECREF(obj), obj)
    func(Py_BORROW(PyFrame_GetCode(frame->f_code)));

since it calls PyFrame_GetCode() twice when proceed by the C preprocessor and so leaks a reference:

func(Py_XDECREF(PyFrame_GetCode(frame->f_code)), PyFrame_GetCode(frame->f_code));

Attached PR implements Py_Borrow() function as a static inline function.

[ronaldoussoren]

I'm -1 on adding this API as it is inherently unsafe and is bound to be used in locations where its contract is not honoured (especially outside of the core).

Note that the pre-condition on the argument is that you now only own a reference to the object, but that there's also someone else that owns a reference.

It is better to bite the bullet and go for correct reference count updates when converting to the new PyFrame_GetCode API.

[vstinner]

Ronald Oussoren: "I'm -1 on adding this API as it is inherently unsafe and is bound to be used in locations where its contract is not honoured (especially outside of the core)."

I'm trying to update old C extensions which already rely on borrowed references.

I expect that only my script will use this function. Developers should not call it directly, but use strong references. I explained it in the function documentation.

Obviously, using strong references is safer, but it's not possible to automate the conversion of borrowed references to strong references. Maybe it's possible on simple functions, but not in the general code (long functions with loops, function calls, goto, etc.).

Note that the pre-condition on the argument is that you now only own a reference to the object, but that there's also someone else that owns a reference.

Py_Borrow() is unsafe by design and should not be used :-)

Replacing frame->f_code with Py_Borrow(PyFrame_GetCode(frame)) doesn't make the code worse nor better in term of reference counting.

It is better to bite the bullet and go for correct reference count updates when converting to the new PyFrame_GetCode API.

My goal is to automate conversion to new C API. Using strong references is really challenging and I expect that it would be done manually.

--

Once a code base is updated to use Py_Borrow(), it might become possible to replace automatically some simple patterns to use strong references.

[vstinner]

My proposed function doesn't have to be public. I updated the issue and my PR to propose to add a *private* function to clarify that it should not be used unless you understand what you do.

[vstinner]

In the meanwhile, I modified my upgrade_pythoncapi.py tool to include pythoncapi_compat.h, and I added _Py_Borrow() and _Py_XBorrow() to pythoncapi_compat.h:

• https://github.com/pythoncapi/upgrade_pythoncapi
• https://github.com/pythoncapi/pythoncapi_compat

[ronaldoussoren]

A private function is less problematic, but still far from ideal. Any use of the function is IMHO a code smell that should be refactored.

I'm primarily against this function in the public API because it will be used where it isn't appropriate because it looks convenient (such as somefuntion(Py_Borrow(PyUnicode_FromUTF8(...)))).

[ronaldoussoren]

BTW. Have you considered using coccinelle <https://coccinelle.gitlabpages.inria.fr/website/\> instead of a python script?

I have no experience with the tool, but it is meant to be used for code transformation ("semantic patches"). I only know of the tool because of articles on LWN.net, I haven't tried using it yet.

[vstinner]

BTW. Have you considered using coccinelle <https://coccinelle.gitlabpages.inria.fr/website/\> instead of a python script?

I wrote Python code just because I like Python. There are likely other more advanced tools like coccinelle, but Python users are more used to install Python scripts and Python is portable.

[ronaldoussoren]

I totally understand, I would have done the same. Coccinelle looks like an interesting tool for refactorings like this, but I prefer to write Python scripts (and often just refactor manually).