bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS by siddhesh · Pull Request #6030 · python/cpython

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

:c:type:`PyCFunction`

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

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

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

It should be compatible with getiterfunc, not PyCFunction.

Likewise, I'll deal with it separately.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Likewise, I'll deal with it separately.

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

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

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

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

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 a team and rhettinger 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

Uh oh!

Conversation

siddhesh commented Mar 8, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddhesh left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddhesh commented Apr 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

siddhesh commented Mar 8, 2018 •

edited by bedevere-bot

Loading

siddhesh left a comment •

edited

Loading