[CMake] Don't set absolute paths as install runpaths on ELF platforms in llvm_setup_rpath()

[finagolfin]

Summary

If any LLVM subprojects are built separately, the LLVM build directory LLVM_LIBRARY_DIR is added to both the build and install runpaths in llvm_setup_rpath(), which is incorrect when installed. Separate the build and install runpaths on ELF platforms and finally remove the incorrect call to this function for compiler-rt, as previously attempted in 21c008d5a5b. That prior attempt was reverted in 959dbd1761c, where it was said to break the build on macOS and Windows, so I made sure to keep those platforms the same.

Two examples of incorrect runpaths that are currently added, one from the latest LLVM 16 toolchain for linux x86_64:

> readelf -d clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.*so | ag "File:|runpath"
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.asan.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.dyndd.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.hwasan_aliases.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.hwasan.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.memprof.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.scudo_standalone.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.tsan.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.ubsan_minimal.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]
File: clang+llvm-16.0.0-x86_64-linux-gnu-ubuntu-18.04/lib/clang/16/lib/x86_64-unknown-linux-gnu/libclang_rt.ubsan_standalone.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/tmp/llvm_release/final/Phase3/Release/llvmCore-16.0.0-final.obj/./lib]

Another is in the Swift toolchain, which builds lldb separately:

> readelf -d swift-5.9-DEVELOPMENT-SNAPSHOT-2023-03-24-a-ubuntu20.04/usr/{bin/lldb*,lib/liblldb.so}|ag "File:|runpath"
File: swift-5.9-DEVELOPMENT-SNAPSHOT-2023-03-24-a-ubuntu20.04/usr/bin/lldb
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/home/build-user/build/buildbot_linux/llvm-linux-x86_64/./lib]
File: swift-5.9-DEVELOPMENT-SNAPSHOT-2023-03-24-a-ubuntu20.04/usr/bin/lldb-argdumper
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/home/build-user/build/buildbot_linux/llvm-linux-x86_64/./lib]
File: swift-5.9-DEVELOPMENT-SNAPSHOT-2023-03-24-a-ubuntu20.04/usr/bin/lldb-server
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/home/build-user/build/buildbot_linux/llvm-linux-x86_64/./lib]
File: swift-5.9-DEVELOPMENT-SNAPSHOT-2023-03-24-a-ubuntu20.04/usr/lib/liblldb.so
 0x000000000000001d (RUNPATH)            Library runpath: [$ORIGIN/../lib:/home/build-user/build/buildbot_linux/llvm-linux-x86_64/./lib:/home/build-user/build/buildbot_linux/swift-linux-x86_64/lib/swift/linux:$ORIGIN/../lib/swift/linux]

This patch should fix this problem of absolute paths from the build host leaking out into the toolchain's runpaths.

@JDevlieghere, please review or suggest who might.
@phosek, let me know if you think this will fix the compiler-rt issues properly. I'm not sure how calling this function ever helped on Windows, as it should exit without setting anything on there.
@jsji, please review the AIX changes, where I tried to separate the build directories from the install directory.

[finagolfin]

Ping @beanz, you added this CMake function with @mgorny in 61e900b4057, please let me know what you think.

[mgorny]

At a first glance, it seems to make sense, at least on Linux. Can't say much about other systems but I guess buildbots will tell.

[finagolfin]

@JDevlieghere, does this need to be run through any further build validation or can it be merged now?

[finagolfin]

Ping @mgorny, can you merge? Or does anything need to be done to check further buildbot validation?

[asb]

