Skip to content

bpo-37151: remove special case for PyCFunction from PyObject_Call #14684

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
encukou merged 3 commits into from
Sep 11, 2019
Merged

bpo-37151: remove special case for PyCFunction from PyObject_Call #14684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0f6c09
bpo-37151: remove special case for PyCFunction from PyObject_Call
jdemeyer Jul 10, 2019
37152cf
bpo-37151: make PyCFunction_Call an alias of PyObject_Call
jdemeyer Jul 16, 2019
5b7f9fc
Deprecate PyCFunction_Call
jdemeyer Jul 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Include/methodobject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ PyAPI_FUNC(int) PyCFunction_GetFlags(PyObject *);
#define PyCFunction_GET_FLAGS(func) \
(((PyCFunctionObject *)func) -> m_ml -> ml_flags)
#endif
PyAPI_FUNC(PyObject *) PyCFunction_Call(PyObject *, PyObject *, PyObject *);
Py_DEPRECATED(3.9) PyAPI_FUNC(PyObject *) PyCFunction_Call(PyObject *, PyObject *, PyObject *);
Copy link
Member

@encukou encukou Sep 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vstinner, is this the right way to deprecate an undocumented part of the stable ABI?

(I'll merge regardless, but I wanted to bring this to your attention.)

Copy link
Member

@vstinner vstinner Sep 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stable ABI is supposed to remain unchanged :-) "Py_DEPRECATED(3.9) PyAPI_FUNC(PyObject *) PyCFunction_Call" is the right usage of Py_DEPRECATED().

I don't know why PyCFunction_Call "leaked" into the stable ABI. I'm in favor of removing it. Start with a deprecation is a good start ;)


