Add version to libLLVM also on non-UNIX

[mati865]

Summary

As discussed in https://reviews.llvm.org/D87521

llvm-config expects versioned library regardless of platform.

[mstorsjo]

This looks sensible to me (sorry for losing track of it earlier) as discussed in D87521.

I tried it out in a few setups to study it closer and it does seem to work safely. In practice no libraries that are built in MSVC configuration have ARG_SONAME set so this should have little to no effect on those configurations.

I can try to get it pushed in a couple days.

[mati865]

Thank you for returning to this.
I'd love to get confirmation from @hans who suggested it in https://reviews.llvm.org/D87521 that this still seems like the best approach.

I'll also need to recheck because I have no idea after all this time if import libraries have correct name (without the version) and do work.

[mstorsjo]

In D89009#2856933, @mati865 wrote:

Thank you for returning to this.
I'd love to get confirmation from @hans who suggested it in https://reviews.llvm.org/D87521 that this still seems like the best approach.

I'll also need to recheck because I have no idea after all this time if import libraries have correct name (without the version) and do work.

The import libraries do seem to use the right name as far as I can see, but there's another aspect I remembered now: This does create unversioned symlinks too (not while building but once you do ninja install). This probably is neat to have, but for cases when things are packaged up in e.g. zip files or something else that doesn't handle symlinks, one might want not to have them. Within llvm-mingw I guess I can have them stripped out before packaging though.

[mati865]

In D89009#2857190, @mstorsjo wrote:

The import libraries do seem to use the right name as far as I can see, but there's another aspect I remembered now: This does create unversioned symlinks too (not while building but once you do ninja install). This probably is neat to have, but for cases when things are packaged up in e.g. zip files or something else that doesn't handle symlinks, one might want not to have them. Within llvm-mingw I guess I can have them stripped out before packaging though.

Oh, nice. As for unversioned symlinks they would be changed into copies when packaging so we will remove them as well.

[mstorsjo]

In D89009#2857227, @mati865 wrote:

In D89009#2857190, @mstorsjo wrote:

The import libraries do seem to use the right name as far as I can see, but there's another aspect I remembered now: This does create unversioned symlinks too (not while building but once you do ninja install). This probably is neat to have, but for cases when things are packaged up in e.g. zip files or something else that doesn't handle symlinks, one might want not to have them. Within llvm-mingw I guess I can have them stripped out before packaging though.

Oh, nice. As for unversioned symlinks they would be changed into copies when packaging so we will remove them as well.

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

[mati865]

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

[hans]

In D89009#2857310, @mati865 wrote:

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

I don't know the details of this well enough. How many/what symlinks are we talking about here? If it would add a lot of binary size to the install (since the symlinks would end up being full copies on Windows) it would be nice to avoid, but if it's not big then it's not a problem.

[mstorsjo]

In D89009#2857904, @hans wrote:

In D89009#2857310, @mati865 wrote:

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

I don't know the details of this well enough. How many/what symlinks are we talking about here? If it would add a lot of binary size to the install (since the symlinks would end up being full copies on Windows) it would be nice to avoid, but if it's not big then it's not a problem.

I think in practice, right now no files are affected in MSVC builds at all, as libLLVM isn’t built there. (But there are plans to make it buildable for MSVC configs so it might matter in the future.) The symlinked files are largish, ~50-100 MB. So maybe we should opt out from those symlinks for now, and reconsider if there’s a concrete case where they are needed?

[hans]

In D89009#2857959, @mstorsjo wrote:

In D89009#2857904, @hans wrote:

In D89009#2857310, @mati865 wrote:

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

I don't know the details of this well enough. How many/what symlinks are we talking about here? If it would add a lot of binary size to the install (since the symlinks would end up being full copies on Windows) it would be nice to avoid, but if it's not big then it's not a problem.

I think in practice, right now no files are affected in MSVC builds at all, as libLLVM isn’t built there. (But there are plans to make it buildable for MSVC configs so it might matter in the future.) The symlinked files are largish, ~50-100 MB. So maybe we should opt out from those symlinks for now, and reconsider if there’s a concrete case where they are needed?

Sounds good to me.

[mstorsjo]

In D89009#2857310, @mati865 wrote:

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

@mati865 - Do you want to update the patch with an if(UNIX) around the symlink lines? I think we've got agreement that we can move forward with this, and it'd be nice to have it in before the 13.x branch in a couple weeks.

[mati865]

In D89009#2882846, @mstorsjo wrote:

In D89009#2857310, @mati865 wrote:

In D89009#2857228, @mstorsjo wrote:

Yep. I guess the question mainly is if we'd want to make this an upstream thing, adding an if(UNIX) around the symlinks on lines 608-613 here, if windows packaging cases would want to omit them anyway?

I have no idea about guidelines for Windows but it'd make sense for me.

