Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mlir] Don't set RPATH for libMLIR #121180

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

makslevental
Copy link
Contributor

@makslevental makslevental commented Dec 27, 2024

By default llvm_add_library calls llvm_setup_rpath, which has the strange effect that even libs inside of install/lib have non-empty self-referential RPATHs; e.g., on Mac

$ llvm-install/lib/libMLIR.dylib | grep RPATH -A4
          cmd LC_RPATH
      cmdsize 32
         path @loader_path/../lib (offset 12)

which is bad/awkward if you want to move libMLIR (like into a wheel...).

Now possibly we want to do this for all shlibs but I think we definitely want to do this for libMLIR because it should have no runtime lib deps.

By default [`llvm_add_library`](https://github.com/llvm/llvm-project/blob/94837c8b5761d20310947be5d2e1e568f67e8c0c/llvm/cmake/modules/AddLLVM.cmake#L681) calls `llvm_setup_rpath`, which has the strange effect that even libs inside of `install/lib` have non-empty self-referential `RPATH`s; e.g., on Mac


```
$ llvm-install/lib/libMLIR.dylib | grep RPATH -A4
          cmd LC_RPATH
      cmdsize 32
         path @loader_path/../lib (offset 12)
```

which is bad/awkward if you want to move `libMLIR` (like into a wheel...).

Now possibly we want to do this for all shlibs but I think we definitely want to do this for `libMLIR` because it should have no runtime lib deps (at least until [this](#108253) lands).
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Dec 27, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 27, 2024

@llvm/pr-subscribers-mlir-core

Author: Maksim Levental (makslevental)

Changes

By default llvm_add_library calls llvm_setup_rpath, which has the strange effect that even libs inside of install/lib have non-empty self-referential RPATHs; e.g., on Mac

$ llvm-install/lib/libMLIR.dylib | grep RPATH -A4
          cmd LC_RPATH
      cmdsize 32
         path @<!-- -->loader_path/../lib (offset 12)

which is bad/awkward if you want to move libMLIR (like into a wheel...).

Now possibly we want to do this for all shlibs but I think we definitely want to do this for libMLIR because it should have no runtime lib deps (at least until this lands).


Full diff: https://github.com/llvm/llvm-project/pull/121180.diff

1 Files Affected:

  • (modified) mlir/tools/mlir-shlib/CMakeLists.txt (+1)
diff --git a/mlir/tools/mlir-shlib/CMakeLists.txt b/mlir/tools/mlir-shlib/CMakeLists.txt
index a33c70c5807bea..293409a51f27ef 100644
--- a/mlir/tools/mlir-shlib/CMakeLists.txt
+++ b/mlir/tools/mlir-shlib/CMakeLists.txt
@@ -35,6 +35,7 @@ if(LLVM_BUILD_LLVM_DYLIB)
     MLIR
     SHARED
     EXCLUDE_FROM_LIBMLIR
+    NO_INSTALL_RPATH
     ${INSTALL_WITH_TOOLCHAIN}
     mlir-shlib.cpp
     ${_OBJECTS}

@llvmbot
Copy link
Member

llvmbot commented Dec 27, 2024

@llvm/pr-subscribers-mlir

Author: Maksim Levental (makslevental)

Changes

By default llvm_add_library calls llvm_setup_rpath, which has the strange effect that even libs inside of install/lib have non-empty self-referential RPATHs; e.g., on Mac

$ llvm-install/lib/libMLIR.dylib | grep RPATH -A4
          cmd LC_RPATH
      cmdsize 32
         path @<!-- -->loader_path/../lib (offset 12)

which is bad/awkward if you want to move libMLIR (like into a wheel...).

Now possibly we want to do this for all shlibs but I think we definitely want to do this for libMLIR because it should have no runtime lib deps (at least until this lands).


Full diff: https://github.com/llvm/llvm-project/pull/121180.diff

1 Files Affected:

  • (modified) mlir/tools/mlir-shlib/CMakeLists.txt (+1)
diff --git a/mlir/tools/mlir-shlib/CMakeLists.txt b/mlir/tools/mlir-shlib/CMakeLists.txt
index a33c70c5807bea..293409a51f27ef 100644
--- a/mlir/tools/mlir-shlib/CMakeLists.txt
+++ b/mlir/tools/mlir-shlib/CMakeLists.txt
@@ -35,6 +35,7 @@ if(LLVM_BUILD_LLVM_DYLIB)
     MLIR
     SHARED
     EXCLUDE_FROM_LIBMLIR
+    NO_INSTALL_RPATH
     ${INSTALL_WITH_TOOLCHAIN}
     mlir-shlib.cpp
     ${_OBJECTS}

@makslevental
Copy link
Contributor Author

ah shoot it libMLIR actually does have a runtime dep on libLLVM

/Users/mlevental/dev_projects/eudsl/llvm-install/lib/libMLIR.dylib:
        @rpath/libMLIR.dylib (compatibility version 0.0.0, current version 0.0.0)
        @rpath/libLLVM.dylib (compatibility version 1.0.0, current version 20.0.0)
        /usr/lib/libc++.1.dylib (compatibility version 1.0.0, current version 1800.105.0)
        /usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1351.0.0)

@makslevental
Copy link
Contributor Author

makslevental commented Dec 27, 2024

FYI I cooked this up as a workaround:

if(APPLE OR UNIX)
  set(_origin_prefix "\$ORIGIN")
  if(APPLE)
    set(_origin_prefix "@loader_path")
  endif()
  if (STANDALONE_BUILD)
    get_target_property(_mlir_loc MLIR LOCATION)
    get_target_property(_llvm_loc LLVM LOCATION)
  else()
    set(_mlir_loc "$<TARGET_FILE:MLIR>")
    set(_llvm_loc "$<TARGET_FILE:LLVM>")
  endif()
  set(_old_rpath "${_origin_prefix}/../lib${LLVM_LIBDIR_SUFFIX}")
  if(APPLE)
    execute_process(COMMAND install_name_tool -rpath "${_old_rpath}" ${_origin_prefix} "${_mlir_loc}" ERROR_VARIABLE rpath_err)
    execute_process(COMMAND install_name_tool -rpath "${_old_rpath}" ${_origin_prefix} "${_llvm_loc}" ERROR_VARIABLE rpath_err)
    # maybe already updated...
    if (rpath_err AND NOT rpath_err MATCHES "no LC_RPATH load command with path: ${_old_rpath}")
      message(FATAL_ERROR "couldn't update rpath because: ${rpath_err}")
    endif()
  else()
    # sneaky sneaky - undocumented
    file(RPATH_CHANGE FILE "${_mlir_loc}" OLD_RPATH "${_old_rpath}" NEW_RPATH "${_origin_prefix}")
    file(RPATH_CHANGE FILE "${_llvm_loc}" OLD_RPATH "${_old_rpath}" NEW_RPATH "${_origin_prefix}")
  endif()
endif()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants