-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Initial changes for llvm shared library build using explicit visibility annotations #96630
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,7 +110,8 @@ | |
/// this attribute will be made public and visible outside of any shared library | ||
/// they are linked in to. | ||
|
||
#if LLVM_HAS_CPP_ATTRIBUTE(gnu::visibility) | ||
#if LLVM_HAS_CPP_ATTRIBUTE(gnu::visibility) && defined(__GNUC__) && \ | ||
!defined(__clang__) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clang should support this under the GNU standard. Can you add a note on why this condition is needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what the need for it was, it was part of @tstellar initial changes to the file that i based my code on. I can just remove it if there wasn't some clear purpose for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clang supports it, but IIRC the rules about where in the function declaration the c++11 style attributes can be placed is different between gcc in clang. See the comments here: https://github.com/tstellar/template-visibility/blob/testing/templates.h There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That file is my pain. I did add a option to the clang tool to switch between the two places where to generate the macro for for functions. I also did try generating export macros on template classes, but it just creates ton a more code that needs to be fixed for MSVC and can be problematic for class static fields when classes with instantiations outside the llvm shared library. |
||
#define LLVM_ATTRIBUTE_VISIBILITY_HIDDEN [[gnu::visibility("hidden")]] | ||
#define LLVM_ATTRIBUTE_VISIBILITY_DEFAULT [[gnu::visibility("default")]] | ||
#elif __has_attribute(visibility) | ||
|
@@ -121,18 +122,79 @@ | |
#define LLVM_ATTRIBUTE_VISIBILITY_DEFAULT | ||
#endif | ||
|
||
|
||
#if (!(defined(_WIN32) || defined(__CYGWIN__)) || \ | ||
(defined(__MINGW32__) && defined(__clang__))) | ||
#define LLVM_LIBRARY_VISIBILITY LLVM_ATTRIBUTE_VISIBILITY_HIDDEN | ||
#if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS) | ||
#define LLVM_EXTERNAL_VISIBILITY LLVM_ATTRIBUTE_VISIBILITY_DEFAULT | ||
#else | ||
#define LLVM_EXTERNAL_VISIBILITY | ||
#endif | ||
|
||
#if (!(defined(_WIN32) || defined(__CYGWIN__)) || \ | ||
(defined(__MINGW32__) && defined(__clang__))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know it was existing code that got rearranged slightly, it was controlling LLVM_EXTERNAL_VISIBILITY before |
||
#define LLVM_LIBRARY_VISIBILITY LLVM_ATTRIBUTE_VISIBILITY_HIDDEN | ||
#define LLVM_ALWAYS_EXPORT LLVM_ATTRIBUTE_VISIBILITY_DEFAULT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how I feel about this abstraction ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the alternative for non windows platforms other than this? I used LLVM_ALWAYS_EXPORT for some special JIT debugger registration functions that were annotated with LLVM_ATTRIBUTE_VISIBILITY_DEFAULT before. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Explicitly spelling out the attribute OSS what I was suggesting. On Linux, protected visibility has interesting behavior where symbols are not interpositionable and are externally visible. Using that is potentially useful for load times on Linux for exported symbols. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know about protected, wouldn't it also indirectly stop symbols being merging across shared libraries and executables by the loader? maybe that would be what you always want for the JIT debugger symbols. |
||
#elif defined(_WIN32) | ||
#define LLVM_ALWAYS_EXPORT __declspec(dllexport) | ||
#define LLVM_LIBRARY_VISIBILITY | ||
#else | ||
#define LLVM_LIBRARY_VISIBILITY | ||
#define LLVM_EXTERNAL_VISIBILITY | ||
#define LLVM_ALWAYS_EXPORT | ||
#endif | ||
|
||
/// LLVM_ABI is the main export/visibility macro to mark something as explicitly | ||
/// exported when llvm is built as a shared library with everything else that is | ||
/// unannotated will have internal visibility. | ||
/// | ||
/// LLVM_EXPORT_TEMPLATE is used on explicit template instantiations in source | ||
/// files that were declared extern in a header. This macro is only set as a | ||
/// compiler export attribute on windows, on other platforms it does nothing. | ||
/// | ||
/// LLVM_TEMPLATE_ABI is for annotating extern template declarations in headers | ||
/// for both functions and classes. On windows its turned in to dllimport for | ||
/// library consumers, for other platforms its a default visibility attribute. | ||
/// | ||
/// LLVM_C_ABI is used to annotated functions and data that need to be exported | ||
/// for the libllvm-c API. This used both for the llvm-c headers and for the | ||
/// functions declared in the different Target's c++ source files that don't | ||
/// include the header forward declaring them. | ||
#ifndef LLVM_ABI_GENERATING_ANNOTATIONS | ||
// Marker to add to classes or functions in public headers that should not have | ||
// export macros added to them by the clang tool | ||
#define LLVM_ABI_NOT_EXPORTED | ||
#if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS) || \ | ||
defined(LLVM_ENABLE_PLUGINS) | ||
Comment on lines
+163
to
+164
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should hoist this into CMake and map this to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just wanted avoid doing it in CMake yet because its more complex to do and easy to get wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay; I guess getting it in is a good first step. The CMake bits, I think you should wait to get sign off from @petrhosek as well. |
||
// Some libraries like those for tablegen are linked in to tools that used | ||
// in the build so can't depend on the llvm shared library. If export macros | ||
// were left enabled when building these we would get duplicate or | ||
// missing symbol linker errors on windows. | ||
#if defined(LLVM_BUILD_STATIC) | ||
#define LLVM_ABI | ||
#define LLVM_TEMPLATE_ABI | ||
#define LLVM_EXPORT_TEMPLATE | ||
#elif defined(_WIN32) && !defined(__MINGW32__) | ||
#if defined(LLVM_EXPORTS) | ||
#define LLVM_ABI __declspec(dllexport) | ||
#define LLVM_TEMPLATE_ABI | ||
#define LLVM_EXPORT_TEMPLATE __declspec(dllexport) | ||
#else | ||
#define LLVM_ABI __declspec(dllimport) | ||
#define LLVM_TEMPLATE_ABI __declspec(dllimport) | ||
#define LLVM_EXPORT_TEMPLATE | ||
#endif | ||
#elif defined(__ELF__) || defined(__MINGW32__) | ||
#define LLVM_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT | ||
#define LLVM_TEMPLATE_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT | ||
#define LLVM_EXPORT_TEMPLATE | ||
#elif defined(__MACH__) || defined(__WASM__) | ||
#define LLVM_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT | ||
#define LLVM_TEMPLATE_ABI | ||
#define LLVM_EXPORT_TEMPLATE | ||
#endif | ||
#else | ||
#define LLVM_ABI | ||
#define LLVM_TEMPLATE_ABI | ||
#define LLVM_EXPORT_TEMPLATE | ||
#endif | ||
#define LLVM_C_ABI LLVM_ABI | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We really should not conflate the LLVM ABI and the LLVM C ABI. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea was it could maybe made separate later on for some kind of optimized llvmc only build. There also exported functions in Target libs like LLVMInitialize* that are used both in C and c++ API. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's actually more towards what my point was :) We shouldn't be defining the C ABI in terms of the C++ ABI. The C ABI is guaranteed stable while the C++ is not. It may be someone wants to not expose the C++ ABI (builds statically) but still build the C interface as a DSO. |
||
#endif | ||
|
||
#if defined(__GNUC__) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ if(LLVM_LINK_LLVM_DYLIB AND LLVM_DYLIB_EXPORTED_SYMBOL_FILE) | |
endif() | ||
|
||
if(LLVM_BUILD_LLVM_DYLIB) | ||
if(MSVC) | ||
if(MSVC AND NOT LLVM_BUILD_LLVM_DYLIB_VIS) | ||
message(FATAL_ERROR "Generating libLLVM is not supported on MSVC") | ||
endif() | ||
if(ZOS) | ||
|
@@ -49,18 +49,25 @@ if(LLVM_BUILD_LLVM_DYLIB) | |
${CMAKE_CURRENT_SOURCE_DIR}/simple_version_script.map.in | ||
${LLVM_LIBRARY_DIR}/tools/llvm-shlib/simple_version_script.map) | ||
|
||
# GNU ld doesn't resolve symbols in the version script. | ||
set(LIB_NAMES -Wl,--whole-archive ${LIB_NAMES} -Wl,--no-whole-archive) | ||
if (NOT LLVM_LINKER_IS_SOLARISLD AND NOT MINGW) | ||
# Solaris ld does not accept global: *; so there is no way to version *all* global symbols | ||
set(LIB_NAMES -Wl,--version-script,${LLVM_LIBRARY_DIR}/tools/llvm-shlib/simple_version_script.map ${LIB_NAMES}) | ||
endif() | ||
if (NOT MINGW AND NOT LLVM_LINKER_IS_SOLARISLD_ILLUMOS) | ||
# Optimize function calls for default visibility definitions to avoid PLT and | ||
# reduce dynamic relocations. | ||
# Note: for -fno-pic default, the address of a function may be different from | ||
# inside and outside libLLVM.so. | ||
target_link_options(LLVM PRIVATE LINKER:-Bsymbolic-functions) | ||
if(MSVC) | ||
target_link_directories(LLVM PRIVATE ${LLVM_LIBRARY_DIR}) | ||
foreach(library ${LIB_NAMES}) | ||
target_link_options(LLVM PRIVATE /WHOLEARCHIVE:${library}.lib) | ||
endforeach() | ||
else() | ||
# GNU ld doesn't resolve symbols in the version script. | ||
set(LIB_NAMES -Wl,--whole-archive ${LIB_NAMES} -Wl,--no-whole-archive) | ||
if (NOT LLVM_LINKER_IS_SOLARISLD AND NOT MINGW) | ||
# Solaris ld does not accept global: *; so there is no way to version *all* global symbols | ||
set(LIB_NAMES -Wl,--version-script,${LLVM_LIBRARY_DIR}/tools/llvm-shlib/simple_version_script.map ${LIB_NAMES}) | ||
endif() | ||
if (NOT MINGW AND NOT LLVM_LINKER_IS_SOLARISLD_ILLUMOS) | ||
# Optimize function calls for default visibility definitions to avoid PLT and | ||
# reduce dynamic relocations. | ||
# Note: for -fno-pic default, the address of a function may be different from | ||
# inside and outside libLLVM.so. | ||
target_link_options(LLVM PRIVATE LINKER:-Bsymbolic-functions) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
endif() | ||
endif() | ||
endif() | ||
|
||
|
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.
Where is
LLVM_BUILD_LLVM_DYLIB_VIS
defined? What does the_VIS
suffix stand for?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.
Its a temporary option to allow testing the build with explicit visibility on Windows and Linux the idea is it will go away in the end, when I've gotten all my changes merged. It has to be separate from LLVM_BUILD_LLVM_DYLIB because there will be linker errors from symbols not exported when building llvm tools until the whole llvm codebase has export visibility macros added to it.
The idea was it was not a publicly defined option so people don't try use and submit bug reports against it. If you want I can publicly define it with a don't use for testing purposes only description.
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.
You could consider making it an internal variable:
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.
I didn't realize it had that mode, I've update code to define it like that.