Enable -Wimplicit-fallthrough compiler warning #6252

Girgias · 2020-10-01T16:02:50Z

Introduce a pseudo-keyword fallthrough (following the Linux Kernal style) to indicate that a switch case fall-through to the following one.
This has a couple of benefits in that it normalizes our fall-through comments (which have various formulation even within the same file) and let's use enable -Wimplicit-fallthrough=3 which is included in -Wextra.

I enabled level 5 temporarily (which disables all comments) to use the newly introduce pseudo-keyword instead of comments in extension code under our control.

A couple of issues remain:

TimeLib would need to be patched upstream to include a /* fallthrough */ comment in an instance or we suppress this warning for DateTime
The JIT: the simplest solution would be to disable the compiler warning for it (or even OpCache altogether), as otherwise we would need to patch the integrated libs.

nikic · 2020-10-12T12:40:58Z

I'm a bit concerned about the annoyance this could cause for extensions... especially as they wouldn't have the macro available on old PHP versions.

Zend/zend_API.c

Girgias · 2020-10-16T16:23:54Z

I'm a bit concerned about the annoyance this could cause for extensions... especially as they wouldn't have the macro available on old PHP versions.

Right, we could potentially lower the level from 3 (the default) to 1 which accepts any comment as a fall-through comment. I suppose amending the skeleton to also generate the macro is the version is less than PHP 8.1 /undef is also an option but that wouldn't help already existing extensions :-/

nikic · 2020-10-26T15:35:45Z

Right, we could potentially lower the level from 3 (the default) to 1 which accepts any comment as a fall-through comment.

That sounds like a good approach to me.

.idea/.gitignore

Girgias · 2021-02-04T15:34:15Z

Hopefully everything should work now, and the decision to use level 1 accepts any comment as a switch case fallthrough comment

nikic

Could you please commit the addition of ZEND_FALLTHROUGH and all its uses, and only leave the build system changes that enable the warning in this PR?

ext/session/tests/session_test_936e28c2e65332e2525cabe7864a6e66

main/spprintf.c

main/snprintf.c

sapi/phpdbg/phpdbg_out.c

nikic · 2021-04-16T10:26:19Z

azure/configure.yml

@@ -66,7 +66,6 @@ steps:
        --with-pdo-dblib \
        --with-pdo-oci=shared,instantclient,/opt/oracle/instantclient \
        --with-oci8=shared,instantclient,/opt/oracle/instantclient \
-        --enable-werror \


Should be dropped now.

nikic · 2021-04-16T10:26:48Z

Build failure on i386, should exclude pcre2lib.

Zend/Zend.m4

nikic · 2021-04-16T10:32:49Z

Actually, the pcre failure should be resolved with implicit-fallthrough=1.

UPGRADING.INTERNALS

twose · 2021-04-19T02:42:17Z

main/streams/plain_wrapper.c

@@ -918,7 +918,8 @@ static int php_stdiop_set_option(php_stream *stream, int option, int value, void
 				case PHP_STREAM_SYNC_FDSYNC:
 					return php_stdiop_sync(stream, 1) == 0 ? PHP_STREAM_OPTION_RETURN_OK : PHP_STREAM_OPTION_RETURN_ERR;
 			}
-
+			/* TODO Should this fallthrough? */
+			ZEND_FALLTHROUGH;


I think we should return PHP_STREAM_OPTION_RETURN_NOTIMPL here...

This might indeed be correct, I was just surprised that CI was failing because of this, so I made a quick commit adding this to see if there are any other ones.

Seems this is the last one which is also finding an actual bug :-)

Or maybe even PHP_STREAM_OPTION_RETURN_ERR, as this means an incorrect option value was passed.

Otherwise this falls through to the next case statement.

And add it back to ext/date, ext/hash, and ext/opcache

This is a sync of the php#6252 after few years: - ext/date: fixed since derickr/timelib@f8caaef - ext/hash: warning happens only on 32-bit build in ext/hash/sha3/generic32lc/KeccakP-1600-inplace32BI.c - ext/opcache: IR JIT doesn't seem to have this issue - ext/pcre remains disabled due to pcre2lib/sljit/sljitNativeARM_64.c

This is a sync of the #6252 after few years: - ext/date: pending recheck in GH-14187 - ext/hash: warning happens only on 32-bit build in ext/hash/sha3/generic32lc/KeccakP-1600-inplace32BI.c - ext/opcache: IR JIT doesn't seem to have this issue - ext/pcre remains disabled due to pcre2lib/sljit/sljitNativeARM_64.c (should be rechecked and fixed upstream)

This is a sync of the php#6252: - ext/date: seems to be also fixed Synced in 743d1fd: - ext/hash - ext/opcache - ext/pcre

Girgias added this to the PHP 8.1 milestone Oct 1, 2020

Girgias force-pushed the enable-Wimplicit-fallthrough branch 2 times, most recently from 05e1d65 to 25b345a Compare October 1, 2020 22:19

Girgias force-pushed the enable-Wimplicit-fallthrough branch 2 times, most recently from 2024e23 to 4c8f0fd Compare October 9, 2020 22:26

nikic reviewed Oct 12, 2020

View reviewed changes

Zend/zend_API.c Outdated Show resolved Hide resolved

Girgias force-pushed the enable-Wimplicit-fallthrough branch from 4c8f0fd to 5fe6948 Compare November 3, 2020 11:26

Girgias force-pushed the enable-Wimplicit-fallthrough branch from 8023794 to 4608151 Compare January 15, 2021 04:07

nikic reviewed Jan 25, 2021

View reviewed changes

.idea/.gitignore Outdated Show resolved Hide resolved

Girgias force-pushed the enable-Wimplicit-fallthrough branch 3 times, most recently from f6804c1 to 1a7aca6 Compare February 4, 2021 12:15

Girgias requested a review from nikic February 4, 2021 15:33

nikic reviewed Apr 6, 2021

View reviewed changes

ext/session/tests/session_test_936e28c2e65332e2525cabe7864a6e66 Outdated Show resolved Hide resolved

main/spprintf.c Outdated Show resolved Hide resolved

main/snprintf.c Outdated Show resolved Hide resolved

sapi/phpdbg/phpdbg_out.c Outdated Show resolved Hide resolved

Girgias force-pushed the enable-Wimplicit-fallthrough branch 2 times, most recently from 55d0488 to f6f842b Compare April 6, 2021 23:51

nikic reviewed Apr 16, 2021

View reviewed changes

Zend/Zend.m4 Outdated Show resolved Hide resolved

nikic reviewed Apr 16, 2021

View reviewed changes

UPGRADING.INTERNALS Outdated Show resolved Hide resolved

Girgias force-pushed the enable-Wimplicit-fallthrough branch 2 times, most recently from 89cf484 to b8c6f99 Compare April 19, 2021 02:34

twose reviewed Apr 19, 2021

View reviewed changes

Girgias added 2 commits April 19, 2021 13:46

Signal that the stream option passed is an error

120562f

Otherwise this falls through to the next case statement.

Use ZEND_FALLTHROUGH instead of a comment in PDO

5be62fd

Drop -Wno-implicit-fallthrough compiler flag

35d8051

And add it back to ext/date, ext/hash, and ext/opcache

Girgias force-pushed the enable-Wimplicit-fallthrough branch from b8c6f99 to 35d8051 Compare April 19, 2021 12:46

nikic approved these changes Apr 19, 2021

View reviewed changes

Girgias merged commit dcdc5d9 into php:master Apr 19, 2021

Girgias deleted the enable-Wimplicit-fallthrough branch April 19, 2021 12:59

petk mentioned this pull request May 9, 2024

Sync -Wno-implicit-fallthrough #14187

Closed

3 tasks

petk added a commit to petk/php-src that referenced this pull request Jun 8, 2024

Sync -Wno-implicit-fallthrough

10c770c

This is a sync of the php#6252: - ext/date: seems to be also fixed Synced in 743d1fd: - ext/hash - ext/opcache - ext/pcre

petk added a commit to petk/php-src that referenced this pull request Jul 22, 2024

Sync -Wno-implicit-fallthrough

6fa8e17

This is a sync of the php#6252: - ext/date: seems to be also fixed Synced in 743d1fd: - ext/hash - ext/opcache - ext/pcre

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enable -Wimplicit-fallthrough compiler warning #6252

Enable -Wimplicit-fallthrough compiler warning #6252

Uh oh!

Girgias commented Oct 1, 2020

Uh oh!

nikic commented Oct 12, 2020

Uh oh!

Uh oh!

Girgias commented Oct 16, 2020

Uh oh!

nikic commented Oct 26, 2020

Uh oh!

Uh oh!

Girgias commented Feb 4, 2021

Uh oh!

nikic left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic Apr 16, 2021

Uh oh!

nikic commented Apr 16, 2021

Uh oh!

Uh oh!

nikic commented Apr 16, 2021

Uh oh!

Uh oh!

twose Apr 19, 2021

Uh oh!

Girgias Apr 19, 2021

Uh oh!

nikic Apr 19, 2021

Uh oh!

Uh oh!

Enable -Wimplicit-fallthrough compiler warning #6252

Enable -Wimplicit-fallthrough compiler warning #6252

Uh oh!

Conversation

Girgias commented Oct 1, 2020

Uh oh!

nikic commented Oct 12, 2020

Uh oh!

Uh oh!

Girgias commented Oct 16, 2020

Uh oh!

nikic commented Oct 26, 2020

Uh oh!

Uh oh!

Girgias commented Feb 4, 2021

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nikic Apr 16, 2021

Choose a reason for hiding this comment

Uh oh!

nikic commented Apr 16, 2021

Uh oh!

Uh oh!

nikic commented Apr 16, 2021

Uh oh!

Uh oh!

twose Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

Girgias Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

nikic Apr 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!