Add -Wextra compiler flag exclude some of the major offender #5151

Girgias · 2020-02-05T17:40:47Z

Although I don't think we will be able to add the -Wextra flag without disabling some of the warnings which are enabled within this short cut. It seems better to be aware that these issues exist within the codebase.

Currently I've disabled 54 flags from -Wextra:

-Wclobbered which arises a lot within the Opcache extensions and the SAPI.
-Wimplicit-fallthrough even with -W-implicit-fallthrough=1 there are still a large amount of them and a lot in the JIT/Opcache which may be generated files?
-Wunused-parameter this one happens a lot in macros and ZEND/PHP_API functions for extensions BC
-Wsign-compare if someone wants to tackle it, the compiler output for -W-sign-compare is a bit more than 1000 lines: https://gist.github.com/Girgias/8c7aefc9687515a31efce589e95fc942
~~-Wmissing-field-initializers only has seven offender but five of them are in PHPDBG, compiler output: https://gist.github.com/Girgias/74251f0a2a2fb08e2262186cc0ec925d~~

Girgias · 2020-02-05T18:15:46Z

Forgot that the basic ./configure doesn't enable everything we test on CI

Edit: And obviously I also forgot that Travis checks less extensions than Azure Pipelines ...

Girgias · 2020-02-05T19:40:23Z

So it seems there are a couple of issues remaining:

/home/travis/build/Girgias/php-src/ext/gd/libgd/gd_interpolation.c: In function ‘gdImageRotateBilinear’:

