On Wed, Feb 9, 2022 at 1:04 PM Petr Viktorin <encukou@gmail.com> wrote:
> > Right now, a large number of macros cast their argument to a type. A
> > few examples:
> >
> > #define PyObject_TypeCheck(ob, type)
> > PyObject_TypeCheck(_PyObject_CAST(ob), type)
> > #define PyTuple_GET_ITEM(op, i) (_PyTuple_CAST(op)->ob_item[i])
> > #define PyDict_GET_SIZE(mp) (assert(PyDict_Check(mp)),((PyDictObject
> > *)mp)->ma_used)
>
> When I look at the Rationale points, and for the first three of these
> macros I didn't find any that sound very convincing:
> - Functions don't have macro pitfalls, but these simple macros don't
> fall into the pits.
> - Fully defining the argument types means getting rid of the cast,
> breaking some code that uses the macro
> - Debugger support really isn't that useful for these simple macros
> - There are no new variables
Using a static inline function, profilers like Linux perf can count
the CPU time spend in static inline functions (on each CPU instruction
when using annotated assembly code). For example, you can see how much
time (accumulated time) is spent in Py_INCREF(), to have an idea of
the cost of reference counting in Python. It's not possible when using
macros.
For debuggers, you're right that Py_INCREF() and PyTuple_GET_ITEM()
macros are very simple and it's not so hard to guess that the debugger
is executing their code in the C code or the assembly code. But the
purpose of PEP 670 is to convert way more complex macros. I wrote a PR
to convert unicodeobject.h macros, IMO there are one of the worst
macros in Python C API:
https://github.com/python/cpython/pull/31221
I always wanted to convert them, but some core devs were afraid of
performance regressions. So I wrote a PEP to prove that there is no
impact on performance.
IMO the new unicodeobject.h code is way more readable. I added two
"kind" variables which have a well defined scope. In macros, no macro
is used currently to avoid macro pitfalls: name conflict if there is
already a "kind" macro where the macro is used.
The conversion to static inline macros also detected a bug with "const
PyObject*": using PyUnicode_READY() on a const Unicode string is
wrong, this function modifies the object if it's not ready (WCHAR
kind). It also detected bugs on "const void *data" used to *write*
into string characters (PyUnicode_WRITE).
Nice example PR. For now what we're suggesting is to proceed with changes where it cannot lead to compilation warnings (failures in the widely used -Werror mode) being introduced. Yes, that may still leave a lot of desirable cleanup to be done. But we don't want to block everything while discussing the rest. Incremental improvement is good even if not everything is being done yet.
Two differing examples from that PR 31221:
Hold off as unsafe for now: Macros that do things like (PyWhateverObject*)(op) such as PyUnicode_CHECK_INTERNED(op) should not have the casting part of macros replaced yet. Doing so could result in a warning or failure at -Werror compile time if someone was not using a PyObject*. Code is supposed to, but the cast means anyone could've used PyUnicodeObject or whatnot itself. Perhaps use a hybrid approach when feasible similar to:
#define PyUnicode_CHECK_INTERNED(op) _PyUnicode_CHECK_INTERNED((PyASCIIObject *)(op))
That should make it safe. And I believe you do mention this technique as something to be used in the PEP.
Safe: PyUnicode_WRITE() in that PR. At first glance that is full of casts on its data and value parameters so it raises suspicion. But further analysis shows that data becomes a void* so there is no casting issue there unless someone were passing a non-pointer in which isn't rightfully something code should ever have done. And value would only be an issue if someone were passing a non-integer type in, that also seems exceedingly unlikely as there'd never be a point in writing code like that. So that kind of change is fine to proceed with.
> The "massive change to working code" part is important. Such efforts
> tend to have unexpected issues, which have an unfortunate tendency to
> surface before release and contribute to release manager burnout.
Aren't you exaggerating a bit? Would you mind to elaborate? Do you
have examples of issues caused by converting macros to static inline
functions?
Not quite the same but I've got a related example similar to what macros casting pointer becoming a function accepting PyObject* without a cast could do:
Between 3.6 and 3.7 we added const to a number of our public Python C APIs.
Migration to 3.7 required updating all sorts of C and C++ extension module code to be pedantically correct, up through its call chain in some cases with matching const declarations on types. (including conditional compilation based on the Python version to support compilation under both during the transition - which is more than a pointer cast change would normally entail).
It was an unintended consequence of a seemingly minor API annotation for pedantic correctness. Cast changes should be easier to fix. It's more the act of needing to fix things when not important that we want to avoid.
The PEP lists many macros converted to static inline functions and
static inline functions converted to regular functions:
https://www.python.org/dev/peps/pep-0670/#macros-converted-to-functions-since-python-3-8
Yep! Those are great success data points to have. Note that Py_INCREF remains a macro so that the cast stays in place:
#define Py_INCREF(op) _Py_INCREF(_PyObject_CAST(op))
-gps