llvm7: cmake fix linking

[pranavk]

This patch should not make things any worse because we were not using the LLVM_LIBS variable anyways originally as explained in the removed comment.

The removed comment says that LLVM_LIBS are linked but it doesn't happen for me with LLVM 7.

Other than that, I replaced the call to llvm_map_components_to_libnames with llvm-config because in some platforms, especially when your LLVM is provided by your distro, only a single DSO is provided (libLLVM-7.so), instead of separate .so for each library. So providing separate libnames to linker will not work. Delegating our job to llvm-config is better as it will automatically detect whether system provides single DSO or seperated DSO.

[regehr]

these look like good changes to me, but I'll note that (building against an LLVM I built myself and another that I downloaded from llvm.org) I have not seen any problems

[pranavk]

I'm not sure how build downloaded from llvm.org distributes the LLVM libraries. But I think unless you specifically mention that you want a single DSO to cmake, a self-built LLVM will always give you separate DSOs in which case llvm_map_components_to_libnames makes sense.

Having said that, how it was able to link even those separate DSOs for you, even though when variable outputted from llvm_map_components_to_libnames was not used, is still a mystery to me.

[eeide]

I will look at this. This stuff was previously discussed in issue #165.

[regehr]

@pranavk on what platforms have you tested this?

[regehr]

@eeide I'll delay my own OS X testing until you've decided whether to merge this

[pranavk]

@regehr not much really. I just wanted creduce to work on my Fedora 29 and this patch fixes it.

[regehr]

this branch works fine for me on OSX using CMake

[mgorny]

To be honest, upstream is actually moving away from llvm-config to using CMake directly, so this would technically be a regression. And I don't really see why it would make any difference compared to the old code.

[mgorny]

I think NOT LLVM_CONFIG_EXECUTABLE should be sufficient.

False if the constant is 0, OFF, NO, FALSE, N, IGNORE, NOTFOUND, the empty string, or ends in the suffix -NOTFOUND.

https://cmake.org/cmake/help/v3.0/command/if.html

[pranavk]

To be honest, upstream is actually moving away from llvm-config to using CMake directly, so this would technically be a regression. And I don't really see why it would make any difference compared to the old code.

I actually thought it's the other way around but thanks for letting me know.

IIRC, the problem I was facing was that llvm_map_components_to_libnames was not able to detect my SHARED_LIB installation of LLVM (single libLLVM7.so) and passing individual libraries to the linker resulting in confused linker.

[mgorny]

Well, I guess using llvm-config is better if it solves your issue and we don't know a better solution.

[pranavk]

If I am the only one facing this issue, it makes me suspect my setup. I will try compiling creduce again to see if I am still facing this issue or it was just a one time thing.

[eeide]

@pranavk Please give me a recipe for building LLVM in a way that causes the problem you are seeing. If you don't want to post it here, pleas send it to me via email. Thanks—

[mgorny]

@eeide, if I may intrude, I think he's talking about an installation where only the dylib is installed and split libLLVM* files are not.

In any case, I think you need to test three main cases:

1. Linking to the dylib.
2. Linking to static split libraries.
3. Linking to shared split libraries.

Generally it's dylib OR (static-split XOR shared-split).

Some projects (like mesa) prefer linking to dylib over split libraries when it's present. Some people think it's better to link to static libraries to avoid problems on updates. Some believe you should give users an option to choose between dylib and split libs (usually presuming split libs would imply static libs).

Note that I don't really know how clang libraries fit here. I think the shared libclang is not 100% equal to LLVM's dylib concept.

[eeide]

I'm sorry to be dense, but how do I tell LLVM to install only the big libLLVM?

[pranavk]

LLVM_BUILD_LLVM_DYLIB and LLVM_LINK_LLVM_DYLIB (https://llvm.org/docs/CMake.html) sounds like they should do the job.

[mgorny]

I don't think there's a clear way to do that. Usually binary distros split the installed result into multiple packages, so in your case that would be equivalent to removing libLLVM*.a ;-).

[eeide]

@pranavk, with both LLVM_BUILD_LLVM_DYLIB an LLVM_LINK_LLVM_DYLIB set, the individual static libraries are still installed.

[eeide]

@mgorny I am tempted to just tell people to install the package that contains the static libraries, then.

[eeide]

I had sort of forgotten that there was a patch on the table!

I will try it, against an LLVM installation tree in which I have removed the installed static libraries.

[pranavk]

@pranavk, with both LLVM_BUILD_LLVM_DYLIB an LLVM_LINK_LLVM_DYLIB set, the individual static libraries are still installed.

Yeah, I don't exactly remember how I did this. I will try to reproduce the problem after the final's week.

[eeide]

@pranavk In your setup, are the libclang* libraries still available as static libraries?

[eeide]

If I just remove all the LLVM .a files, then when I try to configure C-Reduce, CMake does not go through, because the LLVM tree's CMake stuff is inconsistent with the installed files.

It dies at the find_package(LLVM ...) line in the top CMakeLists.txt file (long before I get to your patch).

Below is the error I get:

CMake Error at /disk2/install/lib/cmake/llvm/LLVMExports.cmake:653 (message):
  The imported target "LLVMDemangle" references the file

     "/disk2/install/lib/libLLVMDemangle.a"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/disk2/install/lib/cmake/llvm/LLVMExports.cmake"

  but not all the files it references.

Call Stack (most recent call first):
  /disk2/install/lib/cmake/llvm/LLVMConfig.cmake:150 (include)
  CMakeLists.txt:26 (find_package)


-- Configuring incomplete, errors occurred!

[eeide]

That said, if I put the files back and apply a version of your patch, it links with libLLVM-7.

[eeide]

I have been using a version of the proposed patch in my tree, and it seems to work, so I will likely push it up later today or tomorrow. I want to test it a bit more on systems that I have access to.

[eeide]

Commit be741fc, on master, implements a version of the change suggested in this pull request. Read the commit message for more.

Please let me know if that commit has resolved the issue underlying this pull request. Basically, does the master branch now work in your setup, @pranavk? Thanks!

[mgorny]

It can't resolve any issue because it just changes the value of variable that is not used anywhere.

[eeide]

@mgorny: This is the thing that confused me since commit b5da960.

You are right; the variable is not being used, and presumably the right thing to do would be to use it :-).

For me, the list of LLVM libraries ends up in the link line anyway. For others, it apparently does not. I don't understand why this is.

[eeide]

I have made some progress on figuring out why the LLVM libs "magically appear" for me.

I don't have a configuration in which they don't "magically appear" by default. But at least I can bend my environment so that they do not magically appear in my environment.

[eeide]

OK, now I understand target_link_libraries() better. I can construct a link line that includes LLVM_LIBS explicitly and in a useful way.

To really make sense of this whole thing, I need an example of an environment like @pranavk's in which the current thing doesn't work.

[eeide]

As you might have seen, I effectively reverted be741fc for now. That was the commit that computed LLVM_LIBS using llvm-config, but still did not use the value.

I went back to the "old way" for now, in the interest of making a new C-Reduce release. After the upcoming release, I will revisit this issue, and how LLVM_LIBS should be set and used.