bpo-33738: Fix macros which contradict PEP 384 #7477

ctismer · 2018-06-07T10:27:26Z

During development of the limited API support for PySide,
we saw an error in a macro that accessed a type field.

This patch fixes the 7 errors in the Python headers.
Macros which were not written as capitals were implemented
as function.

To do the necessary analysis again, a script was included that
parses all headers and looks for "->tp_" in sections which can
be reached with active limited API.

It is easily possible to call this script as a test.

Error listing:

../../Include/objimpl.h:243
#define PyObject_IS_GC(o) (PyType_IS_GC(Py_TYPE(o)) &&
(Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o)))
Action: commented only

../../Include/objimpl.h:362
#define PyType_SUPPORTS_WEAKREFS(t) ((t)->tp_weaklistoffset > 0)
Action: commented only

../../Include/objimpl.h:364
#define PyObject_GET_WEAKREFS_LISTPTR(o)
((PyObject **) (((char *) (o)) + Py_TYPE(o)->tp_weaklistoffset))
Action: commented only

../../Include/pyerrors.h:143
#define PyExceptionClass_Name(x)
((char )(((PyTypeObject)(x))->tp_name))
Action: implemented function

../../Include/abstract.h:593
#define PyIter_Check(obj)
((obj)->ob_type->tp_iternext != NULL &&
(obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented)
Action: implemented function

../../Include/abstract.h:713
#define PyIndex_Check(obj)
((obj)->ob_type->tp_as_number != NULL &&
(obj)->ob_type->tp_as_number->nb_index != NULL)
Action: implemented function

../../Include/abstract.h:924
#define PySequence_ITEM(o, i)
( Py_TYPE(o)->tp_as_sequence->sq_item(o, i) )
Action: commented only

https://bugs.python.org/issue33738

During development of the limited API support for PySide, we saw an error in a macro that accessed a type field. This patch fixes the 7 errors in the Python headers. Macros which were not written as capitals were implemented as function. To do the necessary analysis again, a script was included that parses all headers and looks for "->tp_" in serctions which can be reached with active limited API. It is easily possible to call this script as a test. Error listing: ../../Include/objimpl.h:243 #define PyObject_IS_GC(o) (PyType_IS_GC(Py_TYPE(o)) && \ (Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o))) Action: commented only ../../Include/objimpl.h:362 #define PyType_SUPPORTS_WEAKREFS(t) ((t)->tp_weaklistoffset > 0) Action: commented only ../../Include/objimpl.h:364 #define PyObject_GET_WEAKREFS_LISTPTR(o) \ ((PyObject **) (((char *) (o)) + Py_TYPE(o)->tp_weaklistoffset)) Action: commented only ../../Include/pyerrors.h:143 #define PyExceptionClass_Name(x) \ ((char *)(((PyTypeObject*)(x))->tp_name)) Action: implemented function ../../Include/abstract.h:593 #define PyIter_Check(obj) \ ((obj)->ob_type->tp_iternext != NULL && \ (obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented) Action: implemented function ../../Include/abstract.h:713 #define PyIndex_Check(obj) \ ((obj)->ob_type->tp_as_number != NULL && \ (obj)->ob_type->tp_as_number->nb_index != NULL) Action: implemented function ../../Include/abstract.h:924 #define PySequence_ITEM(o, i)\ ( Py_TYPE(o)->tp_as_sequence->sq_item(o, i) ) Action: commented only

During development of the limited API support for PySide, we saw an error in a macro that accessed a type field. This patch fixes the 7 errors in the Python headers. Macros which were not written as capitals were implemented as function. To do the necessary analysis again, a script was included that parses all headers and looks for "->tp_" in sections which can be reached with active limited API. It is easily possible to call this script as a test. Error listing: ../../Include/objimpl.h:243 (Py_TYPE(o)->tp_is_gc == NULL || Py_TYPE(o)->tp_is_gc(o))) Action: commented only ../../Include/objimpl.h:362 Action: commented only ../../Include/objimpl.h:364 ((PyObject **) (((char *) (o)) + Py_TYPE(o)->tp_weaklistoffset)) Action: commented only ../../Include/pyerrors.h:143 ((char *)(((PyTypeObject*)(x))->tp_name)) Action: implemented function ../../Include/abstract.h:593 ((obj)->ob_type->tp_iternext != NULL && \ (obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented) Action: implemented function ../../Include/abstract.h:713 ((obj)->ob_type->tp_as_number != NULL && \ (obj)->ob_type->tp_as_number->nb_index != NULL) Action: implemented function ../../Include/abstract.h:924 ( Py_TYPE(o)->tp_as_sequence->sq_item(o, i) ) Action: commented only

ctismer · 2018-06-07T18:15:36Z

Can I squash these commits, or do I need to re-send the whole thing? (Or leave it in pieces)

zware · 2018-06-07T18:17:52Z

They'll be squashed at merge time, don't worry about it until then :)

ned-deily · 2018-06-09T18:31:33Z

LGTM, although this is not an area of my expertise. Since we are getting very close to 3.7.0rc1 and it would be good to have this resolved, I'm going to merge this to master now so it gets some buildbot exposure. If anyone has any review comments, get them in real soon! Otherwise, I will backport and merge to 3.7 within the next 24 hours.

ned-deily · 2018-06-09T19:33:47Z

Looks like the change introduces a compile warning. Should that be addressed?:

..\Objects\exceptions.c(349): warning C4090: 'return': different 'const' qualifiers [D:\buildarea\3.x.ware-win81-release\build\PCbuild\pythoncore.vcxproj]

http://buildbot.python.org/all/#/builders/12/builds/960

ctismer · 2018-06-09T22:08:21Z

Yes it should be fixed. I was not aware that Python has const-ness, already.
I have amended my patch with "const char *". But since it is merged, I don't see the change.

ned-deily · 2018-06-09T22:16:36Z

@ctismer Thanks. The best thing would be to open a new PR but, if you can paste the diff here, I'll take care of it.

serhiy-storchaka · 2018-06-08T04:42:21Z

Include/abstract.h

 #define PyIter_Check(obj) \
    ((obj)->ob_type->tp_iternext != NULL && \
     (obj)->ob_type->tp_iternext != &_PyObject_NextNotImplemented)
+#else
+PyAPI_FUNC(int) PyIter_Check(PyObject*);


Nitpick: it would look better if add a space between PyObject and *.

serhiy-storchaka · 2018-06-08T05:03:39Z

Include/pyerrors.h

 #define PyExceptionClass_Name(x) \
     ((char *)(((PyTypeObject*)(x))->tp_name))
+#else
+     PyAPI_FUNC(char *) PyExceptionClass_Name(PyObject*);


Should return const char *. And please add a space between PyObject and *.

Will do.
My style is a bit spoiled by Qt's C++ style. They write "func(PyObject *self)"
but "func(PyObject*)".
I am preparing a diff as Ned said, but I can also repeat the PR.

serhiy-storchaka · 2018-06-08T05:07:28Z

Include/pyerrors.h

@@ -140,8 +140,12 @@ PyAPI_FUNC(void) _PyErr_ChainExceptions(PyObject *, PyObject *, PyObject *);
 #define PyExceptionInstance_Check(x)                    \
    PyType_FastSubclass((x)->ob_type, Py_TPFLAGS_BASE_EXC_SUBCLASS)

+#ifndef Py_LIMITED_API
 #define PyExceptionClass_Name(x) \
     ((char *)(((PyTypeObject*)(x))->tp_name))


Actually (char *) is not needed. But it should be documented in What's New that PyExceptionClass_Name() now returns const char * instead of char * (there were similar breaking changes in 3.7).

serhiy-storchaka · 2018-06-08T05:19:02Z

Objects/exceptions.c

@@ -342,6 +342,12 @@ PyException_SetContext(PyObject *self, PyObject *context)
    Py_XSETREF(((PyBaseExceptionObject *)self)->context, context);
 }

+#undef PyExceptionClass_Name
+char *


Return const char *.

serhiy-storchaka · 2018-06-08T05:20:23Z

Objects/abstract.c

@@ -2535,6 +2543,13 @@ PyObject_GetIter(PyObject *o)
    }
 }

+#undef PyIter_Check
+int PyIter_Check(PyObject *obj)


