On riscv64, only require libatomic if actually needed

[jcourreges]

clang and newer gcc releases support byte-sized atomic accesses on riscv64 through inline builtins. In both cases the hard dependency on libatomic added by GH-11321 isn't useful.

Stop using AC_CHECK_LIB() which is too naive to notice that libatomic isn't needed. Instead, PHP_CHECK_FUNC() will retry the check with -latomic if required.

[jcourreges]

PR against the PHP-8.1 branch as the problem didn't appear earlier. It would be nice to have this merged in 8.2 and master.

This has been tested on OpenBSD/riscv64, using clang (libatomic not needed) and gcc-8.4.0 (libatomic needed). The patch is currently used by the OpenBSD ports tree.

[jcourreges]

@danog @devnexen Hi! Since you participated in #11321 you may want to comment on this PR and test on your favorite OS.

[devnexen]

Your solution makes sense to me in that case, but let's see what other people think.

[danog]

LGTM, builds fine on my starfive with GCC 12!

[petk]

Hello, thanks @jcourreges for finding this. Sure, we can add this to PHP-8.1 and above.

Otherwise, the AC_SEARCH_LIBS would be a bit better for this fix instead of the PHP_CHECK_FUNC. If it doesn't prepend the library to LIBS.

With PHP_CHECK_FUNC there will be two new constants defined in main/php_config.h when linking the atomic library is needed or not:

• HAVE_LIBATOMIC
• HAVE___ATOMIC_EXCHANGE_1

Probably these won't interfere with anything out there, but just in case let's try something like this first and then use the PHP_CHECK_FUNC if not possible:

case $host_alias in
  riscv64*)
    AC_SEARCH_LIBS([__atomic_exchange_1],[atomic],[],
      [AC_MSG_ERROR([Problem with enabling atomic. Please check config.log for details.])]
    )
    ;;
esac

[jcourreges]

Hello, thanks @jcourreges for finding this. Sure, we can add this to PHP-8.1 and above.

Otherwise, the AC_SEARCH_LIBS would be a bit better for this fix instead of the PHP_CHECK_FUNC. If it doesn't prepend the library to LIBS.

Quoting the documentation for AC_SEARCH_LIBS() in autoconf-2.69:

 Prepend `-lLIBRARY' to `LIBS' for the first library found to
 contain FUNCTION, and run ACTION-IF-FOUND.  If the function is not
 found, run ACTION-IF-NOT-FOUND.

PHP_ADD_LIBRARY() also modified LIBS before the change I'm proposing, and PHP_CHECK_FUNC() will also call PHP_ADD_LIBRARY() under the hood (if needed). That's desired I think, else I don't see how -latomic would end up into the linker flags. Also, another reason for using it was consistency with the rest of the surrounding checks.

[petk]

@jcourreges from what I've tested, AC_SEARCH_LIBS doesn't prepend the atomic to the LIBS if the program with function compiles in the first run without a library. If the compilation fails without linking library then it tries linking it and if that works, it prepends it to the LIBS.

http://git.savannah.gnu.org/cgit/autoconf.git/tree/lib/autoconf/libs.m4#n52

for ac_lib in '' $2
do
  if test -z "$ac_lib"; then
    ac_res="none required"
  else
    ac_res=-l$ac_lib
    LIBS="-l$ac_lib $5 $ac_func_search_save_LIBS"
  fi
  AC_LINK_IFELSE([], [AS_VAR_SET([ac_Search], [$ac_res])])
  AS_VAR_SET_IF([ac_Search], [break])
done

I've tested it very quickly though. There were otherwise some bugs in the past with this macro, but were very specific to the way the program needed to be compiled together. This one should most likely work ok.

Regarding consistency, this I believe is more consistent. It's in the Autoconf itself. PHP macro defines two more constants otherwise. Well, your call.

P.S.: Plus what is also important, the compilation vs. running in case of cross-compilation. AC_SEARCH_LIBS will only compile and not run the program with the function. Running the program in cross-compilation (with PHP_CHECK_FUNC) will be skipped and default value of "yes" will be done which means that library will be prepended in that case, which is probably not ok.

[jcourreges]

Hi.

@jcourreges from what I've tested, AC_SEARCH_LIBS doesn't prepend the atomic to the LIBS if the program with function compiles in the first run without a library. If the compilation fails without linking library then it tries linking it and if that works, it prepends it to the LIBS.

