llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.03k stars 11.58k forks source link

Wrong libLLVM symbolic links generated for 18.1 #84637

Open finagolfin opened 6 months ago

finagolfin commented 6 months ago

I'm guessing this is related to bumping the minor version for the first time in a long time:

> ls -l /data/data/com.termux/files/usr/lib/libLLVM*so*
lrwxrwxrwx 1 u0_a318 u0_a318        18 Mar  9 20:46 /data/data/com.termux/files/usr/lib/libLLVM-18.so -> libLLVM.so.18.1
-rw------- 1 u0_a318 u0_a318 114521080 Mar  9 12:39 /data/data/com.termux/files/usr/lib/libLLVM.so

We build with -DLLVM_LINK_LLVM_DYLIB=ON to make sure this shared library is included, which is then linked against by the Rust compiler and so on also. I think the relevant CMake config is here, which I notice @tstellar recently modified.

tstellar commented 6 months ago

What should the symlinks look like? Also, in my builds libLLVM.so is a symlink to libLLVM.so.18.1, I'm not sure why you are seeing libLLVM.so as a regular file.

finagolfin commented 6 months ago

This is what they look like for the current LLVM 17 package that we distribute, built with the exact set of CMake build flags I linked above:

> ls -l data/data/com.termux/files/usr/lib/libLLVM*so
lrwxrwxrwx 1 u0_a318 u0_a318        18 Nov 28 21:26 data/data/com.termux/files/usr/lib/libLLVM-17.0.6.so -> libLLVM-17.so
-rw------- 1 u0_a318 u0_a318 115962784 Nov 28 21:26 data/data/com.termux/files/usr/lib/libLLVM-17.so
lrwxrwxrwx 1 u0_a318 u0_a318        18 Nov 28 21:26 data/data/com.termux/files/usr/lib/libLLVM.so -> libLLVM-17.so

What do you see after building LLVM 18 with the shared library?

tstellar commented 6 months ago
$ ls -l libLLVM*so
lrwxrwxrwx. 1 fedora fedora 15 Mar  9 21:04 libLLVM-18.so -> libLLVM.so.18.1
lrwxrwxrwx. 1 fedora fedora 15 Mar  9 21:04 libLLVM.so -> libLLVM.so.18.1
h-vetinari commented 6 months ago

In our builds on conda-forge, https://github.com/llvm/llvm-project/issues/82647 (resp. https://github.com/llvm/llvm-project/commit/4cc7a75aa6ac272a5774ef7ca3f6b2ad095425e3) only fixed the linux side, but on OSX the result is still:

UserWarning: file $PREFIX/lib/libLLVM-18.dylib is a symlink with no target

