Skip to content

Commit 067df26

Browse files
committedNov 24, 2021
Use memrchr() when available
On x86_64 glibc memrchr() uses SSE/AVX CPU extensions and works much faster then naive loop. On x86 32-bit we still use inlined version. memrchr() is a GNU extension. Its prototype becomes available when <string.h> is included with defined _GNU_SOURCE macro. Previously, we defined it in "php_config.h", but some sources may include <string.h> befire it. To avod mess we also pass -D_GNU_SOURCE to C compiler.
1 parent d312a0c commit 067df26

File tree

11 files changed

+39
-9
lines changed

11 files changed

+39
-9
lines changed
 

β€ŽZend/zend_operators.h

+5
Original file line numberDiff line numberDiff line change
@@ -211,6 +211,10 @@ zend_memnstr(const char *haystack, const char *needle, size_t needle_len, const
211211

212212
static zend_always_inline const void *zend_memrchr(const void *s, int c, size_t n)
213213
{
214+
#if defined(HAVE_MEMRCHR) && !defined(i386)
215+
/* On x86 memrchr() doesn't use SSE/AVX, so inlined version is faster */
216+
return (const void*)memrchr(s, c, n);
217+
#else
214218
const unsigned char *e;
215219
if (0 == n) {
216220
return NULL;
@@ -222,6 +226,7 @@ static zend_always_inline const void *zend_memrchr(const void *s, int c, size_t
222226
}
223227
}
224228
return NULL;
229+
#endif
225230
}
226231

227232

β€ŽZend/zend_signal.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@
2525
All other licensing and usage conditions are those of the PHP Group.
2626
*/
2727

28-
#define _GNU_SOURCE
28+
#ifndef _GNU_SOURCE
29+
# define _GNU_SOURCE
30+
#endif
2931
#include <string.h>
3032

3133
#include "zend.h"

β€Žconfigure.ac

+7
Original file line numberDiff line numberDiff line change
@@ -265,6 +265,12 @@ else
265265
AC_MSG_RESULT(no)
266266
fi
267267

268+
dnl The effect of _GNU_SOURCE defined in config.h depeds on includes order
269+
if test "$ac_cv_safe_to_define___extensions__" = yes ; then
270+
AC_MSG_CHECKING(whether to use -D_GNU_SOURCE cflag)
271+
CPPFLAGS="$CPPFLAGS -D_GNU_SOURCE"
272+
AC_MSG_RESULT(yes)
273+
fi
268274

269275
dnl Include Zend configurations.
270276
dnl ----------------------------------------------------------------------------
@@ -609,6 +615,7 @@ vasprintf \
609615
asprintf \
610616
nanosleep \
611617
memmem \
618+
memrchr \
612619
)
613620

614621
AX_FUNC_WHICH_GETHOSTBYNAME_R

β€Žext/pdo_firebird/firebird_driver.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818
#include "config.h"
1919
#endif
2020

21-
#define _GNU_SOURCE
21+
#ifndef _GNU_SOURCE
22+
# define _GNU_SOURCE
23+
#endif
2224

2325
#include "php.h"
2426
#include "zend_exceptions.h"

β€Žext/zlib/zlib_fopen_wrapper.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@
1515
+----------------------------------------------------------------------+
1616
*/
1717

18-
#define _GNU_SOURCE
18+
#ifndef _GNU_SOURCE
19+
# define _GNU_SOURCE
20+
#endif
1921

2022
#include "php.h"
2123
#include "php_zlib.h"

β€Žmain/snprintf.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@
1414
+----------------------------------------------------------------------+
1515
*/
1616

17-
#define _GNU_SOURCE
17+
#ifndef _GNU_SOURCE
18+
# define _GNU_SOURCE
19+
#endif
1820
#include "php.h"
1921

2022
#include <zend_strtod.h>

β€Žmain/spprintf.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,9 @@
7272
* SIO stdio-replacement strx_* functions by Panos Tsirigotis
7373
* <panos@alumni.cs.colorado.edu> for xinetd.
7474
*/
75-
#define _GNU_SOURCE
75+
#ifndef _GNU_SOURCE
76+
# define _GNU_SOURCE
77+
#endif
7678
#include "php.h"
7779

7880
#include <stddef.h>

β€Žmain/streams/cast.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@
1414
+----------------------------------------------------------------------+
1515
*/
1616

17-
#define _GNU_SOURCE
17+
#ifndef _GNU_SOURCE
18+
# define _GNU_SOURCE
19+
#endif
1820
#include "php.h"
1921
#include "php_globals.h"
2022
#include "php_network.h"

β€Žmain/streams/memory.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,9 @@
1414
+----------------------------------------------------------------------+
1515
*/
1616

17-
#define _GNU_SOURCE
17+
#ifndef _GNU_SOURCE
18+
# define _GNU_SOURCE
19+
#endif
1820
#include "php.h"
1921
#include "ext/standard/base64.h"
2022

β€Žmain/streams/streams.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@
1717
+----------------------------------------------------------------------+
1818
*/
1919

20-
#define _GNU_SOURCE
20+
#ifndef _GNU_SOURCE
21+
# define _GNU_SOURCE
22+
#endif
2123
#include "php.h"
2224
#include "php_globals.h"
2325
#include "php_memory_streams.h"

