Add DllMain entry point to libclang.dll
#171465
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (GkvJwa) ChangesFix #154361 Because the destruction processes for EXEs and DLLs differ on Windows, DLLs, as modules, require more additional processing in the CRT (Crt Request Tree). When RPMALLOC is enabled, it initializes from the CRT by calling Full diff: https://github.com/llvm/llvm-project/pull/171465.diff 2 Files Affected:
diff --git a/clang/tools/libclang/CMakeLists.txt b/clang/tools/libclang/CMakeLists.txt
index b0105f5a5f79f..a3b95fd49b68d 100644
--- a/clang/tools/libclang/CMakeLists.txt
+++ b/clang/tools/libclang/CMakeLists.txt
@@ -135,6 +135,7 @@ endif()
add_clang_library(libclang ${ENABLE_SHARED} ${ENABLE_STATIC} INSTALL_WITH_TOOLCHAIN
OUTPUT_NAME ${output_name}
${SOURCES}
+ PARTIAL_SOURCES_INTENDED
DEPENDS
ClangDriverOptions
@@ -168,6 +169,9 @@ if(ENABLE_SHARED)
# If llvm/libclang-cpp dll is also being built for windows clang c++ symbols will still be
# implicitly be exported from libclang.
target_compile_definitions(libclang PRIVATE CLANG_BUILD_STATIC)
+ if(LLVM_ENABLE_RPMALLOC)
+ target_sources(libclang PRIVATE DllMain.cpp)
+ endif()
elseif(APPLE)
set(LIBCLANG_LINK_FLAGS " -Wl,-compatibility_version -Wl,1")
set(LIBCLANG_LINK_FLAGS "${LIBCLANG_LINK_FLAGS} -Wl,-current_version -Wl,${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
diff --git a/clang/tools/libclang/DllMain.cpp b/clang/tools/libclang/DllMain.cpp
new file mode 100644
index 0000000000000..262525e65e055
--- /dev/null
+++ b/clang/tools/libclang/DllMain.cpp
@@ -0,0 +1,24 @@
+
+
+#include <windows.h>
+
+extern "C" {
+
+int rpmalloc_initialize(void);
+void rpmalloc_finalize(void);
+void rpmalloc_thread_initialize(void);
+void rpmalloc_thread_finalize(int release_caches);
+
+BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, LPVOID reserved) {
+ if (reason == DLL_PROCESS_ATTACH) {
+ rpmalloc_initialize();
+ } else if (reason == DLL_PROCESS_DETACH) {
+ rpmalloc_finalize();
+ } else if (reason == DLL_THREAD_ATTACH) {
+ rpmalloc_thread_initialize();
+ } else if (reason == DLL_THREAD_DETACH) {
+ rpmalloc_thread_finalize(1);
+ }
+ return TRUE;
+}
+}
|
|
hello, @slydiman @aganea @zmodem @naveen-seth @AaronBallman @mrexodia |
GkvJwa
requested review from
AaronBallman,
aganea,
naveen-seth,
slydiman and
zmodem
| add_clang_library(libclang ${ENABLE_SHARED} ${ENABLE_STATIC} INSTALL_WITH_TOOLCHAIN | ||
| OUTPUT_NAME ${output_name} | ||
| ${SOURCES} | ||
| PARTIAL_SOURCES_INTENDED |
There was a problem hiding this comment.
As noted below, I think we should always include DllMain.cpp (on Windows), and then I think we don't need this?
| # If llvm/libclang-cpp dll is also being built for windows clang c++ symbols will still be | ||
| # implicitly be exported from libclang. | ||
| target_compile_definitions(libclang PRIVATE CLANG_BUILD_STATIC) | ||
| if(LLVM_ENABLE_RPMALLOC) |
There was a problem hiding this comment.
I think it would be nicer to always include this file (on Windows), and then inside the file we can do different things depending on the allocator?
There was a problem hiding this comment.
Currently, the allocator lacks global macro definitions; adding them might introduce more pollution.
There was a problem hiding this comment.
Maybe adding a macro would be worth it.
Or, if we want an rpmalloc-specific DllMain.cpp, I think it should live closer to rpmalloc in llvm/lib/Support/ and get added from llvm/lib/Support/CMakeLists.txt
clang/tools/libclang/DllMain.cpp
Outdated
| @@ -0,0 +1,24 @@ | |||
|
|
|||
There was a problem hiding this comment.
This needs the LLVM license etc. header.
clang/tools/libclang/DllMain.cpp
Outdated
| if (reason == DLL_PROCESS_ATTACH) { | ||
| rpmalloc_initialize(); | ||
| } else if (reason == DLL_PROCESS_DETACH) { |
There was a problem hiding this comment.
Since we build rpmalloc with ENABLE_PRELOAD, I think we already call _global_rpmalloc_init() at load time, so maybe we shouldn't be doing the initialization calls here?
| extern "C" { | ||
|
|
||
| int rpmalloc_initialize(void); | ||
| void rpmalloc_finalize(void); | ||
| void rpmalloc_thread_initialize(void); | ||
| void rpmalloc_thread_finalize(int release_caches); |
There was a problem hiding this comment.
Can we use a header instead of redeclaring these?
There was a problem hiding this comment.
The reason is the same as the reason for checking LLVM_ENABLE_RPMALLOC in CMake: the overloaded malloc lacks a global macro definition.
clang/tools/libclang/DllMain.cpp
Outdated
| void rpmalloc_thread_finalize(int release_caches); | ||
|
|
||
| BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, LPVOID reserved) { | ||
| if (reason == DLL_PROCESS_ATTACH) { |
There was a problem hiding this comment.
I think that we should use the normal pattern of switch (reason) {.
| # implicitly be exported from libclang. | ||
| target_compile_definitions(libclang PRIVATE CLANG_BUILD_STATIC) | ||
| if(LLVM_ENABLE_RPMALLOC) | ||
| target_sources(libclang PRIVATE DllMain.cpp) |
There was a problem hiding this comment.
Since all the code added here in DllMain.cpp already exists in tree here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Support/rpmalloc/malloc.c#L556 - would it be desirable to add that to {SOURCES} and setting it COMPILE_DEFINITIONS "ENABLE_PRELOAD;BUILD_DYNAMIC_LINK=1"? If we do that, we should also remove extern __declspec(dllexport) on the DllMain definition in rpmalloc, DllMain symbols shouldn't be exported.
5 participants
Fix #154361
Because the destruction processes for EXEs and DLLs differ on Windows, DLLs, as modules, require more additional processing in the CRT. When
RPMALLOCis enabled,RPMALLOC datainitializes from the CRT by callingrpmalloc_initialize, but because there's no explicitdllmain, it can't destroy theRPMALLOC data. This causes the problem, and this patch addsdllmainto fix it.