-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[Clang] Add explicit visibility symbol macros #108276
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
Changes from all commits
d598242
952bb31
7f849a0
66aaa9f
7e8cea6
c0152f0
cccb287
962d215
cce1848
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 |
---|---|---|
@@ -0,0 +1,63 @@ | ||
//===-- 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 having hidden 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 | ||
#endif | ||
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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I said this in the previous PR that clang-format would strip out any indent for this. |
||
|
||
#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.
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.
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 theActOn*
andBuild*
APIs? Probably! But what about theCheck*
APIs? Maybe for most of them. But things likeFillInlineAsmIdentifierInfo
? 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, are we close enough to the limit we should be worried now, or are we far enough away from the limit that we can worry in the future if/when we hit it? CC @rnk @compnerd @zmodem for extra opinions on whether we need to think about this more before landing
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 managed to get clang-cpp library building without /Zc:dllexportInlines- its at 46748 exported symbols.
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.
Would you mind trying an experiment for me? Would you add this to ASTMatchers.h and see how the numbers change for exported symbols:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i did have to export some of the AST matcher variables in another PR #110206.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.