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

Link to libatomic when necessary (eg. on armv6l) #5227

Merged
merged 1 commit into from Oct 11, 2018

Conversation

Projects
None yet
5 participants
@lopsided98
Contributor

lopsided98 commented Oct 5, 2018

This PR fixes #5219. It implements detection of whether std::atomic can be used without linking to libatomic and adds libatomic to the linker arguments if this fails.

It is likely mostly harmless to always link to libatomic (at least with gcc and clang), but this solution was chosen because many other projects have similar logic in their build systems. It is only implemented for the autotools based build, not bazel or cmake, but this was deemed adequate in #5219. I assume the bazel build has the same issue, but I have not tested it.

I tested this on Linux on x86_64, armv6l and armv7l, but I am not very familiar with autotools so this may need to be cleaned up/tweaked.

@lopsided98

This comment has been minimized.

Show comment
Hide comment
@lopsided98

lopsided98 Oct 5, 2018

Contributor

Here is an example of how it could be done with CMake: https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/CheckAtomic.cmake

I was considering adapting this, but I'm not sure how that would work with licensing.

Contributor

lopsided98 commented Oct 5, 2018

Here is an example of how it could be done with CMake: https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/CheckAtomic.cmake

I was considering adapting this, but I'm not sure how that would work with licensing.

@TeBoring TeBoring requested a review from acozzette Oct 7, 2018

@TeBoring TeBoring added the kokoro:run label Oct 7, 2018

@kokoro-team kokoro-team removed the kokoro:run label Oct 7, 2018

@acozzette

This comment has been minimized.

Show comment
Hide comment
@acozzette

acozzette Oct 8, 2018

Contributor

It looks like one test is failing because -latomic is being passed in a case where the linker doesn't know what to do with it. This is happening in a Docker environment that deliberately uses an old toolchain for the purpose of building protoc binaries that are compatible with old versions of libc. I haven't had a chance to figure out exactly what's going wrong but you could try running it by cd'ing to kokoro/linux/cpp_distcheck and running ./build.sh. That requires Docker to be installed, though.

Contributor

acozzette commented Oct 8, 2018

It looks like one test is failing because -latomic is being passed in a case where the linker doesn't know what to do with it. This is happening in a Docker environment that deliberately uses an old toolchain for the purpose of building protoc binaries that are compatible with old versions of libc. I haven't had a chance to figure out exactly what's going wrong but you could try running it by cd'ing to kokoro/linux/cpp_distcheck and running ./build.sh. That requires Docker to be installed, though.

@lopsided98

This comment has been minimized.

Show comment
Hide comment
@lopsided98

lopsided98 Oct 8, 2018

Contributor

That failure seems somewhat strange, because gcc 4.8 should have libatomic AFAIK. There is something else wrong though, because x86_64 builds shouldn't need to link to libatomic, but the configure check is claiming that it is necessary. The test program is probably failing to compile for some other reason. I'll run the test myself and see if I can figure it out.

Contributor

lopsided98 commented Oct 8, 2018

That failure seems somewhat strange, because gcc 4.8 should have libatomic AFAIK. There is something else wrong though, because x86_64 builds shouldn't need to link to libatomic, but the configure check is claiming that it is necessary. The test program is probably failing to compile for some other reason. I'll run the test myself and see if I can figure it out.

@lopsided98

This comment has been minimized.

Show comment
Hide comment
@lopsided98

lopsided98 Oct 8, 2018

Contributor

The libatomic configure test program is failing to compile because -std=c++11 is not in CXXFLAGS in the configure script. By default, the configure script adds -std=c++11 to CXXFLAGS, but the test script overrides CXXFLAGS and does not include -std=c++11. The build normally still works because -std=c++11 gets added somewhere else in the build system.

I think the simplest way to fix this is to always add -std=c++11 to CXXFLAGS, even when the user supplies their own CXXFLAGS. I have modified this PR to include this fix and now the test passes.

Also, the reason linking to libatomic failed is probably that centos has a separate package for libatomic, which is likely not installed.

Contributor

lopsided98 commented Oct 8, 2018

The libatomic configure test program is failing to compile because -std=c++11 is not in CXXFLAGS in the configure script. By default, the configure script adds -std=c++11 to CXXFLAGS, but the test script overrides CXXFLAGS and does not include -std=c++11. The build normally still works because -std=c++11 gets added somewhere else in the build system.

I think the simplest way to fix this is to always add -std=c++11 to CXXFLAGS, even when the user supplies their own CXXFLAGS. I have modified this PR to include this fix and now the test passes.

Also, the reason linking to libatomic failed is probably that centos has a separate package for libatomic, which is likely not installed.

@acozzette

This comment has been minimized.

Show comment
Hide comment
@acozzette

acozzette Oct 8, 2018

Contributor

@lopsided98 Thanks for tracking down the problem. It seems to me that it's probably best to just update tests.sh to set -std=c++11 in CXX_FLAGS instead of adding that in the configure script, because someone might for example want to use C++14, and we wouldn't want to downgrade to C++11 in that case.

Contributor

acozzette commented Oct 8, 2018

@lopsided98 Thanks for tracking down the problem. It seems to me that it's probably best to just update tests.sh to set -std=c++11 in CXX_FLAGS instead of adding that in the configure script, because someone might for example want to use C++14, and we wouldn't want to downgrade to C++11 in that case.

@lopsided98

This comment has been minimized.

Show comment
Hide comment
@lopsided98

lopsided98 Oct 8, 2018

Contributor

Ok, done.

Contributor

lopsided98 commented Oct 8, 2018

Ok, done.

@acozzette acozzette added the kokoro:run label Oct 8, 2018

@kokoro-team kokoro-team removed the kokoro:run label Oct 8, 2018

@acozzette acozzette merged commit ebd38c6 into protocolbuffers:master Oct 11, 2018

28 checks passed

Bazel Kokoro build successful
Details
Linux 32-bit Kokoro build successful
Details
Linux C# Kokoro build successful
Details
Linux C++ Distcheck Kokoro build successful
Details
Linux Golang Kokoro build successful
Details
Linux Java Compatibility Kokoro build successful
Details
Linux Java JDK 7 Kokoro build successful
Details
Linux Java Oracle 7 Kokoro build successful
Details
Linux JavaScript Kokoro build successful
Details
Linux PHP Kokoro build successful
Details
Linux Python Kokoro build successful
Details
Linux Python C++ Kokoro build successful
Details
Linux Python Compatibility Kokoro build successful
Details
Linux Ruby Kokoro build successful
Details
MacOS C++ Kokoro build successful
Details
MacOS C++ Distcheck Kokoro build successful
Details
MacOS JavaScript Kokoro build successful
Details
MacOS Obj-C CocoaPods Integration Kokoro build successful
Details
MacOS Obj-C OS X Kokoro build successful
Details
MacOS Obj-C iOS Debug (Allowed Failure) Kokoro build successful
Details
MacOS Obj-C iOS Release (Allowed Failure) Kokoro build successful
Details
MacOS PHP5.6 Kokoro build successful
Details
MacOS PHP7.0 Kokoro build successful
Details
MacOS Python Kokoro build successful
Details
MacOS Python C++ (Allowed Failure) Kokoro build successful
Details
MacOS Ruby 2.1 Kokoro build successful
Details
MacOS Ruby 2.2 (Allowed Failure) Kokoro build successful
Details
cla/google All necessary CLAs are signed
@acozzette

This comment has been minimized.

Show comment
Hide comment
@acozzette

acozzette Oct 11, 2018

Contributor

Thanks @lopsided98!

Contributor

acozzette commented Oct 11, 2018

Thanks @lopsided98!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment