classification
Title: [C API] Add _Py_Borrow() private function: call Py_XDECREF() and return the object
Type: Stage: resolved
Components: C API Versions: Python 3.10
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: lazka, ronaldoussoren, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-12-01 11:21 by vstinner, last changed 2020-12-01 23:47 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 23591 closed vstinner, 2020-12-01 11:30
Messages (11)
msg382237 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-01 11:21
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.
msg382243 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-12-01 12:01
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.
msg382245 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-01 12:30
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.
msg382247 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-01 12:38
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.
msg382249 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-01 13:20
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
msg382250 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-12-01 13:50
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(...)))``).
msg382252 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-12-01 14:18
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.
msg382255 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-01 14:34
> 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.
msg382258 - (view) Author: Ronald Oussoren (ronaldoussoren) * (Python committer) Date: 2020-12-01 14:43
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).
msg382270 - (view) Author: Christoph Reiter (lazka) * Date: 2020-12-01 16:47
I find it confusing that the function is called Py_Borrow() but steals the reference.
msg382284 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-12-01 23:47
I had more time to think about this issue.

First, I decided to always include "pythoncapi_compat.h" and so there is no strict requirement to add new C API functions to Python.

Second, the _Py_Borrow() function looked nice, but there is a practical issue: its return type is always PyObject* which makes it not convenient to "convert" a function returning a different type.

For example, I replaced "frame->f_code" with "((PyCodeObject *)_Py_Borrow(PyFrame_GetCode(frame)))", but this expression requires 3 levels of parenthesis, it's long and it's hard to read :-( The outter parenthesis is needed if the modified expresion is used to get a code member, like: "frame->f_code->co_name".

I chose another approach: add a "Borrow" variant of the functions that I need.

For example, add _PyFrameGetCodeBorrow() variant of PyFrameGetCode(). This function is only added to pythoncapi_compat.h, and so Python doesn't have to be modified.

I reject my issue :-)
History
Date User Action Args
2020-12-01 23:47:46vstinnersetstatus: open -> closed
resolution: rejected
messages: + msg382284

stage: patch review -> resolved
2020-12-01 16:47:52lazkasetnosy: + lazka
messages: + msg382270
2020-12-01 14:43:56ronaldoussorensetmessages: + msg382258
2020-12-01 14:34:13vstinnersetmessages: + msg382255
2020-12-01 14:18:58ronaldoussorensetmessages: + msg382252
2020-12-01 13:50:46ronaldoussorensetmessages: + msg382250
2020-12-01 13:20:18vstinnersetmessages: + msg382249
2020-12-01 12:38:12vstinnersetmessages: + msg382247
title: [C API] Add Py_Borrow() function: call Py_XDECREF() and return the object -> [C API] Add _Py_Borrow() private function: call Py_XDECREF() and return the object
2020-12-01 12:30:39vstinnersetmessages: + msg382245
2020-12-01 12:01:36ronaldoussorensetnosy: + ronaldoussoren
messages: + msg382243
2020-12-01 11:30:34vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request22462
2020-12-01 11:29:15shihai1991setnosy: + shihai1991
2020-12-01 11:21:37vstinnercreate