[Clang] Add explicit visibility symbol macros #108276
Conversation
clang/tools/libclang/CMakeLists.txt
Outdated
| @@ -166,7 +166,7 @@ if(ENABLE_SHARED) | |||
| set_target_properties(libclang | |||
| PROPERTIES | |||
| VERSION ${LIBCLANG_LIBRARY_VERSION} | |||
| DEFINE_SYMBOL _CINDEX_LIB_) | |||
| DEFINE_SYMBOL _CINDEX_LIB_ DEFINE_SYMBOL CLANG_EXPORTS) | |||
There was a problem hiding this comment.
This will override the _CINDEX_LIB_ symbol, so this seems incorrect.
There was a problem hiding this comment.
I've fixed this and changed it to CLANG_BUILD_STATIC as well as adding a comment why its needed
We could if we didn't need the visibility macros in different states of dllexport and dllimport in the same translation unit for like a LLVM symbol to be import and a Clang symbol to be exported. |
The symbol lookup in PE/COFF is two level and symmetric, bound to the module. As a result, each module needs to explicitly specify the ABI interface (contract). However, given that the interface is annotated differently ( I do agree that we could put this in the commit message, though, this is likely something that deserves a note in the developer documentation because I fear that other developers are going to have this question repeatedly once this work is completed. The biggest point of contention with this work was the fact that it involves an ongoing maintenance cost as the ABI boundary is now being more rigidly defined which is/was not needed to support it on Unix due to the (global) single level binding. |
@fsfod, can you capture this comment in the commit message and the documentation as suggested? |
| // Marker to add to classes or functions in public headers that should not have | ||
| // export macros added to them by the clang tool | ||
| #define CLANG_ABI_NOT_EXPORTED | ||
| #if defined(LLVM_BUILD_LLVM_DYLIB) || defined(LLVM_BUILD_SHARED_LIBS) |
There was a problem hiding this comment.
I don't know about this - we could be building LLVM dynamically and clang statically. I don't think that we should use the same macros for clang.
There was a problem hiding this comment.
I removed that condition and switched this to being controlled from CMake using CLANG_LINK_CLANG_DYLIB
| #define CLANG_TEMPLATE_ABI | ||
| #define CLANG_EXPORT_TEMPLATE | ||
| #endif | ||
| #endif |
There was a problem hiding this comment.
The lack of indentation makes this very difficult to read. There are too many branches between the static and dynamic builds, and the various differences in the annotations.
There was a problem hiding this comment.
I think I said this in the previous PR that clang-format would strip out any indent for this.
fsfod
force-pushed
the
exported-api/clang-vis-macros
branch
from
7df42f8
to
6ea0e40
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Thomas Fransham (fsfod) ChangesThis is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. These macros are similar to the ones i added in #96630, but are for clang. Full diff: https://github.com/llvm/llvm-project/pull/108276.diff 3 Files Affected:
diff --git a/clang/cmake/modules/AddClang.cmake b/clang/cmake/modules/AddClang.cmake
index 5327b5d2f08928..091aec98e93ca3 100644
--- a/clang/cmake/modules/AddClang.cmake
+++ b/clang/cmake/modules/AddClang.cmake
@@ -108,6 +108,17 @@ macro(add_clang_library name)
endif()
llvm_add_library(${name} ${LIBTYPE} ${ARG_UNPARSED_ARGUMENTS} ${srcs})
+ if(MSVC AND NOT CLANG_LINK_CLANG_DYLIB)
+ # Make sure all consumers also turn off visibility macros so there not trying to dllimport symbols.
+ target_compile_definitions(${name} PUBLIC CLANG_BUILD_STATIC)
+ if(TARGET "obj.${name}")
+ target_compile_definitions("obj.${name}" PUBLIC CLANG_BUILD_STATIC)
+ endif()
+ elseif(NOT ARG_SHARED AND NOT ARG_STATIC)
+ # Clang component libraries linked in to clang-cpp are declared without SHARED or STATIC
+ target_compile_definitions("obj.${name}" PUBLIC CLANG_EXPORTS)
+ endif()
+
set(libs ${name})
if(ARG_SHARED AND ARG_STATIC)
list(APPEND libs ${name}_static)
diff --git a/clang/include/clang/Support/Compiler.h b/clang/include/clang/Support/Compiler.h
new file mode 100644
index 00000000000000..a2a8ca1ac962b3
--- /dev/null
+++ b/clang/include/clang/Support/Compiler.h
@@ -0,0 +1,67 @@
+//===-- clang/Support/Compiler.h - Compiler abstraction support -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines explicit visibility macros used to export symbols from
+// clang-cpp
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef CLANG_SUPPORT_COMPILER_H
+#define CLANG_SUPPORT_COMPILER_H
+
+#include "llvm/Support/Compiler.h"
+
+/// CLANG_ABI is the main export/visibility macro to mark something as
+/// explicitly exported when clang is built as a shared library with everything
+/// else that is unannotated will have internal visibility.
+///
+/// CLANG_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.
+///
+/// CLANG_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.
+#ifndef CLANG_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 CLANG_ABI_NOT_EXPORTED
+// 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(CLANG_BUILD_STATIC)
+#define CLANG_ABI
+#define CLANG_TEMPLATE_ABI
+#define CLANG_EXPORT_TEMPLATE
+#elif defined(_WIN32) && !defined(__MINGW32__)
+#if defined(CLANG_EXPORTS)
+#define CLANG_ABI __declspec(dllexport)
+#define CLANG_TEMPLATE_ABI
+#define CLANG_EXPORT_TEMPLATE __declspec(dllexport)
+#else
+#define CLANG_ABI __declspec(dllimport)
+#define CLANG_TEMPLATE_ABI __declspec(dllimport)
+#define CLANG_EXPORT_TEMPLATE
+#endif
+#elif defined(__ELF__) || defined(__MINGW32__) || defined(_AIX)
+#define CLANG_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
+#define CLANG_TEMPLATE_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
+#define CLANG_EXPORT_TEMPLATE
+#elif defined(__MACH__) || defined(__WASM__)
+#define CLANG_ABI LLVM_ATTRIBUTE_VISIBILITY_DEFAULT
+#define CLANG_TEMPLATE_ABI
+#define CLANG_EXPORT_TEMPLATE
+#endif
+#else
+#define CLANG_ABI
+#define CLANG_TEMPLATE_ABI
+#define CLANG_EXPORT_TEMPLATE
+#endif
+
+#endif
diff --git a/clang/tools/libclang/CMakeLists.txt b/clang/tools/libclang/CMakeLists.txt
index 968b46acb784cb..00a1223c0831a7 100644
--- a/clang/tools/libclang/CMakeLists.txt
+++ b/clang/tools/libclang/CMakeLists.txt
@@ -167,6 +167,10 @@ if(ENABLE_SHARED)
PROPERTIES
VERSION ${LIBCLANG_LIBRARY_VERSION}
DEFINE_SYMBOL _CINDEX_LIB_)
+ # Avoid declaring clang c++ symbols that are statically linked into libclang as dllimport'ed.
+ # 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)
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}")
|
|
@compnerd, does this PR require more care at that stage or it is good to go? |
There was a problem hiding this comment.
Looks good to me, too. We can wait a couple of more days for @AaronBallman to have the final say.
vgvassilev
force-pushed
the
exported-api/clang-vis-macros
branch
3 times, most recently
from
022e25d
to
8fc4325
Compare
| /// library consumers, for other platforms its a default visibility attribute. | ||
| #ifndef CLANG_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 |
There was a problem hiding this comment.
| // export macros added to them by the clang tool | |
| // export macros added to them by the clang tool. |
There was a problem hiding this comment.
The downside to this is that it's very difficult to know whether or not an API should or shouldn't be exported by the tool. For example, in Sema, would we want to export the ActOn* and Build* APIs? Probably! But what about the Check* APIs? Maybe for most of them. But things like FillInlineAsmIdentifierInfo? Probably not.
I seem to recall there being a functional limit to how many interfaces can be exposed in a DLL (I could be remembering wrong though, it's been a while!), so is there harm to over-exposing APIs?
There was a problem hiding this comment.
To make plugins flexible we probably should export as much as possible. And yes, some of our plugin infrastructure uses even private things from Sema ;)
we could submit patches for them and for some we already have but they cannot be backported across released versions…
There was a problem hiding this comment.
I think its only really the LLVM DLL built with MSVC or clang-cl without no export inlines that is sitting close to the symbol export limit.
There was a problem hiding this comment.
There was a problem hiding this comment.
I managed to get clang-cpp library building without /Zc:dllexportInlines- its at 46748 exported symbols.
There was a problem hiding this comment.
Would you mind trying an experiment for me? Would you add this to ASTMatchers.h and see how the numbers change for exported symbols:
extern const internal::VariadicDynCastAllOfMatcher<Decl, MSPropertyDecl>
msPropertyDecl;
AST_MATCHER_P(MSPropertyDecl, hasGetter, std::string, Name) {
return Node.hasGetter() && Node.getGetterId()->isStr(Name);
}
I'm asking because AST matchers often have a symbol explosion problem which is easy to overlook. If this adds two exported symbols, I don't worry to much. If it adds 20, it may be more worrying that we're already near 47k out of 65k (not that I think we're going to add that many matchers, just that I worry we'll add more symbols more quickly than we realize).
There was a problem hiding this comment.
i did have to export some of the AST matcher variables in another PR #110206.
There was a problem hiding this comment.
Just those lines didn't add any more symbols, I assume you meant add definition of the variable to a source file as well. Doing that only added one extra symbol. I should add there were 200 existing symbols with VariadicDynCastAllOfMatcher in there name that were exported.
astmatcher symbols.txt
There was a problem hiding this comment.
Thanks! I am surprised those lines didn't add any more symbols -- they're defined inline in the header files. But since that didn't add a pile of new symbols, I think we're probably okay with the amount of headroom we currently have.
|
|
||
| /// CLANG_ABI is the main export/visibility macro to mark something as | ||
| /// explicitly exported when clang is built as a shared library with everything | ||
| /// else that is unannotated having internal visibility. |
There was a problem hiding this comment.
You probably mean hidden visibility? Internal visibility is something slightly different and probably not what you want.
|
Unless there is more discussion or comments worth addressing I'd propose to merge this PR in the next day or two as it is a bottleneck for a few others in the pipeline. |
…trol them We need a different set of visibility macros to LLVM's since on windows you need a different attribute to import and export a symbol unlike other platforms and many translation units will be importing LLVM symbols and export some of Clangs. Updated Clang CMake to define a macro to enable the symbol visibility macros.
…libclang on windows
…o use of the macros yet
Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
These shouldn't be set when the clang tool is generating visibly macros
vgvassilev
force-pushed
the
exported-api/clang-vis-macros
branch
from
d2bfa43
to
cce1848
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/7029 Here is the relevant piece of the build log for the reference |
…ws (#109024) This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. Since windows doesn't implicitly import and merge exported symbols across shared libraries like other platforms we need to explicitly add a extern template declaration for each instantiation of llvm::Registry to force the registry symbols to be dllimport'ed. I've added a new visibility macro that doesn't switch between dllimport and dllexport on windows since the existing macro would be in the wrong mode for llvm::Registry's declared in Clang. This PR also depends Clang symbol visibility macros that will be added by #108276 --------- Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
…ws (llvm#109024) This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. Since windows doesn't implicitly import and merge exported symbols across shared libraries like other platforms we need to explicitly add a extern template declaration for each instantiation of llvm::Registry to force the registry symbols to be dllimport'ed. I've added a new visibility macro that doesn't switch between dllimport and dllexport on windows since the existing macro would be in the wrong mode for llvm::Registry's declared in Clang. This PR also depends Clang symbol visibility macros that will be added by llvm#108276 --------- Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. These macros are similar to the ones i added in llvm#96630, but are for clang. Added explicit symbol visibility macros definitions that are defined in a new header and will be used to for shared library builds of clang to export symbols. Updated clang cmake to define a macro to enable the symbol visibility macros and explicitly disable them for clang tools that always use static linking. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. These macros are similar to the ones i added in llvm#96630, but are for clang. Added explicit symbol visibility macros definitions that are defined in a new header and will be used to for shared library builds of clang to export symbols. Updated clang cmake to define a macro to enable the symbol visibility macros and explicitly disable them for clang tools that always use static linking. --------- Co-authored-by: Aaron Ballman <aaron@aaronballman.com>
…ws (llvm#109024) This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. Since windows doesn't implicitly import and merge exported symbols across shared libraries like other platforms we need to explicitly add a extern template declaration for each instantiation of llvm::Registry to force the registry symbols to be dllimport'ed. I've added a new visibility macro that doesn't switch between dllimport and dllexport on windows since the existing macro would be in the wrong mode for llvm::Registry's declared in Clang. This PR also depends Clang symbol visibility macros that will be added by llvm#108276 --------- Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org>
This is part of the effort to support for enabling plugins on windows by adding better support for building llvm and clang as a DLL. These macros are similar to the ones i added in #96630, but are for clang.
Added explicit symbol visibility macros definitions that are defined in a new header and will be used to for shared library builds of clang to export
symbols.
Updated clang cmake to define a macro to enable the symbol visibility macros and explicitly disable them for clang tools that always use static linking.