[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: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:
> 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?
I don't know if it makes sense. This part was found during the testing of the original problem: https://reviews.llvm.org/D126122#3530383
The issue is fundamentally the same - broken RPATHs and non-functional libraries.
Would you prefer I rather rename the patch to something like "Missing relative $ORIGIN RUNPATH in libcxx/-abi or with LLVM_ENABLE_PER_TARGET_RUNTIME_DIR"?


================
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:
> 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?
I could certainly try.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126122