[libclang] Add missing dllexport annotation

[kikairoya]

All other declarations of clang-c already have CINDEX_LINKAGE.

This missing annotation causes a linker error when building tools/clang/unittests/libclang/CrashTests/libclangCrashTests.exe for the Cygwin target.
On the regular Win32 target, this issue went unnoticed because the entire libclang gtest-based testsuite is currently disabled for that platform.

[llvmbot]

@llvm/pr-subscribers-clang

Author: Tomohiro Kashiwada (kikairoya)

Changes

All other declarations of clang-c already have CINDEX_LINKAGE.

---

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

1 Files Affected:

• (modified) clang/include/clang-c/FatalErrorHandler.h (+3-2)

diff --git a/clang/include/clang-c/FatalErrorHandler.h b/clang/include/clang-c/FatalErrorHandler.h
index 22f34fa815ccf..4f18980dea240 100644
--- a/clang/include/clang-c/FatalErrorHandler.h
+++ b/clang/include/clang-c/FatalErrorHandler.h
@@ -11,6 +11,7 @@
 #define LLVM_CLANG_C_FATAL_ERROR_HANDLER_H
 
 #include "clang-c/ExternC.h"
+#include "clang-c/Platform.h"
 
 LLVM_CLANG_C_EXTERN_C_BEGIN
 
@@ -18,14 +19,14 @@ LLVM_CLANG_C_EXTERN_C_BEGIN
  * Installs error handler that prints error message to stderr and calls abort().
  * Replaces currently installed error handler (if any).
  */
-void clang_install_aborting_llvm_fatal_error_handler(void);
+CINDEX_LINKAGE void clang_install_aborting_llvm_fatal_error_handler(void);
 
 /**
  * Removes currently installed error handler (if any).
  * If no error handler is intalled, the default strategy is to print error
  * message to stderr and call exit(1).
  */
-void clang_uninstall_llvm_fatal_error_handler(void);
+CINDEX_LINKAGE void clang_uninstall_llvm_fatal_error_handler(void);
 
 LLVM_CLANG_C_EXTERN_C_END
 

[mstorsjo]

AFAIK the dllexport annotations have been added using some automated tools, so it may be good to find the person who added the other annotations, so it can be looked into why this was missing here, if the annotations otherwise were seemingly complete enough.

[mstorsjo]

AFAIK the dllexport annotations have been added using some automated tools, so it may be good to find the person who added the other annotations, so it can be looked into why this was missing here, if the annotations otherwise were seemingly complete enough.

Sorry, not sure if this applies here though, I guess the clang-c interface has been available as DLL with dllexports even before, while the C++ interfaces is what is getting dllexport attributes added.

Still it may be good to figure out why this hasn't been an issue so far, for whoever otherwise were using these dllexport annotations.

[kikairoya]

AFAIK the dllexport annotations have been added using some automated tools, so it may be good to find the person who added the other annotations, so it can be looked into why this was missing here, if the annotations otherwise were seemingly complete enough.

Sorry, not sure if this applies here though, I guess the clang-c interface has been available as DLL with dllexports even before, while the C++ interfaces is what is getting dllexport attributes added.

According to git-blame, CINDEX_LINKAGE has been in use for over 16 years. https://github.com/llvm/llvm-project/blame/a124a46748357b9e654adce7a50318a5f0648e48/clang/include/clang-c/Index.h

Still it may be good to figure out why this hasn't been an issue so far, for whoever otherwise were using these dllexport annotations.

I suspect it relates to module definition file (*.def). On MinGW, the symbol is properly exported without annotation.

$ llvm-readobj --coff-exports /ucrt64/bin/libclang.dll | grep clang_install
  Name: clang_install_aborting_llvm_fatal_error_handler

I'll try dig it.

[kikairoya]

Still it may be good to figure out why this hasn't been an issue so far, for whoever otherwise were using these dllexport annotations.

I suspect it relates to module definition file (*.def). On MinGW, the symbol is properly exported without annotation.

Regular win32 targets use libclang.def to choose symbols to be exported (generated from libclang-generic.exports through LLVM_EXPORT_SYMBOL_FILE) but it's disabled on most of Unix platforms.

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

Lines 95 to 98 in 5335925

	if (UNIX AND NOT APPLE AND NOT ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
	set(LLVM_EXPORTED_SYMBOL_FILE)
	set(USE_VERSION_SCRIPT ${LLVM_HAVE_LINK_VERSION_SCRIPT})
	endif()

It seems AND NOT CYGWIN should be added here.

Even if exported symbols are controlled by libclang.def, annotating __declspec(dllimport) correctly is still preferred.

[kikairoya]

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

Lines 88 to 93 in 5335925

	if(MSVC)
	# Avoid LNK4197 by not specifying libclang.exports here.
	# Each functions is exported as "dllexport" in include/clang-c.
	# KB835326
	set(LLVM_EXPORTED_SYMBOL_FILE)
	endif()

MSVC doesn't use libclang.def and doesn't export clang_install_aborting_llvm_fatal_error_handler.

$ llvm-readobj --coff-exports '/c/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/Llvm/bin/libclang.dll' | grep clang_install || echo not found
not found

[mstorsjo]

Still it may be good to figure out why this hasn't been an issue so far, for whoever otherwise were using these dllexport annotations.

I suspect it relates to module definition file (*.def). On MinGW, the symbol is properly exported without annotation.

Regular win32 targets use libclang.def to choose symbols to be exported (generated from libclang-generic.exports through LLVM_EXPORT_SYMBOL_FILE) but it's disabled on most of Unix platforms.

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

Lines 95 to 98 in 5335925

	if (UNIX AND NOT APPLE AND NOT ${CMAKE_SYSTEM_NAME} MATCHES "AIX")
	set(LLVM_EXPORTED_SYMBOL_FILE)
	set(USE_VERSION_SCRIPT ${LLVM_HAVE_LINK_VERSION_SCRIPT})
	endif()

It seems AND NOT CYGWIN should be added here.

That sounds like a good addition too, even if we're doing this as well?

Even if exported symbols are controlled by libclang.def, annotating __declspec(dllimport) correctly is still preferred.

Yeah, making that complete sounds good to me.

[mstorsjo]

LGTM, but can you amend the PR description with the extra info you gathered, about why this hasn't been an issue in other existing configurations?

[kikairoya]

LGTM, but can you amend the PR description with the extra info you gathered, about why this hasn't been an issue in other existing configurations?

Thanks.
I've updated the description.

It seems AND NOT CYGWIN should be added here.

That sounds like a good addition too, even if we're doing this as well?

Yes, it's not mandatory but seems to be recommended. I will submit a new PR later.

[kikairoya]

Yes, it's not mandatory but seems to be recommended. I will submit a new PR later.

#147278