[clangd] Update clangDaemonTweaks to set symbol visibility macros

[fsfod]

Update clangDaemonTweaks cmake target to set explicit symbol visibility macros to correct mode for windows.
This will fix linker duplicate symbols errors from clangDaemonTweaks exporting clang symbols instead of importing them for windows shared library builds using explicit symbol visibly.

This is part of the work to enable LLVM_BUILD_LLVM_DYLIB and LLVM/Clang plugins on window.

[llvmbot]

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang

Author: Thomas Fransham (fsfod)

Changes

Update clangDaemonTweaks cmake target to set explicit symbol visibility macros to correct mode for windows.
This will fix linker duplicate symbols errors from clangDaemonTweaks exporting clang symbols instead of importing them for windows shared library builds using explicit symbol visibly.

This is part of the work to enable LLVM_BUILD_LLVM_DYLIB and LLVM/Clang plugins on window.

---

Full diff: https://github.com/llvm/llvm-project/pull/112304.diff

2 Files Affected:

• (modified) clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt (+1-1)
• (modified) clang/cmake/modules/AddClang.cmake (+2-2)

diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
index 59475b0dfd3d22..fa4ccba52e8831 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
@@ -11,7 +11,7 @@ set(LLVM_LINK_COMPONENTS
 # To enable these tweaks in executables or shared libraries, add
 # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
 # clangd/tool/CMakeLists.txt for an example.
-add_clang_library(clangDaemonTweaks OBJECT
+add_clang_library(clangDaemonTweaks CLANG_IMPORT OBJECT
   AddUsing.cpp
   AnnotateHighlightings.cpp
   DumpAST.cpp
diff --git a/clang/cmake/modules/AddClang.cmake b/clang/cmake/modules/AddClang.cmake
index 091aec98e93ca3..33f47190afa26e 100644
--- a/clang/cmake/modules/AddClang.cmake
+++ b/clang/cmake/modules/AddClang.cmake
@@ -47,7 +47,7 @@ endmacro()
 
 macro(add_clang_library name)
   cmake_parse_arguments(ARG
-    "SHARED;STATIC;INSTALL_WITH_TOOLCHAIN"
+    "SHARED;STATIC;INSTALL_WITH_TOOLCHAIN;CLANG_IMPORT"
     ""
     "ADDITIONAL_HEADERS"
     ${ARGN})
@@ -114,7 +114,7 @@ macro(add_clang_library name)
     if(TARGET "obj.${name}")
       target_compile_definitions("obj.${name}" PUBLIC CLANG_BUILD_STATIC)
     endif()
-  elseif(NOT ARG_SHARED AND NOT ARG_STATIC)
+  elseif(NOT ARG_SHARED AND NOT ARG_STATIC AND NOT ARG_CLANG_IMPORT)
     # Clang component libraries linked in to clang-cpp are declared without SHARED or STATIC
     target_compile_definitions("obj.${name}" PUBLIC CLANG_EXPORTS)
   endif()

[vgvassilev]

@AaronBallman, can you help us move forward here?

[compnerd]

It is unclear what this new option (CLANG_IMPORT) does.

[fsfod]

Its not possible to infer if a OBJECT target should be importing or exporting clang symbols, this option allows OBJECT target to specify it should be importing Clang symbols.

[compnerd]

We should document the option. Why is it specific to clang? I think that the right semantics are, is this library built as a static library or not. The default should be dynamic. At that point, when the library is built as static, a public definition could be added at the library definition to state it is static. That would then implicitly avoid the DLL storage. Otherwise, it is implicitly DLL imported.

#if defined(clang_STATIC)
# define CLANG_ABI
#else
# if defined(_WIN32)
#   if defined(clang_EXPORTS)
#     define CLANG_ABI __declspec(dllexport)
#   else
#     define CLANG_ABI __declspec(dllimport)
#   endif
# else
...
# endif

We don't need all this new custom handling to be wired through out the build to support this. Each library needs only to know its ABI, and in the case that it's a compacted library (i.e. is an object library), we update the export symbol that CMake uses to the DLL that it is compacted into.

Am I missing something in this model?

[compnerd]

This seems like it would change the behaviour of other libraries which were previously being built for shared linking (or at least were exporting their interfaces) to now be built for static linking (or no longer export their symbols).

[fsfod]

what do you mean, its only checking for the arg not being set, so it shouldn't change the behaviour for other libraries.

[compnerd]

Then we should be using if (NOT DEFINED ...). The syntax you have can check for the truthiness of the value.

[llvm-beanz]

cmake_parse_args always defines option values:

For the keywords, these variables will always be defined, and they will be set to TRUE if the keyword is present, or FALSE if it is not.

see: https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html

So ARG_CLANG_IMPORT will always be defined, it may be defined either to True or False.

[AaronBallman]

Ping @petrhosek @Ericson2314 for review

[vgvassilev]

ping.

[llvm-beanz]

What is the case where an object library wouldn't need to act as CLANG_IMPORT? It seems to me like the object library needs to be built assuming that it is importing clang symbols unless it is one of the core clang components.

[fsfod]

Yes I could rename it and make it the set symbol visibility macros to export instead.