Correct, we do agree on this. So both alternatives (PHP_CHECK_FUNC() and AC_SEARCH_LIBS()) behave similarly regarding to LIBS.

Regarding consistency, this I believe is more consistent. It's in the Autoconf itself. PHP macro defines two more constants otherwise.

I find it strange that you find using AC_SEARCH_LIBS() more consistent, when it isn't used anywhere else in configure.ac. The other checks use PHP_CHECK_FUNC(). My gut feeling is that the PHP developers implemented interfaces like PHP_CHECK_FUNC() for valid reasons.

Well, your call.

Not my call: I'm not a PHP developer and I've seldom contributed to this repo in the past. So you folks get to choose what's best in the end.

P.S.: Plus what is also important, the compilation vs. running in case of cross-compilation. AC_SEARCH_LIBS will only compile and not run the program with the function. Running the program in cross-compilation (with PHP_CHECK_FUNC) will be skipped and default value of "yes" will be done which means that library will be prepended in that case, which is probably not ok.

I may be misunderstanding, but PHP_CHECK_FUNC_LIB() doesn't blindly assume that it should link with the candidate library in cross-compilation mode. PHP_CHECK_FUNC_LIB() is only used if the AC_CHECK_LIB() call in PHP_CHECK_FUNC() failed to find the function. So if the function is found with no additional library, the check succeeds (rightfully so) whether or not cross-compilation is done.

Looking at this code further...

AC_DEFUN([PHP_CHECK_FUNC_LIB],[
  ifelse($2,,:,[
  unset ac_cv_lib_$2[]_$1
  unset ac_cv_lib_$2[]___$1
  unset found
  AC_CHECK_LIB($2, $1, [found=yes], [
    AC_CHECK_LIB($2, __$1, [found=yes], [found=no])
  ])

  if test "$found" = "yes"; then
    ac_libs=$LIBS
    LIBS="$LIBS -l$2"
    AC_RUN_IFELSE([AC_LANG_SOURCE([[int main(void) { return (0); }]])],[found=yes],[found=no],[
      dnl Cross compilation.
      found=yes
    ])
    LIBS=$ac_libs
  fi

  if test "$found" = "yes"; then
    PHP_ADD_LIBRARY($2)
    PHP_DEF_HAVE($1)
    PHP_DEF_HAVE(lib$2)
    ac_cv_func_$1=yes
  else
    PHP_CHECK_FUNC_LIB($1,phpshift(phpshift($@)))
  fi
  ])
])

In native compilation, an additional AC_RUN_IFELSE() on a dummy program is performed, which further validates that the candidate library is usable. In cross-compilation mode, no program is run, it is assumed that the library can be used. This looks ok to me.

Does this address your concerns?

[petk]

Yup, sure it addresses my concerns. Thanks for explanations @jcourreges

Your call, since it fixes a bug you're having directly. Otherwise, now there also won't be error thrown in case the check fails. But since this is very limited and specific case all good then. My look on these PHP macros is a bit different though - they do too much for too many cases and they were written for very old Autoconf versions when Autoconf macro sets were more limited. But all good. Don't mind it. This will work completely fine. Ready for PHP-8.1 and above then.

[jcourreges]

Hi,

thank you @petk for the approval.

Otherwise, now there also won't be error thrown in case the check fails. But since this is very limited and specific case all good then.

That deserves an explanation indeed. The function we're checking for is an implementation detail. Erroring out would break ./configure for compilers that don't implement those builtins; even in situations where Zend/zend_atomic.h would pick up another backend. To further shield us of that problem, the check for __atomic_exchange_1 could be restricted to GCC since AFAIK clang isn't affected. On the other hand, other architectures may be affected by the same issue. This PR doesn't try to fix everything, it only attempts to make the current code less annoying.

I'm not used to the PHP development process and CONTRIBUTING.md doesn't mention this: does "RM" in "Waiting on RM" stand for "Release Management" or something? Do I need to do anything else to get those changes in an upcoming PHP release?

Thank you for your support so far on this PR.

[petk]

Merged to PHP-8.1+ Yes, the Waiting for RM label is intended that release manager merges the changes to their branch but we'll just add this like this then.