Fix the OCaml external linking problem

[jpdeplaix]

Summary

WARNING: Right now the patch doesn't work and this error is raised: `get_target_property() called with non-existent target "LLVM".`. Does anybody know why ?

The problem:
If OCaml is not installed in the same tree as LLVM (e.g. with OPAM), the build script doesn't work. Moreover, if SHARED_LIBS is enabled, the resulting binding won't be dynamically linked.

Here is a patch that tries to fix these two issues.
The patch also fixes the build instructions.
I also tried to allow both static and shared libraries if possible (see OPAM package) but my level of comprehension of CMake is quite limited so I gave up. But if anybody have any idea how to do that better, please feel free to discuss this matter here.

[mgorny]

cmake/modules/AddOCaml.cmake

71

You've removed the foreach loop, so llvm_lib is not defined here.

80

I'd dare say -L should go before -l. Not sure if it's strictly necessary but it's certainly a cleaner assumption.

187

Hmm, how is this different from setting LLVM_OCAML_OUT_OF_TREE + CMAKE_INSTALL_PREFIX to match the one used when building LLVM?

[whitequark]

I agree with pretty much all @mgorny says.

[jpdeplaix]

Wow, I replied to the comments two months ago but it seems that Phabricator made those only visible to me... I was supposed to click on submit at the end of the page or something maybe ? (let's see now)

cmake/modules/AddOCaml.cmake

71

The foreach loop is now line 74. get_target_property is supposed to put the property into ${llvm_lib}

80

It's not necessary but I can change that I you wish.

187

I'm not sure what you mean but maybe we also want to but something else (the python binding or I don't know) and it does require to have the prefix set to, let's say /usr/. And let's say we want to install the OCaml binding using OPAM, then the prefix should be: ~/.opam/<ocaml-version>/

[mgorny]

cmake/modules/AddOCaml.cmake

71

Oh, sorry, that was stupid of me. Do you use llvm_libs at all in this branch of code then? Maybe the explicit_map_components... call could be made conditional too.

187

I'd say if you're using LLVM_OCAML_OUT_OF_TREE, then you are obviously not building anything else.

[jpdeplaix]

By the way, does anybody have any idea how to fix the get_target_property() called with non-existent target "LLVM". problem ?
Otherwise I can always do like the patch in use in opam right now: replace the lines 71 and 72 with a single line:

list(APPEND ocaml_flags "-lLLVM-${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX}")

it just works but it's not really a solid solution it seems.

cmake/modules/AddOCaml.cmake

71

Well, llvm_libs is also used bellow (line 188) in :

foreach( llvm_lib ${llvm_libs} )

187

As far as I know it is not forbidden anywhere so I wouldn't say it is obvious. If you prefer I could add something to enforce that.

[whitequark]

By the way, does anybody have any idea how to fix the get_target_property() called with non-existent target "LLVM". problem ?

Can you try the LLVMCore target?

[jpdeplaix]

Can you try the LLVMCore target?

It doesn't seem to work. When I tried I got this:

$ make ocaml_all
/usr/bin/ld: cannot find -lllvm_lib-NOTFOUND

It seems that anywhere-else in the cmake modules, the solutions with ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX} is used.
What I could do is something like this instead, what do you think ?

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 1435859851a..a619d96acd8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -44,6 +44,9 @@ if (NOT PACKAGE_VERSION)
     "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}${LLVM_VERSION_SUFFIX}")
 endif()
 
+set(LLVM_DYLIB_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX}")
+set(LLVM_API_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}${LLVM_VERSION_SUFFIX}")
+
 if ((CMAKE_GENERATOR MATCHES "Visual Studio") AND (CMAKE_GENERATOR_TOOLSET STREQUAL ""))
   message(WARNING "Visual Studio generators use the x86 host compiler by "
                   "default, even for 64-bit targets. This can result in linker "
diff --git a/cmake/modules/AddLLVM.cmake b/cmake/modules/AddLLVM.cmake
index 43341d388f8..e539018cafd 100644
--- a/cmake/modules/AddLLVM.cmake
+++ b/cmake/modules/AddLLVM.cmake
@@ -480,7 +480,7 @@ function(llvm_add_library name)
         PROPERTIES
         # Since 4.0.0, the ABI version is indicated by the major version
         SOVERSION ${LLVM_VERSION_MAJOR}
-        VERSION ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}${LLVM_VERSION_SUFFIX})
+        VERSION ${LLVM_API_VERSION})
     endif()
   endif()
 
@@ -502,8 +502,8 @@ function(llvm_add_library name)
       if(${output_name} STREQUAL "output_name-NOTFOUND")
         set(output_name ${name})
       endif()
-      set(library_name ${output_name}-${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX})
-      set(api_name ${output_name}-${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}${LLVM_VERSION_SUFFIX})
+      set(library_name ${output_name}-${LLVM_DYLIB_VERSION})
+      set(api_name ${output_name}-${LLVM_API_VERSION})
       set_target_properties(${name} PROPERTIES OUTPUT_NAME ${library_name})
       llvm_install_library_symlink(${api_name} ${library_name} SHARED
         COMPONENT ${name}
diff --git a/cmake/modules/AddOCaml.cmake b/cmake/modules/AddOCaml.cmake
index 1d8094cc505..946ebeb52fa 100644
--- a/cmake/modules/AddOCaml.cmake
+++ b/cmake/modules/AddOCaml.cmake
@@ -66,10 +66,18 @@ function(add_ocaml_library name)
     list(APPEND ocaml_flags "-custom")
   endif()
 
+  if(DEFINED LLVM_OCAML_EXTERNAL_LLVM_LIBDIR)
+    list(APPEND ocaml_flags "-ccopt" "-L${LLVM_OCAML_EXTERNAL_LLVM_LIBDIR}" )
+  endif()
+
   explicit_map_components_to_libraries(llvm_libs ${ARG_LLVM})
-  foreach( llvm_lib ${llvm_libs} )
-    list(APPEND ocaml_flags "-l${llvm_lib}" )
-  endforeach()
+  if(BUILD_SHARED_LIBS)
+    list(APPEND ocaml_flags "-lLLVM-${LLVM_DYLIB_VERSION}")
+  else()
+    foreach( llvm_lib ${llvm_libs} )
+      list(APPEND ocaml_flags "-l${llvm_lib}" )
+    endforeach()
+  endif()
 
   get_property(system_libs TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)
   foreach(system_lib ${system_libs})
diff --git a/tools/llvm-config/CMakeLists.txt b/tools/llvm-config/CMakeLists.txt
index 25f99cec978..d1c8229cebc 100644
--- a/tools/llvm-config/CMakeLists.txt
+++ b/tools/llvm-config/CMakeLists.txt
@@ -36,7 +36,6 @@ set(LLVM_CFLAGS "${CMAKE_C_FLAGS} ${CMAKE_C_FLAGS_${uppercase_CMAKE_BUILD_TYPE}}
 set(LLVM_CXXFLAGS "${CMAKE_CXX_FLAGS} ${CMAKE_CXX_FLAGS_${uppercase_CMAKE_BUILD_TYPE}} ${COMPILE_FLAGS} ${LLVM_DEFINITIONS}")
 set(LLVM_BUILD_SYSTEM cmake)
 set(LLVM_HAS_RTTI ${LLVM_CONFIG_HAS_RTTI})
-set(LLVM_DYLIB_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX}")
 set(LLVM_HAS_GLOBAL_ISEL "ON")
 
 # Use the C++ link flags, since they should be a superset of C link flags.

I've highlighted only the changes compared to the base proposal.
Basically what the patch above does is that it aliases ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX} and ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}${LLVM_VERSION_SUFFIX} to names, and uses them whenever it is possible.

[whitequark]

OK I give up too, use the ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX} hack.

[whitequark]

Actually on second look it's not a hack, it's a perfectly fine piece of code. I was misreading LLVM_VERSION_SUFFIX as LLVM_VERSION_PATCH. Sorry for all the wasted time, @jpdeplaix!

[jpdeplaix]

Use ${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX} directly and put -L before -l.

[jpdeplaix]

@whitequark don't worry, I don't mind. Thanks. I've updated the diff with requested change. As far as I tested (using opam and llvm-5.0.0), it's working.

[mgorny]

cmake/modules/AddOCaml.cmake

74

This is wrong. BUILD_SHARED_LIBS does not imply building the dylib. I think you're looking for LLVM_BUILD_LLVM_DYLIB here.

[jpdeplaix]

poke @mgorny
That would be awesome to have this merged before the feature freeze (6th of January I believe)

EDIT: ................ sorry, it happened again. This $@*! bugtracker is kind of counter-intuitive :/

cmake/modules/AddOCaml.cmake

74

Can you help me to understand the difference ? The documentation doesn't really show the difference when used inside cmake: https://llvm.org/docs/CMake.html
Internally it seems to be essentially the same to me. Furthermore it seems to be used twice in this module:

• https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddOCaml.cmake#L44
• https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/AddOCaml.cmake#L65

Do we need to also change between the two there ?

[mgorny]

cmake/modules/AddOCaml.cmake

74

BUILD_SHARED_LIBS = libLLVMFooBar.so instead of .a
LLVM_BUILD_LLVM_DYLIB = libLLVM-x.y.z.so

Interesting enough, the current use seems to be focused on building shared OCaml modules which is different use than elsewhere in LLVM. Maybe it'd be reasonable to have a separate flag for that, and default it to ON (presuming OCaml dll* do not strictly require shared LLVM libraries).

bindings/ocaml/README.txt

 -------------
 
 * OCaml 4.00.0+.
+* ocamlfind.
 * ctypes 0.4+.
 * oUnit 2+ (only required for tests).
 * CMake (to build LLVM).
+* Python 2.7+ (to build LLVM).
 
 Building the bindings
 ---------------------

 The bindings can also be built out-of-tree, i.e. targeting a preinstalled
 LLVM. To do this, configure the LLVM build tree as follows:
 
-    $ cmake -DLLVM_OCAML_OUT_OF_TREE=TRUE \
-            -DCMAKE_INSTALL_PREFIX=[OCaml install prefix] \
+    $ cmake -DLLVM_OCAML_EXTERNAL_LLVM_LIBDIR=[Location of the preinstalled LLVM] \
+            -DCMAKE_BUILD_TYPE=[Build-mode of the preinstalled LLVM] \
+            -DLLVM_TARGETS_TO_BUILD=[Targets built of the preinstalled LLVM] \
+            -DLLVM_OCAML_INSTALL_PATH=[OCaml install prefix] \
             [... any other options]
 
+You might also want to set -DBUILD_SHARED_LIBS depending on whether you want a statically or dynamically linked library.
+This requires at least that you check if your preinstalled LLVM supports dynamic linking.
+
 then build and install it as:
 
     $ make ocaml_all

cmake/modules/AddOCaml.cmake

     list(APPEND ocaml_flags "-custom")
   endif()
 
+  if(DEFINED LLVM_OCAML_EXTERNAL_LLVM_LIBDIR)
+    list(APPEND ocaml_flags "-ccopt" "-L${LLVM_OCAML_EXTERNAL_LLVM_LIBDIR}" )
+  endif()
+
   explicit_map_components_to_libraries(llvm_libs ${ARG_LLVM})
-  foreach( llvm_lib ${llvm_libs} )
-    list(APPEND ocaml_flags "-l${llvm_lib}" )
-  endforeach()
+  if(BUILD_SHARED_LIBS)
+    list(APPEND ocaml_flags "-lLLVM-${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}${LLVM_VERSION_SUFFIX}")
+  else()
+    foreach( llvm_lib ${llvm_libs} )
+      list(APPEND ocaml_flags "-l${llvm_lib}" )
+    endforeach()
+  endif()
 
   get_property(system_libs TARGET LLVMSupport PROPERTY LLVM_SYSTEM_LIBS)
   foreach(system_lib ${system_libs})

     add_dependencies("ocaml_${name}" "ocaml_${ocaml_dep}")
   endforeach()
 
-  if( NOT LLVM_OCAML_OUT_OF_TREE )
+  if(NOT DEFINED LLVM_OCAML_EXTERNAL_LLVM_LIBDIR)
     foreach( llvm_lib ${llvm_libs} )
       add_dependencies("ocaml_${name}" "${llvm_lib}")
     endforeach()

test/CMakeLists.txt

 
 if(TARGET ocaml_llvm)
   # Clear all non-OCaml cross-target dependencies when building out-of-tree.
-  if(LLVM_OCAML_OUT_OF_TREE)
+  if(DEFINED LLVM_OCAML_EXTERNAL_LLVM_LIBDIR)
     set(LLVM_TEST_DEPENDS)
   endif()