Add some compiler flags which aren't included in -Wall or -Wextra #6199

Girgias · 2020-09-23T23:48:50Z

This adds -Wduplicated-cond -Wduplicated-branches -Wlogical-op -Wformat-truncation and -fno-common compiler flags.

-Wlogical-op did seem to find some legit bugs.

EDIT: Need to figure out a way to get around Windows for the GCC pragma

Girgias · 2020-09-24T12:45:48Z

@dstogov could you just confirm that the changes in FFI, SOAP and the JIT are correct as some seem to be bugs and IIRC you are the maintainer of FFI and SOAP.

nikic · 2020-09-24T13:59:03Z

I've moved this to the 8.1 milestone, to avoid additional disruption for PECL extensions this late.

dstogov · 2020-09-30T04:56:58Z

ext/ffi/ffi_parser.c

-		} else if (sym == YY___CONST) {
-			sym = get_sym();
-		} else {
+		} else { /* sym == YY_CONST || sym == YY___CONST || something else */


This file is auto generated. It shouldn't be edited directly.

dstogov · 2020-09-30T05:02:00Z

@dstogov could you just confirm that the changes in FFI, SOAP and the JIT are correct as some seem to be bugs and IIRC you are the maintainer of FFI and SOAP.

FFI, SOAP and JIT looks proper except direct ext/ffi/ffi_parser.c update.

I'll merge these fixes.

dstogov · 2020-09-30T05:41:12Z

@dstogov could you just confirm that the changes in FFI, SOAP and the JIT are correct as some seem to be bugs and IIRC you are the maintainer of FFI and SOAP.

@dstogov could you just confirm that the changes in FFI, SOAP and the JIT are correct as some seem to be bugs and IIRC you are the maintainer of FFI and SOAP.

FFI, SOAP and JIT looks proper except direct ext/ffi/ffi_parser.c update.

I'll merge these fixes.

FFI, SOAP and JIT fixes are merged into PHP-7.3 and above.
Thanks for catching this.

twose · 2020-09-30T15:12:46Z

How about this way:

#if defined(EWOULDBLOCK) && EAGAIN != EWOULDBLOCK
#define IS_TRANSIENT_ERROR(err) (err == EAGAIN || err == EWOULDBLOCK)
#else
#define IS_TRANSIENT_ERROR(err) (err == EAGAIN)
#endif

twose · 2020-09-30T15:17:45Z

I found the same solution in libevent, but libuv not
https://github.com/libevent/libevent/blob/master/util-internal.h#L110

and it looks it's unnecessary to check if EWOULDBLOCK was defined

twose · 2020-10-01T18:43:04Z

Zend/zend_portability.h

+/* This is a work around for GCC bug 69602: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69602 */
+#if defined(EWOULDBLOCK) && EAGAIN != EWOULDBLOCK
+# define IS_TRANSIENT_ERROR(err) (err == EAGAIN || err == EWOULDBLOCK)
+#else
+# define IS_TRANSIENT_ERROR(err) (err == EAGAIN)
+#endif
+


Maybe you could put the code here, it also works on ext/sockets

php-src/main/php_network.h

Lines 35 to 50 in 62c6d69

#ifdef PHP_WIN32

# ifdef EWOULDBLOCK

# undef EWOULDBLOCK

# endif

# ifdef EINPROGRESS

# undef EINPROGRESS

# endif

# define EWOULDBLOCK WSAEWOULDBLOCK

# define EINPROGRESS WSAEWOULDBLOCK

# define fsync _commit

# define ftruncate(a, b) chsize(a, b)

#endif /* defined(PHP_WIN32) */

#ifndef EWOULDBLOCK

# define EWOULDBLOCK EAGAIN

#endif

BTW, the name is inspired by libuv, it defined on the C source file instead of a header file, so it's better to add a prefix on it, or named like ZEND_IS_EAGAIN (I am not sure, naming is not my strength...)

nikic · 2020-10-09T15:28:07Z

ext/imap/php_imap.c

@@ -2746,7 +2746,7 @@ PHP_FUNCTION(imap_sort)
 	}

 	array_init(return_value);
