gh-115432: Add critical section variant that handles a NULL object

[colesbury]

This adds Py_XBEGIN_CRITICAL_SECTION and Py_XEND_CRITICAL_SECTION, which accept a possibly NULL object as an argument. If the argument is NULL, then nothing is locked or unlocked. Otherwise, they behave like Py_BEGIN/END_CRITICAL_SECTION.

• Issue: Add variant of Py_BEGIN_CRITICAL_SECTION that accepts a NULL argument #115432

[colesbury]

cc @tomasr8

[colesbury]

The context here is in the set implementation, we have a number of functions that take a possibly NULL iterable argument:

cpython/Objects/setobject.c

Lines 1015 to 1016 in 514b1c9

	static PyObject *
	make_new_set_basetype(PyTypeObject *type, PyObject *iterable)

We generally want to lock the iterable if it's not NULL. There's enough uses that I think it's worth handling in pycore_critical_section.h rather than in setobject.c.

Related PR: #113800

[tomasr8]

Is the #ifdef needed here? The whole macro should be a no-op when the GIL is enabled

[colesbury]

It's needed because of the &_PyObject_CAST(op)->ob_mutex will only compile on the free-threaded build. In the default build, PyObject does not have an ob_mutex field.

We could have instead put the #ifdef Py_GIL_DISABLED around the whole _PyCriticalSection_BeginOpt function declaration. That would work too.

[tomasr8]

Ah cool, thanks for clearing that up!

[DinoV]

The context here is in the set implementation, we have a number of functions that take a possibly NULL iterable argument:

cpython/Objects/setobject.c

Lines 1015 to 1016 in 514b1c9

	static PyObject *
	make_new_set_basetype(PyTypeObject *type, PyObject *iterable)

I kind of wonder if we do really want to lock all of the iterables... Ultimately the dict case could proceed lock-free and for some arbitrary object that isn't a dict or a list we don't really need to lock it (e.g. for a Python used defined type it's presumably protected by some other Python lock if it needs to be thread safe).

But I could see this as being generally useful too.

[colesbury]

I kind of wonder if we do really want to lock all of the iterables...

Yeah, I think we will want to refine the locking approach in the future, but locking all the iterables works for now. I think we want to actually lock in the dict case so that the whole operation appears atomic, even though the iteration over the dict is thread-safe without a lock. IIRC, we have code that assumes that set(dict) is atomic.