classification
Title: Python macros don’t shield arguments
Type: compile error Stage: resolved
Components: C API Versions: Python 3.10
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: WildCard65, erlendaasland, gregory.p.smith, numberZero, vstinner
Priority: normal Keywords: 3.9regression, patch

Created on 2021-02-09 22:05 by numberZero, last changed 2021-03-22 16:47 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 24533 merged erlendaasland, 2021-02-15 10:46
Messages (17)
msg386746 - (view) Author: Vitaliy (numberZero) Date: 2021-02-09 22:05
There is a lot of macros like:
    #define PyObject_TypeCheck(ob, tp) \
    (Py_IS_TYPE(ob, tp) || PyType_IsSubtype(Py_TYPE(ob), (tp)))
These work fine until an argument happen to contain a comma. That’s possible as a result of other macro’s expansion. E.g. if U(x) is defined as x,
    PyObject_TypeCheck(ob, U(f<a,b>(c)))
expands to
    (Py_IS_TYPE(ob, f<a,b>(c)) || ...)
but < and > aren’t treated as brackets by the preprocessor so Py_IS_TYPE is now invoked with 3 arguments instead of just 2, breaking module compilation.

As arguments are expected to be values, surrounding each with parentheses solves the problem. But there are many such macros so that’s not an one-line fix.

Note: the example is from PyGLM (simplified), it doesn’t compile on 3.9 due to this issue.
msg386754 - (view) Author: William Pickard (WildCard65) * Date: 2021-02-10 03:00
This feels like it's more of an issue with the C++ compiler you're using. (I can tell it's C++ because of template syntax)
msg386760 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2021-02-10 07:24
This bug is valid, that macro and likely others aren't up to best practices.

C macros are a PITA to get right.
msg386822 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-02-11 08:00
I'm replacing macros with static inline functions to avoid issues like this one (ex: bpo-35059). Are you interested to convert the PyObject_TypeCheck() macro to a static inline function?

"""
There is a lot of macros like:
    #define PyObject_TypeCheck(ob, tp) \
    (Py_IS_TYPE(ob, tp) || PyType_IsSubtype(Py_TYPE(ob), (tp)))
These work fine until an argument happen to contain a comma.
"""

This macro also has a common bug of macros: if one argument has a side effect, it is executed twice! For example, if an argument is a function call, the function is called twice.

See:

* https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html
* https://vstinner.github.io/split-include-directory-python38.html "Convert macros to static inline functions"
msg386827 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-02-11 10:41
> Are you interested to convert the PyObject_TypeCheck() macro to a static inline function?

I'd like to give it a shot if it's ok for you all.
msg386998 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-02-15 10:44
Silence implies consent? I'll throw up a PR, then :)
msg387001 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-02-15 10:58
I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.
msg387007 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-02-15 12:06
> I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

That sounds reasonable.
msg387023 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-02-15 16:19
New changeset 4bb2a1ebc569eee6f1b46ecef1965a26ae8cb76d by Erlend Egeberg Aasland in branch 'master':
bpo-43181: Convert PyObject_TypeCheck to static inline function (GH-24533)
https://github.com/python/cpython/commit/4bb2a1ebc569eee6f1b46ecef1965a26ae8cb76d
msg387024 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-02-15 16:21
Thanks Vitaliy for the bug report and Erlend for the fix ;-)

For Python 3.9 and older, a workaround is to wrap the call to PyObject_TypeCheck() with your own static inline function.
msg387360 - (view) Author: Vitaliy (numberZero) Date: 2021-02-19 22:28
Thanks for the fix, it works for my use case. (btw that was
    #define U(...) __VA_ARGS__
and not what I wrote).

> I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

> For Python 3.9 and older, a workaround is to wrap the call to PyObject_TypeCheck() with your own static inline function.

For Python 3.8 that fix wouldn’t be needed as the `tp` argument was parenthesised in the macro.

Yet... the first argument is still unshielded, passed to a macro that expects one single macro argument. That’s not a regression, it wasn’t shielded in 3.8 either, but why not just parenthesise each macro argument that denotes an expression (as opposed to e.g. name)?
msg388509 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-11 15:15
> Yet... the first argument is still unshielded, passed to a macro that expects one single macro argument.

Are you talking about the current code in the master branch or the 3.9 branch? If you are talking about the _PyObject_CAST(ob) call, would you mind to elaborate how it is an issue? The code in master LGTM.

