This is an archive of the discontinued LLVM Phabricator instance.

Add rpath to liblldb so vendors can ship their own python framework (or others)
ClosedPublic

Authored by aadsm on Nov 6 2019, 10:46 PM.

Details

Summary

I want to be able to specify which python framework to use for lldb in macos. With python2.7 we could just rely on the MacOS one but python3.7 is not shipped with the OS.
An alternative is to use the one shipped with Xcode but that could be path dependent or maybe the user doesn't have Xcode installed at all.
A definite solution is to just ship a python framework with lldb. To make this possible I added "@loader_path/../../../" to the rpath so it points to the same directory as the LLDB.framework, this way we can just drop any frameworks there.

Diff Detail

Event Timeline

aadsm created this revision.Nov 6 2019, 10:46 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 6 2019, 10:46 PM
labath added a subscriber: labath.Nov 7 2019, 12:53 AM

In principle, this looks pretty similar to D67942, and my opinion on it is the same -- I don't think we should be in the business of trying to package the transitive set of lldb dependencies. I think the lldb install target should install the stuff that it has built itself, and the rest should be up to some higher level packaging system.

hhb added a comment.Nov 7 2019, 3:00 PM

In principle, this looks pretty similar to D67942, and my opinion on it is the same -- I don't think we should be in the business of trying to package the transitive set of lldb dependencies. I think the lldb install target should install the stuff that it has built itself, and the rest should be up to some higher level packaging system.

+1 it will never end if we go down this way.

I agree on not getting into the business of installing Python ourselves, but there's also the rpath issue. The Windows case is different because you just put all your DLLs in the same directory (or some other directory in your PATH) and you're done. With macOS, you have to specify an rpath for liblldb to be able to find the Python framework. It'd be really nice to support adding additional rpaths in the build system itself (it could be a more generic mechanism; it doesn't have to be Python-specific). CMake does offer CMAKE_INSTALL_RPATH, but that applies to every single target. You could also use install_name_tool after building to modify the rpath, but that feels like a kludge.

How would people feel about a patch that just adds the ability to set additional rpaths on liblldb?

labath added a subscriber: beanz.

I agree on not getting into the business of installing Python ourselves, but there's also the rpath issue. The Windows case is different because you just put all your DLLs in the same directory (or some other directory in your PATH) and you're done. With macOS, you have to specify an rpath for liblldb to be able to find the Python framework. It'd be really nice to support adding additional rpaths in the build system itself (it could be a more generic mechanism; it doesn't have to be Python-specific). CMake does offer CMAKE_INSTALL_RPATH, but that applies to every single target. You could also use install_name_tool after building to modify the rpath, but that feels like a kludge.

How would people feel about a patch that just adds the ability to set additional rpaths on liblldb?

+ @beanz for any cmake/osx/framework insights.

Having the ability to add additional rpaths to the (lib)lldb framework seems fine, if there's no other way to arrange that simply dropping dependent libraries into some directory "just works". It's not just windows where this is possible. On linux (and I assume this applies to non-framework osx builds too), you can do that by just dropping things into the "$prefix/lib" directory -- in our default setup both executables and shared libraries get the rpath of "$ORIGIN/../lib" meaning that both executables and shared libraries can find any additional deps there. Is it possible to do something like that with the framework build too? I.e., set the rpath of the LLDB.framework so that it searches for dependencies next to the framework itself? Then you'd just need to drop the python framework next to lldb...

aadsm added a comment.Nov 8 2019, 9:41 AM

That should work fine yeah. It should be a matter of adding "@loader_path/../../../" to the rpath.

beanz added a comment.Nov 8 2019, 10:13 AM

It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.

In a broader sense, Frameworks are dylibs. They can have all the same rpath magic that a dylib has. When you link a macho, the static linker pulls the install_name from each dylib you link against and embeds those as the dylibs to load. Since Python 3.7's install_name is @rpath based, it means that the dynamic linker will look for it in all the @rpath-relative locations specified by liblldb when linking. The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

