classification
Title: Add -Wimplicit-fallthrough=0 to Makefile ?
Type: compile error Stage: resolved
Components: Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: cstratak, haypo, matrixise, serhiy.storchaka, skrah, zach.ware
Priority: normal Keywords:

Created on 2017-07-13 16:36 by matrixise, last changed 2017-09-12 23:09 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
output.txt matrixise, 2017-07-13 16:35
Pull Requests
URL Status Linked Edit
PR 2698 merged skrah, 2017-07-13 18:42
PR 3132 merged skrah, 2017-08-18 11:49
PR 3142 merged skrah, 2017-08-18 19:24
PR 3157 merged skrah, 2017-08-19 15:39
PR 3205 merged skrah, 2017-08-25 11:50
PR 3518 merged haypo, 2017-09-12 17:39
Messages (26)
msg298299 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2017-07-13 16:35
Hi all,

Since I use the last version of Fedora 26 with gcc-7.1.1, I have these warnings (see output.txt file)

We could add -Wimplicit-fallthrough=0 to Makefile ? it will disable the fallthrough of the coed.

What do you think about that ? What's your feedback on this option and can we use it in the case of CPython ?

Thank you
msg298300 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-13 17:15
It seems to me that by default the compiler recognizes a wide variety of "falls through" comments. Thus we need just add missed comments.

There are warnings emitted when compile imported third-party code. We should ask Stefan for adding "falls through" comments in his libmpdec. And either compile expat with -Wimplicit-fallthrough=0, or wait until warnings will be fixed in upstream, or remove the bundled copy of expat.
msg298302 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-13 17:52
It's a useful warning, but I find it annoying to add 20 "fall through" comments. I may add a pragma at some point.
msg298306 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-13 18:54
New changeset 72b543308ee3087e3fa247981f5cb4be1138c515 by Stefan Krah in branch 'master':
bpo-30923: Suppress fall-through warnings in libmpdec. (#2698)
https://github.com/python/cpython/commit/72b543308ee3087e3fa247981f5cb4be1138c515
msg298307 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-13 18:58
Hmm, that took about 20 min to commit a 3 line diff.  Now I'm watching the buildbots...
msg298312 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-13 20:16
Stéphane, if you want the libmpdec change cherry picked and are willing to do the (significant) work of backporting, I'll merge it.
msg299063 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2017-07-25 12:27
@stefan

In fact I could create the PR for the backports, but I have again the same issues for the rest of CPython.

I have checked your code, and in my case 'gcc' is in the cc string, 'gcc -pthread' and you have fixed for libmpdec.

But for the other modules ?
msg299070 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-25 12:51
Hum, I don't think that it's worth it to backport changes which only fix warnings. Usually, we first focus on fixing all warnings on master.
msg299072 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2017-07-25 12:58
yep, currently, 3.6 and 3.5 are in 'bug fix' mode, and in this case, it's not a bug.
msg299085 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-25 13:50
Well, it's not a bug, but perhaps it is annoying for users of this gcc version if they compile 3.6 often.


Actually, I think gcc should not include this warning in -Wextra.  It's something that could be run manually before a release.


I'd vote for making -Wno-implicit-fallthrough global.
msg299086 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2017-07-25 14:04
Stefan, ask on python-dev ml, or we have to ask to Ned Deily for this version ?
msg299089 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-25 14:17
I'm ok to kill warnings, but I would suggest to first fix most GCC7 warnings because starting to discuss backports. I expect that it will require multiple changes.
msg299090 - (view) Author: Stéphane Wirtel (matrixise) * Date: 2017-07-25 14:22
I am not for a backport, it's not a security fix or a bug fix.

for my case, I just want to "kill" the warnings, maybe we could check the version of gcc and add "-Wimplicit-fallthrough=0".

Here is a good explanation of the 'fallthrough' in gcc7 
https://developers.redhat.com/blog/2017/03/10/wimplicit-fallthrough-in-gcc-7/
msg299091 - (view) Author: Charalampos Stratakis (cstratak) * Date: 2017-07-25 14:32
Yeah the warnings are quit annoying when compiling the master and 3.6 branch.

Fedora 26 currently includes gcc 7 which emits those warnings, other distros will follow up when they update gcc, so I'd think it would be better to fix it.
msg299092 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-25 14:51
We can check for the version, but all versions of gcc that I tested
accept and ignore -Wno-implicit-fallthrough, even though they don't 
actually have -Wimplicit-fallthrough.

Of course they choke on -Wimplicit-fallthrough=0.
msg299093 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-07-25 14:54
I think the fall-through blog notes are slightly overstated. :-)

"The switch fallthrough has been widely considered a design defect in C."


It's an important feature.
msg300483 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-08-18 10:35
The following commit broke _decimal compilation on the "x86 Tiger 3.x" buildbot:

