[CMake] Fix RPATH handling for LLDB.framework

[sgraenitz]

Summary

Generator expressions are not supported in the BUILD_RPATH target property.
BUILD_RPATH is only supported in 3.8+ https://cliutils.gitlab.io/modern-cmake/chapters/intro/newcmake.html
LLDB_FRAMEWORK_INSTALL_DIR should not overwrite, but rather add an install RPATH (and it should be the first)

[xiaobai]

Looks fine to me but you should probably let @sgraenitz look over it.

[sgraenitz]

A chunk of code referred to a generator expression wrapped in quotations and then referred to later again in quotations which caused cmake to think it was something to be referred to as a string.

Are you sure this is the actual reason for your issue? Except for white-space handling the quotations shouldn't make a difference. Having genex's sticking around in build output is typically a sign of a missing generation-time-step for the respective target.

cmake/modules/AddLLDB.cmake

179

Originally I added it only for symmetry with rpaths_install_tree and to have a place for the comment. The mechanism here is different from regular RPATH handling in LLVM, which is why the comment might be useful.

[sgraenitz]

Tested it for the lldb driver and you are right, that the genex ends up in the binary -- interesting. I then also tested with your change, but it gave me the same:

Load command 17
          cmd LC_RPATH
      cmdsize 40
         path $<TARGET_FILE:liblldb> (offset 12)

[sgraenitz]

Looking at the code more closely, it turns out this is not the only issue:

• apparently generator expressions are not supported in the BUILD_RPATH target property
• BUILD_RPATH is only supported in 3.8+ https://cliutils.gitlab.io/modern-cmake/chapters/intro/newcmake.html
• LLDB_FRAMEWORK_INSTALL_DIR should not overwrite, but rather add an install RPATH (and it should be the first)

I prepared a draft and did a few quick tests so far: https://reviews.llvm.org/differential/diff/186517/
What do you think?

@lanza Thanks for bringing this up! Do you want to land the fixes or would you mind me commandeering this revision?

[xiaobai]

@sgraenitz: Oh! Thanks for the explanation and the draft, that's pretty helpful. I wanted to point out that in the draft you are doing a check for CMake version in lldb_setup_framework_rpaths_in_tool but we are also doing a CMake version check in LLDBConfig for the same purpose. Seems like you could consolidate those?

[lanza]

Great, thanks Stefan! You're right, my change still did include the $<TARGET_FILE_DIR:liblldb> in the build tree variant.. I guess I only checked the installed variant before submitting. Feel free to commandeer this.

[sgraenitz]

Submit proposal from Diff 186517

[sgraenitz]

In D57989#1395232, @xiaobai wrote:

we are also doing a CMake version check in LLDBConfig for the same purpose. Seems like you could consolidate those?

Yep that makes sense. Done.

BTW this should not be relevant for release 8.0, because the install RPATH was only broken when configuring with LLDB_BUILD_FRAMEWORK=ON && LLDB_FRAMEWORK_INSTALL_DIR="" explicitly, which is quite unlikely for anyone to do.

[xiaobai]

Excellent! Thank you

In D57989#1396142, @sgraenitz wrote:

BTW this should not be relevant for release 8.0, because the install RPATH was only broken when configuring with LLDB_BUILD_FRAMEWORK=ON && LLDB_FRAMEWORK_INSTALL_DIR="" explicitly, which is quite unlikely for anyone to do.

Makes sense to me.

cmake/modules/AddLLDB.cmake

 #
 # Note that the LLVM install-tree doesn't match the enduser system structure
 # for LLDB.framework, so by default dependent tools will not be functional in
 # their install location. The LLDB_FRAMEWORK_INSTALL_DIR variable allows to fix
 # this. If specified, it causes the install-tree location of the framework to be
 # added as an extra RPATH below.
 #
 function(lldb_setup_framework_rpaths_in_tool name)
-  # In the build-tree, we know the exact path to the binary in the framework.
-  set(rpath_build_tree "$<TARGET_FILE:liblldb>")
-
   # The installed framework is relocatable and can be in different locations.
-  set(rpaths_install_tree "@loader_path/../../../SharedFrameworks")
-  list(APPEND rpaths_install_tree "@loader_path/../../System/Library/PrivateFrameworks")
-  list(APPEND rpaths_install_tree "@loader_path/../../Library/PrivateFrameworks")
+  set(rpaths_install_tree)
 
   if(LLDB_FRAMEWORK_INSTALL_DIR)
-    set(rpaths_install_tree "@loader_path/../${LLDB_FRAMEWORK_INSTALL_DIR}")
+    list(APPEND rpaths_install_tree "@loader_path/../${LLDB_FRAMEWORK_INSTALL_DIR}")
   endif()
 
+  list(APPEND rpaths_install_tree "@loader_path/../../../SharedFrameworks")
+  list(APPEND rpaths_install_tree "@loader_path/../../System/Library/PrivateFrameworks")
+  list(APPEND rpaths_install_tree "@loader_path/../../Library/PrivateFrameworks")
+
+  # In the build-tree, we know the exact path to the framework directory.
+  get_target_property(framework_target_dir liblldb LIBRARY_OUTPUT_DIRECTORY)
+
   # If LLDB_NO_INSTALL_DEFAULT_RPATH was NOT enabled (default), this overwrites
   # the default settings from llvm_setup_rpath().
   set_target_properties(${name} PROPERTIES
     BUILD_WITH_INSTALL_RPATH OFF
-    BUILD_RPATH "${rpath_build_tree}"
+    BUILD_RPATH "${framework_target_dir}"
     INSTALL_RPATH "${rpaths_install_tree}"
   )
 
   add_dependencies(${name} lldb-framework)
 endfunction()

cmake/modules/LLDBConfig.cmake

 option(LLDB_BUILD_FRAMEWORK "Build LLDB.framework (Darwin only)" OFF)
 option(LLDB_NO_INSTALL_DEFAULT_RPATH "Disable default RPATH settings in binaries" OFF)
 
 if(LLDB_BUILD_FRAMEWORK)
   if(NOT APPLE)
     message(FATAL_ERROR "LLDB.framework can only be generated when targeting Apple platforms")
   endif()
   # CMake 3.6 did not correctly emit POST_BUILD commands for Apple Framework targets
-  if(CMAKE_VERSION VERSION_LESS 3.7)
-    message(FATAL_ERROR "LLDB_BUILD_FRAMEWORK is not supported on CMake < 3.7")
+  # CMake < 3.8 did not have the BUILD_RPATH target property
+  if(CMAKE_VERSION VERSION_LESS 3.8)
+    message(FATAL_ERROR "LLDB_BUILD_FRAMEWORK is not supported on CMake < 3.8")
   endif()
 
   set(LLDB_FRAMEWORK_VERSION A CACHE STRING "LLDB.framework version (default is A)")
   set(LLDB_FRAMEWORK_BUILD_DIR bin CACHE STRING "Output directory for LLDB.framework")
   set(LLDB_FRAMEWORK_INSTALL_DIR Library/Frameworks CACHE STRING "Install directory for LLDB.framework")
   set(LLDB_FRAMEWORK_TOOLS darwin-debug;debugserver;lldb-argdumper;lldb-server CACHE STRING
       "List of tools to include in LLDB.framework/Resources")