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.

Sign up for GitHub

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

[clangd] Update clangDaemonTweaks to set symbol visibility macros #112304

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[clangd] Update clangDaemonTweaks to set symbol visibility macros #112304

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d41e927
[clangd] Update clangDaemonTweaks to set symbol visibility macros cor…
fsfod Oct 15, 2024
732af54
Add a CLANG_EXPORT arg to add_clang_library cmake function instead of…
fsfod Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ set(LLVM_LINK_COMPONENTS
# To enable these tweaks in executables or shared libraries, add
# $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
# clangd/tool/CMakeLists.txt for an example.
add_clang_library(clangDaemonTweaks OBJECT
add_clang_library(clangDaemonTweaks CLANG_IMPORT OBJECT
AddUsing.cpp
AnnotateHighlightings.cpp
DumpAST.cpp
Expand Down
4 changes: 2 additions & 2 deletions clang/cmake/modules/AddClang.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ endmacro()

macro(add_clang_library name)
cmake_parse_arguments(ARG
"SHARED;STATIC;INSTALL_WITH_TOOLCHAIN"
"SHARED;STATIC;INSTALL_WITH_TOOLCHAIN;CLANG_IMPORT"
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear what this new option (CLANG_IMPORT) does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not possible to infer if a OBJECT target should be importing or exporting clang symbols, this option allows OBJECT target to specify it should be importing Clang symbols.

Copy link
Member

Choose a reason for hiding this comment

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

We should document the option. Why is it specific to clang? I think that the right semantics are, is this library built as a static library or not. The default should be dynamic. At that point, when the library is built as static, a public definition could be added at the library definition to state it is static. That would then implicitly avoid the DLL storage. Otherwise, it is implicitly DLL imported.

#if defined(clang_STATIC)
# define CLANG_ABI
#else
# if defined(_WIN32)
#   if defined(clang_EXPORTS)
#     define CLANG_ABI __declspec(dllexport)
#   else
#     define CLANG_ABI __declspec(dllimport)
#   endif
# else
...
# endif

We don't need all this new custom handling to be wired through out the build to support this. Each library needs only to know its ABI, and in the case that it's a compacted library (i.e. is an object library), we update the export symbol that CMake uses to the DLL that it is compacted into.

Am I missing something in this model?

""
"ADDITIONAL_HEADERS"
${ARGN})
Expand Down Expand Up @@ -114,7 +114,7 @@ macro(add_clang_library name)
if(TARGET "obj.${name}")
target_compile_definitions("obj.${name}" PUBLIC CLANG_BUILD_STATIC)
endif()
elseif(NOT ARG_SHARED AND NOT ARG_STATIC)
elseif(NOT ARG_SHARED AND NOT ARG_STATIC AND NOT ARG_CLANG_IMPORT)
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it would change the behaviour of other libraries which were previously being built for shared linking (or at least were exporting their interfaces) to now be built for static linking (or no longer export their symbols).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean, its only checking for the arg not being set, so it shouldn't change the behaviour for other libraries.

Copy link
Member

Choose a reason for hiding this comment

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

Then we should be using if (NOT DEFINED ...). The syntax you have can check for the truthiness of the value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

cmake_parse_args always defines option values:

For the keywords, these variables will always be defined, and they will be set to TRUE if the keyword is present, or FALSE if it is not.

see: https://cmake.org/cmake/help/latest/command/cmake_parse_arguments.html

So ARG_CLANG_IMPORT will always be defined, it may be defined either to True or False.

# Clang component libraries linked in to clang-cpp are declared without SHARED or STATIC
target_compile_definitions("obj.${name}" PUBLIC CLANG_EXPORTS)
endif()
Expand Down
Loading