struct PyMethodDef {
const char *ml_name; /* The name of the built-in function/method */
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/C API/2019-07-16-11-02-00.bpo-37151.YKfuNA.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
``PyCFunction_Call`` is now a deprecated alias of :c:func:`PyObject_Call`.
69 changes: 7 additions & 62 deletions Objects/call.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
#include "frameobject.h"


static PyObject *
cfunction_call_varargs(PyObject *func, PyObject *args, PyObject *kwargs);

static PyObject *const *
_PyStack_UnpackDict(PyObject *const *args, Py_ssize_t nargs, PyObject *kwargs,
PyObject **p_kwnames);
Expand Down Expand Up @@ -236,11 +233,6 @@ PyObject_Call(PyObject *callable, PyObject *args, PyObject *kwargs)
if (_PyVectorcall_Function(callable) != NULL) {
return PyVectorcall_Call(callable, args, kwargs);
}
else if (PyCFunction_Check(callable)) {
/* This must be a METH_VARARGS function, otherwise we would be
* in the previous case */
return cfunction_call_varargs(callable, args, kwargs);
}
else {
call = callable->ob_type->tp_call;
if (call == NULL) {
Expand All @@ -261,6 +253,13 @@ PyObject_Call(PyObject *callable, PyObject *args, PyObject *kwargs)
}


PyObject *
PyCFunction_Call(PyObject *callable, PyObject *args, PyObject *kwargs)
{
return PyObject_Call(callable, args, kwargs);
}


/* --- PyFunction call functions ---------------------------------- */

static PyObject* _Py_HOT_FUNCTION
Expand Down Expand Up @@ -364,60 +363,6 @@ _PyFunction_Vectorcall(PyObject *func, PyObject* const* stack,
}


/* --- PyCFunction call functions --------------------------------- */

static PyObject *
cfunction_call_varargs(PyObject *func, PyObject *args, PyObject *kwargs)
{
assert(!PyErr_Occurred());
assert(kwargs == NULL || PyDict_Check(kwargs));

PyCFunction meth = PyCFunction_GET_FUNCTION(func);
PyObject *self = PyCFunction_GET_SELF(func);
PyObject *result;

assert(PyCFunction_GET_FLAGS(func) & METH_VARARGS);
if (PyCFunction_GET_FLAGS(func) & METH_KEYWORDS) {
if (Py_EnterRecursiveCall(" while calling a Python object")) {
Copy link
Contributor

@scoder scoder Jul 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recursion check is gone now. Was that intentional? Any reason why you think we don't need it any more?
Since you're dealing with a public C-API function here, I would advise to keep the current semantics.

Copy link
Contributor Author

@jdemeyer jdemeyer Jul 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The recursion check is gone from PyCFunction_Call but it is in PyObject_Call.

Note that PyCFunction_Call is not a documented C API function, so I assumed that this minor change in behaviour (no longer doing the recursion check) would be acceptable.

return NULL;
}

result = (*(PyCFunctionWithKeywords)(void(*)(void))meth)(self, args, kwargs);

Py_LeaveRecursiveCall();
}
else {
if (kwargs != NULL && PyDict_GET_SIZE(kwargs) != 0) {
PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments",
((PyCFunctionObject*)func)->m_ml->ml_name);
return NULL;
}

if (Py_EnterRecursiveCall(" while calling a Python object")) {
return NULL;
}

result = (*meth)(self, args);

Py_LeaveRecursiveCall();
}

return _Py_CheckFunctionResult(func, result, NULL);
}


PyObject *
PyCFunction_Call(PyObject *func, PyObject *args, PyObject *kwargs)
{
/* For METH_VARARGS, we cannot use vectorcall as the vectorcall pointer
* is NULL. This is intentional, since vectorcall would be slower. */
if (PyCFunction_GET_FLAGS(func) & METH_VARARGS) {
return cfunction_call_varargs(func, args, kwargs);
}
return PyVectorcall_Call(func, args, kwargs);
}


/* --- More complex call functions -------------------------------- */

/* External interface to call any callable object.
Expand Down
37 changes: 36 additions & 1 deletion Objects/methodobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ static PyObject * cfunction_vectorcall_NOARGS(
PyObject *func, PyObject *const *args, size_t nargsf, PyObject *kwnames);
static PyObject * cfunction_vectorcall_O(
PyObject *func, PyObject *const *args, size_t nargsf, PyObject *kwnames);
static PyObject * cfunction_call(
PyObject *func, PyObject *args, PyObject *kwargs);


PyObject *
Expand Down Expand Up @@ -312,7 +314,7 @@ PyTypeObject PyCFunction_Type = {
0, /* tp_as_sequence */
0, /* tp_as_mapping */
(hashfunc)meth_hash, /* tp_hash */
PyCFunction_Call, /* tp_call */
cfunction_call, /* tp_call */
0, /* tp_str */
PyObject_GenericGetAttr, /* tp_getattro */
0, /* tp_setattro */
Expand Down Expand Up @@ -482,3 +484,36 @@ cfunction_vectorcall_O(
Py_LeaveRecursiveCall();
return result;
}


static PyObject *
cfunction_call(PyObject *func, PyObject *args, PyObject *kwargs)
{
assert(!PyErr_Occurred());
assert(kwargs == NULL || PyDict_Check(kwargs));

int flags = PyCFunction_GET_FLAGS(func);
if (!(flags & METH_VARARGS)) {
/* If this is not a METH_VARARGS function, delegate to vectorcall */
return PyVectorcall_Call(func, args, kwargs);
}

/* For METH_VARARGS, we cannot use vectorcall as the vectorcall pointer
* is NULL. This is intentional, since vectorcall would be slower. */
PyCFunction meth = PyCFunction_GET_FUNCTION(func);
PyObject *self = PyCFunction_GET_SELF(func);

PyObject *result;
if (flags & METH_KEYWORDS) {
result = (*(PyCFunctionWithKeywords)(void(*)(void))meth)(self, args, kwargs);
}
else {
if (kwargs != NULL && PyDict_GET_SIZE(kwargs) != 0) {
PyErr_Format(PyExc_TypeError, "%.200s() takes no keyword arguments",
((PyCFunctionObject*)func)->m_ml->ml_name);
return NULL;
}
result = meth(self, args);
}
return _Py_CheckFunctionResult(func, result, NULL);
}
2 changes: 1 addition & 1 deletion Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4997,7 +4997,7 @@ do_call_core(PyThreadState *tstate, PyObject *func, PyObject *callargs, PyObject
PyObject *result;

if (PyCFunction_Check(func)) {
C_TRACE(result, PyCFunction_Call(func, callargs, kwdict));
C_TRACE(result, PyObject_Call(func, callargs, kwdict));
return result;
}
else if (Py_TYPE(func) == &PyMethodDescr_Type) {
Expand Down
2 changes: 1 addition & 1 deletion Tools/gdb/libpython.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1564,7 +1564,7 @@ def is_other_python_frame(self):
return False

if (caller.startswith('cfunction_vectorcall_') or
caller == 'cfunction_call_varargs'):
caller == 'cfunction_call'):
arg_name = 'func'
# Within that frame:
# "func" is the local containing the PyObject* of the
Expand Down