[compiler-rt][cmake] Add option to control shared library builds #139042
Conversation
llvmbot
added
compiler-rt
compiler-rt:asan
|
@llvm/pr-subscribers-compiler-rt-sanitizer Author: Yuta Saito (kateinoigakukun) Changes
Full diff: https://github.com/llvm/llvm-project/pull/139042.diff 1 Files Affected:
diff --git a/compiler-rt/lib/asan/CMakeLists.txt b/compiler-rt/lib/asan/CMakeLists.txt
index e2f39f224df9c..5616a40164bd1 100644
--- a/compiler-rt/lib/asan/CMakeLists.txt
+++ b/compiler-rt/lib/asan/CMakeLists.txt
@@ -1,5 +1,12 @@
# Build for the AddressSanitizer runtime support library.
+set(COMPILER_RT_ASAN_BUILD_SHARED_LIBS_default ON)
+if (CMAKE_SYSTEM_NAME STREQUAL "WASI")
+ set(COMPILER_RT_ASAN_BUILD_SHARED_LIBS_default OFF)
+endif()
+option(COMPILER_RT_ASAN_BUILD_SHARED_LIBS
+ "Build AddressSanitizer shared libraries" ${COMPILER_RT_ASAN_BUILD_SHARED_LIBS_default})
+
set(ASAN_SOURCES
asan_allocator.cpp
asan_activation.cpp
@@ -307,25 +314,27 @@ else()
endif()
set(ASAN_DYNAMIC_WEAK_INTERCEPTION)
- add_compiler_rt_runtime(clang_rt.asan
- SHARED
- ARCHS ${arch}
- OBJECT_LIBS ${ASAN_COMMON_RUNTIME_OBJECT_LIBS}
- RTAsan_dynamic
- # The only purpose of RTAsan_dynamic_version_script_dummy is to
- # carry a dependency of the shared runtime on the version script.
- # Replacing it with a straightforward
- # add_dependencies(clang_rt.asan-dynamic-${arch} clang_rt.asan-dynamic-${arch}-version-list)
- # generates an order-only dependency in ninja.
- RTAsan_dynamic_version_script_dummy
- RTUbsan_cxx
- ${ASAN_DYNAMIC_WEAK_INTERCEPTION}
- CFLAGS ${ASAN_DYNAMIC_CFLAGS}
- LINK_FLAGS ${ASAN_DYNAMIC_LINK_FLAGS}
- ${VERSION_SCRIPT_FLAG}
- LINK_LIBS ${ASAN_DYNAMIC_LIBS}
- DEFS ${ASAN_DYNAMIC_DEFINITIONS}
- PARENT_TARGET asan)
+ if (COMPILER_RT_ASAN_BUILD_SHARED_LIBS)
+ add_compiler_rt_runtime(clang_rt.asan
+ SHARED
+ ARCHS ${arch}
+ OBJECT_LIBS ${ASAN_COMMON_RUNTIME_OBJECT_LIBS}
+ RTAsan_dynamic
+ # The only purpose of RTAsan_dynamic_version_script_dummy is to
+ # carry a dependency of the shared runtime on the version script.
+ # Replacing it with a straightforward
+ # add_dependencies(clang_rt.asan-dynamic-${arch} clang_rt.asan-dynamic-${arch}-version-list)
+ # generates an order-only dependency in ninja.
+ RTAsan_dynamic_version_script_dummy
+ RTUbsan_cxx
+ ${ASAN_DYNAMIC_WEAK_INTERCEPTION}
+ CFLAGS ${ASAN_DYNAMIC_CFLAGS}
+ LINK_FLAGS ${ASAN_DYNAMIC_LINK_FLAGS}
+ ${VERSION_SCRIPT_FLAG}
+ LINK_LIBS ${ASAN_DYNAMIC_LIBS}
+ DEFS ${ASAN_DYNAMIC_DEFINITIONS}
+ PARENT_TARGET asan)
+ endif()
if (SANITIZER_USE_SYMBOLS AND NOT ${arch} STREQUAL "i386")
add_sanitizer_rt_symbols(clang_rt.asan_cxx
|
compiler-rt/lib/asan/CMakeLists.txt
Outdated
| @@ -1,5 +1,12 @@ | |||
| # Build for the AddressSanitizer runtime support library. | |||
|
|
|||
| set(COMPILER_RT_ASAN_BUILD_SHARED_LIBS_default ON) | |||
| if (CMAKE_SYSTEM_NAME STREQUAL "WASI") | |||
There was a problem hiding this comment.
@sbc100 I assume we've never tried the ASan CMake build for emscripten since we have the standalone build scripts?
Anyway this might be a case where we want the same logic for WASI and Emscripten.
compiler-rt/lib/asan/CMakeLists.txt
Outdated
| @@ -1,5 +1,12 @@ | |||
| # Build for the AddressSanitizer runtime support library. | |||
|
|
|||
| set(COMPILER_RT_ASAN_BUILD_SHARED_LIBS_default ON) | |||
There was a problem hiding this comment.
Would it make sense to have some higher level COMPILER_RT_BUILD_SHARED setting to control all of compiler-rt?
Also is not possible to globally disable shared libraries in the whole cmake build rather then adding these library-specific settings for all libs?
Also, there has been some work on making shared libraries work with wasi so why not just enable the building of both shared and unshared? Folks linking static binaries can then use the static one and folks using dynamic linking can use the dynamic one?
There was a problem hiding this comment.
Having a higher-level control sounds good to me. Since the compiler-rt libraries are built outside of the standard add_library CMake convention, it's not possible to disable all shared library build by just setting BUILD_SHARED_LIBS unfortunately.
Also the latest only reason I disable this is just because it looks like building a shared library is currently broken due to #103592. If it works, I think we can ship both shared and static ones. Do you have a plan to merge #128223?
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.wasm32.dir/sanitizer_wasi.cpp.obj: undefined symbol: __heap_base
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.wasm32.dir/sanitizer_wasi.cpp.obj: undefined symbol: __heap_end
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.wasm32.dir/sanitizer_wasi.cpp.obj: undefined symbol: __heap_base
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.wasm32.dir/sanitizer_wasi.cpp.obj: undefined symbol: __heap_end
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.wasm32.dir/sanitizer_wasi.cpp.obj: undefined symbol: __heap_base
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommon.wasm32.dir/sanitizer_wasi.cpp.obj: undefined symbol: __heap_end
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommonLibc.wasm32.dir/sanitizer_common_libcdep.cpp.obj: undefined symbol: __sanitizer::PlatformPrepareForSandboxing(void*)
wasm-ld: error: lib/sanitizer_common/CMakeFiles/RTSanitizerCommonSymbolizer.wasm32.dir/sanitizer_stackdepot.cpp.obj: undefined symbol: __sanitizer::internal_join_thread(void*)
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_allocator.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_descriptions.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_descriptions.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_errors.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_errors.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_errors.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_errors.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_fake_stack.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_fake_stack.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_fake_stack.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_fake_stack.cpp.obj: undefined symbol: __global_base
wasm-ld: error: lib/asan/CMakeFiles/RTAsan_dynamic.wasm32.dir/asan_fake_stack.cpp.obj: undefined symbol: __global_base
wasm-ld: error: too many errors emitted, stopping now (use -error-limit=0 to see all errors)
clang++: error: linker command failed with exit code 1 (use -v to see invocation)
There was a problem hiding this comment.
How about disabling this building the SHARED version based on BUILD_SHARED_LIBS being set or not?
There was a problem hiding this comment.
I need to check how it will work with the ./runtimes unified build but sounds good to me.
I don't think this is a good idea. BUILD_SHARED_LIBS is about how the compiler is built, and that should be separate from how the runtime libraries are built. Also BUILD_SHARED_LIBS is a (LLVM) developer feature and it's well known that it shouldn't be used for release builds, but that's not necessarily true of the library build. |
|
I agree with @dschuff's point about How about making |
Actually you are completely right. compiler-rt is the output of the compiler, not the compiler itself. So in this case its perfectly reasonable for a compiler running on wasi use use shared libraries versions of compiler-rt. Since clang it a cross compiler there could be any number of compiler-rt shared libraries built, even if the version of wasi you are using doesn't support shared libraries the compiler target (e.g. linux) could/does. |
kateinoigakukun
force-pushed
the
pr-909f5c3d0bc3ca6b821d0ee57d3af21ebc50ee60
branch
from
319d596 to
3e1d16d
Compare
kateinoigakukun
changed the title
compiler-rt/CMakeLists.txt
Outdated
| set(COMPILER_RT_BUILD_SHARED_LIBS_DEFAULT ON) | ||
| if(CMAKE_SYSTEM_NAME STREQUAL "WASI") | ||
| set(COMPILER_RT_BUILD_SHARED_LIBS_DEFAULT OFF) | ||
| endif() |
There was a problem hiding this comment.
I think this is wrong because, as I mentioned in another comment, the version of clang that run on wasi is perfectly capable of building shared libraries even if wasi can't consume them (e.g. the cross compilation case).
There was a problem hiding this comment.
I guess if you want to avoid building shared library version of compiler-rt for wasi you would need to check the specific target that compiler-rt is being built for (not CMAKE_SYSTEM_NAME which is where the build is running I think).
However, maybe just better to wait for a fix for the downstream issues with building wasm shared libraries.
There was a problem hiding this comment.
The CMAKE_SYSTEM_NAME variable here points to the target platform where the runtime library runs on, not the platform where compiler runs. The LLVM CMake infrastructure uses ExternalProject_Add in the unified build of compiler + runtime libs to include compiler-rt build graph, and it passes through most of CMake args except for CMAKE_SYSTEM_NAME and a few other exceptions. CMAKE_SYSTEM_NAME is overridden to the target platform by llvm_ExternalProject_Add. So CMake scripts in compiler-rt can assume CMAKE_SYSTEM_NAME is the target platform, not host.
There was a problem hiding this comment.
Ah OK, that makes sense.
Now I'm wondering if we should be somehow setting BUILD_SHARED_LIBS at the point where we set CMAKE_SYSTEM_NAME to WASI? @dschuff I guess BUILD_SHARED_LIBS can have different values for the compiler built itself and for each to the target system which compiler-rt is being built for? So maybe it is OK to use BUILD_SHARED_LIBS?
There was a problem hiding this comment.
Also, I think my other point that default to not building shared libraries under WASI is maybe not the best approach for now since we are close the enabling them in wasi-sdk (i.e. we might soon revert the default).
There was a problem hiding this comment.
It looks like there is an existing mechanism to pass extra CMake options to runtime libs. If we pass RUNTIMES_${TOOLCHAIN_TARGET_TRIPLE}_BUILD_SHARED_LIBS=OFF to the root LLVM unified CMake configuration, it will pass BUILD_SHARED_LIBS=OFF to the compiler-rt, and the BUILD_SHARED_LIBS value configured for the host compiler is not passed through to runtime libs by default. So respecting BUILD_SHARED_LIBS in compiler-rt now sounds reasonable to me.
There was a problem hiding this comment.
Hmm, according to the CI failure, it seems like compiler-rt for the host platform is configured at the same CMake unit of the host compiler, so BUILD_SHARED_LIBS is shared for compiler and runtime libs for the host platform.
I think it's better to have a separate CMake variable to make things simpler.
kateinoigakukun
force-pushed
the
pr-909f5c3d0bc3ca6b821d0ee57d3af21ebc50ee60
branch
2 times, most recently
from
9112b4c to
1915231
Compare
This patch introduces a new option `COMPILER_RT_BUILD_SHARED_LIBS` to control whether shared libraries should be built for runtime libraries in compiler-rt. By default, this option is set to ON, allowing shared libraries to be built. If set to OFF, shared libraries will not be built, which can be useful for projects that do not require them or when building for environments where shared libraries are still not yet matured, such as WebAssembly/WASI.
kateinoigakukun
force-pushed
the
pr-909f5c3d0bc3ca6b821d0ee57d3af21ebc50ee60
branch
from
1915231 to
a9f0e23
Compare
|
I feel like there must already be some kind of option to control whether shared or static libs are built. Maybe this is a question for a runtimes expert like @petrhosek |
This patch introduces a new option
COMPILER_RT_BUILD_SHARED_LIBSto control whether shared libraries should be built for runtime libraries in compiler-rt. By default, this option is set to ON, allowing shared libraries to be built. If set to OFF, shared libraries will not be built, which can be useful for projects that do not require them or when building for environments where shared libraries are still not yet matured, such as WebAssembly/WASI.