-	if (slst != NIL && slst != 0) {
+	if (slst != 0) {


This should either be != NIL or != NULL, as it is a pointer.

nikic · 2020-10-09T15:28:18Z

Zend/Zend.m4

@@ -212,9 +212,12 @@ else
  AC_DEFINE(ZEND_DEBUG,0,[ ])
 fi

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


Why the -fno-common here?

ext/mbstring/libmbfl/filters/mbfilter_euc_jp_win.c

ext/mysqlnd/mysqlnd_ps.c

nikic · 2020-10-09T15:30:25Z

main/php_network.h

+#if EAGAIN != EWOULDBLOCK
+# define IS_TRANSIENT_ERROR(err) (err == EAGAIN || err == EWOULDBLOCK)
+#else
+# define IS_TRANSIENT_ERROR(err) (err == EAGAIN)


Add a PHP_ prefix?

sapi/cli/php_cli_server.c

nikic · 2020-10-10T09:03:04Z

azure/configure.yml

@@ -61,7 +61,6 @@ steps:
        --with-imap \
        --with-kerberos \
        --with-imap-ssl \
-        --enable-werror \


Drop these now, or is there still something left?

There is one remaining failure on 32bits:

/home/vsts/work/1/s/ext/sodium/libsodium.c: In function ‘zif_sodium_crypto_secretstream_xchacha20poly1305_push’: /home/vsts/work/1/s/ext/sodium/libsodium.c:3181:71: warning: logical ‘or’ of equal expressions [-Wlogical-op] 3181 | if (msg_len > crypto_secretstream_xchacha20poly1305_MESSAGEBYTES_MAX || |

ext/imap/php_imap.c

main/php_network.h

nikic

LG assuming CI happy.

Adds the following compiler flags: . -Wduplicated-cond . -Wlogical-op . -Wformat-truncation . -fno-common

Girgias force-pushed the add-sensible-compiler-flags branch from 50bb316 to ae48ad5 Compare September 23, 2020 23:49

Girgias mentioned this pull request Sep 24, 2020

Extensions should have the final say on their compiler flags #6204

Closed

nikic added this to the PHP 8.1 milestone Sep 24, 2020

Girgias force-pushed the add-sensible-compiler-flags branch from 3e5e0a4 to e93f8d4 Compare September 24, 2020 13:24

dstogov reviewed Sep 30, 2020

View reviewed changes

Girgias force-pushed the add-sensible-compiler-flags branch 5 times, most recently from 69cf664 to 2e37b20 Compare September 30, 2020 14:20

Girgias force-pushed the add-sensible-compiler-flags branch 2 times, most recently from 660c2df to 89763bb Compare October 1, 2020 18:04

twose reviewed Oct 1, 2020

View reviewed changes

Girgias force-pushed the add-sensible-compiler-flags branch 2 times, most recently from 1682685 to 5a18e29 Compare October 2, 2020 17:47

nikic reviewed Oct 9, 2020

View reviewed changes

Girgias force-pushed the add-sensible-compiler-flags branch from 5a18e29 to d527a00 Compare October 9, 2020 20:18

nikic reviewed Oct 10, 2020

View reviewed changes

Girgias force-pushed the add-sensible-compiler-flags branch from d527a00 to fc67f11 Compare October 10, 2020 13:57

nikic approved these changes Oct 10, 2020

View reviewed changes

Girgias added 2 commits October 10, 2020 16:09

Add -Wno-logical-op compiler flag to Sodium extension

a4c5b26

Add compiler flags which aren't included in -Wall or -Wextra

ab5b59f

Adds the following compiler flags: . -Wduplicated-cond . -Wlogical-op . -Wformat-truncation . -fno-common

Girgias force-pushed the add-sensible-compiler-flags branch from fc67f11 to ab5b59f Compare October 10, 2020 15:09

php-pulls closed this in 49783e3 Oct 10, 2020

Girgias deleted the add-sensible-compiler-flags branch October 10, 2020 16:27

	#ifdef PHP_WIN32
	# ifdef EWOULDBLOCK
	# undef EWOULDBLOCK
	# endif
	# ifdef EINPROGRESS
	# undef EINPROGRESS
	# endif
	# define EWOULDBLOCK WSAEWOULDBLOCK
	# define EINPROGRESS WSAEWOULDBLOCK
	# define fsync _commit
	# define ftruncate(a, b) chsize(a, b)
	#endif /* defined(PHP_WIN32) */

	#ifndef EWOULDBLOCK
	# define EWOULDBLOCK EAGAIN
	#endif

Add some compiler flags which aren't included in -Wall or -Wextra #6199

Add some compiler flags which aren't included in -Wall or -Wextra #6199

Uh oh!

Conversation

Girgias commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Girgias commented Sep 24, 2020

Uh oh!

nikic commented Sep 24, 2020

Uh oh!

dstogov Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

dstogov commented Sep 30, 2020

Uh oh!

dstogov commented Sep 30, 2020

Uh oh!

twose commented Sep 30, 2020

Uh oh!

twose commented Sep 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twose Oct 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nikic Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

nikic Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nikic Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikic Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

Girgias Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nikic left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Girgias commented Sep 23, 2020 •

edited

Loading

twose commented Sep 30, 2020 •

edited

Loading

twose Oct 1, 2020 •

edited

Loading