This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add library for clangd main function
ClosedPublic

Authored by ivanmurashko on Mar 4 2023, 2:30 AM.

Details

Summary

The diff adds a library and header for clangd main function. That change allows to create custom builds for clangd outside the main LLVM repo. The diff also allows to use build system different from CMake to build clangd. The main reason for such change is an ability to use custom clang-tidy modules (created outside LLVM repo).

Test Plan:

ninja clangd

also check that necessary libs are installed aka

ninja install
...
ls <install folder>/lib/libclangdMain.a

Diff Detail

Event Timeline

ivanmurashko created this revision.Mar 4 2023, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2023, 2:30 AM
Herald added a subscriber: arphaman. · View Herald Transcript
ivanmurashko requested review of this revision.Mar 4 2023, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2023, 2:30 AM
nridge added a subscriber: nridge.Mar 4 2023, 10:37 PM

IIUC we want to link some extra targets into the clangd binary in order to provide extra clang-tidy checks, but *don't* want to customize the binary further.
(The latter can be useful, but this interface isn't sufficient for it).

Can this be solved at the build-system level, without changing the source code in ways that aren't otherwise useful?
For example, with a CLANG_TIDY_EXTRA_CHECKS cmake variable that adds more deps? This seems like it could also work for the clang-tidy binary.

The change addresses comments from D145228. The only one header is copied as a part of the installation. As result the diff contains the minimal changes required to integrated clangd into custom build systems (different from CMake).

Can this be solved at the build-system level, without changing the source code in ways that aren't otherwise useful?
For example, with a CLANG_TIDY_EXTRA_CHECKS cmake variable that adds more deps? This seems like it could also work for the clang-tidy binary.

As it was mentioned at D145228, we use LLVM as a set of libraries and headers that allow us to create custom components. The build system we use for the component is different from CMake (it's buck). Thus, unfortunately, the CMake customization does not work for us.

IIUC we want to link some extra targets into the clangd binary in order to provide extra clang-tidy checks, but *don't* want to customize the binary further.
(The latter can be useful, but this interface isn't sufficient for it).

I agree that the current interface does not provide any way to customize the binary. My primary reason was the following: I tried to keep the changes as minimal as possible to get an ability to build the clangd outside LLVM repo. I assumed that additional customization API can be added later as soon as it is be required.

BTW: I added the copy of the single header file (API) instead of copying all internal headers. That will allow to abandon D145228 and add future external API at the select set of headers in future.

ivanmurashko edited the summary of this revision. (Show Details)Mar 21 2023, 10:27 AM

rebased to the latest LLVM main

removed header install, only lib install has been left after the change

CC: @kadircet

thanks LG and seems to be working in a couple build configurations I tried. but there might still be breakages in different configs, so please be on the watchout after landing this for breakages in https://lab.llvm.org/.

FWIW something like:

# Needed by LLVM's CMake checks because this file defines multiple targets.
set(LLVM_OPTIONAL_SOURCES ClangdToolMain.cpp)

add_clang_library(clangdMain
  Check.cpp
  ClangdMain.cpp
  )

clang_target_link_libraries(clangdMain
  PRIVATE
  clangAST
  clangBasic
  clangDaemon
  clangFormat
  clangFrontend
  clangLex
  clangSema
  clangTidy
  clangTooling
  clangToolingCore
  clangToolingRefactoring
  clangToolingSyntax
  clangdRemoteIndex
  clangdSupport
  $<TARGET_OBJECTS:obj.clangDaemonTweaks>
)

if(CLANGD_BUILD_XPC)
  target_link_libraries(clangdMain
    PRIVATE
    clangdXpcJsonConversions
    clangdXpcTransport
    )
endif()

add_clang_tool(clangd
  ClangdToolMain.cpp
  )

clang_target_link_libraries(clangd
  PRIVATE
  clangdMain
  )

for the CMakeFile seem to cut it.

clang-tools-extra/clangd/tool/CMakeLists.txt
11

we should move this into clangdMain target now

25

it should be enough to have clangdMain here now.

34–35

you can get rid of this rule too

37

you can merge this with the previous rule

Rebase + apply suggestions from @kadircet

ivanmurashko marked 3 inline comments as done.Jun 27 2023, 2:25 PM
ivanmurashko marked an inline comment as done.Jun 27 2023, 2:44 PM

Fixed clangd unit tests

ivanmurashko marked an inline comment as not done.Jul 2 2023, 3:38 AM
ivanmurashko added inline comments.
clang-tools-extra/clangd/tool/CMakeLists.txt
11

Unfortunately it did not work and broke the clangd unit tests.

Note: all other comments were fixed

ivanmurashko added inline comments.Jul 2 2023, 3:39 AM
clang-tools-extra/clangd/tool/CMakeLists.txt
34–35

fixed

37

fixed

thanks LG and seems to be working in a couple build configurations I tried. but there might still be breakages in different configs, so please be on the watchout after landing this for breakages in https://lab.llvm.org/.

@kadircet , could you look at the final version?

rebase before the final push

kadircet accepted this revision.Jul 11 2023, 9:32 AM

thanks

This revision is now accepted and ready to land.Jul 11 2023, 9:32 AM
This revision was automatically updated to reflect the committed changes.

This broke building with -DLLVM_LINK_LLVM_DYLIB=ON:

/usr/bin/ld: tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/AddUsing.cpp.o: in function `clang::clangd::(anonymous namespace)::AddUsing::prepare(clang::clangd::Tweak::Selection const&)':
AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x1c): undefined reference to `clang::clangd::ParsedAST::getASTContext()'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x6d): undefined reference to `clang::clangd::ParsedAST::getASTContext() const'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x9a): undefined reference to `clang::clangd::isHeaderFile(llvm::StringRef, std::optional<clang::LangOptions>)'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0xcc): undefined reference to `clang::clangd::SelectionTree::commonAncestor() const'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x391): undefined reference to `clang::clangd::printNamespaceScope[abi:cxx11](clang::DeclContext const&)'

etc.

indeed, broke my CI with same issue.

This broke building with -DLLVM_LINK_LLVM_DYLIB=ON:

/usr/bin/ld: tools/clang/tools/extra/clangd/refactor/tweaks/CMakeFiles/obj.clangDaemonTweaks.dir/AddUsing.cpp.o: in function `clang::clangd::(anonymous namespace)::AddUsing::prepare(clang::clangd::Tweak::Selection const&)':
AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x1c): undefined reference to `clang::clangd::ParsedAST::getASTContext()'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x6d): undefined reference to `clang::clangd::ParsedAST::getASTContext() const'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x9a): undefined reference to `clang::clangd::isHeaderFile(llvm::StringRef, std::optional<clang::LangOptions>)'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0xcc): undefined reference to `clang::clangd::SelectionTree::commonAncestor() const'
/usr/bin/ld: AddUsing.cpp:(.text._ZN5clang6clangd12_GLOBAL__N_18AddUsing7prepareERKNS0_5Tweak9SelectionE+0x391): undefined reference to `clang::clangd::printNamespaceScope[abi:cxx11](clang::DeclContext const&)'

etc.

This broke building with -DLLVM_LINK_LLVM_DYLIB=ON:

I also ran into this; I pushed a fix in a20d57e83441a69fa2bab86593b18cc0402095d2.

ivanmurashko added a comment.EditedJul 12 2023, 2:34 AM

This broke building with -DLLVM_LINK_LLVM_DYLIB=ON:

I also ran into this; I pushed a fix in a20d57e83441a69fa2bab86593b18cc0402095d2.

Thanks for the fix!