@mati865 - Do you want to update the patch with an if(UNIX) around the symlink lines? I think we've got agreement that we can move forward with this, and it'd be nice to have it in before the 13.x branch in a couple weeks.

Sorry, I have forgotten to add this to my TODO.
I can do it over the weekend, ofc I won't mind if somebody beats it to me.

[mati865]

I had no internet during the weekend (massive storms over here) but so I returned to it yesterday.
I'm unable to build clean main branch at the moment:

`
[2972/3064] Linking CXX shared library bin\libLLVM.dll
FAILED: bin/libLLVM.dll lib/libLLVM.dll.a
cmd.exe /C "cd . && D:\msys64\clang64\bin\c++.exe -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,--gc-sections -shared -o bin\libLLVM.dll -Wl,--out-implib,lib\libLLVM.dll.a -Wl,--major-image-version,0,--minor-image-version,0 @CMakeFiles\LLVM.rsp  && cd ."
lld-link: error: too many exported symbols (max 65535)
`

It's simple change though, should I update diff without testing?

[mstorsjo]

In D89009#2895050, @mati865 wrote:

I had no internet during the weekend (massive storms over here) but so I returned to it yesterday.
I'm unable to build clean main branch at the moment:

`
[2972/3064] Linking CXX shared library bin\libLLVM.dll
FAILED: bin/libLLVM.dll lib/libLLVM.dll.a
cmd.exe /C "cd . && D:\msys64\clang64\bin\c++.exe -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -ffunction-sections -fdata-sections -O3 -DNDEBUG  -Wl,--gc-sections -shared -o bin\libLLVM.dll -Wl,--out-implib,lib\libLLVM.dll.a -Wl,--major-image-version,0,--minor-image-version,0 @CMakeFiles\LLVM.rsp  && cd ."
lld-link: error: too many exported symbols (max 65535)
`

Hmm, that’s a bit of a problem… Such a build does succeed for me at the moment, but I only have the x86, arm and aarch64 targets enabled. Then I end up with around 57k exported symbols from that DLL.

It's simple change though, should I update diff without testing?

That’d be fine with me, I could test building with it.

I don’t have any testcase that would use llvm-config for finding the libs to link though (which is the case that this tries to fix, right?).

[mati865]

Building only X86 target did the trick and it looks good:

$ ls bin/ | rg libLLVM.*dll
libLLVM-13git.dll

$ ls lib/ | rg libLLVM.*dll
libLLVM-13git.dll.a

$ bin/llvm-config --libs
-lLLVM-13git

Example:

$ cat llvm.cpp
#include <llvm/Target/TargetOptions.h>

void foo() {
    llvm::TargetOptions();
}

$ PATH=/e/tmp/build/bin:$PATH

$ clang++ -shared $(llvm-config --cxxflags --ldflags --libs) llvm.cpp -o llvm.dll

$ ntldd llvm.dll
        libLLVM-13git.dll => E:\tmp\build\bin\libLLVM-13git.dll (0x0000000000830000)
        libc++.dll => D:\msys64\clang64\bin\libc++.dll (0x0000000005b80000)
        KERNEL32.dll => C:\Windows\SYSTEM32\KERNEL32.dll (0x00000000062d0000)

[mstorsjo]

LGTM, I can try to push this a bit later. Thanks for testing!

llvm/cmake/modules/AddLLVM.cmake

if(ARG_MODULE OR ARG_SHARED)
     # macros.
     set_target_properties( ${name} PROPERTIES DEFINE_SYMBOL "" )
 
     if (LLVM_EXPORTED_SYMBOL_FILE)
       add_llvm_symbol_exports( ${name} ${LLVM_EXPORTED_SYMBOL_FILE} )
     endif()
   endif()
 
-  if(ARG_SHARED AND UNIX)
+  if(ARG_SHARED)
     if(NOT APPLE AND ARG_SONAME)
       get_target_property(output_name ${name} OUTPUT_NAME)
       if(${output_name} STREQUAL "output_name-NOTFOUND")
         set(output_name ${name})
       endif()
       set(library_name ${output_name}-${LLVM_VERSION_MAJOR}${LLVM_VERSION_SUFFIX})
       set(api_name ${output_name}-${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}${LLVM_VERSION_SUFFIX})
       set_target_properties(${name} PROPERTIES OUTPUT_NAME ${library_name})
+      if(UNIX)
         llvm_install_library_symlink(${api_name} ${library_name} SHARED
           COMPONENT ${name})
         llvm_install_library_symlink(${output_name} ${library_name} SHARED
           COMPONENT ${name})
       endif()
     endif()
+  endif()
 
   if(ARG_STATIC)
     set(libtype PUBLIC)
   else()
     # We can use PRIVATE since SO knows its dependent libs.
     set(libtype PRIVATE)
   endif()