β€Žsapi/fpm/fpm/fpm_trace_pread.c

+3-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
/* (c) 2007,2008 Andrei Nigmatulin */
22

3-
#define _GNU_SOURCE
3+
#ifndef _GNU_SOURCE
4+
# define _GNU_SOURCE
5+
#endif
46
#define _FILE_OFFSET_BITS 64
57

68
#include "fpm_config.h"

8 commit comments

Comments
 (8)

jmikola commented on Dec 8, 2021

@jmikola
Member

@dstogov: I was testing ext-mongodb against PHP's master branch and ran into several "implicit declaration" warnings for memrchr, which lead me to this commit. Does this impact third-party extensions that may also include <string.h> (either directly or through other dependencies)? If so, does this warrant a note in UPGRADING.INTERNALS?

Adding -D_GNU_SOURCE to our CFLAGS in config.m4 seemed to do the trick. Not sure about config.w32 (or if this is even relevant there), but I'd plan to look into that down the line.

dstogov commented on Dec 8, 2021

@dstogov
MemberAuthor

@jmikola sorry for troubles. You already explained almost everything yourself.
Prototype of memrchr is defined in <string.h>, but it's visible only if <string.h> included after _GNU_SOURCE is defined.
Actually, _GNU_SOURCE is defined in config.h but some sources include <string.h> before config.h.
We fixed this in main PHP source tree by adding -D_GNU_SOURCE into CFLAGS, but I didn't think about third-party extensions.
Probably, we will need to add -D_GNU_SOURCE through phpize. I'll try to take a look.

Windows build shouldn't be affected.

jmikola commented on Feb 15, 2022

@jmikola
Member

@dstogov: I was just reminded about this comment chain while sorting through my GitHub notifications.

Probably, we will need to add -D_GNU_SOURCE through phpize. I'll try to take a look.

Is this still outstanding and/or being tracked anywhere? I haven't compiled ext-mongodb against the master branch since I first reported this, but figured there was no harm in asking.

derickr commented on Jun 16, 2022

@derickr
Member

@dstogov I am running into this with Xdebug on PHP 8.2 as well now, and forcing to add -D_GNU_SOURCE in my config.m4 does the trick, but of course meaningless for platforms without GNU on it, although it doesn't hurt. The following patch makes it work for me on Linux at least, and I can make a PR so that everything on CI checks this (if phpize is checked at all somehow):

diff --git scripts/phpize.m4 scripts/phpize.m4
index 1f0cbd70eb..2d2dcb2e6c 100644
--- scripts/phpize.m4
+++ scripts/phpize.m4
@@ -165,6 +165,7 @@ install_targets="install-modules install-headers"
 phplibdir="`pwd`/modules"
 CPPFLAGS="$CPPFLAGS -DHAVE_CONFIG_H"
 CFLAGS_CLEAN='$(CFLAGS)'
+CFLAGS="$(CFLAGS) -D_GNU_SOURCE"
 CXXFLAGS_CLEAN='$(CXXFLAGS)'
 
 test "$prefix" = "NONE" && prefix="/usr/local"

derickr commented on Jun 16, 2022

@derickr
Member

I've made a PR for this now: #8807

derickr commented on Jun 17, 2022

@derickr
Member

And FWIW @jmikola , this PR is now merged.

jmikola commented on Jul 14, 2023

@jmikola
Member

@dstogov: Hello again. Fancy coincidence coming back to this issue after all this time.

The MongoDB driver recently received an issue (mongodb/mongo-php-driver#1445) reporting a build failure with PHP 8.2 and Alpine Linux, which is due to one of our dependencies failing to detect the version of strerror_r(3) being used (XSI or GNU). The user tested a wide span of PHP versions and their research lead them back to the configure.ac changes in this commit, which added -D_GNU_SOURCE to PHP's build options. They also reported that the behavior was identical when building ext-mongodb on its own or as a static extension, which may be thanks to @derickr's previous phpize PR.

Anyway, we're tracking the issue in CDRIVER-4679 and will get it sorted out.

The main reason I'm commenting here is to ask whether PHP might have a similar issue with strerror_r() compatibility. A code search revealed that is called once in zend_strerror_noreturn(). PHP ignores the return value, which makes it immune to the int-conversion error we experienced, but I think it may not be handling an edge case in the GNU variation. Consider the following from the man page description (emphasis mine):

The XSI-compliant strerror_r() is preferred for portable applications. It returns the error string in the user-supplied buffer buf of length buflen.

The GNU-specific strerror_r() returns a pointer to a string containing the error message. This may be either a pointer to a string that the function stores in buf, or a pointer to some (immutable) static string (in which case buf is unused). If the function stores a string in buf, then at most buflen bytes are stored (the string may be truncated if buflen is too small and errnum is unknown). The string always includes a terminating null byte.

If the GNU variant of strerror_r() returns a pointer to a static string, buf is left empty and the returned message will be incomplete. This wouldn't manifest as a build error, but it seems like it could be a subtle bug. I'm not familiar with how zend_strerror_noreturn() is used but I wanted to bring this to your attention.

dstogov commented on Jul 24, 2023

@dstogov
MemberAuthor

@jmikola I supose the problem should be fixed by #11624

Please sign in to comment.