Bug 242680

Summary: devel/subversion Fails to build on FreeBSD 12.1-RELEASE-p1 32 bit: libapr-1.so: undefined reference to `__sync_*
Product: Ports & Packages Reporter: canardo <canardo909>
Component: Individual Port(s)Assignee: Piotr Kubaj <pkubaj>
Status: Closed FIXED    
Severity: Affects Some People CC: apache, jhibbits, joneum, lev, lfmorrison, pkubaj, powerpc
Priority: --- Keywords: needs-qa
Version: LatestFlags: bugzilla: maintainer-feedback? (apache)
koobs: maintainer-feedback? (lev)
koobs: merge-quarterly?
Hardware: powerpc   
OS: Any   
Attachments:
Description Flags
log none

Description canardo 2019-12-17 10:30:40 UTC
Problem found when building devel/subversion on FreeBSD 12.1-RELEASE-p1 powerpc 32 bit.


# cd /usr/ports/devel/subversion
# make install clean

===>  Building for subversion-1.13.0
cd subversion/tests/afl && /bin/sh "/usr/ports/devel/subversion/work/subversion-1.13.0/libtool" --tag=CC --silent --mode=link cc  -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing    -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing   -fstack-protector-strong   -L/usr/local/lib -L/usr/local/lib/db5 -L/usr/local/lib   -L/usr/local/lib  -rpath /usr/local/lib  -o afl-x509  afl-x509.lo ../../../subversion/libsvn_subr/libsvn_subr-1.la -L/usr/local/lib -lapr-1 -lintl
/usr/local/lib/libapr-1.so: undefined reference to `__sync_val_compare_and_swap_8'
/usr/local/lib/libapr-1.so: undefined reference to `__sync_lock_test_and_set_8'
/usr/local/lib/libapr-1.so: undefined reference to `__sync_sub_and_fetch_8'
/usr/local/lib/libapr-1.so: undefined reference to `__sync_fetch_and_sub_8'
/usr/local/lib/libapr-1.so: undefined reference to `__sync_fetch_and_add_8'
*** [subversion/tests/afl/afl-x509] Error code 1

make[2]: stopped in /usr/ports/devel/subversion/work/subversion-1.13.0
1 error

make[2]: stopped in /usr/ports/devel/subversion/work/subversion-1.13.0
===> Compilation failed unexpectedly.
Try to set MAKE_JOBS_UNSAFE=yes and rebuild before reporting the failure to
the maintainer.
*** Error code 1

Stop.
make[1]: stopped in /usr/ports/devel/subversion
*** Error code 1

Stop.
make: stopped in /usr/ports/devel/subversion



NB: subversion is a requirement to build devel/git
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2019-12-20 04:58:37 UTC
*** Bug 242698 has been marked as a duplicate of this bug. ***
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2019-12-20 05:02:38 UTC
^Triage: This (and bug 242680) looks like a devel/apr2 (libapr) issue, re-assign accordingly
Comment 3 lfmorrison 2020-01-03 17:35:17 UTC
These missing symbols appear to be associated with GCC's extensions for atomic data handling.

See, for example:
https://gcc.gnu.org/onlinedocs/gcc-4.2.0/gcc/Atomic-Builtins.html (base GCC 4.2 specification)

https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/_005f_005fsync-Builtins.html#g_t_005f_005fsync-Builtins (deprecated GCC9 feature)

https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/_005f_005fatomic-Builtins.html#g_t_005f_005fatomic-Builtins (currently supported GCC9 equivalent)

It is possible that linkage to GCC's libatomic might be needed in order to resolve these symbols? (I know that libatomic.so is installed as part of the GCC9 port for powerpc, but I don't know if there's an equivalent library supplied with the base compiler.)
Comment 4 Piotr Kubaj freebsd_committer freebsd_triage 2020-01-03 22:47:53 UTC
(In reply to lfmorrison from comment #3)
You could try building on head, it may already be fixed there after https://svnweb.freebsd.org/base?view=revision&revision=356308
Comment 5 lfmorrison 2020-01-04 10:56:38 UTC
(In reply to Piotr Kubaj from comment #4)
That looks promising, but I am not in a position to try switching away from stable right now. It also looks like that commit deliberately limited its scope to only the kernel for the time being, with effort in the future to make it visible to userspace.

In any event, apr is also capable of providing its own workarounds for working atomically if compiler built-ins are not available. A couple different approaches are available, ranging from full userspace emulation of atomics using POSIX mutexes, to some CPU-specific inline assembly to achieve atomic operations; for powerpc, it is capable of providing a hybrid approach, using inline assembly for 32-bit atomics and falling back to its POSIX mutex emulation for 64-bit.

Unfortunately, apr's configure script, as it is currently written, doesn't know how to deal with a situation in which the compiler provides 32-bit atomic built-ins, but it doesn't provide corresponding 64-bit built-ins: It just tests for the usability and correct behavior of the built-in using a host-specific "int", and if that works then it assumes that corresponding built-ins will be available for every integer type. That's not currently the case for powerpc.

I can see a few different ways forward:
1) It ought to be possible to force apr to always use mutex atomic emulation all the time by defining "-DUSE_ATOMICS_GENERIC" in CFLAGS. This would fix atomic64, but it would also impose unnecessary performance penalties on atomic32.

2) It also ought to be possible to override the configure script's automatic determination that compiler built-ins are available, using "-UHAVE_ATOMIC_BUILTINS" in CFLAGS. For powerpc, if gnu compiler extensions are enabled, then this ought to result in an inline assembly atomic32 and a mutex emulated atomic64. Probably a fair compromise.