This commit seems to be breaking my standard build config (at least, an incremental build fails when trying to regenerate ninja, with many errors of the form:

CMake Error at cmake/modules/AddLLVM.cmake:588 (add_library):
  The install of the LLVMDemangle target requires changing an RPATH from the
  build tree, but this is not supported with the Ninja generator unless on an
  ELF-based or XCOFF-based platform.  The CMAKE_BUILD_WITH_INSTALL_RPATH
  variable may be set to avoid this relinking step.
Call Stack (most recent call first):
  cmake/modules/AddLLVM.cmake:848 (llvm_add_library)
  cmake/modules/AddLLVM.cmake:825 (add_llvm_library)
  lib/Demangle/CMakeLists.txt:1 (add_llvm_component_library)

and a final error:

CMake Generate step failed.  Build files cannot be regenerated correctly.

This is with a build config like the following (split dwarf and shared libs are I suppose slightly uncommon):

cmake -G Ninja -DCMAKE_BUILD_TYPE="Debug" \
  -DLLVM_ENABLE_PROJECTS="clang;lld" \
  -DBUILD_SHARED_LIBS=True -DLLVM_USE_SPLIT_DWARF=True \
  -DLLVM_BUILD_TESTS=True \
  -DLLVM_CCACHE_BUILD=ON \
  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
  -DLLVM_ENABLE_LLD=True \
  -DLLVM_TARGETS_TO_BUILD="all" \
  -DLLVM_APPEND_VC_REV=False ../../llvm

My usual release config seems ok:

cmake -G Ninja -DCMAKE_BUILD_TYPE="Release" \
  -DLLVM_ENABLE_PROJECTS="clang;lld" \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DLLVM_CCACHE_BUILD=ON \
  -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ \
  -DLLVM_ENABLE_LLD=True \
  -DLLVM_TARGETS_TO_BUILD="all" \
  -DLLVM_APPEND_VC_REV=False ../../llvm

[finagolfin]

@asb, thanks for letting us know, could you share more info on what platform you are building for? Your CMake error says this is supported on ELF, so you are building on macOS? As you can see in the OP above, @phosek reported a similar error before, so I coded this very defensively to always apply BUILD_WITH_INSTALL_RPATH for Mac and Windows. Perhaps this also affects you on other rare configurations, so we will need more info on your config to come up with a fix.

[asb]

In D146918#4282895, @buttaface wrote:

@asb, thanks for letting us know, could you share more info on what platform you are building for? Your CMake error says this is supported on ELF, so you are building on macOS? As you can see in the OP above, @phosek reported a similar error before, so I coded this very defensively to always apply BUILD_WITH_INSTALL_RPATH for Mac and Windows. Perhaps this also affects you on other rare configurations, so we will need more info on your config to come up with a fix.

I'm on bog standard Linux (current Arch Linux, on x86_64). The issue occurs when just rerunning cmake --build . or ninja within my usual debug build directory (i.e. an incremental build). Reverting this patch fixes it. However, I've since tested creating a new build directory and redoing cmake -G Ninja <stuff I listed in my last message> and that appears OK. So this is perhaps not a critical issue, though I can't remember the last time I had something similar.

[finagolfin]

OK, so this sounds like a CMake error with getting confused by the updated config, so we will have to tell people to do a clean build after this change.

@JDevlieghere, is there some standard way of notifying LLVM devs to do a clean rebuild after this commit?

[finagolfin]

@JDevlieghere, what do you think about nominating this for LLVM 16.0.4? It seems pretty safe, along with the followup in D148866.

[peterwaller-arm]

@finagolfin

If any LLVM subprojects are built separately, the LLVM build directory LLVM_LIBRARY_DIR is added to both the build and install runpaths in llvm_setup_rpath(), which is incorrect when installed.

Can I check if this statement is correct? It doesn't sound so to me.

I think this broke the nixpkgs build, which installs LLVM before doing an out of tree clang build which gets installed separately. The LLVM build directory is unavailable at clang build time. In this scenario, LLVM's LLVMConfig.cmake sets LLVM_LIBRARY_DIR=the install path during clang's find_package(LLVM) via the installed LLVMConfig.cmake;

[peterwaller-arm]

Looks like my previous message was truncated. I think it is 'not incorrect' for the rpath to point to LLVM_LIBRARY_DIR, which is the install path at least for the nixpkgs llvm install. In this case, the prefix clang is installed to differs from the one LLVM is installed to. If the rpath isn't set for clang there, it can't find libLLVM.

[finagolfin]

Can I check if this statement is correct?

Sure, simply run the readelf commands from this commit message or something similar on the clang toolchain you built, both in your build and install directories, and perhaps with this commit reverted too.

I think it is 'not incorrect' for the rpath to point to LLVM_LIBRARY_DIR, which is the install path at least for the nixpkgs llvm install.

You may be right that it is okay to add LLVM_LIBRARY_DIR to the install runpath if the subproject built separately is being built against already-installed LLVM libraries, but that is not usually the case. For example, compiler-rt is always built separately, so it previously always had these incorrect runpaths to the LLVM build directory, as I showed with the official LLVM 16 build for linux in this commit message.

Given that it isn't usually the case, we certainly shouldn't add that install runpath by default, and you can always add it to the configuration yourself for scenarios like yours.

[peterwaller-arm]

Sure, simply run the readelf commands from this commit message or something similar on the clang toolchain you built, both in your build and install directories, and perhaps with this commit reverted too.

To clarify, I meant to ask if it's correct to say "It's incorrect for LLVM_LIBRARY_DIR to be in the install rpath", because I wondered how else the cmake configuration was going to configure this automatically.

For what it's worth, I found this differential by noticing that in the broken build, the _install_rpath had been changed relative to the working one and that the ninja install phase was removing the rpath from the binaries.

you can always add it to the configuration yourself for scenarios like yours.

If I understand you correctly then, it is not expected that the find_package(LLVM) machinery will produce a functioning binary without also setting CMAKE_INSTALL_RPATH to point to the LLVM install directory? Is that right? I suppose that is what I am questioning, I'd have hoped the build system generator would have made that work.

I don't yet fully understand what is or isn't meant to be supported, perhaps you can help? Some questions that come to mind:

• Shouldn't users be setting LLVM_LIBRARY_DIR to their install dir rather than the build dir?
• Under what circumstances is it desirable to set LLVM_LIBRARY_DIR to the build directory?
• If they're setting it to the build directory rather than the install directory, why is it incorrect to refer to the build directory as has been configured, once installed?
• It seems suspicious to me that it has been necessary to diverge behavior across platforms here ("it was said to break the build on macOS and Windows", from the commit message).

In my mind the problem with asking others to set CMAKE_INSTALL_RPATH manually is that this is meant to be a detail encapsulated in the build system by find_package(LLVM), is it not? It's nice if LLVM works as a cmake package like any other, rather than having to special case it. It seems to me this would break other users of find_package(LLVM).

I wonder if there is a better way to support pointing an LLVM_LIBRARY_DIR at a build directory (and stripping the rpath at install time) but I don't yet understand the use case well enough.

[finagolfin]

To clarify, I meant to ask if it's correct to say "It's incorrect for LLVM_LIBRARY_DIR to be in the install rpath"

I was unaware that LLVM_LIBRARY_DIR can be overridden to an installed directory and was only aware of the much more common usecase where it is set to the LLVM build directory by default, so my statement should be amended to "It's incorrect for LLVM_LIBRARY_DIR to be in the install rpath much of the time," now that you pointed this out.

it is not expected that the find_package(LLVM) machinery will produce a functioning binary without also setting CMAKE_INSTALL_RPATH to point to the LLVM install directory? Is that right?

I would say that these runpaths have been handled somewhat crudely so far, and favored some uncommon usecases over more common ones. I think that variable presents one way to override that, so I pointed it out.

I suppose that is what I am questioning, I'd have hoped the build system generator would have made that work.

Given that LLVM_LIBRARY_DIR can be either a build directory or an install directory, I don't see how CMake can figure that out automatically: it will require developer input.

Shouldn't users be setting LLVM_LIBRARY_DIR to their install dir rather than the build dir?

As my link above shows, the default is the build dir.

Under what circumstances is it desirable to set LLVM_LIBRARY_DIR to the build directory?

Unsure, I'm just dealing with that as it is now.

If they're setting it to the build directory rather than the install directory, why is it incorrect to refer to the build directory as has been configured, once installed?

Can you rephrase this? I don't know what you mean. The best I can make out is that you mean, "why is it incorrect to add the build directory to the install runpath?" Because as shown in the examples above, the build directory usually doesn't exist in the finally installed system.

It seems suspicious to me that it has been necessary to diverge behavior across platforms here ("it was said to break the build on macOS and Windows", from the commit message).

That was only for the change in compiler-rt, that change to llvm_setup_rpath() was reverted in D148866. As noted in a prior comment on this patch, I coded this very defensively because of the prior failed attempts to remove the compiler-rt runpaths that I also linked, but based on @asb's comment above about seeing this error on linux too, that was almost certainly a mistaken attribution by those devs in the past, at least on Windows. That's why I removed that Windows line later, and had no problem.

I think you are right that this patch broke your usecase, but your usecase is so uncommon, ie building the base LLVM libs and clang separately then installing them to different install prefixes, that we should prioritize the more common ones. However, if the existing CMake overrides don't work well for you, feel free to submit a patch with another toggle that works well for your usecase.

[peterwaller-arm]

First off, thanks for the discourse!

Can you rephrase this? I don't know what you mean. [...]

You're right, I didn't express this well. My thinking was that logically: if e.g. clang is broken when you install it because it's pointing at an LLVM build directory (which no longer exists), why wouldn't you also install LLVM? And therefore, why wouldn't you point LLVM_LIBRARY_DIR at the LLVM install rather than the build directory?

The logical issue is based on these notions that I currently hold:

• The change in this differential affects installs.
• If the install is the thing which is broken and needs fixing, we are installing, by definition.
• Why not do the right thing for installing overall, and therefore point LLVM_LIBRARY_DIR to the install path?
• Nobody expects to point LLVM_LIBRARY_DIR to a build dir, and then remove that build dir, and expect the installed references work.
• If you're going to expect it to work in the end (with a removed bulid directory), LLVM must be installed, and therefore LLVM_LIBRARY_DIR should point to that existing install location.

Can you explain a need to a) point LLVM_LIBRARY_DIR to a build directory, b) where a downstream project will get installed, and c) LLVM will not?

It seems to me that if you're pointing LLVM_LIBRARY_DIR at a build directory which differs from the installed one, that is another risky case for something breaking, since the configurations of the build and install may not match.

it is set to the LLVM build directory by default [ed: link to https://github.com/llvm/llvm-project/blob/d763c6e5e2d0a6b34097aa7dabca31e9aff9b0b6/llvm/CMakeLists.txt#L424]

I'm not seeing that as evidence that it's set to the build directory as a more common use case -- what I see there is that those values are meant to match up with llvm-config, which I think of as an install artifact which can be used to determine paths for out of tree projects to build and linking against LLVM, which I would expect to contain install paths and not build paths (?).

[finagolfin]

if e.g. clang is broken when you install it because it's pointing at an LLVM build directory (which no longer exists), why wouldn't you also install LLVM?

Not sure what you mean. It is broken because clang has a non-existent build directory in its install runpath, like /home/build-user/build/buildbot_linux/ in the example above. Installing LLVM in a separate install directory like /usr/bin/ changes nothing about that.

And therefore, why wouldn't you point LLVM_LIBRARY_DIR at the LLVM install rather than the build directory?

Simple, you may not want to install the freshly-built LLVM on the build host and then build against it, as you may not even have root on there. There are many scenarios where you simply want to build against the LLVM build directory, many more than building against the LLVM install directory, which is why the build directory is the default.

It seems to me that if you're pointing LLVM_LIBRARY_DIR at a build directory which differs from the installed one, that is another risky case for something breaking, since the configurations of the build and install may not match.

Not risky at all, it is much more common than your Nixpkgs scenario.

I'm not seeing that as evidence that it's set to the build directory as a more common use case

Generally, the default is the most common usecase, I suspect you will find very few LLVM build recipes that override that default LLVM_LIBRARY_DIR to set their own.

what I see there is that those values are meant to match up with llvm-config, which I think of as an install artifact which can be used to determine paths for out of tree projects to build and linking against LLVM, which I would expect to contain install paths and not build paths

You have the prominent counter-examples of the official LLVM and Swift toolchains using the default of the LLVM build directories instead, as evidenced by this commit message, and I suspect most of the distro LLVM build recipes I linked above simply use that default of the LLVM build directory too.

compiler-rt/cmake/Modules/AddCompilerRT.cmake

foreach(libname ${libnames})
     set_target_properties(${libname} PROPERTIES FOLDER "Compiler-RT Runtime")
     if(LIB_LINK_LIBS)
       target_link_libraries(${libname} PRIVATE ${LIB_LINK_LIBS})
     endif()
     if(builtins_${libname})
       target_link_libraries(${libname} PRIVATE ${builtins_${libname}})
     endif()
     if(${type} STREQUAL "SHARED")
-      if(COMMAND llvm_setup_rpath)
-        llvm_setup_rpath(${libname})
+      if(APPLE OR WIN32)
+        set_property(TARGET ${libname} PROPERTY BUILD_WITH_INSTALL_RPATH ON)
       endif()
       if(WIN32 AND NOT CYGWIN AND NOT MINGW)
         set_target_properties(${libname} PROPERTIES IMPORT_PREFIX "")
         set_target_properties(${libname} PROPERTIES IMPORT_SUFFIX ".lib")
       endif()
       if (APPLE AND NOT CMAKE_LINKER MATCHES ".*lld.*")
         # Ad-hoc sign the dylibs when using Xcode versions older than 12.
         # Xcode 12 shipped with ld64-609.

llvm/cmake/modules/AddLLVM.cmake

function(llvm_setup_rpath name)
   if (APPLE)
     set(_install_name_dir INSTALL_NAME_DIR "@rpath")
     set(_install_rpath "@loader_path/../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
   elseif(${CMAKE_SYSTEM_NAME} MATCHES "AIX" AND BUILD_SHARED_LIBS)
     # $ORIGIN is not interpreted at link time by aix ld.
     # Since BUILD_SHARED_LIBS is only recommended for use by developers,
     # hardcode the rpath to build/install lib dir first in this mode.
     # FIXME: update this when there is better solution.
-    set(_install_rpath "${LLVM_LIBRARY_OUTPUT_INTDIR}" "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+    set(_build_rpath "${LLVM_LIBRARY_OUTPUT_INTDIR}" ${extra_libdir})
+    set(_install_rpath "${CMAKE_INSTALL_PREFIX}/lib${LLVM_LIBDIR_SUFFIX}")
   elseif(UNIX)
-    set(_install_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+    set(_build_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}" ${extra_libdir})
+    set(_install_rpath "\$ORIGIN/../lib${LLVM_LIBDIR_SUFFIX}")
     if(${CMAKE_SYSTEM_NAME} MATCHES "(FreeBSD|DragonFly)")
       set_property(TARGET ${name} APPEND_STRING PROPERTY
                    LINK_FLAGS " -Wl,-z,origin ")
     endif()
     if(LLVM_LINKER_IS_GNULD)
       # $ORIGIN is not interpreted at link time by ld.bfd
       set_property(TARGET ${name} APPEND_STRING PROPERTY
                    LINK_FLAGS " -Wl,-rpath-link,${LLVM_LIBRARY_OUTPUT_INTDIR} ")
     endif()
   else()
     return()
   endif()
 
-  # Enable BUILD_WITH_INSTALL_RPATH unless CMAKE_BUILD_RPATH is set.
+  # Enable BUILD_WITH_INSTALL_RPATH unless CMAKE_BUILD_RPATH is set and not
+  # building for macOS or Windows, as those two platforms seemingly require it.
   if("${CMAKE_BUILD_RPATH}" STREQUAL "")
+    if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin|Windows")
       set_property(TARGET ${name} PROPERTY BUILD_WITH_INSTALL_RPATH ON)
+    else()
+      set_property(TARGET ${name} APPEND PROPERTY BUILD_RPATH "${_build_rpath}")
+    endif()
   endif()
 
   set_target_properties(${name} PROPERTIES
                         INSTALL_RPATH "${_install_rpath}"
                         ${_install_name_dir})
 endfunction()
 
 function(setup_dependency_debugging name)