Fix builds on systems with RTLD_DEFAULT enabled #91
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AsanAlloc.cpp defines alignedAlloc which in return is exported as an FFI in ASan.hs cAlignedAlloc. If we don't include AsanAlloc.cpp in cxx-sources, it will be missing from compilation. In most instances this can be okay, as lazy loading usually prevents an error from appearing. However in instances where a linker requires it to be there, it will fail, such as on compiling hsthrift + glean on NixOS. The right thing here is to make sure all functions we export in the FFI are available. One open question here for peopl with deeper understanding of cabal+cxx compilation, is to include the header file in install-includes or not. For now i did not as it was not required to make the compilation succeed.
Util.Dll is exports functions that don't exist in RTS of GHC 8, but GHC 8 is required for compilation, hence on systems with a RTLD_DEFAULT enabled ghc or LINK_BIND_NOW=1 set, we get linker errors. Luckily our main downstream Glean does not use Util.Dll, so we can just remove this. Alternatively we can either require GHC 9 (and upgrade all our dependencies) or remove setHighMemDynamic and other functions that aren't available in GHC 8 (such as loadNativeObj and unloadNativeObj)
c9fb3a4 (Initial Commit) already removes the compilation of Utils/GFlags.cpp, but we still include Utils/GFlags.hs which depends on GFlags.cpp exported functions. This will fail at runtime, and on systems with LD_BIND_NOW=1 or an RTLD_DEFAULT enabled ghc this leads to link errors on build time. So remove it.
On systems with LD_BIND_NOW set or ghc with RTLD_DEFAULT linking enable, compilaton of fb-util fails due to missing symbol `common_hs_releaseGlobalCPUExecutor`. This is defined in Executor.cpp, so we need to include it into the compilation. Now I tried to add Utils/Executor.cpp and still got the errors. When I moved the file to cpp/Executor.cpp and included it, it worked. Now I have no clue why that is, and i tried to clean build artifacts but still had issues when Executor.cpp is not in cpp/. So I left it, without actually knowing _why_ this is.
We rely on folly which in return relies on libevent and fmt. We must include these during linking to avoid linking errors due to missing symbols.
…yrep During compilation with LD_BIND_NOW or ghc with RTLD_DEFAULT enabled (such as on NixOS) compiling of thrift-cpp-channel fails due to the following missing symbols: _ZN6apache6thrift4type13SemiAnyStruct10readNoXferINS0_20BinaryProtocolReaderEEEvPT_ _ZN6apache6thrift4type13ProtocolUnion16__fbthrift_clearEv _ZN6apache6thrift4typeltERKNS1_8ProtocolES4_ These are compiled into thrifttyperep.a thriftyanrep.a thrifttype.a We need to include them when linking. Note that the order matters due to the static libraries having dependencies on each other. For example thriftanyrep depends on thrifttype, hence thrifttype must come _after_ thriftanyrep.
folly depends on libzstd and zlib during compilation. On systems with LD_BIND_NOW set or ghc with RTLD_DEFAULT linking enabled, the linking of thrift-cpp-channel will fail due to libzstd and zlib missing. Add these to the pkgconfig dependencies to be correctly linked.
libfolly is included statically in fbthrift. Linking libfolly both statically
will cause the linker check of gflags to fail with the message:
ERROR: something wrong with flag 'folly_memory_idler_purge_arenas' in
file 'folly/detail/MemoryIdler.cpp'.
One possibility: file 'folly/detail/MemoryIdler.cpp' is being
linked both statically and dynamically into this executable.
coming from:
https://chromium.googlesource.com/external/gflags/src/+/b85dc3e1691f7106fd4fd54deb225088be14bc16/gflags.cc#726
Removing libfolly from dynamic linking will result in thrift-cpp-channel
to correctly compile.
facebook-github-bot
added
the
CLA Signed
|
It would be advisable to add a build job with non lazy symbol loading to CI, in order to ensure this doesn't break again. |
Issue facebookincubator#88 and facebookincubator#90 describe that depending on system GHC can lazily link all symbols or strictly link them at link time. To ensure we consistently test that all the symbols in the binary are available, we need to set LD_BIND_NOW to a non empty string. This will force ld to resolv all symbols (see https://man7.org/linux/man-pages/man8/ld.so.8.html) at link time.
|
@pepeiborra has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pepeiborra I added the env variable to the github workflow. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following PR addresses various build issues that occur when GHC does load symbols strictly at build times. @exi's did an investigation in #90. GHC on NixOS is compiled with _GNU_SOURCE leading to dlsym using RTLD_DEFAULT, causing strict loading. For hsthrift his means various symbols cannot be found at compile time and error out.
With the following stack I am able to build hsthrift and Glean NixOS. However I have not tested it on any other platform just yet (Notable the docker image).
I want to highlight commit 83ed60e, since it moves Utils/Executor.cpp to cpp/Executor.cpp and I am not sure if that's okay.
The rest of the PR should be straight forward.
This is branch includes PR #89 and should cleanly rebase if needed.