(this is in a setting where the unversioned libLLVM.dylib intentionally isn't available yet, but libLLVM.18.1.dylib is)

tstellar commented 6 months ago

@h-vetinari Can you post the output of ls -l libLLVM*dylib* from the install directory?

finagolfin commented 6 months ago

@tstellar, I assume the libLLVM.so.18.1 library exists for you? I took a look at the Android build log and it shows exactly the two files I listed being installed, so it appears this is the result of the LLVM build, not any post-build modifications in our Termux packaging scripts.

What build flags do you pass in to CMake when building LLVM? I've linked our LLVM-specific flags above and we use more flags for all CMake builds.

I presume the problem is the difference in build flags or some platform-specific CMake issue.

h-vetinari commented 6 months ago

@h-vetinari Can you post the output of ls -l libLLVM*dylib* from the install directory?

Certainly:

+ ls -l $PREFIX/lib/libLLVM*dylib*
lrwxr-xr-x  1 runner  staff         18 Mar 12 06:00 $PREFIX/lib/libLLVM-18.dylib -> libLLVM.dylib.18.1
-rwxr-xr-x  1 runner  staff  128049832 Mar 12 05:56 $PREFIX/lib/libLLVM.18.1.dylib
lrwxr-xr-x  1 runner  staff         18 Mar 12 06:00 $PREFIX/lib/libLLVM.dylib -> libLLVM.18.1.dylib

The issue is the symlink pointing to libLLVM-18.dylib -> libLLVM.dylib.18.1 (note 18.1 at the end) vs. the library being called libLLVM.18.1.dylib (i.e. version in between)

tstellar commented 6 months ago

@finagolfin Just to be clear the problem you are seeing is that libLLVM.so is not a symlink to libLLVM.so.18.1 ?

tstellar commented 6 months ago

@h-vetinari Is there any reason why you can't link against libLLVM.18.1.dylib ?

h-vetinari commented 6 months ago

@h-vetinari Is there any reason why you can't link against libLLVM.18.1.dylib ?

No there isn't, but the point is that the CMake build as-is generates a broken symlink, and I'd prefer to fix it in LLVM rather than having to carry a manual fix.

finagolfin commented 6 months ago

Just to be clear the problem you are seeing is that libLLVM.so is not a symlink to libLLVM.so.18.1 ?

Yep, that would be one way to fix it. We link our Rust compiler build against libLLVM-18.so, so obviously that is broken now. I will manually fix it in our build script for now, but like @h-vetinari, I think it would be better if the LLVM CMake config generated these properly again in future releases.

tstellar commented 6 months ago

@h-vetinari Are you able to test this fix on MacOS: #85163

tstellar commented 6 months ago

@finagolfin I still can't reproduce the issue you are seeing with the symlinks. Do you have a build log or something I can pull the cmake invocation from?

h-vetinari commented 6 months ago

@h-vetinari Are you able to test this fix on MacOS: #85163

I'm currently trying out the following

diff --git a/llvm/cmake/modules/AddLLVM.cmake b/llvm/cmake/modules/AddLLVM.cmake
index 3bc78b0dc935..b3505e2b749e 100644
--- a/llvm/cmake/modules/AddLLVM.cmake
+++ b/llvm/cmake/modules/AddLLVM.cmake
@@ -2089,7 +2089,13 @@ function(llvm_install_library_symlink name dest type)

   set(full_name ${CMAKE_${type}_LIBRARY_PREFIX}${name}${CMAKE_${type}_LIBRARY_SUFFIX})
   if (ARG_SOVERSION)
-    set(full_dest ${CMAKE_${type}_LIBRARY_PREFIX}${dest}${CMAKE_${type}_LIBRARY_SUFFIX}.${ARG_SOVERSION})
+    if(${CMAKE_SYSTEM_NAME} MATCHES "Darwin")
+      # macOS puts the SOVERSION in the middle, i.e. we need to point to libLLVM.18.1.dylib
+      set(full_dest ${CMAKE_${type}_LIBRARY_PREFIX}${dest}.${ARG_SOVERSION}${CMAKE_${type}_LIBRARY_SUFFIX})
+    else()
+      # linux has the SOVERSION at the end, i.e. libLLVM.so.18.1
+      set(full_dest ${CMAKE_${type}_LIBRARY_PREFIX}${dest}${CMAKE_${type}_LIBRARY_SUFFIX}.${ARG_SOVERSION})
+    endif()
   else()
     set(full_dest ${CMAKE_${type}_LIBRARY_PREFIX}${dest}${CMAKE_${type}_LIBRARY_SUFFIX})
   endif()

I expect this to work, because the only problem AFAICT is the hard-coded format not being adapted to macOS. If it doesn't work, I'll try your suggestion. If it works I'm happy to raise a PR (or try your suggestion nevertheless, if you like your approach better).

h-vetinari commented 6 months ago

So my patch works, but I think yours is more elegant. I'll try it next.

finagolfin commented 6 months ago

I still can't reproduce the issue you are seeing with the symlinks.

OK, if you are not seeing it, maybe it is something specific to Android. I will manually create the symlink for now and look into the underlying issue in the coming weeks.

h-vetinari commented 6 months ago

So my patch works, but I think yours is more elegant. I'll try it next.

Yours works as well, so I suggest to merge https://github.com/llvm/llvm-project/pull/85163 - thanks for the quick handling!

finagolfin commented 5 months ago

I can no longer reproduce with 18.1.2, which I just shipped for Android:

> ls -l $PREFIX/lib/libLLVM*so*
lrwxrwxrwx 1 u0_a318 u0_a318        18 Mar 26 20:24 /data/data/com.termux/files/usr/lib/libLLVM-18.so -> libLLVM.so
-rw------- 1 u0_a318 u0_a318 114525472 Mar 26 18:06 /data/data/com.termux/files/usr/lib/libLLVM.so

Was something merged to fix this or should I look into it, to make sure it wasn't some fluke that might break again?

tstellar commented 5 months ago

@finagolfin I'm not sure what is going on in your build system, but having libLLVM-18.so be a symlink to libLLVM.so is a (bug)[https://github.com/llvm/llvm-project/issues/82647]. However, this was fixed in 18.1.0-rc4. Do you have a complete build log for your build?

finagolfin commented 5 months ago

Do you have a complete build log for your build?

Here is where the symlink is created on our CI for the Android AArch64 library, not sure if that's the level of detail you want though.

I will spend some time looking into that CMake config to see what changed and why it behaves differently on Android.

tstellar commented 5 months ago

Can you update the build so that the logs record the full cmake command that was used? Without that, it's going to be hard to debug this.

finagolfin commented 5 months ago

I will look into this manually and report back instead.

h-vetinari commented 2 months ago

I think this issue can be closed

finagolfin commented 2 months ago

No, @tstellar says the current behavior is a bug, even though it works for me on Android now. I will look into why it behaves the way it does now and report back, then we can decide if anything needs to be done.