Repository navigation
Invalid function cast warnings with gcc 8 for METH_NOARGS #77193
Description
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 = <Date 2018-11-27.19:45:30.755>
created_at = <Date 2018-03-06.11:18:12.991>
labels = ['3.8', 'build', '3.7']
title = 'Invalid function cast warnings with gcc 8 for METH_NOARGS'
updated_at = <Date 2020-03-12.12:38:56.761>
user = 'https://github.com/siddhesh'bugs.python.org fields:
activity = <Date 2020-03-12.12:38:56.761>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-11-27.19:45:30.755>
closer = 'serhiy.storchaka'
components = ['Build']
creation = <Date 2018-03-06.11:18:12.991>
creator = 'siddhesh'
dependencies = ['33031']
files = []
hgrepos = []
issue_num = 33012
keywords = ['patch']
message_count = 49.0
messages = ['313317', '313319', '313320', '313875', '313879', '313886', '313889', '313890', '314795', '314962', '315123', '315909', '315937', '315938', '315948', '316355', '316356', '316357', '316372', '317668', '317670', '317742', '317747', '317794', '317823', '317825', '317826', '317853', '317854', '318538', '326181', '327754', '328410', '330498', '330505', '330506', '330526', '330545', '330549', '330551', '330552', '330690', '330694', '337197', '337199', '341153', '341155', '341156', '364015']
nosy_count = 15.0
nosy_names = ['terry.reedy', 'gregory.p.smith', 'vstinner', 'pmpp', 'xdegaye', 'martin.panter', 'serhiy.storchaka', 'ztane', 'eitan.adler', 'siddhesh', 'yan12125', 'cstratak', 'miss-islington', 'resmord', 'asn']
pr_nums = ['6030', '6652', '6748', '6749', '6757', '7112', '10736', '10744', '10748', '10750', '10751', '10795', '10796', '12179', '14814']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue33012'
versions = ['Python 3.7', 'Python 3.8']
Activity
siddhesh commentedon Mar 6, 2018
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. The fix is pretty simple but needs to be applied widely. I'm slowly knocking them off in my spare time; WIP here, which has a few other types of warnings mixed in that I'll sift out during submission and also create separate bug reports for:
https://github.com/siddhesh/cpython/tree/func-cast
I'll clean up and post PR(s) once I am done but I figured I should file this report first since it is a pretty big change in terms of number of files touched and wanted to be sure that I'm making changes the way the community prefers.
serhiy-storchaka commentedon Mar 6, 2018
Argument Clinic generates the following declaration for the second parameter of METH_NOARGS functions:
It would be nice to follow the same style.
If the first parameter is of type PyObject* too, the explicit cast to PyCFunction can be removed.
Skip the curses module. It will be converted to Argument Clinic.
stratakis commentedon Mar 6, 2018
We are getting hit by that quite often on Fedora, with the transition to gcc 8 and it creates unnecessary noise at our build logs. Thanks for working on that.
When you sent your PR I can test it within our build system and verify if it works.
ztane commentedon Mar 15, 2018
I don't have GCC 8 so I cannot verify this bug, but *function pointer casts* are fine - any function pointer can be cast to any other function pointer - it is only that they must *not* be called unless cast back again to be compatible with the function definition. Any fix to the contrary might well *cause* undefined behaviour!
Could you provide a sample of the *actual warnings* so that they could be studied?
siddhesh commentedon Mar 15, 2018
Please see the attached PR; METH_NOARGS callbacks are inconsistent in their signature and many have just one argument while they're called with two arguments, the second being NULL. The patch fixes these to consistently take and call with two arguments.
Here's one of a few hundred.
Objects/bytesobject.c:3085:25: warning: cast between incompatible function types from ‘PyObject * ()(striterobject *)’ {aka ‘struct _object * ()(struct <anonymous> *)’} to ‘PyObject * ()(PyObject *, PyObject *)’ {aka ‘struct _object * ()(struct _object *, struct _object *)’} [-Wcast-function-type]
{"__reduce__", (PyCFunction)striter_reduce, METH_NOARGS,
^
This is a new warning in gcc8, so you'll likely not find much reference, but here's a gcc bug report that might give you more context:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84531
ztane commentedon Mar 15, 2018
Yea, I looked into
ceval.cand the function is called incorrectly, so there is undefined behaviour there - it has been wrong all along, in 3.5 all the way down to 2-somethingThe warning in GCC shouldn't probably have been enabled at all in
-Wall -Wextrabecause the cast is explicit. However, it is somewhat true.However, the correct way to fix would be to have the METH_NOARGS case cast the function to the right prototype. There exists lots of existing code that *is* going to break too.
Perhaps PyCFunction should declare no prototype, i.e. empty parentheses, for backwards compatibility:
and deprecate it; start using a new typedef for it - and then add proper casts in every place that call a function.
siddhesh commentedon Mar 15, 2018
The explicit cast is precisely what enables the more nuanced function cast warning where it checks the function for type compatibility. That is, it will check the types of the return and arguments and then warn in case of mismatch.
AFAICT, there is no right prototype for the METH_NOARGS function; there used to be a PyCFunctionWithNoArguments but that seems to have fallen out of favour some time back. The prototype that seems to be commonly in use (and in clinic as well, which is what I based my patches on) is the PyCFunction, which seems like a safe way to do things.
I have a patch in the works that makes it PyObject *(*)(PyObject *, PyObject *, ...)
which allows for two compulsory arguments (fits in with most cases, provided the METH_NOARGS patch is accepted) and then the rest depending on the type of the cast function. The rest of the PyMethodDef functions are much simpler fixes this way. It also seems like a more intuitive description of the callbacks.
Then there are getter and setter and other function pointers that need similar fixes to METH_NOARGS.
siddhesh commentedon Mar 15, 2018
I forgot to clarify that the function cast warning allows for variable argument casts as a wildcard, which is my basis for the PyObject *(*)(PyObject *, PyObject *, ...) fix proposal.
vadmium commentedon Apr 2, 2018
Siddhesh, it looks like your fixes make the C function signatures match the signature expected in the PyMethodDef structure. If so, I suggest to remove the (PyCFunction) casts from those structure definitions as well. For instance, now that we have
PyObject *Noddy_name(Noddy *self, PyObject *Py_UNUSED(ignored))
I suggest changing
PyMethodDef Noddy_methods[] = {
{"name", (PyCFunction)Noddy_name, METH_NOARGS, ...
to
PyMethodDef Noddy_methods[] = {
{"name", Noddy_name, METH_NOARGS, ...
I suspect the casts were only added to hide compiler warnings related to this bug.
If you are proposing to add an ellipsis (...) to the definition of PyCFunction, that seems misguided. I understand this is incompatible under standard C. Are you relying on a GCC extension perhaps? Python is used with other compilers too.
vadmium commentedon Apr 5, 2018
Sorry, I realize there is a problem remaining with the pointer types for "Noddy_name" (Noddy vs PyObject pointers), so you can't remove the cast there. But my suggestion should still apply to other places, for instance the "error_out" method in Doc/howto/cporting.rst.
siddhesh commentedon Apr 9, 2018
Fair enough, I'll reduce my scope of changes for this patchset, especially since I'm unable to find enough time to work on the remaining changes I had thought of in the coming weeks.
I'll post an updated patch soonish.
serhiy-storchaka commentedon Apr 29, 2018
New changeset 55edd0c by Serhiy Storchaka (Siddhesh Poyarekar) in branch 'master':
bpo-33012: Fix invalid function cast warnings with gcc 8 for METH_NOARGS. (GH-6030)
55edd0c
25 remaining items
vstinner commentedon Nov 27, 2018
I still get something 100 warnings with GCC 8.2.1 on Fedora 29. I wrote PR 10744 to add a macro to cast a function pointer.
I chose to add a macro, so it will be easier to spot where we cast function pointers and change the implementation (cast) if needed at a single place.
serhiy-storchaka commentedon Nov 27, 2018
New changeset ad8ac54 by Serhiy Storchaka in branch '3.7':
bpo-33012: Fix signatures of METH_NOARGS functions. (GH-10736) (GH-10748)
ad8ac54
miss-islington commentedon Nov 27, 2018
New changeset d5c8bd8 by Miss Islington (bot) in branch '3.6':
bpo-33012: Fix signatures of METH_NOARGS functions. (GH-10736) (GH-10748)
d5c8bd8
serhiy-storchaka commentedon Nov 27, 2018
New changeset 1c60715 by Serhiy Storchaka in branch 'master':
bpo-33012: Fix more invalid function cast warnings with gcc 8. (GH-10751)
1c60715
serhiy-storchaka commentedon Nov 27, 2018
Only one function cast warning is left, and it is a separate issue: bpo-33015.
miss-islington commentedon Nov 29, 2018
New changeset 1659c08 by Miss Islington (bot) in branch '3.7':
bpo-33012: Fix more invalid function cast warnings with gcc 8. (GH-10751)
1659c08
vstinner commentedon Nov 29, 2018
New changeset 77000bb by Victor Stinner in branch '3.6':
bpo-33012: Fix more invalid function cast warnings with gcc 8. (GH-10751) (GH-10796)
77000bb
vstinner commentedon Mar 5, 2019
I marked bpo-36197 as a duplicate of this issue:
"""
gcc -pthread -c -Wno-unused-result -Wsign-compare -DDYNAMIC_ANNOTATIONS_ENABLED=1 -g -Og -Wall -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Objects/memoryobject.o Objects/memoryobject.c
Objects/memoryobject.c:3112:21: warning: cast between incompatible function types from 'PyObject * ()(PyMemoryViewObject *, PyObject *, PyObject *)' {aka 'struct _object * ()(struct <anonymous> *, struct _object *, struct _object *)'} to 'PyObject * ()(PyObject *, PyObject *)' {aka 'struct _object * ()(struct _object *, struct _object *)'} [-Wcast-function-type]
{"tobytes", (PyCFunction)memory_tobytes, METH_VARARGS|METH_KEYWORDS, memory_tobytes_doc},
I am preparing a small PR for this issue.
"""
=> PR 12179
vstinner commentedon Mar 5, 2019
New changeset 359a2f3 by Victor Stinner (Stéphane Wirtel) in branch 'master':
bpo-33012: Fix compilation warnings in memoryobject.c and _collectionsmodule.c (GH-12179)
359a2f3
cryptomilk commentedon Apr 30, 2019
Looking at:
359a2f3
This is not fixing the underlying issue but hiding it. The right fix would be to use a union for ml_meth providing members for the 3 different function. So the developer could assign them correctly and the compiler would warn if he would do something wrong. Casting to (void *) is just hiding the problem not fixing it!
serhiy-storchaka commentedon Apr 30, 2019
Yes, and this particular case was fixed in adfffc7.
cryptomilk commentedon Apr 30, 2019
And how do you deal with METH_VARARGS|METH_KEYWORDS functions which have 3 arguments?
vstinner commentedon Mar 12, 2020
FYI I modified SystemError("bad call flags") error message to include the method name in bpo-39884.