php / php-src

The PHP Interpreter
https://www.php.net
Other
38.22k stars 7.75k forks source link

Building litespeed sapi on riscv64 fails #7914

Open andypost opened 2 years ago

andypost commented 2 years ago

Description

Building with --enable-litespeed on risv64 fails with

/usr/lib/gcc/riscv64-alpine-linux-musl/10.3.1/../../../../riscv64-alpine-linux-musl/bin/ld: sapi/litespeed/.libs/lsapilib.o: in function `lsapi_close_connection':
/home/buildozer/aports/community/php7/src/php-7.4.20/sapi/litespeed/lsapilib.c:438: undefined reference to `__sync_lock_test_and_set_1'
/usr/lib/gcc/riscv64-alpine-linux-musl/10.3.1/../../../../riscv64-alpine-linux-musl/bin/ld: sapi/litespeed/.libs/lsapilib.o: in function `LSAPI_Prefork_Accept_r':
/home/buildozer/aports/community/php7/src/php-7.4.20/sapi/litespeed/lsapilib.c:3572: undefined reference to `__sync_lock_test_and_set_1'
/usr/lib/gcc/riscv64-alpine-linux-musl/10.3.1/../../../../riscv64-alpine-linux-musl/bin/ld: sapi/litespeed/.libs/lsapilib.o: in function `lsapi_prefork_server_accept':
/home/buildozer/aports/community/php7/src/php-7.4.20/sapi/litespeed/lsapilib.c:3157: undefined reference to `__sync_lock_test_and_set_1'
collect2: error: ld returned 1 exit status
make: *** [Makefile:404: sapi/litespeed/php] Error 1

Debug shows that it caused by usage __sync_lock_test_and_set_1 with constant, see https://gitlab.alpinelinux.org/alpine/aports/-/issues/12775#note_204733

Two solutions:

  1. Get PHP upstream to use something other than ё__sync_lock_test_and_setё if they need to store values other than the immediate constant 1 since GCC upstream explicitly documents that this is not supported on all architectures.
  2. Get GCC upstream to support __sync_lock_test_and_set with values other than a immediate constant 1 on the RISC-V architecture (if technically possible).

It fails for all supported versions 7.4+ but 8.0+ is priority

PHP Version

PHP 8.0+

Operating System

Alpinelinux

nmeum commented 2 years ago

Debug shows that it caused by usage __sync_lock_test_and_set_1 with constant

According to the GCC documentation, the problem likely is that some architecture only support __sync_lock_test_and_set_1 with reduced functionality, meaning:

[…] the only valid value to store is the immediate constant 1. The *exact value actually stored in `ptr` is implementation defined**.

The RISC-V architecture seems to be one of these architectures. For this reason, my understanding is that portable code should not use __sync_lock_test_and_set_1 with a store value other than the constant 1 (in general code using a different value seems to rely on implementation defined behavior). Unfortunately, the lsapilib.c file does presently use __sync_lock_test_and_set_1 with other store values in three places:

https://github.com/php/php-src/blob/7d5a9dd05da714917535571539487720280de839/sapi/litespeed/lsapilib.c#L445-L446

https://github.com/php/php-src/blob/7d5a9dd05da714917535571539487720280de839/sapi/litespeed/lsapilib.c#L1589-L1591

https://github.com/php/php-src/blob/7d5a9dd05da714917535571539487720280de839/sapi/litespeed/lsapilib.c#L3318-L3319

Since there is no builtin supporting this on the RISC-V architecture, this causes the linking error mentioned in the original post when PHP is compiled with --enable-litespeed. The three code parts linked above correspond to the three “undefined symbol“ linking errors.

andypost commented 2 years ago

As I see gcc suggests to replace this functions with __atomic ones but looking at sapi/fpm/fpm/fpm_atomic.h it has own implementations.

related clang docs https://llvm.org/docs/Atomics.html#libcalls-sync

EDIT related for pgsql https://postgrespro.ru/list/id/CA+hUKGL1m-zJeFvY_yOAJbNUh-aPR5fk_zHM=HuD6KVzYRhCsg@mail.gmail.com

andypost commented 2 years ago

History shows that all this __sync functions been added in 1e7f1b90e81

cmb69 commented 2 years ago

The primary maintainer of the litespeed SAPI is George Wang, but apparently he doesn't have a Github account yet.

andypost commented 2 years ago

btw patch works for master/8.2 as well

andypost commented 1 year ago

So it should be replaced with __atomic_store()

andypost commented 1 year ago

Btw grep shows that only sapi/fpm has check for this build-ins, probably it needs broader fix

php-src$ git grep '__sync_'
ext/opcache/jit/vtune/ittnotify_config.h:359:#define __TBB_machine_fetchadd4(addr, val) __sync_fetch_and_add(addr, val)
sapi/fpm/config.m4:292:  AC_MSG_CHECKING([if gcc supports __sync_bool_compare_and_swap])
sapi/fpm/config.m4:295:    return (__sync_bool_compare_and_swap(&variable, 1, 2)
sapi/fpm/config.m4:296:           && __sync_add_and_fetch(&variable, 1)) ? 1 : 0;
sapi/fpm/config.m4:299:    AC_DEFINE(HAVE_BUILTIN_ATOMIC, 1, [Define to 1 if gcc supports __sync_bool_compare_and_swap() a.o.])
sapi/fpm/fpm/fpm_atomic.h:19:#define atomic_cmp_set(a,b,c) __sync_bool_compare_and_swap(a,b,c)
sapi/fpm/fpm/fpm_atomic.h:86:#define atomic_cmp_set(a,b,c) __sync_bool_compare_and_swap(a,b,c)
sapi/litespeed/lsapilib.c:444:        __sync_fetch_and_sub(s_busy_workers, 1);
sapi/litespeed/lsapilib.c:1593:                        __sync_fetch_and_add(s_busy_workers, 1);
sapi/litespeed/lsapilib.c:2921:        __sync_lock_release(s_busy_workers);
sapi/litespeed/lsapilib.c:2923:        __sync_lock_release(s_accepting_workers);
sapi/litespeed/lsapilib.c:2967:            if (__sync_bool_compare_and_swap(&child_status->m_state,
sapi/litespeed/lsapilib.c:2972:                    __sync_fetch_and_sub(s_busy_workers, 1);
sapi/litespeed/lsapilib.c:2974:            else if (__sync_bool_compare_and_swap(&child_status->m_state,
sapi/litespeed/lsapilib.c:2979:                    __sync_fetch_and_sub(s_accepting_workers, 1);
sapi/litespeed/lsapilib.c:3252:                accepting = __sync_add_and_fetch(s_accepting_workers, 0);
sapi/litespeed/lsapilib.c:3321:                    __sync_add_and_fetch(s_busy_workers, 1);
sapi/litespeed/lsapilib.c:3408:        __sync_add_and_fetch(s_busy_workers, 1);
sapi/litespeed/lsapilib.c:3525:                accepting = __sync_add_and_fetch(s_accepting_workers, 0);
sapi/litespeed/lsapilib.c:3610:        ret = __sync_fetch_and_add(s_busy_workers, 0);
sapi/litespeed/lsapilib.c:3657:                    __sync_fetch_and_add(s_accepting_workers, 1);
sapi/litespeed/lsapilib.c:3663:                    __sync_fetch_and_sub(s_accepting_workers, 1);
sapi/litespeed/lsapilib.c:3717:                            __sync_fetch_and_add(s_busy_workers, 1);
sapi/litespeed/lsapilib.c:4388:    return __sync_add_and_fetch(s_global_counter, cnt);