classification
Title: Turn on extra warnings on GCC
Type: enhancement Stage: resolved
Components: Build Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: SilentGhost, benjamin.peterson, haypo, martin.panter, python-dev, serhiy.storchaka, skrah, ztane
Priority: normal Keywords: patch

Created on 2015-02-27 22:29 by serhiy.storchaka, last changed 2016-09-13 05:32 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
configure_extra_warnings.patch serhiy.storchaka, 2015-03-18 20:36 review
configure_extra_warnings2.patch serhiy.storchaka, 2016-09-09 22:18 review
Messages (20)
msg236851 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-02-27 22:29
What if make GCC emit extra warnings? Adding following options:

-Wall -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers

will make GCC emit all warnings except few types that would produce too many warnings. With these options the compilation is almost clean on 32-bit Linux.
msg236887 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2015-02-28 14:20
+1. I think the flags should go into CFLAGS_NODIST.
msg238469 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2015-03-18 20:36
I'm not well experienced with autoconf. Here is my attempt.
msg269017 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-06-21 19:59
Could anyone please make a review?
msg271153 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-07-24 14:05
-Wall is already added (unconditionally) to $OPT above. What’s the point of also adding it to $CFLAGS_NODIST as well? Does it have to be added to both?
msg272444 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-11 13:03
The proposed options add exactly one warning for me (ignoring warnings from libffi). How would you work around this:

./Include/pymem.h:136:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]
   ( ((size_t)(n) > PY_SSIZE_T_MAX / sizeof(type)) ? NULL : \
                  ^
/media/disk/home/proj/python/cpython/Modules/_ssl.c:4435:22: note: in expansion of macro ‘PyMem_New’
         _ssl_locks = PyMem_New(PyThread_type_lock, _ssl_locks_count);
                      ^~~~~~~~~
msg272480 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-11 20:16
Just add -Wno-type-limits.
msg272499 - (view) Author: Antti Haapala (ztane) * Date: 2016-08-12 05:30
I don't think adding -Wno-type-limits is a good idea.

The good question is how that can be happening, e.g. how PY_SSIZE_T_MAX divided by sizeof anything can be *more* than max(size_t)? E.g now that I stare at the code, *that* warning should be impossible if everything is correct. It means either that the RHS is negative or size_t is defined to be 32-bit in this compilation unit whereas PY_SSIZE_T is 64-bit. Neither sound like a good idea.
msg272500 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-08-12 05:54
I didn’t look too closely, but I did see that _ssl_locks_count is unsigned int. I was compiling for x86-64 Linux, where int is 32 bits and size_t is 64. So maybe GCC was optimizing the (size_t) cast away, and then rightfully warning that the largest unsigned int will never exceed the largest possible array size.
msg272501 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-08-12 06:44
Seems GCC is such smart, that can deduce that the maximal value of _ssl_locks_count is never larger than PY_SSIZE_T_MAX / sizeof(PyThread_type_lock). The other workaround is making _ssl_locks_count of type size_t instead of unsigned int, but I wouldn't do this with good reasons.
msg272502 - (view) Author: Antti Haapala (ztane) * Date: 2016-08-12 07:30
Ah, indeed, I somehow missed that. Though, there is no good reason for it being unsigned either; as the actual type in SSL API's is of type int. Another argument of type int is cast to unsigned just for the comparison on line 4419, and unsigned int counters i and j are used in function _setup_ssl_threads.

The variable could be safely changed to `size_t` (along with those index variables) without it affecting anything at all, as it is a static variable within that module and only used to hold a size of an array, and never passed back to another function.
msg275470 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-09 22:18
Rebased patch. Removed save_CFLAGS="$CFLAGS" and CFLAGS="$save_CFLAGS" lines as Marin suggested.

Benjamin, seems you are experienced with autoconf. Could you please make a review?
msg275586 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-09-10 07:04
Can one of the -Wall flags be dropped? What is the difference between $OPT and $CFLAGS_NODIST?

gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Wstrict-prototypes    -std=c99 -Wall -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers   -I. -IInclude -I./Include    -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c
msg275830 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-11 19:03
New changeset 99adb5df8cb6 by Serhiy Storchaka in branch 'default':
Issue #23545: Turn on extra warnings on GCC.
https://hg.python.org/cpython/rev/99adb5df8cb6
msg275832 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-11 19:09
AFAIK $CFLAGS_NODIST is used only for compiling the core and standard modules, $OPT is included in $CFLAGS and also used for compiling third-party extensions. Since -Wall is already included in $OPT, there is no need of it in $CFLAGS_NODIST. Removed this part. Thank you for your review Martin.
msg276072 - (view) Author: SilentGhost (SilentGhost) * Date: 2016-09-12 17:12
The commit didn't resolve the warning though.
msg276075 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-12 17:27
All warnings were resolved at the moment of providing previous version, but the code was changed too much last days. New warnings was added and disappeared in these days. Now we can just sit and resolve the leftover.
msg276076 - (view) Author: Roundup Robot (python-dev) Date: 2016-09-12 17:28
New changeset da485c6c744e by Stefan Krah in branch 'default':
Issue #23545: Adding -Wextra in setup.py is no longer necessary, since it
https://hg.python.org/cpython/rev/da485c6c744e
msg276077 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2016-09-12 17:30
Agreed. SilentGhost, it's probably best to open new issues for the warnings.
msg276178 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-09-13 05:32
I think Silent Ghost is talking about my -Wtype-limits warning, which is still present. That is the only warning I am getting. I suspect you won’t see it with a 32-bit build.
History
Date User Action Args
2016-09-13 05:32:13martin.pantersetmessages: + msg276178
2016-09-12 17:30:33skrahsetmessages: + msg276077
2016-09-12 17:28:24python-devsetmessages: + msg276076
2016-09-12 17:27:00serhiy.storchakasetmessages: + msg276075
2016-09-12 17:12:23SilentGhostsetnosy: + SilentGhost
messages: + msg276072
2016-09-11 19:09:12serhiy.storchakasetstatus: open -> closed
resolution: fixed
messages: + msg275832

stage: patch review -> resolved
2016-09-11 19:03:01python-devsetnosy: + python-dev
messages: + msg275830
2016-09-10 07:04:56martin.pantersetmessages: + msg275586
2016-09-09 22:18:48serhiy.storchakasetfiles: + configure_extra_warnings2.patch

messages: + msg275470
2016-09-07 21:23:31serhiy.storchakasetnosy: + benjamin.peterson
2016-08-12 07:30:03ztanesetmessages: + msg272502
2016-08-12 06:44:45serhiy.storchakasetmessages: + msg272501
2016-08-12 05:54:28martin.pantersetmessages: + msg272500
2016-08-12 05:30:37ztanesetnosy: + ztane
messages: + msg272499
2016-08-11 20:16:11serhiy.storchakasetmessages: + msg272480
2016-08-11 13:03:38martin.pantersetmessages: + msg272444
2016-07-24 14:05:50martin.pantersetnosy: + martin.panter
messages: + msg271153
2016-06-21 20:15:55hayposetnosy: + haypo
2016-06-21 19:59:07serhiy.storchakasetmessages: + msg269017
versions: + Python 3.6, - Python 3.5
2015-03-18 20:36:59serhiy.storchakasetfiles: + configure_extra_warnings.patch
keywords: + patch
messages: + msg238469

stage: patch review
2015-02-28 14:20:10skrahsetnosy: + skrah
messages: + msg236887
2015-02-27 22:29:43serhiy.storchakacreate