Quuxplusone / LLVMBugzillaTest

0 stars 0 forks source link

libLTO.dylib dylib id regression (no longer @rpath-relative) #30398

Open Quuxplusone opened 7 years ago

Quuxplusone commented 7 years ago
Bugzilla Link PR31425
Status NEW
Importance P normal
Reported by Jeremy Huddleston Sequoia (jeremyhu@apple.com)
Reported on 2016-12-19 00:39:26 -0800
Last modified on 2016-12-19 04:01:37 -0800
Version trunk
Hardware PC All
CC chris.bieneman@gmail.com, diana.picus@linaro.org, john.brawn@arm.com, llvm-bugs@lists.llvm.org, mgrang@codeaurora.org, sgundapa@codeaurora.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also
The change from trunk r286059 to r290074 has changed the install names of
libLLVM.dylib, libclang.dylib, and libLTO.dylib to be absolute rather than
@rpath-relative.

I suspect it may have been intentional for libLLVM and libclang, but
libLTO.dylib should remain @rpath-relative such that we can have the linker
pick the correct one based on the toolchain being used.

$ otool -L /opt/local/libexec/llvm-devel/lib/libLTO.dylib
/opt/local/libexec/llvm-devel/lib/libLTO.dylib:
    /opt/local/libexec/llvm-devel/lib/libLTO.dylib (compatibility version 1.0.0,
current version 4.0.0)
    /opt/local/libexec/llvm-devel/lib/libLLVM.dylib (compatibility version 1.0.0,
current version 4.0.0)
    /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 307.5.0)
    /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version
1238.50.2)
Quuxplusone commented 7 years ago
e7ffbb5a0ee6a7f07c507dc757cffc9bb57532e3 is the first bad commit
commit e7ffbb5a0ee6a7f07c507dc757cffc9bb57532e3
Author: Mandeep Singh Grang <mgrang@codeaurora.org>
Date:   Tue Nov 8 00:45:05 2016 +0000

    [CMake] Fix llvm_setup_rpath function

    Summary:
    Set _install_rpath to CMAKE_INSTALL_RPATH if it is defined, so that eventually
    INSTALL_RPATH is set to CMAKE_INSTALL_RPATH.
    The "if(NOT DEFINED CMAKE_INSTALL_RPATH)" was missing a corresponding else
    clause.
    This also cleans up the fix made in r285908.

    Patch by Azharuddin Mohammed

    Reviewers: john.brawn, sgundapa, beanz

    Subscribers: chapuni, mgorny, llvm-commits

    Differential Revision: https://reviews.llvm.org/D26289

    git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@286184 91177308-0d34-0410-b5e6-96231b3b80d8

:040000 040000 d46f070148427e3148d18a256c86ad64336da87f
14ee01d0a099894c415b3d7b6f9b3cc41e4e057b M  cmake
Quuxplusone commented 7 years ago
The configure line being used is:

/opt/local/bin/cmake -DCMAKE_INSTALL_PREFIX='/opt/local/libexec/llvm-devel' -
DCMAKE_BUILD_TYPE=Debug -DCMAKE_BUILD_WITH_INSTALL_RPATH=ON -
DCMAKE_C_COMPILER="$CC" -DCMAKE_COLOR_MAKEFILE=ON -DCMAKE_CXX_COMPILER="$CXX" -
DCMAKE_FIND_FRAMEWORK=LAST -DCMAKE_MODULE_PATH=/opt/local/share/cmake/Modules -
DCMAKE_VERBOSE_MAKEFILE=ON -Wno-dev -
DCMAKE_INSTALL_NAME_DIR=/opt/local/libexec/llvm-devel/lib -
DCMAKE_INSTALL_RPATH=/opt/local/libexec/llvm-devel/lib -
DCMAKE_SYSTEM_PREFIX_PATH="/opt/local/libexec/llvm-devel;/opt/local;/usr" -
DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_ENABLE_RTTI=ON -DLLVM_INCLUDE_TESTS=OFF -
DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_ENABLE_FFI=ON -DLLVM_BINDINGS_LIST=none -
DFFI_INCLUDE_DIR=/opt/local/include -DFFI_LIBRARY_DIR=/opt/local/lib -
DLLVM_ENABLE_ASSERTIONS=ON -DCMAKE_C_FLAGS_RELEASE="-DNDEBUG" -
DCMAKE_CXX_FLAGS_RELEASE="-DNDEBUG" -DCMAKE_OSX_ARCHITECTURES="x86_64" -
DCMAKE_OSX_DEPLOYMENT_TARGET="10.12" -DCMAKE_OSX_SYSROOT="/" ../llvm

---

So that's triggering the early-exit from llvm_setup_rpath:

+  if(CMAKE_INSTALL_RPATH)
+    return()
+  endif()
Quuxplusone commented 7 years ago

I think part of the problem here is that CMAKE_INSTALL_NAME_DIR and CMAKE_INSTALL_RPATH are being set by MacPorts' cmake build system wrapper and they're now being used unconditionally by llvm's build system (instead of selectively ignored).

Also, the current behavior is exiting early based on CMAKE_INSTALL_RPATH alone. It seems like the logic there should involve CMAKE_INSTALL_NAME_DIR as well.

I'm not really sure what the correct behavior is here, but what we have right now is not it.

Quuxplusone commented 7 years ago
So I dropped both CMAKE_INSTALL_RPATH and CMAKE_INSTALL_NAME_DIR from the
configure line and things are slightly better, so I'll go with that as a
workaround for now, but we should address the issues here:

  1. llvm_setup_rpath is overloaded in what it does.  We should have one function that is in charge of setting up LC_RPATH to correctly find the things the target links against and another function that controls what the install name should be for this target.  There's really no reason that those two tasks should be conflated, and setting
CMAKE_INSTALL_RPATH should have no impact on the INSTALL_NAME_DIR for a target.

  2. We should probably have an option to configure the INSTALL_NAME_DIR for libLTO differently than other dylibs, since it should almost always be @rpath whereas we actually want libLLVM.dylib and libclang.dylib to not be @rpath-relative.