Nitpick: Break a line between int and PyIter_Check.

Ok, will do. Although all other #undef's are without newline.

Eeek, misunderstood!
Sure, you wanted a line after int!
I must have been very tired...

serhiy-storchaka · 2018-06-10T04:58:04Z

Seems I forgot to press the "Submit review" button after adding comments.

ctismer · 2018-06-10T08:16:44Z

Ok, I hope I got all changes right. Doing a new PR.

serhiy-storchaka · 2018-06-10T11:57:33Z

Since this PR has been merged, my comments except the one about the result of PyExceptionClass_Name are outdated. They where just style nitpicks. And I have opened a separate issue for the latter.

tekknolagi · 2020-06-19T17:59:48Z

Was this ever backported to Python 3.7?

ned-deily · 2020-06-22T08:25:49Z

@tekknolagi No, I don't think they were ever backported to 3.7. See the discussion on the still open bug issue (https://bugs.python.org/issue33738) which is also the place for comments. In any case, with the upcoming release of 3.7.8, 3.7 has moved to security-fix mode, precluding changes like this.

the-knights-who-say-ni added the CLA signed label Jun 7, 2018

bedevere-bot added the awaiting review label Jun 7, 2018

ctismer requested a review from a team as a code owner June 7, 2018 17:17

ned-deily requested a review from serhiy-storchaka June 8, 2018 04:33

ned-deily merged commit ea62ce7 into python:master Jun 9, 2018

bedevere-bot removed the awaiting review label Jun 9, 2018

serhiy-storchaka reviewed Jun 10, 2018

View reviewed changes

ned-deily pushed a commit that referenced this pull request Jun 10, 2018

bpo-33738: Address review comments in GH #7477 (GH-7585)

8398713

serhiy-storchaka mentioned this pull request Jan 29, 2025

Remove broken PySequence_Fast() function from the limited C API capi-workgroup/decisions#55

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-33738: Fix macros which contradict PEP 384 #7477

bpo-33738: Fix macros which contradict PEP 384 #7477

ctismer commented Jun 7, 2018 •

edited by bedevere-bot

Loading

ctismer commented Jun 7, 2018

zware commented Jun 7, 2018

ned-deily commented Jun 9, 2018

ned-deily commented Jun 9, 2018

ctismer commented Jun 9, 2018 •

edited

Loading

ned-deily commented Jun 9, 2018

serhiy-storchaka Jun 8, 2018

serhiy-storchaka Jun 8, 2018

ctismer Jun 10, 2018 •

edited

Loading

serhiy-storchaka Jun 8, 2018

serhiy-storchaka Jun 8, 2018

serhiy-storchaka Jun 8, 2018

ctismer Jun 10, 2018

ctismer Jun 23, 2018

serhiy-storchaka commented Jun 10, 2018

ctismer commented Jun 10, 2018

serhiy-storchaka commented Jun 10, 2018

tekknolagi commented Jun 19, 2020

ned-deily commented Jun 22, 2020

bpo-33738: Fix macros which contradict PEP 384 #7477

bpo-33738: Fix macros which contradict PEP 384 #7477

Conversation

ctismer commented Jun 7, 2018 • edited by bedevere-bot Loading

ctismer commented Jun 7, 2018

zware commented Jun 7, 2018

ned-deily commented Jun 9, 2018

ned-deily commented Jun 9, 2018

ctismer commented Jun 9, 2018 • edited Loading

ned-deily commented Jun 9, 2018

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

ctismer Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

serhiy-storchaka Jun 8, 2018

Choose a reason for hiding this comment

ctismer Jun 10, 2018

Choose a reason for hiding this comment

ctismer Jun 23, 2018

Choose a reason for hiding this comment

serhiy-storchaka commented Jun 10, 2018

ctismer commented Jun 10, 2018

serhiy-storchaka commented Jun 10, 2018

tekknolagi commented Jun 19, 2020

ned-deily commented Jun 22, 2020

ctismer commented Jun 7, 2018 •

edited by bedevere-bot

Loading

ctismer commented Jun 9, 2018 •

edited

Loading

ctismer Jun 10, 2018 •

edited

Loading