> That’s not a regression, it wasn’t shielded in 3.8 either, but why not just parenthesise each macro argument that denotes an expression (as opposed to e.g. name)?

In the master branch, Include/ and its subdirectories contains 158 .h files for a total of 20K lines of C code. It's not easy to check all files. If you spotted bugs, would you mind to give examples, or a way to automatically detect bugs?

As I wrote previously, I dislike macros. If someone is changed, I would prefer to convert the function into a static inline function which doesn't have macros pitfalls.
msg388714 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-03-15 09:00
> As I wrote previously, I dislike macros. If someone is changed, I would prefer to convert the function into a static inline function which doesn't have macros pitfalls.

Should we create a meta issue for this? Most macros are trivial, and can safely be converted, given that they're not used as l-values. Converting one header file at the time would make it easy to review, and it would be possible to make good progress in a few months time.
msg388727 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-15 12:01
If possible, I would prefer to only replace macros with static inline functions if the changes avoids clear macro pitfalls. It's not because macros have pitfalls that we must replace them all blindly.

Also, this issue is closed. At this point, I'm not convinced that it's worth it to fix PyObject_TypeCheck() in Python 3.8 and 3.9, so I leave the issue closed.
msg388730 - (view) Author: Erlend E. Aasland (erlendaasland) * (Python triager) Date: 2021-03-15 12:07
> If possible, I would prefer to only replace macros with static inline functions if the changes avoids clear macro pitfalls.

Yes, of course. Should I create a meta issue for this, or do you prefer raising one issue pr. fix?
msg388733 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-15 12:52
You can create a single issue, and later we will see if it's needed to split it into sub-issues.
msg389331 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2021-03-22 16:47
Me:
> I don't think that PR 24533 should be backported to Python 3.8 and Python 3.9. I prefer to avoid any risk of regression, and so only change Python 3.10.

Sometimes. I'm wise :-D This change broken 2 extension modules which rely on the fact that the macro magically add parenthesis:

python-cvxopt: "if Matrix_Check((PyObject *)val)" with: "#define Matrix_Check(self) PyObject_TypeCheck(self, &matrix_tp)"
https://bugzilla.redhat.com/show_bug.cgi?id=1941557
PR proposed: https://github.com/cvxopt/cvxopt/pull/190

python-Bottleneck: "if PyArray_Check(a_obj) {" with: "#define PyArray_Check(op) PyObject_TypeCheck(op, &PyArray_Type)"
https://bugzilla.redhat.com/show_bug.cgi?id=1941559

"if func(...) {" is not valid C code, but the old macro added magically parenthesis!

It's hard to say if the Python change is a backward incompatible or not... I prefer to keep it and fix the few broken C extensions.
History
Date User Action Args
2021-03-22 16:47:15vstinnersetmessages: + msg389331
2021-03-15 12:52:56vstinnersetmessages: + msg388733
2021-03-15 12:07:33erlendaaslandsetmessages: + msg388730
2021-03-15 12:01:40vstinnersetmessages: + msg388727
2021-03-15 09:00:06erlendaaslandsetmessages: + msg388714
2021-03-11 15:15:20vstinnersetmessages: + msg388509
2021-02-19 22:28:36numberZerosetmessages: + msg387360
2021-02-15 16:21:14vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg387024

stage: patch review -> resolved
2021-02-15 16:19:31vstinnersetmessages: + msg387023
2021-02-15 12:06:22erlendaaslandsetmessages: + msg387007
2021-02-15 10:58:15vstinnersetmessages: + msg387001
versions: - Python 3.9
2021-02-15 10:46:02erlendaaslandsetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request23320
2021-02-15 10:44:31erlendaaslandsetmessages: + msg386998
2021-02-11 10:41:10erlendaaslandsetmessages: + msg386827
2021-02-11 08:48:06erlendaaslandsetnosy: + erlendaasland
2021-02-11 08:00:16vstinnersetmessages: + msg386822
2021-02-10 07:26:15gregory.p.smithsetnosy: + vstinner
2021-02-10 07:24:50gregory.p.smithsettype: compile error
versions: + Python 3.10
keywords: + 3.9regression
nosy: + gregory.p.smith

messages: + msg386760
stage: needs patch
2021-02-10 03:00:11WildCard65setnosy: + WildCard65
messages: + msg386754
2021-02-09 22:05:50numberZerocreate