-
Notifications
You must be signed in to change notification settings - Fork 82
build: use llvm_config and full symbols resolution #45
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
Conversation
Fixes: intel#44 This adds -Wl,--no-undefined to the library build command line and assures that we don't have missed symbols, i.e. library will be able to load. With -Wl,--no-undefined we are getting a range of unresolved symbols which is being fixed by building against all llvm libraries. This also refactors how llvm libraries are fetched: we now use llvm_config(). Signed-off-by: Dmitry Rogozhkin <dmitry.v.rogozhkin@intel.com>
CI build fails with: |
I've tried this locally, and it fails with: /usr/bin/ld: CMakeFiles/common_clang.dir/common_clang.cpp.o: in function |
@tjaalton : This unresolved symbol looks coming from https://github.com/KhronosGroup/SPIRV-LLVM-Translator/blob/master/include/LLVMSPIRVLib.h while my change primary focuses on LLVM libs. I suspect this originates from the difference in how we configure spirv translator and opencl_clang. Here is what I do:
So, I did not pass any specific flags to configure openc;l_clang. Did you? Especially, did you attempt to pass any of these: Another difference could be that you are most probably build on Ubuntu and I am not. As a result, it could be that opencl_clang has some Ubuntu specific path which you hit and I do not. |
@tjaalton : and even more specifically, could you, please, check that you see -lLLVMSPIRVLib in the common_clang linker command line: If you don't can you, please, try with -DLLVMSPIRV_INCLUDED_IN_LLVM=OFF. |
I traced my configuration of common_clang and here is what I hit to include SPIRV library: Line 224 in 5715ced
So, here are values of the key options on my side:
If I read the code correctly, then if the LLVM was configured and built with LLVM_LINK_LLVM_DYLIB=ON then common_clang will miss -lLLVMSPIRVLib in linker command and expects to find definitions directly from LLVM library, i.e. LLVM should be built with SPIRV translator. I think that's the case for your setup @tjaalton : can you, please, confirm? I think that the way to mitigate this in the current build system would be to use both -DLLVMSPIRV_INCLUDED_IN_LLVM=OFF and -DSPIRV_TRANSLATOR_DIR=/path/to/installed/spirv/translator. If you won't specify the second option build system should spit an error here: Line 46 in 5715ced
However, to me approach to detect SPIRV library on the system is questionable. I have questions:
|
LLVM in debian/ubuntu is built with LLVM_LINK_LLVM_DYLIB=ON, yes. I can't find any mention of LLVMSPIRVLib on the repo, so I'd say it's not included in the LLVM build. I've packaged spirv-llvm-translator myself and it's available, but it only builds a static library which apparently can't be used by opencl-clang (using LLVMSPIRV_INCLUDED_IN_LLVM=OFF, SPIRV_TRANSLATOR_DIR=/usr/lib): /usr/bin/ld: /usr/lib/gcc/x86_64-linux-gnu/8/../../../../lib/libLLVMSPIRVLib.a(SPIRVWriter.cpp.o): relocation R_X86_64_PC32 against symbol `_ZTVSt15basic_streambufIcSt11char_traitsIcEE@@GLIBCXX_3.4' can not be used when making etc. |
@tjaalton Here is Makefile I'm using to build spirv translator and opencl-clang on Ubuntu 18.04 in this ppa
|
I also saw this issue and I fixed it with -DCMAKE_CXX_FLAGS="-fPIC". However, using However, @tjaalton, may be the right way to do will be to try produce shared library for spirv translator rather than static? I did not try that yet. I would consider add |
Thanks, llvmspirvlib built with -fPIC fixed opencl-clang here. But yes, having lllvmspirvlib provide a proper shared library with a soname would be great. But I'll use this for now. |
oh well :) Now I get this from debhelper which is checking the shared lib for it's dependencies: dpkg-shlibdeps: error: cannot find library libLTO.so.8 needed by debian/libcommon-clang8/usr/lib/libcommon_clang.so.8 (ELF format: 'elf64-x86-64' abi: '0201003e00000000'; RPATH: '') these are from the dev libcommon_clang probably shouldn't link against those? |
Emm... I don't see that common_clang links against those:
It don't refer to them in cmake neither with my patch nor without... Could that be some problem in the debian meta-data you composed for the package? Or maybe some behavior caused by |
right, it's caused by this patch, should've made that clear |
@tjaalton : I just built spirv translator with -DBUILD_SHARED_LIBS=ON (and without -fPIC) and common_clang was succefully built afterwards against libLLVMSPIRVLib.so.8 |
Yeah, seems to work fine.. is this patch still needed? |
@@ -271,6 +235,8 @@ add_llvm_library(${TARGET_NAME} SHARED | |||
${ADDITIONAL_LIBS} | |||
${CMAKE_DL_LIBS}) | |||
|
|||
llvm_config(${TARGET_NAME} all) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably be llvm_config(${TARGET_NAME} USE_SHARED all) if I'm reading https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/LLVM-Config.cmake#L67 correctly. Without it, I got linker errors during build because linker is trying to link against a bunch of libraries that don't exist on clearlinux (all compiled into libLLVM.so).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please take a look at #53
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tripzero : you might need USE_SHARED if you build against LLVM built into dynamic library, but I built LLVM into range of static libraries (well, this was a default) and if I follow your suggestion I get:
[ 92%] Linking CXX shared library libcommon_clang.so
/opt/rh/devtoolset-7/root/usr/libexec/gcc/x86_64-redhat-linux/7/ld: cannot find -lLLVM
Hence, some condition might be needed to differentiate between 2 kind of LLVM builds: dynamic and static. I originally missed this attribute and thought hat llvm_config will take care about differentiation on its own.
I see that @AlexeySotkin provides slightly different way to handle the same in #53. Let's take a look on it, it might be a better option if it provides internal differentiation between dynamic or static LLVM builds
@tjaalton : yes, this one is needed (or its modified/fixed variant). We work to finalize the change and merge it. |
This should not be needed anymore after #53 and #54 landed. Please, pay attention at #53 (comment) discussion. |
Fixes: #44
This adds -Wl,--no-undefined to the library build command line and
assures that we don't have missed symbols, i.e. library will be
able to load.
With -Wl,--no-undefined we are getting a range of unresolved symbols
which is being fixed by building against all llvm libraries. This also
refactors how llvm libraries are fetched: we now use llvm_config().
Signed-off-by: Dmitry Rogozhkin dmitry.v.rogozhkin@intel.com