[CMake] Don't explicitly use LLVM_LIBRARY_DIR in standalone builds

[xiaobai]

Summary

This line is unnecessary because add_llvm_executable will handle
linking the correct LLVM libraries for you. LLDB standalone builds are totally
find without this.

In the best case, having this line here is harmless. In the worst case it can
cause link issues.

If you build lldb-server for android using the standalone build, this line
will cause LLVM_LIBRARY_DIR to be the first place you look for libraries.
This is an issue because if you built libc++, it will try to link against
that one instead of the one from the android NDK. Meanwhile, the LLVM libraries
you're linking against were linked against the libc++ from the NDK.

Ideally, we would take advantage of the AFTER option for link_directories(), but
that was not available in LLDB's minimum supported version of CMake (CMake 3.4.3).

[labath]

The reasoning makes sense to me, but maybe one of the standalone folks could confirm that.

[sgraenitz]

Major concern from my side is that clang does something like this too:

clang/CMakeLists.txt
86:  set(LLVM_LIBRARY_DIR ${LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
125:  link_directories("${LLVM_LIBRARY_DIR}")

It looks like AddLLVM.cmake uses it in function configure_lit_site_cfg:

# They below might not be the build tree but provided binary tree.
set(LLVM_SOURCE_DIR ${LLVM_MAIN_SRC_DIR})
set(LLVM_BINARY_DIR ${LLVM_BINARY_DIR})
string(REPLACE "${CMAKE_CFG_INTDIR}" "${LLVM_BUILD_MODE}" LLVM_TOOLS_DIR "${LLVM_TOOLS_BINARY_DIR}")
string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLVM_LIBS_DIR  "${LLVM_LIBRARY_DIR}")

So we should always have some value for LLVM_LIBRARY_DIR. Not sure about the link_directories line.
Did you build and run test suite with this change? How does this string Replace work then?

This is an issue because if you built libc++, it will try to link against that one instead of the one from the android NDK.

Looking in-tree first seems like a reasonable default. You are cross-compiling only lldb-server so it can run on Android right? Everything else is built for your host platform?

[xiaobai]

In D60180#1453106, @sgraenitz wrote:

Major concern from my side is that clang does something like this too:

clang/CMakeLists.txt
86:  set(LLVM_LIBRARY_DIR ${LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
125:  link_directories("${LLVM_LIBRARY_DIR}")

It looks like AddLLVM.cmake uses it in function configure_lit_site_cfg:

# They below might not be the build tree but provided binary tree.
set(LLVM_SOURCE_DIR ${LLVM_MAIN_SRC_DIR})
set(LLVM_BINARY_DIR ${LLVM_BINARY_DIR})
string(REPLACE "${CMAKE_CFG_INTDIR}" "${LLVM_BUILD_MODE}" LLVM_TOOLS_DIR "${LLVM_TOOLS_BINARY_DIR}")
string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLVM_LIBS_DIR  "${LLVM_LIBRARY_DIR}")

So we should always have some value for LLVM_LIBRARY_DIR. Not sure about the link_directories line.
Did you build and run test suite with this change? How does this string Replace work then?

Having the definition is fine, but the link_directories line is what causes the issue. I definitely built, but from I looked at my tmux history from last night and it looks like I ran the test suite from a unified build directory instead of an lldb standalone build directory, so I'll run the test suite there and report back. Knowing what I know now, I expect it to fail, so I'll need to revise and test this more thoroughly.

This is an issue because if you built libc++, it will try to link against that one instead of the one from the android NDK.

Looking in-tree first seems like a reasonable default. You are cross-compiling only lldb-server so it can run on Android right? Everything else is built for your host platform?

No, everything is being built for android. Cross-compiling lldb-server involves cross-compiling llvm libraries, clang libraries, and if you've got swift in the picture, swift host libraries. LLVM and clang libraries are built against the libc++ from the android NDK, but in standalone builds, LLDB will try to link against the freshly built libc++ from LLVM. You get loads of undefined references to symbols from the NDK's libc++.

[xiaobai]

So I ran the lldb test suite from a standalone build tree, and this patch didn't change anything. I added some logging to the CMake and LLVM_LIBRARY_DIR is being set correctly. It appears to be getting it from the LLVMConfig we get from find_package(LLVM). I think we could also get rid of LLVM_BINARY_DIR if that's the case, since it appears we only set it for lit.

From the looks of it, clang needs to set it manually because they rely on llvm-config instead of using find_package.

[sgraenitz]

From the looks of it, clang needs to set it manually because they rely on llvm-config instead of using find_package.

Please note that clang *is* using find_package, just that instead of removing the old llvm-config invocation, it was deprecated first. The code I referred to *is not* in the deprecated llvm-config code!

LLVM_LIBRARY_DIR is being set correctly. It appears to be getting it from the LLVMConfig we get from find_package(LLVM).

Yes LLVMConfig sets the variable, but it's not going to be in the cache:

llvm-macosx-x86_64/lib/cmake/llvm/LLVMConfig.cmake
178:set(LLVM_LIBRARY_DIR "/path/to/llvm-macosx-x86_64/./lib")

Does cmake -L ... still dump it? Do in-tree builds have a cache entry for it? Are other subprojects relying on it? (unlikely for LLDB)
Pardon my stubbornness, but fixing such things a few weeks later is just too annoying.

I think we could also get rid of LLVM_BINARY_DIR if that's the case, since it appears we only set it for lit.

Yes cleanup is very important, but let's not focus only on what *can* be removed. IMHO a good order of priorities in build system code may be:

1. keep differences between in-tree and standalone builds small
2. keep differences between subprojects small
3. keep the code clean

If we are really sure that this code it outdated, please remove it and ideally also initiate a similar cleanup in clang, compiler-rt, lld, etc. (or ping someone to have a look).

[xiaobai]

In D60180#1454634, @sgraenitz wrote:

LLVM_LIBRARY_DIR is being set correctly. It appears to be getting it from the LLVMConfig we get from find_package(LLVM).

Yes LLVMConfig sets the variable, but it's not going to be in the cache:

llvm-macosx-x86_64/lib/cmake/llvm/LLVMConfig.cmake
178:set(LLVM_LIBRARY_DIR "/path/to/llvm-macosx-x86_64/./lib")

Does cmake -L ... still dump it? Do in-tree builds have a cache entry for it? Are other subprojects relying on it? (unlikely for LLDB)

No, invoking cmake -L ... does not dump this variable with this patch applied. In-tree builds don't dump it either, since it's not a cache variable when it's an in-tree build. It appears only standalone builds of subprojects do this.
No other subprojects are relying on LLDB having this set from what I can tell.

Pardon my stubbornness, but fixing such things a few weeks later is just too annoying.

I actually appreciate the level of scrutiny. I'd like to move more carefully here, since I know that my initial move from llvm-config to find_package(LLVM) gave you some issues with tests.
From what I remember, you added back LLVM_MAIN_SRC_DIR, LLVM_MAIN_INCLUDE_DIR, LLVM_LIBRARY_DIR, and LLVM_BINARY_DIR to unbreak the unittests.
I know LLVM_MAIN_SRC_DIR is used to make sure the gtest library is built and the headers are found correctly. I think setting this is the right thing to do since LLVM only exports LLVM_BUILD_MAIN_SRC_DIR. LLVM_MAIN_INCLUDE_DIR is for TableGen, and from what I can tell this should come from LLVMConfig now as well. As for the other two, LLVM_BINARY_DIR and LLVM_BINARY_DIR, these are used to configure lit in LLDB. These should be picked up through find_package(LLVM). If there's a situation where any of these variables don't get propagated from the LLVMConfig, I'd like to know about it so I can add comments above these variables explaining why these variables are set since it's non-obvious just from grepping the lldb tree.

I think we could also get rid of LLVM_BINARY_DIR if that's the case, since it appears we only set it for lit.

Yes cleanup is very important, but let's not focus only on what *can* be removed. IMHO a good order of priorities in build system code may be:

1. keep differences between in-tree and standalone builds small
2. keep differences between subprojects small
3. keep the code clean

If we are really sure that this code it outdated, please remove it and ideally also initiate a similar cleanup in clang, compiler-rt, lld, etc. (or ping someone to have a look).

I agree and think this is a handy methodology. A large part of my motivation behind this change is because I'm noticing a difference between the in-tree and standalone builds that is breaking a use case I want to support. In order to keep the differences between subprojects small, I don't mind removing it from other subprojects as well. That is, as soon as I'm more confident that this isn't going to break anything. :)

[sgraenitz]

[LLVM_LIBRARY_DIR is] not a cache variable when it's an in-tree build

Great, then please go ahead.

From what I remember, you added back [...]

Yes this was a quick-fix. I didn't have the patience in that moment to check each of them individually.

If there's a situation where any of these variables don't get propagated from the LLVMConfig, I'd like to know about it so I can add comments above these variables explaining why these variables are set since it's non-obvious just from grepping the lldb tree.

It's usually a good idea to check both, the LLVMConfig.cmake from a build-tree and from an install-tree. They are quite different and LLDB standalone should work with both.

You can check the green dragon bot that we added recently: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/
It builds against both versions (and BTW it also dumps CMake cache entries).

[sgraenitz]

No, everything is being built for android. Cross-compiling lldb-server involves cross-compiling llvm libraries, clang libraries, and if you've got swift in the picture, swift host libraries. LLVM and clang libraries are built against the libc++ from the android NDK, but in standalone builds, LLDB will try to link against the freshly built libc++ from LLVM. You get loads of undefined references to symbols from the NDK's libc++.

For the link_directories line: if you cross-compile the entire tree and this tree contains libc++, wouldn't it be natural that LLDB will try to link against the freshly built libc++?
If this is not your intention, then maybe we need a LLDB_EXTERNAL_LIBCXX option instead?

[xiaobai]

In D60180#1456263, @sgraenitz wrote:

No, everything is being built for android. Cross-compiling lldb-server involves cross-compiling llvm libraries, clang libraries, and if you've got swift in the picture, swift host libraries. LLVM and clang libraries are built against the libc++ from the android NDK, but in standalone builds, LLDB will try to link against the freshly built libc++ from LLVM. You get loads of undefined references to symbols from the NDK's libc++.

For the link_directories line: if you cross-compile the entire tree and this tree contains libc++, wouldn't it be natural that LLDB will try to link against the freshly built libc++?
If this is not your intention, then maybe we need a LLDB_EXTERNAL_LIBCXX option instead?

I don't agree that this is the natural thing to do. From my perspective, you should be using the same sysroot to build LLVM and LLDB. In my specific problem, when you build any of the lldb tools (e.g. lldb-server) it fails to link because you are building the tool against the freshly built libc++ but the llvm libraries you had previously built were linked against the NDK's libc++.

In D60180#1456239, @sgraenitz wrote:

If there's a situation where any of these variables don't get propagated from the LLVMConfig, I'd like to know about it so I can add comments above these variables explaining why these variables are set since it's non-obvious just from grepping the lldb tree.

It's usually a good idea to check both, the LLVMConfig.cmake from a build-tree and from an install-tree. They are quite different and LLDB standalone should work with both.

You can check the green dragon bot that we added recently: http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-standalone/
It builds against both versions (and BTW it also dumps CMake cache entries).

Ooh, I like this bot. Thanks for pointing it out!

[sgraenitz]

you should be using the same sysroot to build LLVM and LLDB. In my specific problem, when you build any of the lldb tools (e.g. lldb-server) it fails to link because you are building the tool against the freshly built libc++ but the llvm libraries you had previously built were linked against the NDK's libc++.

Ok agreed.

clang and lld do the same and/but it's old code (3+ years). Well, hope it's well tested. Please keep an eye on the bots.

[labath]

In D60180#1456263, @sgraenitz wrote:

No, everything is being built for android. Cross-compiling lldb-server involves cross-compiling llvm libraries, clang libraries, and if you've got swift in the picture, swift host libraries. LLVM and clang libraries are built against the libc++ from the android NDK, but in standalone builds, LLDB will try to link against the freshly built libc++ from LLVM. You get loads of undefined references to symbols from the NDK's libc++.

For the link_directories line: if you cross-compile the entire tree and this tree contains libc++, wouldn't it be natural that LLDB will try to link against the freshly built libc++?
If this is not your intention, then maybe we need a LLDB_EXTERNAL_LIBCXX option instead?

Well.. you definitely shouldn't link against the fresh libc++, but still use the headers from the system one (which I suspect is the cause of the undefined symbols Alex is seeing).

FWIW, I don't think cross-compilation should be a factor here. I can see somebody may want to build a fresh libc++ and then link llvm (all of it) against that, but that is true regardless of whether you're cross-compiling or not. However, I wouldn't expect this to be the default because then the built binaries wouldn't play well with anything else on this system, if the system happens to use a different C++ library (or just an incompatible version of libc++).

cmake/modules/LLDBStandalone.cmake

if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   find_package(Clang REQUIRED CONFIG
     HINTS "${LLDB_PATH_TO_CLANG_BUILD}" NO_CMAKE_FIND_ROOT_PATH)
 
   # We set LLVM_CMAKE_PATH so that GetSVN.cmake is found correctly when building SVNVersion.inc
   set(LLVM_CMAKE_PATH ${LLVM_CMAKE_DIR} CACHE PATH "Path to LLVM CMake modules")
 
   set(LLVM_MAIN_SRC_DIR ${LLVM_BUILD_MAIN_SRC_DIR} CACHE PATH "Path to LLVM source tree")
   set(LLVM_MAIN_INCLUDE_DIR ${LLVM_MAIN_INCLUDE_DIR} CACHE PATH "Path to llvm/include")
-  set(LLVM_LIBRARY_DIR ${LLVM_LIBRARY_DIR} CACHE PATH "Path to llvm/lib")
   set(LLVM_BINARY_DIR ${LLVM_BINARY_DIR} CACHE PATH "Path to LLVM build tree")
 
   set(lit_file_name "llvm-lit")
   if(CMAKE_HOST_WIN32 AND NOT CYGWIN)
     set(lit_file_name "${lit_file_name}.py")
   endif()
   set(LLVM_DEFAULT_EXTERNAL_LIT "${LLVM_TOOLS_BINARY_DIR}/${lit_file_name}" CACHE PATH "Path to llvm-lit")
 
if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
   set(LLVM_INCLUDE_TESTS ON CACHE INTERNAL "")
 
   set(CMAKE_INCLUDE_CURRENT_DIR ON)
   include_directories(
     "${CMAKE_BINARY_DIR}/include"
     "${LLVM_INCLUDE_DIRS}"
     "${CLANG_INCLUDE_DIRS}")
 
-  link_directories("${LLVM_LIBRARY_DIR}")
-
   set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
   set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
   set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
 
   set(LLDB_BUILT_STANDALONE 1)
 endif()