[C API] PEP 674: Disallow using macros (Py_TYPE and Py_SIZE) as l-value

[vstinner]

BPO	45476
Nosy	@malemburg, @rhettinger, @vstinner, @gareth-rees, @erlend-aasland, @arhadthedev
PRs	bpo-45476: Convert PyFloat_AS_DOUBLE() to static inline #28961 bpo-45476: Disallow using PyFloat_AS_DOUBLE() as l-value #28976 bpo-45476: Add _Py_RVALUE() macro #29860 bpo-45476: Disallow using asdl_seq_GET() as l-value #29866
Files	pep674_regex.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-10-14.21:17:31.326>
labels = ['expert-C-API', '3.11']
title = '[C API] PEP 674: Disallow using macros (Py_TYPE and Py_SIZE) as l-value'
updated_at = <Date 2022-01-27.20:29:40.417>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2022-01-27.20:29:40.417>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['C API']
creation = <Date 2021-10-14.21:17:31.326>
creator = 'vstinner'
dependencies = []
files = ['50462']
hgrepos = []
issue_num = 45476
keywords = ['patch']
message_count = 41.0
messages = ['403950', '403956', '403961', '403965', '403990', '403995', '403999', '404001', '404010', '404039', '406343', '406344', '406345', '406346', '406348', '406996', '407283', '407358', '407374', '407375', '407395', '407410', '407412', '407416', '407417', '407456', '407463', '407528', '407529', '407531', '407532', '407536', '407872', '407875', '411774', '411802', '411803', '411826', '411827', '411831', '411921']
nosy_count = 6.0
nosy_names = ['lemburg', 'rhettinger', 'vstinner', 'gdr@garethrees.org', 'erlendaasland', 'arhadthedev']
pr_nums = ['28961', '28976', '29860', '29866']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue45476'
versions = ['Python 3.11']

[vstinner]

The Python C API provides "AS" functions to convert an object to another type, like PyFloat_AS_DOUBLE(). These macros can be abused to be used as l-value: "PyFloat_AS_DOUBLE(obj) = new_value;". It prevents to change the PyFloat implementation and makes life harder for Python implementations other than CPython.

I propose to convert these macros to static inline functions to disallow using them as l-value.

I made a similar change for Py_REFCNT(), Py_TYPE() and Py_SIZE(). For these functions, I added "SET" variants: Py_SET_REFCNT(), Py_SET_TYPE(), Py_SET_SIZE(). Here, I don't think that the l-value case is legit, and so I don't see the need to add a way to *set* a value.

For example, I don't think that PyFloat_SET_DOUBLE(obj, value) would make sense. A Python float object is supposed to be immutable.

[rhettinger]

These macros can be abused to be used as l-value

You could simply document, "don't do that". Also if these is a need to make an assignment, you're now going to have to create a new setter function to fill the need.

We really don't have to go on thin ice converting to functions that might or might not be inlined depending on compiler specific nuances.

AFAICT, no one has ever has problems with these being macros. There really isn't a problem to be solved and the "solution" may in fact introduce new problems that we didn't have before.

Put me down for a -1 on the these blanket macro-to-inline function rewrites. The premise is almost entirely a matter of option, "macros are bad, functions are good" and a naive assumption, "inline functions" always inline.

[vstinner]

Raymond:

AFAICT, no one has ever has problems with these being macros.

This issue is about the API of PyFloat_AS_DOUBLE(). Implementing it as a macro or a static inline function is an implementation detail which doesn't matter. But I don't know how to disallow "PyFloat_AS_DOUBLE(obj) = value" if it is defined as a macro.

Have a look at the Facebook "nogil" project which is incompatible with accessing directly the PyObject.ob_refcnt member:
"Extensions must use Py_REFCNT and Py_SET_REFCNT instead of directly accessing reference count fields"
https://docs.google.com/document/d/18CXhDb1ygxg-YXNBJNzfzZsDFosB5e6BfnXLlejd9l0/edit

Raymond:

You could simply document, "don't do that".

Documentation doesn't work. Developers easily fall into traps when it's possible to fall. See bpo-30459 for such trap with PyList_SET_ITEM() and PyCell_SET() macros. They were misused by two Python projects.

Raymond:

We really don't have to go on thin ice converting to functions that might or might not be inlined depending on compiler specific nuances.

Do you have a concrete example where a static inline function is not inlined, whereas it was inlined when it was a macro? So far, I'm not aware of any performance issue like that.

There were attempts to use __attribute__((always_inline)) (Py_ALWAYS_INLINE), but so far, using it was not a clear win.

[vstinner]

I searched for "PyFloat_AS_DOUBLE.*=" regex in the PyPI top 5000 projects. I couldn't find any project doing that.

I only found perfectly safe comparisons:

traits/ctraits.c: if (PyFloat_AS_DOUBLE(value) <= PyFloat_AS_DOUBLE(low)) {
traits/ctraits.c: if (PyFloat_AS_DOUBLE(value) >= PyFloat_AS_DOUBLE(high)) {
c/_cffi_backend.c: return PyFloat_AS_DOUBLE(ob) != 0.0;
pandas/_libs/src/klib/khash_python.h: ( PyFloat_AS_DOUBLE(a) == PyFloat_AS_DOUBLE(b) );

[malemburg]

I am with Raymond on this one.

If "protecting against wrong use" is the only reason to go down the slippery path of starting to rely on compiler optimizations for performance critical operations, the argument is not good enough.

If people do use macros in l-value mode, it's their problem when their code breaks, not ours. Please don't forget that we are operating under the consenting adults principle: we expect users of the CPython API to use it as documented and expect them to take care of the fallout, if things break when they don't.

We don't need to police developers into doing so.

[vstinner]

For PyObject, I converted Py_REFCNT(), Py_TYPE() and Py_SIZE() to static inline functions to enforce the usage of Py_SET_REFCNT(), Py_SET_TYPE() and Py_SET_SIZE(). Only a minority of C extensions are affected by these changes. Also, there is more pressure from recent optimization projects to abstract accesses to PyObject members.

I agree that it doesn't seem that "AS" functions are abused to *set* the inner string:

• PyByteArray_AS_STRING()
• PyBytes_AS_STRING()
• PyFloat_AS_DOUBLE()

If "protecting against wrong use" is the only reason to go down the slippery path of starting to rely on compiler optimizations for performance critical operations, the argument is not good enough.

Again, I'm not aware of any performance issue caused by short static inline functions like Py_TYPE() or the proposed PyFloat_AS_DOUBLE(). If there is a problem, it should be addressed, since Python uses more and more static inline functions.

static inline functions is a common feature of C language. I'm not sure where your doubts of bad performance come from.

Using static inline functions has other advantages. It helps debugging and profiling, since the function name can be retrieved by debuggers and profilers when analysing the machine code. It also avoids macro pitfalls (like abusing a macro to use it as an l-value ;-)).

[malemburg]

On 15.10.2021 11:43, STINNER Victor wrote:

Again, I'm not aware of any performance issue caused by short static inline functions like Py_TYPE() or the proposed PyFloat_AS_DOUBLE(). If there is a problem, it should be addressed, since Python uses more and more static inline functions.

static inline functions is a common feature of C language. I'm not sure where your doubts of bad performance come from.

Inlining is something that is completely under the control of the
used compilers. Compilers are free to not inline function marked for
inlining, which can result in significant slowdowns on platforms
which are e.g. restricted in RAM and thus emphasize on small code size,
or where the CPUs have small caches or not enough registers (think
micro-controllers).

The reason why we have those macros is because we want the developers to be
able to make a conscious decision "please inline this code unconditionally
and regardless of platform or compiler". The developer will know better
what to do than the compiler.

If the developer wants to pass control over to the compiler s/he can use
the corresponding C function, which is usually available (and then, in many
cases, also provides error handling).

Using static inline functions has other advantages. It helps debugging and profiling, since the function name can be retrieved by debuggers and profilers when analysing the machine code. It also avoids macro pitfalls (like abusing a macro to use it as an l-value ;-)).

Perhaps, but then I never had to profile macro use in the past. Instead,
what I typically found was that using macros results in faster code when
used in inner loops, so profiling usually guided me to use macros instead
of functions.

That said, the macros you have inlined so far were all really trivial,
so a compiler will most likely always inline them (the number of machine
code instructions for the call would be more than needed for
the actual operation).

Perhaps we ought to have a threshold for making such decisions, e.g.
number of machine code instructions generated for the macro or so, to
not get into discussions every time :-)

A blanket "static inline" is always better than a macro is not good
enough as an argument, though.

Esp. in PGO driven optimizations the compiler could opt for using
the function call rather than inlining if it finds that the code
in question is not used much and it needs to save space to have
loops fit into CPU caches.

[gareth-rees]

If the problem is accidental use of the result of PyFloat_AS_DOUBLE() as an lvalue, why not use the comma operator to ensure that the result is an rvalue?

The C99 standard says "A comma operator does not yield an lvalue" in §6.5.17; I imagine there is similar text in other versions of the standard.

The idea would be to define a helper macro like this:

    /* As expr, but can only be used as an rvalue. */
    #define Py_RVALUE(expr) ((void)0, (expr))

and then use the helper where needed, for example:

    #define PyFloat_AS_DOUBLE(op) Py_RVALUE(((PyFloatObject *)(op))->ob_fval)

[vstinner]

#define Py_RVALUE(expr) ((void)0, (expr))

Oh, that's a clever trick!

I wrote #73162 which uses it.