Add -Wimplicit-fallthrough=0 to Makefile ?

[matrixise]

BPO	30923
Nosy	@vstinner, @skrah, @zware, @serhiy-storchaka, @matrixise, @stratakis
PRs	bpo-30923: Suppress fall-through warnings in libmpdec. #2698 bpo-30923: Revert flag that is not recognized by an obsolete gcc v… #3132 bpo-30923: Disable warning that has been part of -Wextra since gcc-7.0. #3142 bpo-30923: Silence fall-through warnings included in -Wextra since gc… #3157 bpo-30923: Silence fall-through warnings in libexpat build. #3205 [3.6] bpo-30923: Silence fall-through warnings included in -Wextra since gcc-7.0 #3518 [3.6] bpo-30923, bpo-31279: Fix GCC warnings #4620
Files	output.txt

^{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 2017-08-25.12:23:13.249>
created_at = <Date 2017-07-13.16:36:00.767>
labels = ['build', '3.7']
title = 'Add -Wimplicit-fallthrough=0 to Makefile ?'
updated_at = <Date 2017-11-29.23:00:37.850>
user = 'https://github.com/matrixise'

bugs.python.org fields:

activity = <Date 2017-11-29.23:00:37.850>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-08-25.12:23:13.249>
closer = 'skrah'
components = []
creation = <Date 2017-07-13.16:36:00.767>
creator = 'matrixise'
dependencies = []
files = ['47012']
hgrepos = []
issue_num = 30923
keywords = []
message_count = 27.0
messages = ['298299', '298300', '298302', '298306', '298307', '298312', '299063', '299070', '299072', '299085', '299086', '299089', '299090', '299091', '299092', '299093', '300483', '300490', '300526', '300527', '300585', '300620', '300622', '300833', '300835', '302003', '307269']
nosy_count = 6.0
nosy_names = ['vstinner', 'skrah', 'zach.ware', 'serhiy.storchaka', 'matrixise', 'cstratak']
pr_nums = ['2698', '3132', '3142', '3157', '3205', '3518', '4620']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'compile error'
url = 'https://bugs.python.org/issue30923'
versions = ['Python 3.7']

[matrixise]

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

[serhiy-storchaka]

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.

[skrah]

It's a useful warning, but I find it annoying to add 20 "fall through" comments. I may add a pragma at some point.

[skrah]

New changeset 72b5433 by Stefan Krah in branch 'master':
bpo-30923: Suppress fall-through warnings in libmpdec. (bpo-2698)
72b5433

[skrah]

Hmm, that took about 20 min to commit a 3 line diff. Now I'm watching the buildbots...

[skrah]

Stéphane, if you want the libmpdec change cherry picked and are willing to do the (significant) work of backporting, I'll merge it.

[matrixise]

@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 ?

[vstinner]

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.

[matrixise]

yep, currently, 3.6 and 3.5 are in 'bug fix' mode, and in this case, it's not a bug.

[skrah]

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.

[matrixise]

Stefan, ask on python-dev ml, or we have to ask to Ned Deily for this version ?

[vstinner]

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.

[matrixise]

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/

[stratakis]

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.

[skrah]

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.

[skrah]

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.

[vstinner]

The following commit broke _decimal compilation on the "x86 Tiger 3.x" buildbot:

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

bpo-30923: Suppress fall-through warnings in libmpdec. (bpo-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"

[skrah]

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.

[skrah]

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.

[skrah]

New changeset d73a960 by Stefan Krah in branch 'master':
bpo-30923: Disable warning that has been part of -Wextra since gcc-7.0. (bpo-3142)
d73a960

[skrah]

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.

[skrah]

New changeset f432a32 by Stefan Krah in branch 'master':
bpo-30923: Silence fall-through warnings included in -Wextra since gcc-7.0. (bpo-3157)
f432a32

[skrah]

Cherry picking has too many conflicts, I'm not backporting this myself.

[skrah]

New changeset 9e1e6f5 by Stefan Krah in branch 'master':
bpo-30923: Silence fall-through warnings in libexpat build. (bpo-3205)
9e1e6f5

[skrah]

All warnings except for bpo-31275 are dealt with.

[vstinner]

New changeset c0e7736 by Victor Stinner in branch '3.6':
[3.6] bpo-30923: Silence fall-through warnings included in -Wextra since gcc-7.0 (bpo-3518)
c0e7736

[vstinner]

New changeset dedcbee by Victor Stinner in branch '3.6':
[3.6] bpo-30923, bpo-31279: Fix GCC warnings (bpo-4620)
dedcbee