Skip to content
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

[Clang] Add explicit visibility symbol macros #108276

Merged
merged 9 commits into from
Oct 14, 2024
11 changes: 11 additions & 0 deletions clang/cmake/modules/AddClang.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
63 changes: 63 additions & 0 deletions clang/include/clang/Support/Compiler.h
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// export macros added to them by the clang tool
// export macros added to them by the clang tool.

Copy link
Collaborator

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 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?

Copy link
Contributor

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…

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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:

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).

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

#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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
4 changes: 4 additions & 0 deletions clang/tools/libclang/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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}")
Expand Down
Loading