[cmake] Make dependencies of lldb libraries private

[labath]

Summary

The dependencies of our libraries (only liblldb, really) we marked as public, which caused all
their dependencies to be repeated when linking any executables to them. This is a problem because
then all the .a files would be linked twice, once to liblldb and once again to lldb. I am not
sure why, but for some reason this only surfaced when doing a LLVM_LINK_LLVM_DYLIB=ON build,
where we ended having two copies of some symbols and different parts of code referring to
different copies.

Setting the dependencies as private required a fixup of the argdumper link command, as it was not
actually linking to liblldb (it was referring to symbols from the lldb_private namespace, which
are not exposed in liblldb), but rather to the individual component libraries (where these
symbols are still available). Since these symbols are now not added to the command line as
dependencies of liblldb, I needed to add them explicitly.

[labath]

I don't quite know how windows linking works, so please give this a run to make sure it does not break anything for you.

[zturner]

It's a holiday over here today, so i can't look at this until tomorrow.

[tfiala]

Setting the dependencies as private required a fixup of the argdumper link command, as it was not
actually linking to liblldb (it was referring to symbols from the lldb_private namespace, which
are not exposed in liblldb),

At some point lldb-argdumper is planned to be reworked just slightly so it had no dependencies on the lldb core. (That would have avoided this I suspect.)

[labath]

Zachary, could you take a look at this please. I'd like to squeeze this into 3.8 if it is working..

At some point lldb-argdumper is planned to be reworked just slightly so it had no dependencies on the lldb core. (That would have avoided this I suspect.)

It would, although I don't see a problem with the argdumper re-using the json parser from lldb (even though the parser is not the public interface of liblldb). The whole LINKER_SUPPORTS_GROUPS thing could be beautified a bit (I might do that afterwards), but otherwise, the whole thing seems reasonable to me.

[zturner]

I get linker errors on Windows with this. The reason is that Windows doesn't have getopt, but getopt is called from from Driver.cpp and linked into the main lldb executable. Since liblldb is built as a shared library, when the keyword is public it's linking the liblldb inputs into the executable, which works. But when liblldb uses the private keyword, then the inputs don't get fed into lldb.exe and it fails to link.

A better solution would involve layering the windows getopt implementation more appropriately so that it could just be linked directly into lldb.exe. But for now, I think you will need to put this keyword change behind an OS flag so that it continues to use public on Windows (perhaps with a comment explaining why this is necessary)

[labath]

Use private keyword for linux only

[labath]

I think a similar command as for argdumper would fix this (probably target_link_libraries(lldb, lldbHost)). I thought about doing that instead, but then I realised that netbsd probably needs this as well, and lldb-mi seems to be using getopt also, etc. So I figured I'll make this as surgical as possible, and enable the fix on linux only.

When this gets into 3.8, I will add a separate patch, which sets the private keyword everywhere, getopt (through lldbHost) to lldb and lldb-mi on relevant platforms. How does that sound?

[zturner]

Unfortunately that's not going to work. Because lldb libraries are not layered very well, linking against any one library is going to cause a transitive link dependency on every other library. I did a lot of work to improve that in order to get the Python stuff separated out, but it's still not in a great state. So right now, there's no way to just link against host, because that's the same as linking against everything

[labath]

Let's try a different approach. This should disable the logic of
LINK_LLVM_DYLIB for lldb binaries, and thereby enabling that build
to work. It should be the safest thing in the short term, and we
can figure out a better fix later.

[labath]

Ping?

[zturner]

Sorry for the delay, lgtm

lldb/trunk/cmake/modules/AddLLDB.cmake

macro(add_lldb_library name)
   #PIC not needed on Win
   if (NOT MSVC)
     set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fPIC")
   endif()
 
   if (PARAM_OBJECT)
     add_library(${name} ${libkind} ${srcs})
   else()
-    llvm_add_library(${name} ${libkind} ${srcs})
+    llvm_add_library(${name} ${libkind} DISABLE_LLVM_LINK_LLVM_DYLIB ${srcs})
 
     lldb_link_common_libs(${name} "${libkind}")
 
     if (PARAM_SHARED)
       if (LLDB_LINKER_SUPPORTS_GROUPS)
         target_link_libraries(${name} ${cmake_2_8_12_PUBLIC}
                     -Wl,--start-group ${CLANG_USED_LIBS} -Wl,--end-group)
       else()
macro(add_lldb_library name)
   # but it is simple enough to make all of LLDB depend on some of those
   # headers without negatively impacting much of anything.
   add_dependencies(${name} libclang)
 
   set_target_properties(${name} PROPERTIES FOLDER "lldb libraries")
 endmacro(add_lldb_library)
 
 macro(add_lldb_executable name)
-  add_llvm_executable(${name} ${ARGN})
+  add_llvm_executable(${name} DISABLE_LLVM_LINK_LLVM_DYLIB ${ARGN})
   set_target_properties(${name} PROPERTIES FOLDER "lldb executables")
 endmacro(add_lldb_executable)
 
 # Support appending linker flags to an existing target.
 # This will preserve the existing linker flags on the
 # target, if there are any.
 function(lldb_append_link_flags target_name new_link_flags)
   # Retrieve existing linker flags.