[lldb][cmake] Remove local rpaths from the build host on ELF platforms #6456
Conversation
| elseif(APSM_BOOTSTRAPPING_MODE STREQUAL "BOOTSTRAPPING") | ||
| target_link_directories(${target} PRIVATE "${LLDB_SWIFT_LIBS}/macosx") | ||
| set(SWIFT_RPATH "${LLDB_SWIFT_LIBS}/macosx") | ||
| set(SWIFT_BUILD_RPATH "${LLDB_SWIFT_LIBS}/macosx") | ||
| set(SWIFT_INSTALL_RPATH "${LLDB_SWIFT_LIBS}/macosx") |
There was a problem hiding this comment.
I don't know if this rpath needs to be set for both the build and install on macOS, but just kept it the same for now.
There was a problem hiding this comment.
It shouldn't be. Currently, SWIFT_RPATH is set as both the build and install rpath for macOS below, whereas I simply use two variables set to the same rpath on macOS after this pull, ie the end result should be the same.
I only had to copy these for macOS because on linux I need to set different build and install rpaths.
| set(SWIFT_RPATH "${LLDB_SWIFT_LIBS}/linux;$ORIGIN/../lib/swift/linux") | ||
| string(TOLOWER ${CMAKE_SYSTEM_NAME} platform) | ||
| set(SWIFT_BUILD_RPATH "${LLDB_SWIFT_LIBS}/${platform}") | ||
| set(SWIFT_INSTALL_RPATH "$ORIGIN/swift/${platform}") |
There was a problem hiding this comment.
As long as I'm modifying this line, I extended it to support more platforms and modified it to look in swift/os, not ../lib/swift/os. The former works fine for liblldb.so, whereas the latter is more general and can be used by lldb executables in bin/ too.
However, I checked and the only one it's added to is lldb-test, which the Swift toolchain doesn't ship. I can change this back to the more general ../lib/swift/os if wanted.
finagolfin
changed the title
|
@augusto2112, please run the CI on this. |
|
@swift-ci test |
|
Alright, good sign that the CI passes, could someone build a toolchain for linux? I'll check its runpaths to make sure everything is okay. |
|
@bnbarham, mind triggering a CI build of the linux toolchain? |
|
@swift-ci Please Build Toolchain Linux Platform |
|
@swift-ci please build toolchain Linux platform EDIT: Apparently doesn't work in llvm-project. I've started over in swiftlang/swift#62616 |
|
@buttaface toolchain build succeeded - https://ci.swift.org/job/swift-PR-toolchain-Linux/386 |
|
@bnbarham, would you run the CI on this pull with swiftlang/swift-integration-tests#114 applied? That will make sure I got everything, maybe by running the CI from swiftlang/swift#62616 again if it can't be done from this repo. |
|
Tests or toolchain again? |
|
Just the tests should be enough. |
|
swiftlang/swift-integration-tests#114 @swift-ci please test |
|
@bnbarham, this repo doesn't run the integration tests, as I suspected, you will have to run these two pulls on that blank Swift repo pull again. |
|
Alright, this passed the linux CI on the linked Swift pull, and the new integration tests I added confirmed that all these incorrect local This is ready for review and merge. @JDevlieghere, since this pull is really fixing behavior that is only broken on this Swift fork, I'd like to get this in before the 5.9 branch, then we can go back and look at what should be upstreamed next week. I definitely want to upstream removing that However, I suspect that will take weeks to get that larger fix through the upstream review, and I'd like to get this smaller fix into the 5.9 branch before that. |
|
Ping @JDevlieghere and @bnbarham, what do you think about getting this in? It only fixes problems specific to this Swift fork, though elements of it can be spun off and used as part of a broader pull later to fix these rpath issues more generally upstream. |
I guess I still don't understand in what way the issue is Swift-specific. I'm very wary when it comes to changing RPATHs, this is something that has backfired many times, both on macOS and Linux. I would feel more comfortable with a targeted patch that changes the RPATH for a particular library that needs it, rather than changing a high-level setting ( |
|
Let me break this down one by one:
This rpath issue only happens when one of these LLVM sub-projects is built separately. Since only this Swift toolchain builds lldb separately all the time and most non-Swift projects build all the LLVM sub-projects together, this primarily affects this Swift fork and not upstream LLVM.
This pull only affects lldb on linux, it doesn't touch it on macOS. The compiler-rt libraries have their rpaths removed on all platforms, but only after checking to make sure the currently added rpaths are all wrong for those libraries.
I'm not sure how you can say the lldb driver is "seemingly unrelated" to I think we've shown with the passing CI runs and the successful CI toolchain build, which I then tested locally, that these changes don't break anything. I have submitted many pulls before removing and adjusting rpaths for the Swift toolchain and so far nothing has broken. I have some history with getting this right. |
The standalone build has always been a supported configuration for upstream LLDB. We have a dedicated bot for it. Recently there has been an RFC to formalize that across LLVM subprojects. This configuration doesn't exist strictly for Swift so saying that this is Swift specific is a stretch.
That doesn't really address my concern, Linux is a supported configuration, both upstream and downstream.
Thanks for explaining this part. That wasn't clear to me from the PR description or the commit message. I would definitely recommend including that in the latter. Anyway if that's the case (and I trust you when you say that it is) then that should just go upstream as I suggested earlier. I'm not an expert on the Linux configuration but there are upstream contributors that are and care about the standalone build. Those folks are better suited to review this and think of any potential fallout that might not affect the Swift fork. If they think it's fine, I have no issue with cherrypicking this to the Swift release branch.
I'm not so much worried about these changes breaking anything as the divergence with upstream.
While I appreciate your contributions, I'm not sure what it adds to this (technical) discussion. |
Understood, now that 5.9 has branched, we can take our time and get some of this in upstream first, since that is what you prefer. |
|
I looked into this bootstrapping logic just now and it's a bit off for linux. The bootstrapping modes matter for macOS, but shouldn't for linux, as it doesn't check the bootstrapping mode anyway.
Should I |
|
regarding CI: you can check in the log which bootstrapping mode was used. It's printed when cmake is configuring, e.g. |
|
So I think for linux it's fine, because CI is already testing the |
|
@eeckstein, that is the log output when building the compiler, this lldb build is wholly separate. This is the relevant lldb CMake config I was looking at, where there's no However, looking at that giant list closely now, I just noticed this, Please approve if you're okay with the rpath updates for linux in this pull. |
|
@bnbarham, one last CI run and we can get this in. |
|
swiftlang/swift-integration-tests#114 @swift-ci please test |
|
@bnbarham, this CI won't run integration tests. Anyway, no need to run that, as it will depend on getting my upstream LLVM patch in first. |
I was just copying what I ran above :). If it doesn't run them it shouldn't matter. |
|
Huh, now it's failing the lldb tests on linux, strange, since the larger version of this pull passed them before. I'll look into it. |
|
Built this locally and spent some time looking into it: the issue appears to be that this build rpath I'm splitting off isn't applied because Since the LLVM patch I submitted upstream removes that CMake flag for ELF platforms, this pull depends on that patch going in first. Once that's in, this should work again. |
|
Upstream patch was just merged, so I cherry-picked it here to this stable branch and rebased. @bnbarham, if you would kick off another CI run here, let's see if this works now. |
|
@swift-ci please test |
|
Alright, linux CI passed now and single test failing on mac CI, something related to a tsan race, appears spurious: this is ready to go in. |
… in llvm_setup_rpath() 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 21c008d. That prior attempt was reverted in 959dbd1, 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. Differential revision: https://reviews.llvm.org/D146918
Recent commit llvm@8f833f8 modified the installation rpath and did not set `BUILD_WITH_INSTALL_RPATH` correctly on AIX, which led to installation failures on AIX. This patch sets `BUILD_WITH_INSTALL_RPATH` on AIX to fix the installation failures. Reviewed By: buttaface, daltenty Differential Revision: https://reviews.llvm.org/D148866
Also, add rpath support for more platforms, like Android and the BSDs.
|
Another AIX/Windows change to this CMake method was merged upstream, so I cherry-picked that commit too and rebased. One last CI run and we can get this in. |
…a /home runpath from the build host again Also, update the liblldb.so runpath for swiftlang/llvm-project#6456.
|
@bnbarham, please run the CI on the main compiler repo with this pull and swiftlang/swift-integration-tests#114, which I recently updated for this pull, and we can make sure these runpath changes are working well. |
|
Alright, this passed CI along with the integration test. Both were approved and the only changes I made after that is to add the upstream commits here and modified the integration test for the new liblldb.so runpath in my last commit here. Since this is all passing the CI, would you merge this pull and swiftlang/swift-integration-tests#114 together now, @bnbarham? They have to be merged around the same time, as the integration test depends on this pull. |
|
@swift-ci please test |
|
Passed all CI, ready for merge. |
|
Ping @bnbarham, this one and the integration test can be merged, will submit this for 5.9 next. |
Also, add rpath support for more platforms, like Android and the BSDs.
I'm removing the last references to the CI build in linux runpaths, as in swiftlang/sourcekit-lsp#715, and most of these remaining are in lldb:
I believe the reason that only the lldb binaries have these
/home/build-user/runpaths is because lldb is built separately by the Swift build, so then the lldb build adds the local build directory to the runpath too.@eeckstein, please review, since you wrote the
add_properties_for_swift_modules()function that I'm modifying here.@compnerd, would be good to get your CMake input.