-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Fuchsia] Build with -fvisibility=default #67067
Conversation
3d54dc4
to
3aa0af9
Compare
@llvm/pr-subscribers-clang ChangesThere was an issue with relative vtables when two TU's which define the same vtable object are built with different default visibilities. Some TU's are built with -fvisibility=hidden in the code base, grep for CMAKE_CXX_VISIBILITY_PRESET to find them. Our whole toolchain, is statically linked, and built with -fPIE anyway, so the cost of overriding local CMAKE_CXX_VISIBILITY_PRESET properties is not high. I've counted that adding this flag increases our llvm binary by 13 relocations. Frankly I'm not sure where those are even coming from. It would be preferable to use hidden visibility, but that breaks liblld. This can be solved by setting LLDB_EXPORT_ALL_SYMBOLS. After that some ORC tests fail which do symbolic lookup in the tests. It seems that setting CMAKE_CXX_VISIBILITY_PRESET=hidden will not be worth the maintenance burden. Setting it to default works to unblock using relative vtables, so we can just go with that. Full diff: https://github.com/llvm/llvm-project/pull/67067.diff 1 Files Affected:
diff --git a/clang/cmake/caches/Fuchsia-stage2.cmake b/clang/cmake/caches/Fuchsia-stage2.cmake
index bc4d9f1462b1814..28931fef012775a 100644
--- a/clang/cmake/caches/Fuchsia-stage2.cmake
+++ b/clang/cmake/caches/Fuchsia-stage2.cmake
@@ -52,6 +52,7 @@ set(ENABLE_LINKER_BUILD_ID ON CACHE BOOL "")
set(ENABLE_X86_RELAX_RELOCATIONS ON CACHE BOOL "")
set(CMAKE_BUILD_TYPE Release CACHE STRING "")
+set(CMAKE_CXX_VISIBILITY_PRESET default CACHE STRING "")
if (APPLE)
set(CMAKE_OSX_DEPLOYMENT_TARGET "10.13" CACHE STRING "")
elseif(WIN32)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was an issue with relative vtables when two TU's which define the same vtable object are built with different default visibilities. Some TU's are built with -fvisibility=hidden in the code base, grep for CMAKE_CXX_VISIBILITY_PRESET to find them. Our whole toolchain, is statically linked, and built with -fPIE anyway, so the cost of overriding local CMAKE_CXX_VISIBILITY_PRESET properties is not high. I've counted that adding this flag increases our llvm binary by 13 relocations. Frankly I'm not sure where those are even coming from. It would be preferable to use hidden visibility, but that breaks liblld. This can be solved by setting LLDB_EXPORT_ALL_SYMBOLS. After that some ORC tests fail which do symbolic lookup in the tests. It seems that setting CMAKE_CXX_VISIBILITY_PRESET=hidden will not be worth the maintenance burden. Setting it to default works to unblock using relative vtables, so we can just go with that.
3aa0af9
to
62f1432
Compare
I think the code formatting failure is because there are no source files changed in this commit. I feel fine pushing this. |
There was an issue with relative vtables when two TU's which define the same vtable object are built with different default visibilities. Some TU's are built with -fvisibility=hidden in the code base, grep for CMAKE_CXX_VISIBILITY_PRESET to find them. Our whole toolchain, is statically linked, and built with -fPIE anyway, so the cost of overriding local CMAKE_CXX_VISIBILITY_PRESET properties is not high. I've counted that adding this flag increases our llvm binary by 13 relocations. Frankly I'm not sure where those are even coming from.
It would be preferable to use hidden visibility, but that breaks liblld. This can be solved by setting LLDB_EXPORT_ALL_SYMBOLS. After that some ORC tests fail which do symbolic lookup in the tests. It seems that setting CMAKE_CXX_VISIBILITY_PRESET=hidden will not be worth the maintenance burden. Setting it to default works to unblock using relative vtables, so we can just go with that.