bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS #6030

siddhesh · 2018-03-08T15:11:05Z

gcc 8 has added a new warning heuristic to detect invalid function casts and a stock python build seems to hit that warning quite often. The most common is the cast of a METH_NOARGS function (that uses just one argument) to a PyCFunction. Fix this by adding a dummy argument to all the affected callback functions.

I have split the changes by directory and also made a separate commit where the change was not just a trivial dummy argument addition, as is the case in a4ee3ad and 58eaf31.

https://bugs.python.org/issue33012

serhiy-storchaka · 2018-03-08T17:47:53Z

Doc/extending/newtypes.rst

-a positional argument tuple or keyword argument dictionary. This method is
-equivalent to the Python method::
+a positional argument tuple or keyword argument dictionary. We add an unused
+argument to allow for a safe cast to a PyCFunction. This method is equivalent to


:c:type:`PyCFunction`

serhiy-storchaka · 2018-03-08T18:11:37Z

Objects/odictobject.c

-    {"__delitem__",     (PyCFunction)odict_mp_ass_sub,  METH_NOARGS,
-     odict_delitem__doc__},
+    {"__delitem__",     (PyCFunction)odict_mp_ass_sub,
+     METH_VARARGS | METH_KEYWORDS, odict_delitem__doc__},


This looks incorrect. But an existing code looks incorrect too. I suggest to revert this change (and all other questionable changes) and left them to a separate issue -- bpo-33031.

OK, dropped from this PR.

serhiy-storchaka · 2018-03-08T18:20:49Z

Objects/odictobject.c

@@ -1466,7 +1472,7 @@ odict_dealloc(PyODictObject *self)
 /* tp_repr */

 static PyObject *
-odict_repr(PyODictObject *self)
+odict_repr(PyODictObject *self, PyObject *Py_UNUSED(ignored))


It should be compatible with reprfunc, not PyCFunction.

I'll drop this one too since it looks like it will need an additional wrapper like odict_init.

serhiy-storchaka · 2018-03-08T18:21:32Z

Objects/odictobject.c

@@ -1609,15 +1615,15 @@ odict_richcompare(PyObject *v, PyObject *w, int op)
 /* tp_iter */

 static PyObject *
-odict_iter(PyODictObject *od)
+odict_iter(PyODictObject *od, PyObject *Py_UNUSED(ignored))


It should be compatible with getiterfunc, not PyCFunction.

Likewise, I'll deal with it separately.

serhiy-storchaka · 2018-03-08T18:45:01Z

Modules/_collectionsmodule.c

@@ -1532,7 +1532,7 @@ deque_bool(dequeobject *deque)
 }

 static PyObject *
-deque_get_maxlen(dequeobject *deque)
+deque_get_maxlen(dequeobject *deque, PyObject *Py_UNUSED(ignored))


It is getter.

void *Py_UNUSED(ignored) ``

I'm tackling getters in a separate patch, so I'll drop this.

serhiy-storchaka · 2018-03-08T18:52:57Z

Objects/descrobject.c

@@ -439,7 +439,7 @@ calculate_qualname(PyDescrObject *descr)
 }

 static PyObject *
-descr_get_qualname(PyDescrObject *descr)
+descr_get_qualname(PyDescrObject *descr, PyObject *Py_UNUSED(ignored))


void *Py_UNUSED(ignored)

Likewise, I've got getter changes in another branch, so I'll fix it there.

serhiy-storchaka · 2018-03-08T18:58:06Z

Objects/descrobject.c

@@ -1139,7 +1139,7 @@ wrapper_text_signature(wrapperobject *wp, void *closure)
 static PyObject *
 wrapper_qualname(wrapperobject *wp)


Needs the second argument of type void *. As well as other getters.

I've made a separate bug report for the getters: issue33029.

serhiy-storchaka · 2018-03-08T19:01:30Z

Objects/longobject.c

@@ -4554,7 +4554,7 @@ long_or(PyObject *a, PyObject *b)
 }

 static PyObject *
-long_long(PyObject *v)
+long_long(PyObject *v, PyObject *Py_UNUSED(ignored))


It is used both as PyCFunction and getter. It would be safer to declare the second parameter as void *.

serhiy-storchaka · 2018-03-08T19:02:23Z

Objects/odictobject.c

 /* forward */
-static int odict_init(PyObject *self, PyObject *args, PyObject *kwds);
+static PyObject *
+odict_init(PyObject *self, PyObject *Py_UNUSED(ignored))


I suggest to left this for a separate issue.

OK, I'll drop this and the other patch from this PR and make a different one.

serhiy-storchaka · 2018-03-08T19:06:35Z

Modules/_collectionsmodule.c

