Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect whether libatomic is needed rather than hard-coding for mips #8

Merged
merged 2 commits into from
Jan 27, 2017

Conversation

jrtc27
Copy link
Contributor

@jrtc27 jrtc27 commented Jan 26, 2017

Fixes FTBFS on powerpc, since it needs libatomic too for C11 atomics, and possibly m68k. I haven't actually tested a full build, but configuring now detects that it can use C11 atomics with libatomic, so it should at least get further:

-- Performing Test HAVE_GCC_ATOMIC_BUILTINS
-- Performing Test HAVE_GCC_ATOMIC_BUILTINS - Failed
-- Performing Test HAVE_GCC_C11_ATOMICS_WITHOUT_LIBATOMIC
-- Performing Test HAVE_GCC_C11_ATOMICS_WITHOUT_LIBATOMIC - Failed
-- Performing Test HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC
-- Performing Test HAVE_GCC_C11_ATOMICS_WITH_LIBATOMIC - Success

@jrtc27
Copy link
Contributor Author

jrtc27 commented Jan 27, 2017

Hm, having left the build running, it fails at linking:

[ 12%] Linking CXX executable strings-t
cd /build/mariadb-10.1-10.1.21/builddir/unittest/strings && /usr/bin/cmake -E cmake_link_script CMakeFiles/strings-t.dir/link.txt --verbose=1
/usr/bin/powerpc-linux-gnu-g++   -g -O2 -fdebug-prefix-map=/build/mariadb-10.1-10.1.21=. -fstack-protector-strong -Wformat -Werror=format-security -pie -fPIC -Wl,-z,relro,-z,now -fstack-protector --param=ssp-buffer-size=4 -DWITH_INNODB_DISALLOW_WRITES -fno-exceptions -fno-rtti -O3 -g -static-libgcc -fno-omit-frame-pointer -fno-strict-aliasing -Wno-uninitialized -D_FORTIFY_SOURCE=2 -DDBUG_OFF  -Wl,-z,relro -Wl,-z,now CMakeFiles/strings-t.dir/strings-t.c.o  -o strings-t  -lpthread ../mytap/libmytap.a ../../strings/libstrings.a ../../mysys/libmysys.a ../../dbug/libdbug.a ../../mysys_ssl/libmysys_ssl.a ../../mysys/libmysys.a ../../dbug/libdbug.a ../../mysys_ssl/libmysys_ssl.a -lz -lm -ldl ../../strings/libstrings.a ../../extra/yassl/libyassl.a ../../extra/yassl/taocrypt/libtaocrypt.a -lpthread 
CMakeFiles/my_atomic-t.dir/my_atomic-t.c.o: In function `my_atomic_add64':
./builddir/unittest/mysys/./include/my_atomic.h:257: undefined reference to `__atomic_fetch_add_8'
./builddir/unittest/mysys/./include/my_atomic.h:257: undefined reference to `__atomic_fetch_add_8'
./builddir/unittest/mysys/./include/my_atomic.h:257: undefined reference to `__atomic_fetch_add_8'
collect2: error: ld returned 1 exit status
unittest/mysys/CMakeFiles/my_atomic-t.dir/build.make:105: recipe for target 'unittest/mysys/my_atomic-t' failed

Seems like CMAKE_REQUIRED_LIBRARIES isn't actually being used? I guess I could add it to EXTRA_REQ_LIBRARIES; that might work? I don't really know CMake very well; maybe you know what the problem/solution is?

@ottok
Copy link
Owner

ottok commented Jan 27, 2017

The original MIPS 64 patch was written by @cvicentiu, I will let him comment on the issue.

Do you @jrtc27 want me to merge this as-is already now or wait for a perfect solution? PowerPC (the 32-bit one) is not an official architecture in Stretch, but MIPS variants are, and I'd rather not want to risk MIPS build failures at this stage just to partially improve PowerPC build progress. Do you anticipate any regressions with this? I don't have time to do more than just one amd64 test build before next upload (which is urgent due to the screw-ups I did yesterday).

@jrtc27
Copy link
Contributor Author

jrtc27 commented Jan 27, 2017

Yeah, don't merge this yet, it will almost certainly break 32-bit MIPS builds.

@jrtc27
Copy link
Contributor Author

jrtc27 commented Jan 27, 2017

If you don't mind waiting a few hours before uploading, though, I'd appreciate it, as I'm going to take a look at this this morning, but I understand if you want to upload a non-broken version ASAP.

@ottok
Copy link
Owner

ottok commented Jan 27, 2017

I can wait. Thanks a lot for your contributions to this package!

@jrtc27
Copy link
Contributor Author

jrtc27 commented Jan 27, 2017

Updated. Seems to work on powerpc (although the test suite looks like it has a couple of failures on the porterbox). I'm also doing a mipsel build on eller, but if you want to get this out quickly, I suggest you merge this and start your amd64 test build now while mipsel builds? I will report back once it has finished compiling (test suite is irrelevant since it's ignored by debian/rules).

@ottok
Copy link
Owner

ottok commented Jan 27, 2017

I'll wait for a few more hours. The 10-day limit to Feb 5th is already missed since upload yesterday evening failed, and it will not matter if I upload today at 15pm or 20pm.

@jrtc27
Copy link
Contributor Author

jrtc27 commented Jan 27, 2017

The mipsel build on eller is at 76%, and has definitely been using the atomics operations based on the compiler output. I think it's safe to assume it still works.

@cvicentiu
Copy link
Contributor

@jrtc27: Yes, The second version of your commits are good. The -DEXTRA_REQ_LIBRARIES was a hack I did to tell cmake to link with libatomic. I think you're solution is better and is safe to push.

The CMAKE_REQUIRED_LIBRARIES variable is only used when doing compile checks and doesn't actually tell the linker to use it at build time. You have to use TARGET_LINK_LIBRARIES, like you did in your second approach to actually link it.

Looks good to me now.

@ottok ottok merged commit a0e8fa1 into ottok:master Jan 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants