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

[Arcadiy Ivanov via Phabricator via libcxx-commits]

arcivanov added inline comments.


================
Comment at: libcxx/src/CMakeLists.txt:184
 endif()
 
+if (NOT APPLE AND LIBCXX_ENABLE_SHARED AND LIBCXXABI_USE_LLVM_UNWINDER AND
----------------
phosek wrote:
> Once we require CMake >=3.14, we should be able to use https://cmake.org/cmake/help/latest/prop_tgt/BUILD_RPATH_USE_ORIGIN.html. Can you leave a `TODO` comment here for that?
I will


================
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'")
----------------
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.


================
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()
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126122