[libcxx-commits] [PATCH] D126122: Fix RUNPATH not accounting for LLVM_ENABLE_PER_TARGET_RUNTIME_DIR

[Petr Hosek via Phabricator via libcxx-commits]

phosek requested changes to this revision.
phosek added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/src/CMakeLists.txt:185-186
 
+if (NOT APPLE AND LIBCXX_ENABLE_SHARED AND LIBCXXABI_USE_LLVM_UNWINDER AND
+        NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
+    add_library_flags("-Wl,-rpath,'$$ORIGIN'")
----------------
arcivanov wrote:
> phosek wrote:
> > What's the motivation behind this condition? Why not set this unconditionally?
> I don't know the side-effects of setting this unconditionally, i.e. I went with a narrowly-tailored solution and principle of least surprise, only capturing the condition where a hard runtime failure would occur without introducing the RPATH into $ORIGIN. 
> 
> The motivation is to only introduce this when:
> 
> 1) LIBCXX is an SO; and
> 2) There is an unwinder used that is NOT linked statically; and
> 3) You actually build shared unwinder.
This part of the change seems unrelated to `LLVM_ENABLE_PER_TARGET_RUNTIME_DIR`, only the `llvm_setup_rpath` change should be needed. If that's correct, could we split this part of the change into a separate patch?


================
Comment at: libcxx/src/CMakeLists.txt:187
+        NOT LIBCXXABI_STATICALLY_LINK_UNWINDER_IN_SHARED_LIBRARY AND (TARGET unwind_shared OR HAVE_LIBUNWIND))
+    add_library_flags("-Wl,-rpath,'$$ORIGIN'")
+endif()
----------------
arcivanov wrote:
> phosek wrote:
> > This flag may not be supported by all linkers so we need to check if it's supported first.
> I would appreciate help here. 
> Would using logic similar to `llvm_setup_rpath` satisfy you or is there a better solution you see?
> I could limit this further to only UNIX which is non-FreeBSD/DragonFly, which should cover all linkers?
Rather than manually setting the linker flag, could we instead set the `BUILD_RPATH` and `INSTALL_RPATH` property on the shared targets?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126122/new/

https://reviews.llvm.org/D126122