[PATCH] D25304: cmake: Set the proper rpath in add_llvm_executable and llvm_add_library

[Chris Bieneman via llvm-commits]

beanz added inline comments.


================
Comment at: cmake/modules/AddLLVM.cmake:494
+    else()
+      set_target_properties( ${name} PROPERTIES BUILD_WITH_INSTALL_RPATH OFF )
+      set_property(TARGET ${name} APPEND_STRING PROPERTY INSTALL_RPATH ":${LLVM_LIBRARY_DIR}")
----------------
plevine wrote:
> beanz wrote:
> > plevine wrote:
> > > beanz wrote:
> > > > Why are you setting this off? This doesn't seem right to me.
> > > Whenever building clang with BUILD_WITH_INSTALL_RPATH set to ON, it fails with 
> > > > warning: libclangLex.so.4.0, needed by lib32/libclangFormat.so.4.0.0, not found (try using -rpath or -rpath-link)
> > > > /usr/lib/gcc/x86_64-pc-linux-gnu/6.2.0/../../../../x86_64-pc-linux-gnu/bin/ld: warning: libclangAST.so.4.0, needed by lib32/libclangToolingCore.so.4.0.0, not found (try using -rpath or -rpath-link)
> > > 
> > > along with undefined reference errors pertaining to clang library symbols.  The libraries do include in their rpath "$ORIGIN/../lib32" which does correctly point back to the library directory even at build time so I admit I'm at a loss.  Regardless, clang does build correctly with BUILD_WITH_INSTALL_RPATH set to OFF and the correct and necessary  rpath exists at runtime.  I'm open to any insight you can offer and any changes you would suggest.
> > Unfortunately I'm not sure I'm the right person to help with the Linux issue here. What I can say is that I did not see this issue on Darwin, and if you turn BUILD_WITH_INSTALL_RPATH off on Darwin the binaries get the wrong rpath in the build tree.
> [[ https://sourceware.org/bugzilla/show_bug.cgi?id=16936 | Here's the problem ]].  The build-time linker doesn't respect `$ORIGIN` whether with `-rpath` or `--rpath-link`.  When testing on the command line in the build directory, by changing `$ORIGIN/../lib32` to  `./lib32` it links fine.
This seems wrong to me. In general rpaths are intended to be consumed by the loader, not the static linker. They get encoded into the final binary. Rather than disabling building with the install rpath, maybe this should be fixed by adding linker search paths.

Disabling the install rpath means that binaries need to be re-linked before installing them because the binaries aren't in their final linked form in the build directory.


================
Comment at: cmake/modules/AddLLVM.cmake:316
+# Set the proper rpath for shared libraries and executables
+function(set_rpath name)
+  set_target_properties( ${name} PROPERTIES BUILD_WITH_INSTALL_RPATH ON )
----------------
Please rename this to follow the conventions in this file (i.e. `llvm_set_rpath`).


================
Comment at: cmake/modules/AddLLVM.cmake:317
+function(set_rpath name)
+  set_target_properties( ${name} PROPERTIES BUILD_WITH_INSTALL_RPATH ON )
+  if (APPLE)
----------------
Can you refactor this to reduce the number of places where `set_target_properties` is called?

This should be able to have a single call site something like:


```
  set_target_properties(${name} PROPERTIES
            BUILD_WITH_INSTALL_RPATH On
            INSTALL_RPATH "${_install_rpath}"
            ${_install_name_dir})
```

Where `_install_rpath` is the rpath setting, and `_install_name_dir` is only defined on Apple to `set(_install_name_dir INSTALL_NAME_DIR "@rpath")`.


================
Comment at: cmake/modules/AddLLVM.cmake:327
+      endif()
+    endif(NOT DEFINED CMAKE_INSTALL_RPATH)
+  endif()
----------------
As convention we don't put the condition inside endif blocks.


================
Comment at: cmake/modules/AddLLVM.cmake:471
   if(ARG_SHARED)
+    set_rpath( ${name} )
+  endif()
----------------
Rather than adding a condition block for this can you hoist it up under the `add_library` call above in the `elseif(ARG_SHARED)` block?


https://reviews.llvm.org/D25304