Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

dvrogozh
Copy link
Contributor

@dvrogozh dvrogozh commented Mar 8, 2019

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

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>
@dvrogozh
Copy link
Contributor Author

dvrogozh commented Mar 8, 2019

CI build fails with:
/usr/bin/ld: cannot find -ledit
Can someone, please, check system installation of llvm, what is returned by llvm_config(target all) and that returned libraries really exist on the system? I think this could be some CI/system misconfiguration issue.

@tjaalton
Copy link

I've tried this locally, and it fails with:

/usr/bin/ld: CMakeFiles/common_clang.dir/common_clang.cpp.o: in function Compile': ./build/./common_clang.cpp:305: undefined reference to llvm::writeSpirv(llvm::Module*, llvm::raw_ostream&, std::__cxx11::basic_string<char, std::char_traits, std::allocator >&)'

@dvrogozh
Copy link
Contributor Author

@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:

# for SPIRV-LLVM-Translator:
cmake -DLLVM_DIR=$PREFIX/lib/cmake/llvm -DCMAKE_CXX_FLAGS="-fPIC" -DCMAKE_INSTALL_PREFIX=/path/to/install ..

# for opencl_clang:
cmake -DLLVM_DIR=$PREFIX/lib/cmake/llvm -DCMAKE_INSTALL_PREFIX=/path/to/install ..

So, I did not pass any specific flags to configure openc;l_clang. Did you? Especially, did you attempt to pass any of these:
-DLLVMSPIRV_INCLUDED_IN_LLVM=OFF -DSPIRV_TRANSLATOR_DIR=/path/to/installed/spirv/translator

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.

@dvrogozh
Copy link
Contributor Author

@tjaalton : and even more specifically, could you, please, check that you see -lLLVMSPIRVLib in the common_clang linker command line:
CMakeFiles/common_clang.dir/link.txt:1:/opt/rh/devtoolset-7/root/usr/bin/c++ -fPIC -Wl,-rpath-link, -Wl,-O3 -Wl,--gc-sections -Wl,--no-undefined -Wl,-Bsymbolic -Wl,--version-script=/home/dvrogozh/git/github/tmp/common_clang/common_clang.map -shared -Wl,-soname,libcommon_clang.so.8 -o libcommon_clang.so.8 CMakeFiles/common_clang.dir/common_clang.cpp.o <...> -lLLVMSPIRVLib <...>

If you don't can you, please, try with -DLLVMSPIRV_INCLUDED_IN_LLVM=OFF.

@dvrogozh
Copy link
Contributor Author

I traced my configuration of common_clang and here is what I hit to include SPIRV library:

set(LLVM_LIBS ${LLVM_LIBS} SPIRVLib)

So, here are values of the key options on my side:

  • USE_PREBUILT_LLVM=ON
  • LLVMSPIRV_INCLUDED_IN_LLVM=ON
  • LLVM_LINK_LLVM_DYLIB=OFF

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:

message(FATAL_ERROR "SPIRV_TRANSLATOR_DIR is required")

However, to me approach to detect SPIRV library on the system is questionable. I have questions:

  1. How likely it is that Linux distros will include SPIRV translator into LLVM build? From my perspective it is more likely to be in a separate package.
  2. If SPIRV translator is not part of LLVM build, then SPIRV_TRANSLATOR_DIR should not be actually required since it provides LLVMSPIRVLib.pc and build system can actually detect SPIRV translator via standard detection mechanism - no need for the custom one
    I will submit another issue to discuss this further.

@tjaalton
Copy link

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
a shared object; recompile with -fPIC

etc.

@JacekDanecki
Copy link

@tjaalton Here is Makefile I'm using to build spirv translator and opencl-clang on Ubuntu 18.04 in this ppa

all: spirv
        mkdir build_opencl_clang ; cd build_opencl_clang; cmake -DCOMMON_CLANG_LIBRARY_NAME=opencl_clang -DLLVMSPIRV_INCLUDED_IN_LLVM=OFF -DSPIRV_TRANSLATOR_DIR=../build_spirv/install -DLLVM_NO_DEAD_STRIP=ON -DCMAKE_INSTALL_PREFIX=/usr ../opencl_clang && make all -j`nproc`

spirv:
        mkdir build_spirv ; cd build_spirv; cmake ../llvm-spirv -DCMAKE_INSTALL_PREFIX=install -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCMAKE_BUILD_TYPE=Release && make llvm-spirv -j`nproc` && make install

install:
        make -C build_opencl_clang DESTDIR=${DESTDIR} install

@dvrogozh
Copy link
Contributor Author

I also saw this issue and I fixed it with -DCMAKE_CXX_FLAGS="-fPIC". However, using -DCMAKE_POSITION_INDEPENDENT_CODE=ON is a better (cmake-stylish) way to do the same.

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 -DBUILD_SHARED_LIBS=ON to spirv configure line. Will try this later today...

@tjaalton
Copy link

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.

@tjaalton
Copy link

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: '')
dpkg-shlibdeps: error: cannot find library libOptRemarks.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
llvm-8-dev: /usr/lib/llvm-8/lib/libLTO.so
llvm-8-dev: /usr/lib/llvm-8/lib/libOptRemarks.so.8

libcommon_clang probably shouldn't link against those?

@dvrogozh
Copy link
Contributor Author

Emm... I don't see that common_clang links against those:

[dvrogozh@dvrscl common_clang]$ fgrep -rsn LTO
[dvrogozh@dvrscl common_clang]$ fgrep -rsn OptRemarks
[dvrogozh@dvrscl common_clang]$

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 llvm_config(<target> all) which is probably more likely.

@tjaalton
Copy link

right, it's caused by this patch, should've made that clear

@dvrogozh
Copy link
Contributor Author

@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

@tjaalton
Copy link

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)
Copy link
Contributor

@tripzero tripzero Mar 20, 2019

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).

Copy link
Contributor

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

Copy link
Contributor Author

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

@dvrogozh
Copy link
Contributor Author

@tjaalton : yes, this one is needed (or its modified/fixed variant). We work to finalize the change and merge it.

@dvrogozh
Copy link
Contributor Author

This should not be needed anymore after #53 and #54 landed. Please, pay attention at #53 (comment) discussion.

@dvrogozh dvrogozh closed this Mar 25, 2019
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.

5 participants