commit 72b543308ee3087e3fa247981f5cb4be1138c515
Author: Stefan Krah <skrah@bytereef.org>
Date:   Thu Jul 13 20:54:20 2017 +0200

    bpo-30923: Suppress fall-through warnings in libmpdec. (#2698)

http://buildbot.python.org/all/builders/x86%20Tiger%203.x/builds/1068/steps/compile/logs/stdio

building '_decimal' extension
gcc -fno-strict-aliasing -Wsign-compare -g -O0 -Wall -Wstrict-prototypes -std=c99 -Wextra -Wno-unused-parameter -Wno-missing-field-initializers -DUNIVERSAL=1 -I/Users/db3l/buildarea/3.x.bolen-tiger/build/Modules/_decimal/libmpdec -I./Include -I. -I/usr/local/include -I/Users/db3l/buildarea/3.x.bolen-tiger/build/Include -I/Users/db3l/buildarea/3.x.bolen-tiger/build -c /Users/db3l/buildarea/3.x.bolen-tiger/build/Modules/_decimal/_decimal.c -o build/temp.macosx-10.4-i386-3.7-pydebug/Users/db3l/buildarea/3.x.bolen-tiger/build/Modules/_decimal/_decimal.o -Wno-implicit-fallthrough
cc1: error: unrecognized command line option "-Wno-implicit-fallthrough"
msg300490 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-18 11:24
Thanks. I tried to revert it, but got:

remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/master.
remote: error: 2 of 2 required status checks are expected.
To https://github.com/python/cpython.git
 ! [remote rejected]       master -> master (protected branch hook declined)
error: failed to push some refs to 'https://github.com/python/cpython.git'


So I'm supposed to spend 20 min creating an irrelevant PR to revert a 3 line diff?!

I would never have finished _decimal under these conditions.
msg300526 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-18 18:44
So I installed gcc-7.2.0 from source. Hilariously compiling gcc *itself*
emits fallthrough warnings!

Then I tried to be a good open source drone and add 20 /* fall through */
comments to libmpdec.

gcc is too stupid to recognize the /* fall through */ at the #endif
position.


Perhaps the author of this gcc warning wants to submit a patch.
msg300527 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-18 19:39
New changeset d73a960c575207539c3f9765cff26d4fff400b45 by Stefan Krah in branch 'master':
bpo-30923: Disable warning that has been part of -Wextra since gcc-7.0. (#3142)
https://github.com/python/cpython/commit/d73a960c575207539c3f9765cff26d4fff400b45
msg300585 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-19 15:50
PR 3157 addresses everything apart from expat and

  https://github.com/python/cpython/blob/master/Modules/cjkcodecs/_codecs_iso2022.c#L816


I'm not sure about that one. It looks harmless but a bit odd.
msg300620 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-21 11:10
New changeset f432a3234f9f2ee09bd40be03e06bf72865ee375 by Stefan Krah in branch 'master':
bpo-30923: Silence fall-through warnings included in -Wextra since gcc-7.0. (#3157)
https://github.com/python/cpython/commit/f432a3234f9f2ee09bd40be03e06bf72865ee375
msg300622 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-21 11:24
Cherry picking has too many conflicts, I'm not backporting this myself.
msg300833 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-25 12:07
New changeset 9e1e6f528f3fec16b9bd99f5ee38048ffec04a81 by Stefan Krah in branch 'master':
bpo-30923: Silence fall-through warnings in libexpat build. (#3205)
https://github.com/python/cpython/commit/9e1e6f528f3fec16b9bd99f5ee38048ffec04a81
msg300835 - (view) Author: Stefan Krah (skrah) * (Python committer) Date: 2017-08-25 12:23
All warnings except for #31275 are dealt with.
msg302003 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-09-12 23:09
New changeset c0e77364ca29df6cfb311e79892955c92bd8e595 by Victor Stinner in branch '3.6':
[3.6] bpo-30923: Silence fall-through warnings included in -Wextra since gcc-7.0 (#3518)
https://github.com/python/cpython/commit/c0e77364ca29df6cfb311e79892955c92bd8e595
History
Date User Action Args
2017-09-12 23:09:46hayposetmessages: + msg302003
2017-09-12 17:39:15hayposetpull_requests: + pull_request3513
2017-08-25 12:23:13skrahsetstatus: open -> closed
type: compile error
messages: + msg300835

resolution: fixed
stage: resolved
2017-08-25 12:07:52skrahsetmessages: + msg300833
2017-08-25 11:50:26skrahsetpull_requests: + pull_request3242
2017-08-21 11:24:20skrahsetmessages: + msg300622
2017-08-21 11:10:02skrahsetmessages: + msg300620
2017-08-19 15:50:27skrahsetmessages: + msg300585
2017-08-19 15:39:06skrahsetpull_requests: + pull_request3194
2017-08-18 19:39:35skrahsetmessages: + msg300527
2017-08-18 19:24:52skrahsetpull_requests: + pull_request3177
2017-08-18 18:44:44skrahsetmessages: + msg300526
2017-08-18 11:49:41skrahsetpull_requests: + pull_request3166
2017-08-18 11:24:13skrahsetmessages: + msg300490
2017-08-18 10:35:14hayposetmessages: + msg300483
2017-07-25 14:54:36skrahsetmessages: + msg299093
2017-07-25 14:51:12skrahsetmessages: + msg299092
2017-07-25 14:32:18cstrataksetmessages: + msg299091
2017-07-25 14:22:49matrixisesetmessages: + msg299090
2017-07-25 14:17:24hayposetmessages: + msg299089
2017-07-25 14:04:29matrixisesetmessages: + msg299086
2017-07-25 13:50:12skrahsetmessages: + msg299085
2017-07-25 12:58:04matrixisesetmessages: + msg299072
2017-07-25 12:51:20hayposetnosy: + haypo
messages: + msg299070
2017-07-25 12:27:20matrixisesetmessages: + msg299063
2017-07-24 17:15:13cstrataksetnosy: + cstratak
2017-07-24 17:10:14zach.warelinkissue31017 superseder
2017-07-13 20:16:06skrahsetmessages: + msg298312
2017-07-13 18:58:49skrahsetmessages: + msg298307
2017-07-13 18:54:22skrahsetmessages: + msg298306
2017-07-13 18:42:43skrahsetpull_requests: + pull_request2764
2017-07-13 17:58:54zach.waresetnosy: + zach.ware
2017-07-13 17:52:15skrahsetmessages: + msg298302
2017-07-13 17:15:51serhiy.storchakasetnosy: + skrah, serhiy.storchaka
messages: + msg298300
2017-07-13 16:36:00matrixisecreate