[Clang] Fix visibility macros for c-index-test

[kkent030315]

Fixes #152083 - duplicate symbols in c-index-test

• Eliminated the use of CLANG_EXPORTS for Clang component libraries.
• Removed unnecessary dllimport attributes from Compiler.h. They aren't strictly required (the linker will resolve them).

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (kkent030315)

Changes

Fixes #152083 - duplicate symbols in c-index-test

• Eliminated the use of CLANG_EXPORTS for Clang component libraries.
• Removed unnecessary dllimport attributes from Compiler.h. They aren't strictly required (the linker will resolve them).

---

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

2 Files Affected:

• (modified) clang/cmake/modules/AddClang.cmake (-3)
• (modified) clang/include/clang/Support/Compiler.h (+2-2)

diff --git a/clang/cmake/modules/AddClang.cmake b/clang/cmake/modules/AddClang.cmake
index 4059fc3e986c7..9dcc77d5a5184 100644
--- a/clang/cmake/modules/AddClang.cmake
+++ b/clang/cmake/modules/AddClang.cmake
@@ -115,9 +115,6 @@ macro(add_clang_library name)
     if(TARGET "obj.${name}")
       target_compile_definitions("obj.${name}" PUBLIC CLANG_BUILD_STATIC)
     endif()
-  elseif(TARGET "obj.${name}" AND NOT ARG_SHARED AND NOT ARG_STATIC)
-    # Clang component libraries linked to clang-cpp are declared without SHARED or STATIC
-    target_compile_definitions("obj.${name}" PUBLIC CLANG_EXPORTS)
   endif()
 
   set(libs ${name})
diff --git a/clang/include/clang/Support/Compiler.h b/clang/include/clang/Support/Compiler.h
index e1ae3eda4ccc2..1aab64e11521c 100644
--- a/clang/include/clang/Support/Compiler.h
+++ b/clang/include/clang/Support/Compiler.h
@@ -45,8 +45,8 @@
 #define CLANG_TEMPLATE_ABI
 #define CLANG_EXPORT_TEMPLATE __declspec(dllexport)
 #else
-#define CLANG_ABI __declspec(dllimport)
-#define CLANG_TEMPLATE_ABI __declspec(dllimport)
+#define CLANG_ABI
+#define CLANG_TEMPLATE_ABI
 #define CLANG_EXPORT_TEMPLATE
 #endif
 #elif defined(__ELF__) || defined(__MINGW32__) || defined(_AIX) ||             \

[kkent030315]

This also eliminates 5000+ unnecessary symbol exports in clang.dll and clang.exe on non-static builds.

To test this change turn CLANG_INCLUDE_TESTS and LLVM_BUILD_STATIC on/off. This patch should only take an effects on Windows builds.

cmake -S llvm -B build -G Ninja -DLLVM_ENABLE_PROJECTS="clang" -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD="X86" -DLLVM_BUILD_TESTS=OFF -DCLANG_INCLUDE_TESTS={OFF,ON} -DLLVM_BUILD_STATIC={OFF,ON}

[kkent030315]

Probably related to #163349, I'm still not yet sure if this is the best solution. Please let me know if you have better ideas, thanks.

[kikairoya]

I believe this breaks dylib builds (-DCLANG_LINK_CLANG_DYLIB=ON), which is still in progress.
__declspec(dllimport) is essential for that.

[kkent030315]

I believe this breaks dylib builds (-DCLANG_LINK_CLANG_DYLIB=ON), which is still in progress. __declspec(dllimport) is essential for that.

How so? Can you be more specific how this would break dylib builds, and what exactly is in the progress?

As mentioned in the comment dllimport attribute is not strictly required, it's safe to remove it.

[kkent030315]

Apparently -DCLANG_LINK_CLANG_DYLIB=ON is still a lot of mess right now, couldn't get it built on main.

RuntimeLibcallInfo.h:37:3: error: attribute 'dllexport' cannot be applied to member of 'dllexport' class
   37 |   LLVM_ABI static AnalysisKey Key;

[kikairoya]

How so? Can you be more specific how this would break dylib builds, and what exactly is in the progress?

As mentioned in the comment dllimport attribute is not strictly required, it's safe to remove it.

For code symbols, the attribute isn't strictly required, just suboptimal; lacking of dllimport causes additional jumps. However, for data symbols, it's mandatory (except for MinGW auto-import feature). See also: #149639 (comment).
I'm not sure about the current progress of dylib builds for Windows.

CLANG_ABI is supposed to annotate clang-cpp.dll (which exposes C++ interfaces), but not libclang.dll (which exposes C interfaces). I think the problem is that libclang.dll is exporting C++ interfaces; that looks unintended.

The issue report uses a GNU style command line for the windows-msvc target, but such that configuration doesn't seem to have been tested well. Perhaps some of if (MSVC) blocks should be changed to (WIN32 AND NOT MINGW).

[kikairoya]

Perhaps some of if (MSVC) blocks should be changed to (WIN32 AND NOT MINGW).

(MSVC OR CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC") might be better. https://gitlab.kitware.com/cmake/cmake/-/issues/19724

[kkent030315]

@kikairoya Thank you for the detailed info! I appreciate.

I think the problem is that libclang.dll is exporting C++ interfaces; that looks unintended.

Yes, it is. In the case we're not modifying dllimport attributes in Compiler.h what would you think is the best/better solution to fix c-index-test?

[kkent030315]

Perhaps some of if (MSVC) blocks should be changed to (WIN32 AND NOT MINGW).

(MSVC OR CMAKE_CXX_SIMULATE_ID STREQUAL "MSVC") might be better. gitlab.kitware.com/cmake/cmake/-/issues/19724

I don't think this is suitable for this, let me do some tests. (I did the similar before submitting this PR, didnt worked iirc)

[kikairoya]

In the case we're not modifying dllimport attributes in Compiler.h what would you think is the best/better solution to fix c-index-test?

We should not to export the C++ interfaces, to align with every other build configurations (e.g. windows-msvc with clang-cl, mingw, linux, ...)

I don't think this is suitable for this, let me do some tests. (I did the similar before submitting this PR, didnt worked iirc)

I guess there are bunch of related blocks.

llvm-project/clang/cmake/modules/AddClang.cmake

Lines 111 to 117 in e7f6038

	if(MSVC AND NOT CLANG_LINK_CLANG_DYLIB)
	# Make sure all consumers also turn off visibility macros so they're not
	# trying to dllimport symbols.
	target_compile_definitions(${name} PUBLIC CLANG_BUILD_STATIC)
	if(TARGET "obj.${name}")
	target_compile_definitions("obj.${name}" PUBLIC CLANG_BUILD_STATIC)
	endif()

llvm-project/clang/tools/libclang/CMakeLists.txt

Lines 119 to 121 in e7f6038

	if (MSVC AND ENABLE_SHARED AND ENABLE_STATIC)
	unset(ENABLE_STATIC)
	endif()

llvm-project/llvm/cmake/modules/AddLLVM.cmake

Lines 1389 to 1390 in e7f6038

	if (MSVC)
	set(mangling microsoft)

(These may not be all; every related block should be fixed.)