[CMake][OCaml] Make OCaml bindings suitable for out-of-tree install #123478

alan-j-hu · 2025-01-18T21:30:53Z

CMAKE_{STATIC,SHARED}_LIBRARY_SUFFIX reports incorrect suffixes for Mac. Use .so and .a for both Mac and Linux.
Add LLVM_OCAML_EXTERNAL_LLVM_LIBDIR option.
LLVM dynamic library is always suffixed with major version

- CMAKE_{STATIC,SHARED}_LIBRARY_SUFFIX reports incorrect suffixes for Mac. Use .so and .a for both Mac and Linux. - Add LLVM_OCAML_EXTERNAL_LLVM_LIBDIR option. - LLVM dynamic library is always suffixed with major version

nikic · 2025-01-18T22:16:28Z

CMAKE_{STATIC,SHARED}_LIBRARY_SUFFIX reports incorrect suffixes for Mac. Use .so and .a for both Mac and Linux.

What's the incorrect suffix that gets reported? Your change doesn't really make sense to me, as the dylib suffix for MacOs is .dylib, not .so.

alan-j-hu · 2025-01-19T01:45:34Z

@nikic

See https://github.com/ocaml/opam-source-archives/blob/main/patches/llvm/fix-macos.patch.14.0.6, for example. This patch is necessary to distribute the LLVM OCaml bindings on opam.

In the given CMake snippet, we are working in OCaml land, not C/C++ land. The OCaml compiler generates .so and .a for its outputs, including on Mac.

alan-j-hu · 2025-01-19T04:34:25Z

Fixed up more of the logic.

In the CMake code of concern, we are talking about the outputs of ocamlmklib. This should output .o and .a files on Mac, which should render the usage of CMake variables incorrect. See https://ocaml.org/manual/5.3/intfc.html#s%3Aocamlmklib.

nikic · 2025-01-23T08:39:05Z

llvm/cmake/modules/AddOCaml.cmake

  set(ocaml_inputs)

  set(ocaml_outputs "${bin}/${name}.cma")
+  # Always use -custom when building in-tree


Why do we always use -custom for in-tree builds? Based on https://ocaml.org/manual/4.10/intfc.html#ss:staticlink-c-code, do I correctly understand that you need -custom if you're linking statically? Wasn't the previous BUILD_SHARED_LIBS based condition correct for that?

alan-j-hu · 2025-01-25T05:09:29Z

Thank you for the review. The CMake code on the main branch is

  if( NOT BUILD_SHARED_LIBS )
    list(APPEND ocaml_flags "-custom")
  endif()

I did some investigation and figured out that if you leave out the -custom flag, when you run the test suite, you will hit errors that look like

: CommandLine Error: Option 'experimental-debuginfo-iterators' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options

and the tests will fail.

This seems to be an issue with CommandLine, talked about at https://discourse.llvm.org/t/rfc-commandline-allow-loading-1-library-linking-to-the-same-libllvm-version/67542. If BUILD_SHARED_LIBS=ON, then this error will occur. Passing the -custom flag when building the OCaml bindings forces static linking, avoiding this issue.

However, it seems that when building out-of-tree and installing the bindings as an OCaml package, the -custom flag causes bytecode executables not to work. I get errors that look like:

/usr/bin/ld: cannot find -lllvm_analysis: No such file or directory
/usr/bin/ld: cannot find -lllvm_executionengine: No such file or directory
/usr/bin/ld: cannot find -lctypes_stubs: No such file or directory
/usr/bin/ld: cannot find -lintegers_stubs: No such file or directory
/usr/bin/ld: cannot find -lllvm_target: No such file or directory
/usr/bin/ld: cannot find -lllvm: No such file or directory
collect2: error: ld returned 1 exit status

(I tried testing this with LLVM 14, which is the last version published on OPAM to build with CMake, but the bytecode executable failed to build for a different reason that has since been fixed: https://discuss.ocaml.org/t/llvm-symbol-not-found-for-bytecode-compilation/8728.)

So, it seems that when building in-tree, -custom is necessary for building bytecode executables, but when building out-of-tree and installing, -custom causes the bytecode executable to fail. (The -custom flag only affects bytecode executables generated by ocamlc, not native executables generated by ocamlopt.)

alan-j-hu · 2025-01-26T01:33:14Z

I uncovered an issue when running OCaml bytecode executables, compiled using the LLVM static library. It's difficult for me to test this PR because of the way OCaml packages are handled. Therefore, I am currently not confident in my ability to ensure this PR is correct. I need to take a breather and figure out a good way to test the OCaml package installation (perhaps via CI), before continuing to work on this.

Ericson2314 · 2025-01-26T18:27:36Z

llvm/cmake/modules/AddOCaml.cmake

+         "${bin}/lib${name}.a")
+    if ( NOT ocaml_custom )
      list(APPEND ocaml_outputs
-           "${bin}/dll${name}${CMAKE_SHARED_LIBRARY_SUFFIX}")
+           "${bin}/dll${name}.so")


Why did you change the suffixes? What about Windows when we have .lib and .dll?

Oh I see past talk on this, I still don't like this solution, however.

Ericson2314

IMO the solution is to move the the bindings outside of /llvm and instead imitate how the other "packages" works. (See MLIR_STANDALONE_BUILD, CLANG_STANDALONE_BUILD, OPENMP_STANDALONE_BUILD etc.)

Ericson2314 · 2025-01-26T18:39:02Z

llvm/bindings/ocaml/README.txt

    $ cmake -DLLVM_OCAML_OUT_OF_TREE=TRUE \
-            -DCMAKE_INSTALL_PREFIX=[Preinstalled LLVM path] \
-            -DLLVM_OCAML_INSTALL_PATH=[OCaml install prefix] \
+            -DLLVM_OCAML_EXTERNAL_LLVM_LIBDIR=[LLVM libdir, e.g. `llvm-config --libdir`] \


We shouldn't do this, instead with an OCAML_BINDINGS_BUILD_STANDALONE we would do a findPackage(LLVM ...). That in turn would set LLVM_LIBRARY_DIR to be the external dir, and we wouldn't need the conditions below.

alan-j-hu requested a review from Ericson2314 January 18, 2025 21:31

llvmbot added the cmake Build system in general and CMake in particular label Jan 18, 2025

alan-j-hu requested a review from nikic January 18, 2025 21:31

alan-j-hu added 3 commits January 18, 2025 21:43

Remove usage of CMAKE_SHARED_LIBRARY_SUFFIX elsewhere in AddOCaml.cmake

583a0e1

In AddOCaml.cmake, replace broken BUILD_SHARED_LIBS logic

8ae9be9

Revise instructions in OCaml bindings README

3fcb90f

nikic reviewed Jan 23, 2025

View reviewed changes

Pass -custom for in-tree build, but not for out-of-tree build

0e007ad

Define ocaml_custom

efb9b9f

Ericson2314 reviewed Jan 26, 2025

View reviewed changes

Ericson2314 requested changes Jan 26, 2025

View reviewed changes

Ericson2314 reviewed Jan 26, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CMake][OCaml] Make OCaml bindings suitable for out-of-tree install #123478

[CMake][OCaml] Make OCaml bindings suitable for out-of-tree install #123478

Uh oh!

alan-j-hu commented Jan 18, 2025

Uh oh!

nikic commented Jan 18, 2025

Uh oh!

alan-j-hu commented Jan 19, 2025

Uh oh!

alan-j-hu commented Jan 19, 2025

Uh oh!

nikic Jan 23, 2025

Uh oh!

alan-j-hu commented Jan 25, 2025

Uh oh!

alan-j-hu commented Jan 26, 2025

Uh oh!

Ericson2314 Jan 26, 2025

Uh oh!

Ericson2314 Jan 26, 2025

Uh oh!

Ericson2314 left a comment

Uh oh!

Ericson2314 Jan 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[CMake][OCaml] Make OCaml bindings suitable for out-of-tree install #123478

Are you sure you want to change the base?

[CMake][OCaml] Make OCaml bindings suitable for out-of-tree install #123478

Uh oh!

Conversation

alan-j-hu commented Jan 18, 2025

Uh oh!

nikic commented Jan 18, 2025

Uh oh!

alan-j-hu commented Jan 19, 2025

Uh oh!

alan-j-hu commented Jan 19, 2025

Uh oh!

nikic Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

alan-j-hu commented Jan 25, 2025

Uh oh!

alan-j-hu commented Jan 26, 2025

Uh oh!

Ericson2314 Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

Ericson2314 left a comment

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Jan 26, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants