Description

picnixz

opened on Dec 22, 2024

Issue body actions

Feature or enhancement

A huge part of the CPython code base uses Argument Clinic for generating methods for classes or standalone functions. Generally, methods are then used as follows:

static PyMethodDef foo_methods[] = {
    _FOOMODULE_FOO_BAR_METHODDEF
    {NULL, NULL}
};

where each <MODULE>_<CLASS>_<METHOD>_METHODDEF is something like

PyDoc_STRVAR(_foomodule_Foo_bar__doc__,
"bar($self, /)\n"
"--\n"
"\n");

#define _FOOMODULE_FOO_BAR_METHODDEF 		\
    {										\
        "bar", 								\
        (PyCFunction)_foomodule_Foo_bar, 	\
        METH_NOARGS, 						\
        _foomodule_Foo_bar__doc__           \
    },

We also have

// @file Modules/foomodule.c

/*[clinic input]
_foomodule.Foo.bar

[clinic start generated code]*/

static PyObject *
_foomodule_Foo_bar_impl(FooObject *self)
/*[clinic end generated code: output=... input=...]*/
{
	Py_RETURN_NONE;
}

// @file Modules/clinic/foomodule.c.h

static PyObject *
_foomodule_Foo_bar_impl(FooObject *self);

static PyObject *
_foomodule_Foo_bar(FooObject *self, PyObject *Py_UNUSED(ignored))
{
    return _foomodule_Foo_bar_impl(self);
}

Now, in order to fix #111178, we generally convert non-AC clinic methods by changing FooObject * to PyObject * and doing a pointer cast in the function body. See #111178 for a wider discussion. The issue is that we cannot easily do the same for AC-generated code since the prototype is part of the auto-generated block.

We can do it if we specify self: self(type="PyObject *") instead of not specifying self in the AC input, but this is an overkill. Instead, AC should always generate two functions: an implementation function which takes a typed object (i.e., FooObject) and a public dispatcher which takes a PyObject *self and possibly unused arguments. Stated otherwise:

static PyObject *
-_foomodule_Foo_bar(FooObject *self, PyObject *Py_UNUSED(ignored))
+_foomodule_Foo_bar(PyObject *self, PyObject *Py_UNUSED(ignored))
{
-    return _foomodule_Foo_bar_impl(self);
+    return _foomodule_Foo_bar_impl((FooObject *)self);
}

I decided to open a separate issue to track this change orthogonally to the UBSan failures fixes that can be achieved without any AC changes. I hope that this will not deterioriate the performances of CPython too much.

---

Note: this does not only affect methods as above; this affects any method that is casted to PyCFunction or anything else with incompatible function pointer signatures.

Activity

1. picnixz

   added

   type-featureA feature request or enhancementA feature request or enhancement

   topic-argument-clinic

   on Dec 22, 2024
2. picnixz commented on Dec 26, 2024

   

   picnixz

   on Dec 26, 2024

   MemberAuthor

   More actions

   cc @ZeroIntensity

   Reacted by Peter Bierma
3. ZeroIntensity commented on Dec 29, 2024

   

   ZeroIntensity

   on Dec 29, 2024

   Member

   More actions

   Took a stab at this. For simple cases, all that needs to change is this property to always return PyObject *, and then the functionality to cast it is already implemented:

   cpython/Tools/clinic/libclinic/converters.py

   Lines 1183 to 1188 in f9a5a3a

   	def parser_type(self) -> str:
   	assert self.type is not None
   	if self.function.kind in {METHOD_INIT, METHOD_NEW, STATIC_METHOD, CLASS_METHOD}:
   	tp, _ = correct_name_for_self(self.function)
   	return tp
   	return self.type

   We'll still need to change some other things though, because some generated clinic code relies on specific structures (e.g. fields in PyTypeObject *). More importantly, we'll have to manually go through and audit where we need to change casts for areas that call clinic funtions manually, typically for locking. For example, I did this with the SSL thread safety fix:

   cpython/Modules/_ssl.c

   Line 937 in f9a5a3a

   if (_ssl__SSLSocket_owner_set(self, owner, NULL) == -1) {

   Here, self is a PySSLSocket * rather than a PyObject *, so changing that would break this case. We'll probably need to deal with that on an agonizing case-by-case basis.

   Reacted by Erlend E. Aasland
4. picnixz commented on Dec 29, 2024

   

   picnixz

   on Dec 29, 2024

   MemberAuthor

   More actions

   I'm not afraid of doing the case-by-case, considering this is what I've been doing for the rest. But thanks for digging into this, I haven't looked at clinic a lot!
5. picnixz commented on Mar 24, 2025

   

   picnixz

   on Mar 24, 2025

   MemberAuthor

   More actions

   Well... I forgot about that but Victor completed it in #130682, #131102, #131157, #131159, and I've completed some in #131605. Since I've marked as part of gh-111178, I'll just close this issue.
6. picnixz

   closed this as completedon Mar 24, 2025

Issue actions