[cmake] Add a new flag MERGE_INTO_LLVM_DYLIB to llvm_add_library

[kikairoya]

In case of dylib build (i.e. -DLLVM_LINK_LLVM_DYLIB), add_llvm_pass_plugin with LLVM_${plugin}_LINK_INTO_TOOLS embeds the plugin into libLLVM using llvm_add_library(... DISABLE_LLVM_LINK_LLVM_DYLIB ...), which enables preprocessor macro -DLLVM_BUILD_STATIC.
This definition is correct for a library or an executable which is built on top of libLLVM, but wrong for a plugin which is embedded into libLLVM.

This patch adds a new flag MERGE_INTO_LLVM_DYLIB to llvm_add_library, to support embedding a plugin by add_llvm_pass_plugin with LLVM_${plugin}_LINK_INTO_TOOLS.

[Meinersbur]

Which combinations of flags BUILD_SHARED_LIBS, LLVM_LINK_LLVM_DYLIB, LLVM_BUILD_LLVM_DYLIB, LLVM_LINK_POLLY_INTO_TOOLS, LLVM_LINK_BYE_INTO_TOOLS, LLVM_ABI_BREAKING_CHECKS, build modes (LLVM_ENABLE_PROJECTS=polly, Polly standalone build), and operating systems did you test?

This seems very redundant with DISABLE_LLVM_LINK_LLVM_DYLIB. Isn't MERGE_INTO_LLVM_DYLIB just the abscence of DISABLE_LLVM_LINK_LLVM_DYLIB?

Is the problem that add_llvm_pass_plugin only passes OBJECT which does not handle the same way as other libraries?

[kikairoya]

Which combinations of flags BUILD_SHARED_LIBS, LLVM_LINK_LLVM_DYLIB, LLVM_BUILD_LLVM_DYLIB, LLVM_LINK_POLLY_INTO_TOOLS, LLVM_LINK_BYE_INTO_TOOLS, LLVM_ABI_BREAKING_CHECKS, build modes (LLVM_ENABLE_PROJECTS=polly, Polly standalone build), and operating systems did you test?

I've tested ninja check-polly (ninja check-polly LLVM for dylib builds) on the following:

• x86_64-windows-msvc
  • static build (with LLVM_EXPORT_SYMBOLS_FOR_PLUGINS)
  • -DLLVM_LINK_LLVM_DYLIB=ON
    (showing warning: test suite 'Polly-Unit' contained no tests; might be caused by lacking of LLVM_ABI)
  • not supported: -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_LINK_POLLY_INTO_TOOLS=OFF
  • not supported: Bye
• x86_64-cygwin (very similar to x86_64-mingw)
  • static build
  • -DBUILD_SHARED_LIBS=ON
  • -DLLVM_LINK_LLVM_DYLIB=ON
  • not supported: -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_LINK_POLLY_INTO_TOOLS=OFF
  • not supported: Bye
• x86_64-linux
  • static build
  • -DLLVM_LINK_LLVM_DYLIB=ON
  • -DLLVM_LINK_LLVM_DYLIB=ON -DLLVM_LINK_POLLY_INTO_TOOLS=OFF

Other configurations are going on.

This seems very redundant with DISABLE_LLVM_LINK_LLVM_DYLIB. Isn't MERGE_INTO_LLVM_DYLIB just the abscence of DISABLE_LLVM_LINK_LLVM_DYLIB?

Is the problem that add_llvm_pass_plugin only passes OBJECT which does not handle the same way as other libraries?

DISABLE_LLVM_LINK_LLVM_DYLIB isn't suitable here, because it defines LLVM_BUILD_STATIC and doesn't define LLVM_EXPORTS. In practice, that disables __declspec(dllexport) and causes the plugin's entry point to be hidden.
BTW, one might argue here; a DLL might have to export all public interfaces, even if they were defined in a statically linked library, otherwise a symbol dup will cause easily.

Just dropping DISABLE_LLVM_LINK_LLVM_DYLIB isn't an option either, because it causes a recursive dependency between LLVM and Polly.
A plugin built with LLVM_LINK_xxx_INTO_TOOLS=ON can't depend on the monolithic dylib LLVM, and this constraint is exactly what was cared by DISABLE_LLVM_LINK_LLVM_DYLIB.

Treating statically linked plugins as COMPONENT_LIB, as other regular components of LLVM, also isn't viable; the CMakeLists.txt for the plugin are processed after the component library list has already been finalized.

Thus, we need a flag that behaves like DISABLE_LLVM_LINK_LLVM_DYLIB, but defines LLVM_EXPORTS and doesn't define LLVM_BUILD_STATIC. Dropping LLVM_BUILD_STATIC from DISABLE_LLVM_LINK_LLVM_DYLIB might be also an option but it would affect very widely.

[Meinersbur]

Is it correct that this is supposed to fix Polly with -DLLVM_LINK_LLVM_DYLIB=ON in Windows (#156440), or other problems as well? With #109483, libLLVM.dll in Windows is a releatively new development. There seem to be other related PRs such as #115431. I think this should be coordinated with the people working on LLVM_ABI annotation. @fsfod

It currently is not clear to me when MERGE_INTO_LLVM_DYLIB should be used. The description uses "foreign project library", but when is a library "foreign"?

• -DLLVM_LINK_LLVM_DYLIB=ON
  (showing warning: test suite 'Polly-Unit' contained no tests

llvm-lit prints this warning when it does not find any unittest executables in the unittest build dir. ISLToolsTests.exe, IslTests.exe should be present there.

[compnerd]

I really do not like this direction. We already have the proper tools for this in CMake: Object Libraries. We should explicitly create the object library which will effectively "merge" the libraries. All of this custom scaffolding in LLVM makes it harder for newcomers to the project and prevents the use of the existing documentation from being useful.

[petrhosek]

I really do not like this direction. We already have the proper tools for this in CMake: Object Libraries. We should explicitly create the object library which will effectively "merge" the libraries. All of this custom scaffolding in LLVM makes it harder for newcomers to the project and prevents the use of the existing documentation from being useful.

I fully agree, this change is adding more complexity making it harder to understand and maintain the LLVM build system. We should be instead looking for opportunities to simplify our build system by leveraging standard CMake mechanisms like object libraries which seems to be the best fit for what you're trying to achieve with this change.