/home/travis/build/Girgias/php-src/ext/gd/libgd/gd_interpolation.c:1777:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

    if ((m >= 0) && (m < src_h - 1) && (n >= 0) && (n < src_w - 1)) {

           ^

/home/travis/build/Girgias/php-src/ext/gd/libgd/gd_interpolation.c:1777:42: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

    if ((m >= 0) && (m < src_h - 1) && (n >= 0) && (n < src_w - 1)) {

                                          ^

/home/travis/build/Girgias/php-src/ext/gd/libgd/gd_interpolation.c: At top level:

~~The GD ones need some investigations as our GD bundled lib is custom and it isn't clear if m and n should be unsigned.~~

Fixed with ac89139

/home/travis/build/Girgias/php-src/ext/mbstring/php_mbregex.c: In function ‘_php_mb_regex_ereg_search_exec’:

/home/travis/build/Girgias/php-src/ext/mbstring/php_mbregex.c:1465:13: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

     if (beg >= 0 && beg <= end && end <= len) {

             ^

/home/travis/build/Girgias/php-src/ext/mbstring/php_mbregex.c: In function ‘zif_mb_ereg_search_getregs’:

/home/travis/build/Girgias/php-src/ext/mbstring/php_mbregex.c:1604:12: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

    if (beg >= 0 && beg <= end && end <= len) {

            ^

/home/travis/build/Girgias/php-src/ext/mbstring/php_mbregex.c: At top level:

Fixed by c7094d8

In file included from /home/travis/build/Girgias/php-src/Zend/zend_execute.c:4511:0:

/home/travis/build/Girgias/php-src/Zend/zend_vm_execute.h:59488:23: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]

 static const uint32_t ZEND_FASTCALL zend_vm_get_opcode_handler_idx(uint32_t spec, const zend_op* op)

                       ^

Fixed with: d4de3f9

ext/phar/phar_path_check.c: In function ‘phar_path_check’:

ext/phar/phar_path_check.c:48:24: warning: suggest braces around empty body in an ‘if’ statement [-Wempty-body]

  if ((YYLIMIT - YYCURSOR) < 4) YYFILL(4);

                        ^

ext/phar/phar_path_check.re: At top level:

The mbstring ones I have trouble understanding how the compiler figures out that it's unsigned as from my limit search it seems that in oniguruma those are *int

Finally the ~~VM and~~ Phar ones I don't have a clue how to fix.

Girgias · 2020-02-06T01:09:21Z

Azure pipelines also flags the following extensions:

Tidy:

In file included from /home/vsts/work/1/s/ext/tidy/tidy.c:30:0:
/usr/include/tidy/tidy.h:2095:13: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
 TIDY_EXPORT const ctmbstr TIDY_CALL TidyLangWindowsName( const tidyLocaleMapItem *item );
             ^~~~~
/usr/include/tidy/tidy.h:2101:13: warning: type qualifiers ignored on function return type [-Wignored-qualifiers]
 TIDY_EXPORT const ctmbstr TIDY_CALL TidyLangPosixName( const tidyLocaleMapItem *item );
             ^~~~~

And LDAP:

/home/vsts/work/1/s/ext/ldap/ldap.c: In function ‘_php_ldap_control_to_array’:
/home/vsts/work/1/s/ext/ldap/ldap.c:260:37: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]
    if ( context && (context->bv_len >= 0) ) {
                                     ^~

Edit fixed by: c3e65d9

nikic · 2020-02-06T10:58:57Z

The mbstring ones I have trouble understanding how the compiler figures out that it's unsigned as from my limit search it seems that in oniguruma those are *int

While the values coming from onig are ptrdiff_t, we store them in size_t. I'm not sure whether that's a bug, or whether these values are always non-negative...

Girgias · 2020-02-06T11:04:13Z

The mbstring ones I have trouble understanding how the compiler figures out that it's unsigned as from my limit search it seems that in oniguruma those are *int

While the values coming from onig are ptrdiff_t, we store them in size_t. I'm not sure whether that's a bug, or whether these values are always non-negative...

Well I'm imagining that the beginning part must be always greater than 0 otherwise it doesn't make sense? Or could there be a thing with negative offsets?

Girgias · 2020-02-10T10:57:30Z

I've locally enabled the -Wmissing-field-initializers compiler flag and there are way more than 7 but most (95%) of them are located in the mbstring extension.

https://gist.github.com/Girgias/273fe236e58d4f1e96e85700f55f568b

Girgias · 2020-02-27T01:08:50Z

So 32 bits has now these added warnings since the addition of Sodium:

/home/vsts/work/1/s/ext/sodium/libsodium.c: In function ‘zif_sodium_crypto_aead_aes256gcm_encrypt’:
/home/vsts/work/1/s/ext/sodium/libsodium.c:2025:35: warning: comparison is always false due to limited range of data type [-Wtype-limits]
 if ((unsigned long long) msg_len > (16ULL * ((1ULL << 32) - 2ULL)) - crypto_aead_aes256gcm_ABYTES) {
                                  ^
/home/vsts/work/1/s/ext/sodium/libsodium.c: In function ‘zif_sodium_crypto_aead_aes256gcm_decrypt’:
/home/vsts/work/1/s/ext/sodium/libsodium.c:2090:52: warning: comparison is always false due to limited range of data type [-Wtype-limits]
 if (ciphertext_len - crypto_aead_aes256gcm_ABYTES > 16ULL * ((1ULL << 32) - 2ULL)) {
                                                   ^
/home/vsts/work/1/s/ext/sodium/libsodium.c: In function ‘zif_sodium_crypto_aead_chacha20poly1305_ietf_encrypt’:
/home/vsts/work/1/s/ext/sodium/libsodium.c:2284:35: warning: comparison is always false due to limited range of data type [-Wtype-limits]
 if ((unsigned long long) msg_len > 64ULL * (1ULL << 32) - 64ULL) {
                                  ^

The one I'm a bit more concerned about is the MacOs/Clang warning: performing pointer arithmetic on a null pointer has undefined behaviour [-Wnull-pointer-arithmetic] which, from my understanding, is the same as the GCC -Wclobbered which is UB, which we probably should fix.

Girgias · 2020-02-27T01:26:58Z

Seems I missed some FFI warnings (only on ARM):

/home/travis/build/Girgias/php-src/ext/ffi/ffi.c: In function ‘zend_ffi_add_enum_val’:

/home/travis/build/Girgias/php-src/ext/ffi/ffi.c:5411:29: warning: comparison is always false due to limited range of data type [-Wtype-limits]

   if (!is_signed && val->ch < 0) {

                             ^

Fixed by: 69b7d01

Girgias · 2020-03-25T15:37:11Z

Sees a new warning got introduced in the JIT:

In file included from /home/travis/build/php/php-src/ext/opcache/jit/zend_jit.c:3242:0:

ext/opcache/jit/zend_jit_trace.c: In function ‘is_checked_guard’:

ext/opcache/jit/zend_jit_trace.c:779:11: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits]

   if (idx >= 0) {

           ^

/home/travis/build/php/php-src/ext/opcache/jit/zend_jit.c: At top level:

Edit: Fixed by 3e6667d

ext/phar/config.m4

nikic

LG assuming CI passes.

nikic · 2020-04-14T15:31:29Z

Zend/Zend.m4

@@ -221,7 +221,10 @@ else
  AC_DEFINE(ZEND_DEBUG,0,[ ])
 fi

-test -n "$GCC" && CFLAGS="$CFLAGS -Wall -Wno-strict-aliasing"
+test -n "$GCC" && CFLAGS="$CFLAGS -Wall -Wno-strict-aliasing -Wextra -Wno-implicit-fallthrough -Wno-unused-parameter -Wno-sign-compare"


Move -Wextra before -Wno-strict-aliasing.

nikic · 2020-04-14T15:31:41Z

Zend/Zend.m4

@@ -221,7 +221,10 @@ else
  AC_DEFINE(ZEND_DEBUG,0,[ ])
 fi

-test -n "$GCC" && CFLAGS="$CFLAGS -Wall -Wno-strict-aliasing"
+test -n "$GCC" && CFLAGS="$CFLAGS -Wall -Wno-strict-aliasing -Wextra -Wno-implicit-fallthrough -Wno-unused-parameter -Wno-sign-compare"
+dnl Check if compiler supports -Wn-clobbered (only GCC)


Suggested change

dnl Check if compiler supports -Wn-clobbered (only GCC)

dnl Check if compiler supports -Wno-clobbered (only GCC)

This is an issue in the provided library and needs to be fixed upstream.

This may happen on 32bits

The compile warnings which are explicitly suppressed are: * -Wno-implicit-fallthrough * -Wno-unused-parameter * -Wno-sign-compare * -Wno-clobbered, only with GCC

Girgias force-pushed the add-compiler-warnings branch from a7beb9f to 89216d1 Compare February 6, 2020 00:37

Girgias force-pushed the add-compiler-warnings branch 2 times, most recently from 47f95a3 to 2320191 Compare February 10, 2020 10:55

Girgias force-pushed the add-compiler-warnings branch from 2320191 to 8725f30 Compare February 21, 2020 12:25

This was referenced Feb 22, 2020

Change mbstring values from size_t to ptrdiff_t as that's what they return #5196

Closed

[-Wignored-qualifiers] warning in tidy htacg/tidy-html5#866

Closed

Girgias force-pushed the add-compiler-warnings branch 2 times, most recently from 9ef2fc5 to 8520e8c Compare February 27, 2020 00:51

Girgias force-pushed the add-compiler-warnings branch 6 times, most recently from 2f97307 to 98af091 Compare February 28, 2020 00:12

Girgias force-pushed the add-compiler-warnings branch from 98af091 to e63f3f7 Compare March 25, 2020 14:53

Girgias force-pushed the add-compiler-warnings branch from 6076440 to dd650ad Compare March 26, 2020 02:02

Girgias mentioned this pull request Mar 26, 2020

Explicitly declare the char as signed in zend_ffi_val. #5302

Closed

Girgias force-pushed the add-compiler-warnings branch from dd650ad to 8af5625 Compare March 26, 2020 13:10

This was referenced Mar 26, 2020

Fix for [-Wtype-limits] compiler warning on 32bits platforms in Sodium extension #5304

Closed

Fix [-Wtype-limit] compiler warning jedisct1/libsodium-php#202

Closed

Girgias force-pushed the add-compiler-warnings branch from 644944a to 1a48979 Compare March 26, 2020 20:28

nikic reviewed Mar 26, 2020

View reviewed changes

ext/phar/config.m4 Outdated Show resolved Hide resolved

Girgias mentioned this pull request Apr 12, 2020

Fixed compilation warnings on big endian system #5373

Closed

Girgias force-pushed the add-compiler-warnings branch 12 times, most recently from 107cca1 to efb655e Compare April 14, 2020 15:12

nikic approved these changes Apr 14, 2020

View reviewed changes

Girgias added 3 commits April 14, 2020 17:53

Add -Wno-ignored-qualifiers compiler flag to Tidy extension.

1b1882c

This is an issue in the provided library and needs to be fixed upstream.

Add -Wno-type-limits compiler flag to Sodium extension

43e2baf

This may happen on 32bits

Add -Wextra compiler warnings and exclude the trigger happy ones

8d3d741

The compile warnings which are explicitly suppressed are: * -Wno-implicit-fallthrough * -Wno-unused-parameter * -Wno-sign-compare * -Wno-clobbered, only with GCC

Girgias force-pushed the add-compiler-warnings branch from efb655e to 8d3d741 Compare April 14, 2020 15:54

php-pulls closed this in 1c334db Apr 14, 2020

Girgias deleted the add-compiler-warnings branch April 14, 2020 20:24

This was referenced Mar 14, 2023

use local variable for retval to avoid clobbering newrelic/newrelic-php-agent#640

Merged

fix(agent):Update logic needed to call PHP functions from the agent. newrelic/newrelic-php-agent#638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add -Wextra compiler flag exclude some of the major offender #5151

Add -Wextra compiler flag exclude some of the major offender #5151

Girgias commented Feb 5, 2020 •

edited

Loading

Girgias commented Feb 5, 2020 •

edited

Loading

Girgias commented Feb 5, 2020 •

edited

Loading

Girgias commented Feb 6, 2020 •

edited

Loading

nikic commented Feb 6, 2020

Girgias commented Feb 6, 2020

Girgias commented Feb 10, 2020

Girgias commented Feb 27, 2020 •

edited

Loading

Girgias commented Feb 27, 2020 •

edited

Loading

Girgias commented Mar 25, 2020 •

edited

Loading

nikic left a comment

nikic Apr 14, 2020

nikic Apr 14, 2020

	dnl Check if compiler supports -Wn-clobbered (only GCC)
	dnl Check if compiler supports -Wno-clobbered (only GCC)

Add -Wextra compiler flag exclude some of the major offender #5151

Add -Wextra compiler flag exclude some of the major offender #5151

Conversation

Girgias commented Feb 5, 2020 • edited Loading

Girgias commented Feb 5, 2020 • edited Loading

Girgias commented Feb 5, 2020 • edited Loading

Girgias commented Feb 6, 2020 • edited Loading

nikic commented Feb 6, 2020

Girgias commented Feb 6, 2020

Girgias commented Feb 10, 2020

Girgias commented Feb 27, 2020 • edited Loading

Girgias commented Feb 27, 2020 • edited Loading

Girgias commented Mar 25, 2020 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

nikic Apr 14, 2020

Choose a reason for hiding this comment

nikic Apr 14, 2020

Choose a reason for hiding this comment

Girgias commented Feb 5, 2020 •

edited

Loading

Girgias commented Feb 5, 2020 •

edited

Loading

Girgias commented Feb 5, 2020 •

edited

Loading

Girgias commented Feb 6, 2020 •

edited

Loading

Girgias commented Feb 27, 2020 •

edited

Loading

Girgias commented Feb 27, 2020 •

edited

Loading

Girgias commented Mar 25, 2020 •

edited

Loading