Page MenuHomePhabricator

Fix the OCaml external linking problem
Needs RevisionPublic

Authored by jpdeplaix on Aug 31 2017, 2:19 AM.

Details

Reviewers
whitequark
mgorny
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.

Diff Detail

Repository
rL LLVM

Event Timeline

jpdeplaix created this revision.Aug 31 2017, 2:19 AM
mgorny added inline comments.Aug 31 2017, 7:05 AM
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 requested changes to this revision.Oct 15 2017, 12:06 AM

I agree with pretty much all @mgorny says.

This revision now requires changes to proceed.Oct 15 2017, 12:06 AM

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 added inline comments.Oct 17 2017, 8:06 AM
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.

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.

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?

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.

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

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 updated this revision to Diff 120149.Oct 24 2017, 4:54 PM
jpdeplaix edited edge metadata.

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

jpdeplaix marked 2 inline comments as done.Oct 24 2017, 4:57 PM

@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.

whitequark accepted this revision.Oct 24 2017, 5:05 PM
This revision is now accepted and ready to land.Oct 24 2017, 5:05 PM
mgorny requested changes to this revision.Oct 25 2017, 2:29 AM
mgorny added inline comments.
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.

This revision now requires changes to proceed.Oct 25 2017, 2:29 AM

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:

Do we need to also change between the two there ?

mgorny added inline comments.Dec 11 2017, 12:09 PM
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).