3) I'd really like to make the configure script a little more intelligent, and give it the ability to independently detect the presence of atomic32 versus atomic64 built-ins, and allow the use of built-in atomic32 together with emulated atomic64. To do this, I think it would also be necessary to patch the source code to make use of this more fine-grained information.

4) Another option, I suppose, would be to take advantage of the fact that the compiler does automatically convert attempts to use atomic64 into a function call. We could and actually supply a fallback to the atomic64 mutex emulation via a function of the same name. On an architectures where the compiler built-in is available for both atomic32 and atomic64, the built-in would be used and the emulation function would go unreferenced. However, on an architecture which supplies only built-in atomic32 but not atomic64, the emulation function would be automatically used instead.
Comment 6 lfmorrison 2020-01-04 11:01:54 UTC
(In reply to lfmorrison from comment #5)
Correction: configure tests for built-in atomic using a host-specific "long". That is still inadequate to tell for sure about the availability of atomic64 on all architectures.
Comment 7 lfmorrison 2020-01-04 21:04:20 UTC
First experiment: Building apr1 with -DUSE_ATOMICS_GENERIC. Not ideal due to possible performance penalties. But it does result in a usable libapr-1.so file, with which it is possible to build, link, and test subversion successfully.

That, at least, is a fallback position. I'll move on to try some other options now, which would hopefully allow atomic32 to continue to work with native performance.
Comment 8 Jochen Neumeister freebsd_committer freebsd_triage 2020-03-14 21:12:32 UTC
Unfortunately I have no possibility to find a solution as I do not own any powerpc hardware.
If someone has a patch here that works, he is welcome.
Otherwise I have to close here :-(
Comment 9 lfmorrison 2020-03-14 22:29:35 UTC
The patch for this problem should actually be applied to apr1, not subversion itself.

I've had a patch for apr1 that seems promising, but it was messy because it added new content to the configure script - and looking at the FreeBSD port maintenance guide, they recommended not applying patches directly against configure, but rather against configure.in and adding USES=autoreconf to the port's makefile.

However, there's already a patch in this port which is applied directly against configure (not configure.in) and I wasn't sure what would be the correct way to make all these moving parts play nicely together.

Basically, my patch takes the existing test in configure.in which checks for compiler built-in atomic operations on an unsigned long, and duplicates it with a guaranteed 64-bit type. (Autoconf can already ensures that the necessary symbol to represent a 64-bit type is available for the purposes of running this test.)

Then, the next part of my patch was applied against include/arch/unix/apr_arch_atomic.h. In the section dealing with checking to see if autoconf was able to detect the presence of built-in atomics generally, it also checked to see if autoconf was able to detect 64-bit atomics specifically; if it did not, then the patch would define NEED_ATOMICS_GENERIC64.

Next, in atomic/unix/builtins.c I modified apr_atomic_init() to optionally cascade into apr__atomic_generic64_init() if NEED_ATOMICS_GENERIC64 was defined.

Finally, in atomic/unix/builtins64.c, the check for USE_ATOMICS_BUILTINS was replaced with a test for both the presence of USE_ATOMICS_BUILTINS and the absence of NEED_ATOMICS_GENERIC64.

This all worked, but I was concerned about any corner cases I might be missing because of the fact that I was overwriting the previously patched version of configure with my patched version of configure.in together with autoreconf.

I also didn't have ready access to another platform to test and confirm that things were still running properly there.
Comment 10 Justin Hibbits freebsd_committer freebsd_triage 2020-03-14 22:39:19 UTC
I believe this can be addressed by adding:

CONFIGURE_ENV+= ap_cv_atomic_builtins=no

to the port Makefile.
Comment 11 Justin Hibbits freebsd_committer freebsd_triage 2020-03-14 22:40:32 UTC
(In reply to Justin Hibbits from comment #10)

Note, this should be guarded by a

.if ${ARCH} == powerpc || ${ARCH} == powerpcspe
....
.endif

of course.  Otherwise it pessimizes every architecture.
Comment 12 lfmorrison 2020-03-14 22:43:47 UTC
(In reply to Justin Hibbits from comment #10)
That would certainly be a less messy patch - as long as strict ANSI mode is not being enforced, the inline assembly form of PPC32 atomics would still be used for 32-bit types, and the generic version would only be imposed for 64-bit operations.
Comment 13 Piotr Kubaj freebsd_committer freebsd_triage 2020-04-01 16:28:12 UTC
Created attachment 212934 [details]
log

Why was this issue closed? It's still valid, subversion doesn't build on powerpc on 12.1-RELEASE (log attached). Since this must have been a mistake, I'm reopening.
Comment 14 Piotr Kubaj freebsd_committer freebsd_triage 2020-04-01 17:02:58 UTC
Patch by jhibbits works, so closing.
Comment 15 commit-hook freebsd_committer freebsd_triage 2020-04-01 17:03:47 UTC
A commit references this bug:

Author: pkubaj
Date: Wed Apr  1 17:02:29 UTC 2020
New revision: 530224
URL: https://svnweb.freebsd.org/changeset/ports/530224

Log:
  devel/apr1: fix build of devel/subversion on powerpc(spe)

  Those architectures don't have atomic functions working.

  PR:		242680
  Submitted by:	jhibbits
  Reported by:	canardo909@gmx.com

Changes:
  head/devel/apr1/Makefile