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

Linker bug compiling on Linux #3

Closed
joshlf opened this Issue Nov 7, 2018 · 7 comments

Comments

Projects
None yet
3 participants
@joshlf
Member

joshlf commented Nov 7, 2018

When compiling on Linux, I get the following linker error:

/usr/bin/ld: /home/vagrant/tmp/mundane/target/debug/build/mundane-9145b5e5c96cb8ea/out/boringssl/build_2/crypto/libcrypto_0_2_0.a(cbb.c.o): relocation R_X86_64_32 against `.rodata' can not be used when making a shared object; recompile with -fPIC
          /home/vagrant/tmp/mundane/target/debug/build/mundane-9145b5e5c96cb8ea/out/boringssl/build_2/crypto/libcrypto_0_2_0.a: error adding symbols: Bad value
          collect2: error: ld returned 1 exit status

This is surprising because, if I'm not mistaken, @erickt has successfully compiled on Linux before, implying that this is either a nondeterministic issue or has to do with my setup in particular.

@inejge

This comment has been minimized.

Contributor

inejge commented Nov 15, 2018

On Ubuntu 16.04, amd64, cargo build --release worked but cargo test --release deterministically failed with the above error. Without going too deeply into the rabbit hole, adding -fPIC to the compiler flags (as suggested in the error message) in boringssl/boringssl/CMakeLists.txt and rebuilding made the tests pass:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1586d34..6cc8d5e 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -84,7 +84,7 @@ endif()
 if(CMAKE_COMPILER_IS_GNUCXX OR CLANG)
   # Note clang-cl is odd and sets both CLANG and MSVC. We base our configuration
   # primarily on our normal Clang one.
-  set(C_CXX_FLAGS "-Werror -Wformat=2 -Wsign-compare -Wmissing-field-initializers -Wwrite-strings")
+  set(C_CXX_FLAGS "-Werror -Wformat=2 -Wsign-compare -Wmissing-field-initializers -Wwrite-strings -fPIC")
   if(MSVC)
     # clang-cl sets different default warnings than clang. It also treats -Wall
     # as -Weverything, to match MSVC. Instead -W3 is the alias for -Wall.

I noticed that -fPIC is mentioned in a couple of other build files, but haven't tried to whether it's possible to make it appear in CMakeLists.txt before starting the build.

@mati865

This comment has been minimized.

mati865 commented Nov 22, 2018

I believe it could work on 32bit Linux distributions because of:

https://github.com/google/boringssl/blob/9113e0996fd445ce187ae9dfeabfc95805b947a2/crypto/CMakeLists.txt#L22-L23

32bit libs are built with -fPIC but it cannot be enabled so easily for amd64 because rypto/cipher_extra/asm/aes128gcmsiv-x86_64.pl takes only two arguments, file format and file name.

@joshlf

This comment has been minimized.

Member

joshlf commented Nov 26, 2018

@inejge Would your approach work w/o having to edit CMakeLists.txt (or any other source file from the BoringSSL source tree)? Or do you think that's the only way of passing -fPIC?

@inejge

This comment has been minimized.

Contributor

inejge commented Nov 27, 2018

It could be done by postprocessing the build files generated by CMake.

find . -name flags.make -o -name link.txt -o -name build.ninja | xargs sed -i '/-fPIC/n; s/-Wwrite-strings/& -fPIC/'

I also made it work with a mundane-private copy of CMakeLists.txt. It's a bit painful, since CMake really likes relative paths and having CMakeLists.txt in the root of the source tree:

--- boringssl/boringssl/CMakeLists.txt  2018-11-27 08:53:52.451245046 +0100
+++ boringssl/cmake/fpic/CMakeLists.txt 2018-11-27 13:56:58.918880686 +0100
@@ -9,13 +9,15 @@
 # Defer enabling C and CXX languages.
 project(BoringSSL NONE)
 
+set(ACTUAL_SOURCE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/../../boringssl)
+
 if(WIN32)
   # On Windows, prefer cl over gcc if both are available. By default most of
   # the CMake generators prefer gcc, even on Windows.
   set(CMAKE_GENERATOR_CC cl)
 endif()
 
-include(sources.cmake)
+include(${ACTUAL_SOURCE_DIR}/sources.cmake)
 
 enable_language(C)
 enable_language(CXX)
@@ -84,7 +86,7 @@
 if(CMAKE_COMPILER_IS_GNUCXX OR CLANG)
   # Note clang-cl is odd and sets both CLANG and MSVC. We base our configuration
   # primarily on our normal Clang one.
-  set(C_CXX_FLAGS "-Werror -Wformat=2 -Wsign-compare -Wmissing-field-initializers -Wwrite-strings")
+  set(C_CXX_FLAGS "-Werror -Wformat=2 -Wsign-compare -Wmissing-field-initializers -Wwrite-strings -fPIC")
   if(MSVC)
     # clang-cl sets different default warnings than clang. It also treats -Wall
     # as -Weverything, to match MSVC. Instead -W3 is the alias for -Wall.
@@ -490,10 +492,10 @@
 
 # Add minimal googletest targets. The provided one has many side-effects, and
 # googletest has a very straightforward build.
-add_library(boringssl_gtest third_party/googletest/src/gtest-all.cc)
-target_include_directories(boringssl_gtest PRIVATE third_party/googletest)
+add_library(boringssl_gtest ${ACTUAL_SOURCE_DIR}/third_party/googletest/src/gtest-all.cc)
+target_include_directories(boringssl_gtest PRIVATE ${ACTUAL_SOURCE_DIR}/third_party/googletest)
 
-include_directories(third_party/googletest/include)
+include_directories(${ACTUAL_SOURCE_DIR}/third_party/googletest/include)
 
 # Declare a dummy target to build all unit tests. Test targets should inject
 # themselves as dependencies next to the target definition.
@@ -508,12 +510,12 @@
 
 add_library(crypto_test_data OBJECT crypto_test_data.cc)
 
-add_subdirectory(crypto)
-add_subdirectory(ssl)
-add_subdirectory(ssl/test)
-add_subdirectory(fipstools)
-add_subdirectory(tool)
-add_subdirectory(decrepit)
+add_subdirectory(${ACTUAL_SOURCE_DIR}/crypto ${CMAKE_CURRENT_BINARY_DIR}/crypto)
+add_subdirectory(${ACTUAL_SOURCE_DIR}/ssl ${CMAKE_CURRENT_BINARY_DIR}/ssl)
+add_subdirectory(${ACTUAL_SOURCE_DIR}/ssl/test ${CMAKE_CURRENT_BINARY_DIR}/ssl/test)
+add_subdirectory(${ACTUAL_SOURCE_DIR}/fipstools ${CMAKE_CURRENT_BINARY_DIR}/fipstools)
+add_subdirectory(${ACTUAL_SOURCE_DIR}/tool ${CMAKE_CURRENT_BINARY_DIR}/tool)
+add_subdirectory(${ACTUAL_SOURCE_DIR}/decrepit ${CMAKE_CURRENT_BINARY_DIR}/decrepit)
 
 if(FUZZ)
   add_subdirectory(fuzz)

For this to work, the crypto and util subdirectories from the BoringSSL source tree must be symlinked to boringssl/cmake/fpic, where the modified CMakeLists.txt resides.

@joshlf

This comment has been minimized.

Member

joshlf commented Nov 28, 2018

I'm sorry for leading y'all astray. I asked one of the BoringSSL folks, and they pointed me to the CMAKE_POSITION_INDEPENDENT_CODE variable. At least on my machine, it works if you simply pass that variable to CMake. Does this PR work for y'all?

@inejge

This comment has been minimized.

Contributor

inejge commented Nov 28, 2018

Yup, the tests pass on Ubuntu 64-bit.

@joshlf

This comment has been minimized.

Member

joshlf commented Nov 28, 2018

OK cool; I'll submit that.

@joshlf joshlf closed this in 04ba210 Dec 6, 2018

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