Additional patch to Clangs CMake code is required in case LLVM was built with `LLVM_LINK_LLVM_DYLIB=ON`

[guitargeek]

Description

Additional patches to Clangs CMake code are required in case LLVM was built with LLVM_LINK_LLVM_DYLIB=ON, similar to the patches that were done in ROOT here:

• [CMake] Fixes and improvements for buildng with builtin_clang=OFF #18386

diff --git a/interpreter/llvm-project/clang/CMakeLists.txt b/interpreter/llvm-project/clang/CMakeLists.txt
index 5f2b7f064d..3a2e593aa7 100644
--- a/interpreter/llvm-project/clang/CMakeLists.txt
+++ b/interpreter/llvm-project/clang/CMakeLists.txt
@@ -29,6 +29,15 @@ if(CLANG_BUILT_STANDALONE)
   endif()

   find_package(LLVM REQUIRED HINTS "${LLVM_CMAKE_DIR}")
+
+  # Disable linking against shared LLVM. We have to do this after
+  # find_package(LLVM), which overwrites LLVM_LINK_LLVM_DYLIB to what it was
+  # set when building LLVM. Clang needs to link LLVM statically, otherwise it
+  # won't work for Cling. By doing forcing this, we can build Clang on top of
+  # existing LLVM builds even they were built with LLVM_LINK_LLVM_DYLIB=ON,
+  # which is usually the case for system packages.
+  set(LLVM_LINK_LLVM_DYLIB FALSE)
+
   list(APPEND CMAKE_MODULE_PATH "${LLVM_DIR}")

   # Turn into CACHE PATHs for overwritting

Where should this patch go? Would it be reasonable to do it in our fork of LLVM and then sync ROOT with it?
https://github.com/root-project/llvm-project

@hahnjo, your advice would be very appreciated here!

Reproducer

• take LLVM build with LLVM_LINK_LLVM_DYLIB=ON (usually the case for the system packages)
• build patched Clang for ROOT standalone
• try to build ROOT with builtin_llvm=OFF and builtin_clang=OFF

Only with the patch in the Clang CMakeLists.txt, it works for me.

Additional context

See also this issue for some context on why it is necessary to statically link against LLVM:

• build: Allow building ROOT using the Clang shared library #17698

[guitargeek]

Some more context for the curious: I have made my custom clang-root package for NixOS, and this is the only patch still required to make things run very smoothly:
https://github.com/guitargeek/dotfiles/blob/master/nixos/pkgs/clang-root.nix#L34

I'm also intending to upstream this patched Clang to nix packages with the upcoming ROOT release:
NixOS/nixpkgs#390706

[ferdymercury]

Related hurdle: #17209 (comment)

[devajithvs]

This is only a problem in ROOT: #12156, standalone cling should be unaffected (dynamic LLVM).

I'm not very in favor for a patch to our LLVM fork as we’re actively trying to reduce such patches. It would be better if we can handle this from cling/ROOT side.

Ideally, ROOT should also build with LLVM_LINK_LLVM_DYLIB=ON, I did look into this briefly and wasn't able to quite figure out a fix that time. I will have a look again with fresh eyes to see what can be done/possible? to make ROOT build with LLVM_LINK_LLVM_DYLIB=ON

[guitargeek]

Hi @devajithvs, well if you can fix it on the ROOT side that would of course be great.

However, I think that this is maybe an Clang problem and not necessarily a ROOT problem. Here are the relevant two commits in LLVM:

• llvm/llvm-project@3b8b3c2
• llvm/llvm-project@5fed24a

The commit message of the second linked commit says:

By only exporting the setting if it is enabled, out-of-tree clients get the correct default, but may still choose if they can.

In our example, Clang is the out-of-tree client, and I don't see the reason why it should not be possible to configure it to use the non-default value of LLVM_LINK_LLVM_DYLIB. So to me, this looks like an oversight in Clangs configuration options.

Also, probably you are familiar with this discussion already, but doesn't hurt to link it again:

• Building fails with builtin_llvm=OFF: CommandLine Error: Option 'W' registered more than once! #12156

[pcanal]

Ideally, ROOT should also build with LLVM_LINK_LLVM_DYLIB=ON,

Note that besides CMake things, this might not be desirable as this may leads to symbols leaking and/or conflicting with other package (OpenGL things, etc) that also use LLVM.

[guitargeek]

Correct.

I have made a suggestion upstream, let's see what people say!
llvm/llvm-project#135570

[hahnjo]

I'm not very in favor for a patch to our LLVM fork as we’re actively trying to reduce such patches.

I would be even stronger here: builtin_llvm=OFF and builtin_clang=OFF are not officially supported configurations, they are maintained on a best-effort basis at the moment. The user will have to compile Clang from our sources anyway to get our patches. For that reason, we must not add downstream patches just for this. Also in the future, please start by describing the problem and not just stating one narrow solution...

It would be better if we can handle this from cling/ROOT side.

If that works, that's fine with me. Note that we link all libraries into libCling, usually statically, so that is the place where we can influence choices.

Ideally, ROOT should also build with LLVM_LINK_LLVM_DYLIB=ON, I did look into this briefly and wasn't able to quite figure out a fix that time. I will have a look again with fresh eyes to see what can be done/possible? to make ROOT build with LLVM_LINK_LLVM_DYLIB=ON

For future reference: The first problem encountered is that rootcling relies on the LLVM command line parsing classes, but registers an option that conflicts with LLVM. One possible approach is to use a different way for option parsing, potentially Clang's.

[hahnjo]

Ideally, ROOT should also build with LLVM_LINK_LLVM_DYLIB=ON,

Note that besides CMake things, this might not be desirable as this may leads to symbols leaking and/or conflicting with other package (OpenGL things, etc) that also use LLVM.

This is only for builtin_llvm=OFF in which case the user expressed their wish to not care about this.

[guitargeek]

I would be even stronger here: builtin_llvm=OFF and builtin_clang=OFF are not officially supported configurations, they are maintained on a best-effort basis at the moment

I don't think we can afford this stance. The builtin_llvm=OFF and builtin_clang=OFF is used for example by the Conda package, which is very widely used by the HEP community if we like it or not...

This is only for builtin_llvm=OFF in which case the user expressed their wish to not care about this.

I don't understand why this is related: the user might want to build with builtin_llvm=OFF and still care about leaking symbols and therefore statically link against LLVM, no?

[guitargeek]

Not relevant for me anymore to have this upstream, I'm happy to track such controversial patches myself 👍