@@ -512,7 +512,7 @@ deque_inplace_concat(dequeobject *deque, PyObject *other)
 }

 static PyObject *
-deque_copy(PyObject *deque)
+deque_copy(PyObject *deque, PyObject *Py_UNUSED(ignored))


Explicit cast to PyCFunction at line 1590 is not needed.

The same with other functions which exactly match the signature of PyCFunction.

I've kept the casts in place because we will eventually need it. To fully fix the warnings, the ml_meth type should be like so:

PyObject *(*) (PyObject *, PyObject *, ...)

so that it matches with all of the allowed method types provided that there is an explicit cast in place. Right now I am not sure if I should change the PyCFunction type to that or to make a new PyCFunctionAny just for ml_meth. Since we're on this topic, what do you think is the preferred change? I suppose both change API, but not in a completely incompatible way.

The casts are not needed if the signature matches PyCFunction. It is still needed if the function has type PyCFunctionWithKeywords, PyNoArgsFunction, etc. Removing unneeded casts will clean up the code and can prevent introducing new bugs when the signature be changed unintentionally.

siddhesh

Thanks for the review, I'm pushing a version with most of the fixes you suggested, except for removal of the cast. I've kept the casts in place because we will eventually need it. To fully fix the warnings, the ml_meth type should be like so:

PyObject *(*) (PyObject *, PyObject *, ...)

so that it matches with all of the allowed method types provided that there is an explicit cast in place. Right now I am not sure if I should change the PyCFunction type to that or to make a new PyCFunctionAny just for ml_meth. Since we're on this topic, what do you think is the preferred change? I suppose both change API, but not in a completely incompatible way.

siddhesh · 2018-03-08T19:15:18Z

Objects/descrobject.c

@@ -1139,7 +1139,7 @@ wrapper_text_signature(wrapperobject *wp, void *closure)
 static PyObject *
 wrapper_qualname(wrapperobject *wp)


I've made a separate bug report for the getters: issue33029.

siddhesh · 2018-03-08T19:16:21Z

Objects/odictobject.c

 /* forward */
-static int odict_init(PyObject *self, PyObject *args, PyObject *kwds);
+static PyObject *
+odict_init(PyObject *self, PyObject *Py_UNUSED(ignored))


OK, I'll drop this and the other patch from this PR and make a different one.

siddhesh · 2018-03-08T19:25:04Z

Modules/_collectionsmodule.c

@@ -512,7 +512,7 @@ deque_inplace_concat(dequeobject *deque, PyObject *other)
 }

 static PyObject *
-deque_copy(PyObject *deque)
+deque_copy(PyObject *deque, PyObject *Py_UNUSED(ignored))


I've kept the casts in place because we will eventually need it. To fully fix the warnings, the ml_meth type should be like so:

PyObject *(*) (PyObject *, PyObject *, ...)

so that it matches with all of the allowed method types provided that there is an explicit cast in place. Right now I am not sure if I should change the PyCFunction type to that or to make a new PyCFunctionAny just for ml_meth. Since we're on this topic, what do you think is the preferred change? I suppose both change API, but not in a completely incompatible way.

siddhesh · 2018-03-08T19:26:36Z

Modules/_collectionsmodule.c

@@ -1532,7 +1532,7 @@ deque_bool(dequeobject *deque)
 }

 static PyObject *
-deque_get_maxlen(dequeobject *deque)
+deque_get_maxlen(dequeobject *deque, PyObject *Py_UNUSED(ignored))


I'm tackling getters in a separate patch, so I'll drop this.

siddhesh · 2018-03-08T19:29:03Z

Objects/descrobject.c

@@ -439,7 +439,7 @@ calculate_qualname(PyDescrObject *descr)
 }

 static PyObject *
-descr_get_qualname(PyDescrObject *descr)
+descr_get_qualname(PyDescrObject *descr, PyObject *Py_UNUSED(ignored))


Likewise, I've got getter changes in another branch, so I'll fix it there.

siddhesh · 2018-03-08T19:34:33Z

Objects/odictobject.c

@@ -1466,7 +1472,7 @@ odict_dealloc(PyODictObject *self)
 /* tp_repr */

 static PyObject *
-odict_repr(PyODictObject *self)
+odict_repr(PyODictObject *self, PyObject *Py_UNUSED(ignored))


I'll drop this one too since it looks like it will need an additional wrapper like odict_init.

siddhesh · 2018-03-08T19:35:36Z

Objects/odictobject.c

@@ -1609,15 +1615,15 @@ odict_richcompare(PyObject *v, PyObject *w, int op)
 /* tp_iter */

 static PyObject *
-odict_iter(PyODictObject *od)
+odict_iter(PyODictObject *od, PyObject *Py_UNUSED(ignored))


Likewise, I'll deal with it separately.

siddhesh · 2018-03-08T19:38:53Z

Objects/longobject.c

@@ -4554,7 +4554,7 @@ long_or(PyObject *a, PyObject *b)
 }

 static PyObject *
-long_long(PyObject *v)
+long_long(PyObject *v, PyObject *Py_UNUSED(ignored))


siddhesh · 2018-03-08T19:40:44Z

Objects/odictobject.c

-    {"__delitem__",     (PyCFunction)odict_mp_ass_sub,  METH_NOARGS,
-     odict_delitem__doc__},
+    {"__delitem__",     (PyCFunction)odict_mp_ass_sub,
+     METH_VARARGS | METH_KEYWORDS, odict_delitem__doc__},


OK, dropped from this PR.

METH_NOARGS functions need only a single argument but they are cast into a PyCFunction, which takes two arguments. This triggers an invalid function cast warning in gcc8 (unreleased as of now) due to the argument mismatch. Fix this by adding a dummy unused argument.

siddhesh · 2018-04-22T19:14:17Z

Hi, here's the new patchset with dropped casts and resolved conflicts.

serhiy-storchaka · 2018-04-22T19:12:53Z

PC/bdist_wininst/install.c

@@ -631,7 +631,7 @@ static PyObject *PyMessageBox(PyObject *self, PyObject *args)
    return g_Py_BuildValue("i", rc);
 }

-static PyObject *GetRootHKey(PyObject *self)
+static PyObject *GetRootHKey(PyObject *self, PyObject *Py_UNUSED(ignored))


Casting to PyCFunction below no longer needed. And this is the only use of PyCFunction, thus its declaration can be removed.

Ah I missed this one, I'll fix it.

serhiy-storchaka · 2018-04-22T19:16:51Z

Modules/_collectionsmodule.c

@@ -512,7 +512,7 @@ deque_inplace_concat(dequeobject *deque, PyObject *other)
 }

 static PyObject *
-deque_copy(PyObject *deque)
+deque_copy(PyObject *deque, PyObject *Py_UNUSED(ignored))


The casts are not needed if the signature matches PyCFunction. It is still needed if the function has type PyCFunctionWithKeywords, PyNoArgsFunction, etc. Removing unneeded casts will clean up the code and can prevent introducing new bugs when the signature be changed unintentionally.

METH_NOARGS functions need only a single argument but they are cast into a PyCFunction, which takes two arguments. This triggers an invalid function cast warning in gcc8 (unreleased as of now) due to the argument mismatch. Fix this by adding a dummy unused argument wherever it isn't present and mark it as unused to make it consistent with other similar uses.

METH_NOARGS functions need only a single argument but they are cast into a PyCFunction, which takes two arguments. This triggers an invalid function cast warning in gcc8 (unreleased as of now) due to the argument mismatch. Fix this by adding a dummy unused argument.

siddhesh requested review from rhettinger and a team as code owners March 8, 2018 15:11

the-knights-who-say-ni added the CLA signed label Mar 8, 2018

bedevere-bot added the awaiting review label Mar 8, 2018

serhiy-storchaka reviewed Mar 8, 2018

View reviewed changes

siddhesh commented Mar 8, 2018

View reviewed changes

siddhesh added 2 commits April 23, 2018 00:30

serhiy-storchaka reviewed Apr 22, 2018

View reviewed changes

siddhesh and others added 4 commits April 23, 2018 01:09

Add NEWS item for the internal METH_NOARGS changes

e83b872

serhiy-storchaka approved these changes Apr 29, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Apr 29, 2018

serhiy-storchaka merged commit 55edd0c into python:master Apr 29, 2018

bedevere-bot removed the awaiting merge label Apr 29, 2018

siddhesh deleted the meth-noargs branch April 30, 2018 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS #6030

bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS #6030

siddhesh commented Mar 8, 2018 •

edited by bedevere-bot

Loading

serhiy-storchaka Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Mar 8, 2018

siddhesh Mar 8, 2018

serhiy-storchaka Apr 22, 2018

siddhesh left a comment •

edited

Loading

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh Mar 8, 2018

siddhesh commented Apr 22, 2018

serhiy-storchaka Apr 22, 2018

siddhesh Apr 22, 2018

serhiy-storchaka Apr 22, 2018

		@@ -1139,7 +1139,7 @@ wrapper_text_signature(wrapperobject wp, void closure)
		static PyObject *
		wrapper_qualname(wrapperobject *wp)

bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS #6030

bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS #6030

Conversation

siddhesh commented Mar 8, 2018 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddhesh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddhesh commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddhesh commented Mar 8, 2018 •

edited by bedevere-bot

Loading

siddhesh left a comment •

edited

Loading