It is a bit gross that Python does an @rpath install name, that is generally against convention. I can see adding an option to add rpaths to liblldb making sense. I certainly agree LLDB shouldn't be in the business of packaging transitive dependencies.

In a broader sense, Frameworks are dylibs. They can have all the same rpath magic that a dylib has. When you link a macho, the static linker pulls the install_name from each dylib you link against and embeds those as the dylibs to load. Since Python 3.7's install_name is @rpath based, it means that the dynamic linker will look for it in all the @rpath-relative locations specified by liblldb when linking. The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

The assumption here is that you'd have a directory layout like the following:

|- LLDB.framework
  |- Versions
    |- A
      |- LLDB
|- Python3.framework
  |- Versions
    |- 3.7
      |- Python3

Python3's install name is @rpath/Python3.framework/Versions/3.7/Python3, so by adding -rpath "@loader_path/../../.." to LLDB, you make @labath's request work:

Is it possible to do something like that with the framework build too? I.e., set the rpath of the LLDB.framework so that it searches for dependencies next to the framework itself? Then you'd just need to drop the python framework next to lldb...

The assumption, of course, is that LLDB is being built as a framework as well, but I believe that's the standard setup on Darwin.

aadsm updated this revision to Diff 228634.Nov 10 2019, 10:29 PM

Updating to only add an rpath that points to the directory where LLDB.frameworks gets installed.

The proposed path in this patch, -rpath "@loader_path/../../../", uses the @loader_path expansion which is the directory containing the binary that the load command is in (in this case liblldb's directory). Popping 3 directories up from that is likely not sane in most build configurations, but if it works for you meh...

This is exactly what Apple ships with Xcode today:

$ otool -l /Applications/Xcode.app/Contents/SharedFrameworks/LLDB.framework/Versions/A/liblldbPluginScriptInterpreterPython3.dylib | grep RPATH -A 2
          cmd LC_RPATH
      cmdsize 72
         path @loader_path/../../../../../../../../Library/Frameworks/ (offset 12)
--
          cmd LC_RPATH
      cmdsize 72
         path @loader_path/../../../../../Developer/Library/Frameworks/ (offset 12)
--
          cmd LC_RPATH
      cmdsize 72
         path @loader_path/../../../../Developer/Library/Frameworks/ (offset 12)
--
          cmd LC_RPATH
      cmdsize 48
         path @loader_path/../../../../Frameworks (offset 12)
--
          cmd LC_RPATH
      cmdsize 40
         path @loader_path/../../../ (offset 12)

$ otool -L /Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.7/Python3
/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.7/Python3:
	@rpath/Python3.framework/Versions/3.7/Python3 (compatibility version 3.7.0, current version 3.7.0)

But they build liblldbPluginScriptInterpreterPython3 as a dylib instead of statically inside liblldb (so that they can have both 2.7 and 3.7).

labath accepted this revision.Nov 11 2019, 1:23 AM

This seems reasonable to me.

path @loader_path/../../../../../../../../Library/Frameworks/

LOL :)

This revision is now accepted and ready to land.Nov 11 2019, 1:23 AM

Commit message needs to be updated.

lldb/cmake/modules/LLDBFramework.cmake
124

You may wanna add the reason for this to the comment as well.

aadsm edited the summary of this revision. (Show Details)Nov 11 2019, 10:19 AM
aadsm retitled this revision from Add cmake variables to specify a python framework to ship with lldb to Add rpath to liblldb so vendors can ship their own python framework (or others).
smeenai added inline comments.Nov 11 2019, 10:26 AM
lldb/cmake/modules/LLDBFramework.cmake
124

Doesn't need to be super detailed ... even just adding something like "so that we can find any frameworks installed beside it" should be enough.

aadsm updated this revision to Diff 228765.Nov 11 2019, 1:33 PM
aadsm edited the summary of this revision. (Show Details)

Clarify the point of the install_path

smeenai accepted this revision.Nov 11 2019, 2:30 PM

LGTM

lldb/cmake/modules/LLDBFramework.cmake
125

Nit: 80 columns

This revision was automatically updated to reflect the committed changes.