Page MenuHomePhabricator

[CMake] Fix RPATH handling for LLDB.framework
ClosedPublic

Authored by sgraenitz on Feb 8 2019, 5:38 PM.

Details

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)

Diff Detail

Event Timeline

lanza created this revision.Feb 8 2019, 5:38 PM

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

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 ↗(On Diff #186085)

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.

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)

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

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?

@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 added a comment.Feb 12 2019, 6:15 PM

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 commandeered this revision.Feb 13 2019, 5:41 AM
sgraenitz edited reviewers, added: lanza; removed: sgraenitz.
sgraenitz retitled this revision from Convert genexp that used an actual string instead of the genexp to [CMake] Fix RPATH handling for LLDB.framework.
sgraenitz edited the summary of this revision. (Show Details)
sgraenitz updated this revision to Diff 186634.Feb 13 2019, 5:44 AM

Submit proposal from Diff 186517

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 accepted this revision.Feb 13 2019, 12:17 PM

Excellent! Thank you

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.

This revision is now accepted and ready to land.Feb 13 2019, 12:17 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2019, 9:36 AM