From c6b2b784299b5b8c2081b4e7e76eeab80e9b81ee Mon Sep 17 00:00:00 2001 From: Kirill Bobyrev Date: Tue, 19 May 2020 16:59:04 +0200 Subject: [PATCH] [clangd-remote] Replace YAML serialization with proper Protobuf messages Summary: YAML serialization was used in the Proof of Concept for simplicity. This patch replaces implements Protobuf (de) serialization of almost all types that need to be transferred over the protocol. Reviewers: sammccall Reviewed By: sammccall Subscribers: mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D79862 --- clang-tools-extra/clangd/index/remote/Client.cpp | 3 +- clang-tools-extra/clangd/index/remote/Index.proto | 54 +++++++- .../index/remote/marshalling/Marshalling.cpp | 151 +++++++++++++++++++-- clang-tools-extra/clangd/unittests/CMakeLists.txt | 14 ++ clang-tools-extra/clangd/unittests/TestTU.cpp | 5 + clang-tools-extra/clangd/unittests/TestTU.h | 1 + .../clangd/unittests/remote/MarshallingTests.cpp | 93 +++++++++++++ 7 files changed, 302 insertions(+), 19 deletions(-) create mode 100644 clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp diff --git a/clang-tools-extra/clangd/index/remote/Client.cpp b/clang-tools-extra/clangd/index/remote/Client.cpp index 6d9cd6d..366a37a 100644 --- a/clang-tools-extra/clangd/index/remote/Client.cpp +++ b/clang-tools-extra/clangd/index/remote/Client.cpp @@ -46,8 +46,7 @@ class IndexClient : public clangd::SymbolIndex { } auto Sym = fromProtobuf(Reply.stream_result(), &Strings); if (!Sym) - elog("Received invalid {0}: {1}", ReplyT::descriptor()->name(), - Reply.stream_result().yaml_serialization()); + elog("Received invalid {0}", ReplyT::descriptor()->name()); Callback(*Sym); } SPAN_ATTACH(Tracer, "status", Reader->Finish().ok()); diff --git a/clang-tools-extra/clangd/index/remote/Index.proto b/clang-tools-extra/clangd/index/remote/Index.proto index 0cd9738..f3af29d 100644 --- a/clang-tools-extra/clangd/index/remote/Index.proto +++ b/clang-tools-extra/clangd/index/remote/Index.proto @@ -10,6 +10,8 @@ syntax = "proto3"; package clang.clangd.remote; +// Semantics of SymbolIndex match clangd::SymbolIndex with all required +// structures corresponding to their clangd::* counterparts. service SymbolIndex { rpc Lookup(LookupRequest) returns (stream LookupReply) {} @@ -34,7 +36,7 @@ message FuzzyFindRequest { repeated string scopes = 2; bool any_scope = 3; uint32 limit = 4; - bool resricted_for_code_completion = 5; + bool restricted_for_code_completion = 5; repeated string proximity_paths = 6; repeated string preferred_types = 7; } @@ -63,7 +65,49 @@ message RefsReply { } } -// FIXME(kirillbobyrev): Properly serialize symbols and refs instead of passing -// YAML. -message Ref { string yaml_serialization = 1; } -message Symbol { string yaml_serialization = 1; } +message Symbol { + string id = 1; + SymbolInfo info = 2; + string name = 3; + SymbolLocation definition = 4; + string scope = 5; + SymbolLocation canonical_declarattion = 6; + int32 references = 7; + uint32 origin = 8; + string signature = 9; + string template_specialization_args = 10; + string completion_snippet_suffix = 11; + string documentation = 12; + string return_type = 13; + string type = 14; + repeated HeaderWithReferences headers = 15; + uint32 flags = 16; +} + +message Ref { + SymbolLocation location = 1; + uint32 kind = 2; +} + +message SymbolInfo { + uint32 kind = 1; + uint32 subkind = 2; + uint32 language = 3; + uint32 properties = 4; +} + +message SymbolLocation { + Position start = 1; + Position end = 2; + string file_uri = 3; +} + +message Position { + uint32 line = 1; + uint32 column = 2; +} + +message HeaderWithReferences { + string header = 1; + uint32 references = 2; +} diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp index 60a258a..e95132d 100644 --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp @@ -7,13 +7,90 @@ //===----------------------------------------------------------------------===// #include "Marshalling.h" +#include "Index.pb.h" +#include "Protocol.h" #include "index/Serialization.h" +#include "index/Symbol.h" +#include "index/SymbolID.h" +#include "index/SymbolLocation.h" +#include "index/SymbolOrigin.h" #include "support/Logger.h" +#include "clang/Index/IndexSymbol.h" +#include "llvm/ADT/SmallVector.h" +#include "llvm/Support/StringSaver.h" namespace clang { namespace clangd { namespace remote { +namespace { + +clangd::SymbolLocation::Position fromProtobuf(const Position &Message) { + clangd::SymbolLocation::Position Result; + Result.setColumn(static_cast(Message.column())); + Result.setLine(static_cast(Message.line())); + return Result; +} + +Position toProtobuf(const clangd::SymbolLocation::Position &Position) { + remote::Position Result; + Result.set_column(Position.column()); + Result.set_line(Position.line()); + return Result; +} + +clang::index::SymbolInfo fromProtobuf(const SymbolInfo &Message) { + clang::index::SymbolInfo Result; + Result.Kind = static_cast(Message.kind()); + Result.SubKind = static_cast(Message.subkind()); + Result.Lang = static_cast(Message.language()); + Result.Properties = + static_cast(Message.properties()); + return Result; +} + +SymbolInfo toProtobuf(const clang::index::SymbolInfo &Info) { + SymbolInfo Result; + Result.set_kind(static_cast(Info.Kind)); + Result.set_subkind(static_cast(Info.SubKind)); + Result.set_language(static_cast(Info.Lang)); + Result.set_properties(static_cast(Info.Properties)); + return Result; +} + +clangd::SymbolLocation fromProtobuf(const SymbolLocation &Message, + llvm::UniqueStringSaver *Strings) { + clangd::SymbolLocation Location; + Location.Start = fromProtobuf(Message.start()); + Location.End = fromProtobuf(Message.end()); + Location.FileURI = Strings->save(Message.file_uri()).begin(); + return Location; +} + +SymbolLocation toProtobuf(const clangd::SymbolLocation &Location) { + remote::SymbolLocation Result; + *Result.mutable_start() = toProtobuf(Location.Start); + *Result.mutable_end() = toProtobuf(Location.End); + *Result.mutable_file_uri() = Location.FileURI; + return Result; +} + +HeaderWithReferences +toProtobuf(const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader) { + HeaderWithReferences Result; + Result.set_header(IncludeHeader.IncludeHeader.str()); + Result.set_references(IncludeHeader.References); + return Result; +} + +clangd::Symbol::IncludeHeaderWithReferences +fromProtobuf(const HeaderWithReferences &Message) { + return clangd::Symbol::IncludeHeaderWithReferences{Message.header(), + Message.references()}; +} + +} // namespace + clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request) { clangd::FuzzyFindRequest Result; Result.Query = Request->query(); @@ -22,7 +99,7 @@ clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request) { Result.AnyScope = Request->any_scope(); if (Request->limit()) Result.Limit = Request->limit(); - Result.RestrictForCodeCompletion = Request->resricted_for_code_completion(); + Result.RestrictForCodeCompletion = Request->restricted_for_code_completion(); for (const auto &Path : Request->proximity_paths()) Result.ProximityPaths.push_back(Path); for (const auto &Type : Request->preferred_types()) @@ -32,21 +109,50 @@ clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request) { llvm::Optional fromProtobuf(const Symbol &Message, llvm::UniqueStringSaver *Strings) { - auto Result = symbolFromYAML(Message.yaml_serialization(), Strings); - if (!Result) { - elog("Cannot convert Symbol from Protobuf: {}", Result.takeError()); + if (!Message.has_info() || !Message.has_definition() || + !Message.has_canonical_declarattion()) { + elog("Cannot convert Symbol from Protobuf: {}", Message.ShortDebugString()); return llvm::None; } - return *Result; + clangd::Symbol Result; + auto ID = SymbolID::fromStr(Message.id()); + if (!ID) { + elog("Cannot convert parse SymbolID {} from Protobuf: {}", ID.takeError(), + Message.ShortDebugString()); + return llvm::None; + } + Result.ID = *ID; + Result.SymInfo = fromProtobuf(Message.info()); + Result.Name = Message.name(); + Result.Scope = Message.scope(); + Result.Definition = fromProtobuf(Message.definition(), Strings); + Result.CanonicalDeclaration = + fromProtobuf(Message.canonical_declarattion(), Strings); + Result.References = Message.references(); + Result.Origin = static_cast(Message.origin()); + Result.Signature = Message.signature(); + Result.TemplateSpecializationArgs = Message.template_specialization_args(); + Result.CompletionSnippetSuffix = Message.completion_snippet_suffix(); + Result.Documentation = Message.documentation(); + Result.ReturnType = Message.return_type(); + Result.Type = Message.type(); + for (const auto &Header : Message.headers()) { + Result.IncludeHeaders.push_back(fromProtobuf(Header)); + } + Result.Flags = static_cast(Message.flags()); + return Result; } + llvm::Optional fromProtobuf(const Ref &Message, llvm::UniqueStringSaver *Strings) { - auto Result = refFromYAML(Message.yaml_serialization(), Strings); - if (!Result) { - elog("Cannot convert Ref from Protobuf: {}", Result.takeError()); + if (!Message.has_location()) { + elog("Cannot convert Ref from Protobuf: {}", Message.ShortDebugString()); return llvm::None; } - return *Result; + clangd::Ref Result; + Result.Location = fromProtobuf(Message.location(), Strings); + Result.Kind = static_cast(Message.kind()); + return Result; } LookupRequest toProtobuf(const clangd::LookupRequest &From) { @@ -64,7 +170,7 @@ FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From) { RPCRequest.set_any_scope(From.AnyScope); if (From.Limit) RPCRequest.set_limit(*From.Limit); - RPCRequest.set_resricted_for_code_completion(From.RestrictForCodeCompletion); + RPCRequest.set_restricted_for_code_completion(From.RestrictForCodeCompletion); for (const auto &Path : From.ProximityPaths) RPCRequest.add_proximity_paths(Path); for (const auto &Type : From.PreferredTypes) @@ -84,13 +190,34 @@ RefsRequest toProtobuf(const clangd::RefsRequest &From) { Symbol toProtobuf(const clangd::Symbol &From) { Symbol Result; - Result.set_yaml_serialization(toYAML(From)); + Result.set_id(From.ID.str()); + *Result.mutable_info() = toProtobuf(From.SymInfo); + Result.set_name(From.Name.str()); + *Result.mutable_definition() = toProtobuf(From.Definition); + Result.set_scope(From.Scope.str()); + *Result.mutable_canonical_declarattion() = + toProtobuf(From.CanonicalDeclaration); + Result.set_references(From.References); + Result.set_origin(static_cast(From.Origin)); + Result.set_signature(From.Signature.str()); + Result.set_template_specialization_args( + From.TemplateSpecializationArgs.str()); + Result.set_completion_snippet_suffix(From.CompletionSnippetSuffix.str()); + Result.set_documentation(From.Documentation.str()); + Result.set_return_type(From.ReturnType.str()); + Result.set_type(From.Type.str()); + for (const auto &Header : From.IncludeHeaders) { + auto *NextHeader = Result.add_headers(); + *NextHeader = toProtobuf(Header); + } + Result.set_flags(static_cast(From.Flags)); return Result; } Ref toProtobuf(const clangd::Ref &From) { Ref Result; - Result.set_yaml_serialization(toYAML(From)); + Result.set_kind(static_cast(From.Kind)); + *Result.mutable_location() = toProtobuf(From.Location); return Result; } diff --git a/clang-tools-extra/clangd/unittests/CMakeLists.txt b/clang-tools-extra/clangd/unittests/CMakeLists.txt index a1f6af6..b907dfe 100644 --- a/clang-tools-extra/clangd/unittests/CMakeLists.txt +++ b/clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -22,6 +22,12 @@ if(CLANG_BUILT_STANDALONE) endif() endif() +if (CLANGD_ENABLE_REMOTE) + include_directories(${CMAKE_CURRENT_BINARY_DIR}/../index/remote) + add_definitions(-DGOOGLE_PROTOBUF_NO_RTTI=1) + set(REMOTE_TEST_SOURCES remote/MarshallingTests.cpp) +endif() + add_custom_target(ClangdUnitTests) add_unittest(ClangdUnitTests ClangdTests Annotations.cpp @@ -87,6 +93,8 @@ add_unittest(ClangdUnitTests ClangdTests support/TestTracer.cpp support/TraceTests.cpp + ${REMOTE_TEST_SOURCES} + $ ) @@ -116,6 +124,12 @@ target_link_libraries(ClangdTests LLVMTestingSupport ) +if (CLANGD_ENABLE_REMOTE) + target_link_libraries(ClangdTests + PRIVATE + clangdRemoteMarshalling) +endif() + if (CLANGD_BUILD_XPC) add_subdirectory(xpc) endif () diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp index 6d9de48..824c4cc 100644 --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -114,6 +114,11 @@ SymbolSlab TestTU::headerSymbols() const { AST.getCanonicalIncludes())); } +RefSlab TestTU::headerRefs() const { + auto AST = build(); + return std::get<1>(indexMainDecls(AST)); +} + std::unique_ptr TestTU::index() const { auto AST = build(); auto Idx = std::make_unique(/*UseDex=*/true); diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h index 415326e..2be294f 100644 --- a/clang-tools-extra/clangd/unittests/TestTU.h +++ b/clang-tools-extra/clangd/unittests/TestTU.h @@ -70,6 +70,7 @@ struct TestTU { ParsedAST build() const; ParseInputs inputs() const; SymbolSlab headerSymbols() const; + RefSlab headerRefs() const; std::unique_ptr index() const; }; diff --git a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp new file mode 100644 index 0000000..8e68cfb --- /dev/null +++ b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp @@ -0,0 +1,93 @@ +//===--- MarshallingTests.cpp ------------------------------------*- 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 +// +//===----------------------------------------------------------------------===// + +#include "../TestTU.h" +#include "index/Serialization.h" +#include "index/remote/marshalling/Marshalling.h" +#include "llvm/Support/StringSaver.h" +#include "gtest/gtest.h" + +namespace clang { +namespace clangd { +namespace remote { +namespace { + +TEST(RemoteMarshallingTest, SymbolSerialization) { + const auto *Header = R"( + // This is a class. + class Foo { + public: + Foo(); + + int Bar; + private: + double Number; + }; + /// This is a function. + char baz(); + template + T getT (); + )"; + const auto TU = TestTU::withHeaderCode(Header); + const auto Symbols = TU.headerSymbols(); + // Sanity check: there are more than 5 symbols available. + EXPECT_GE(Symbols.size(), 5UL); + llvm::BumpPtrAllocator Arena; + llvm::UniqueStringSaver Strings(Arena); + for (auto &Sym : Symbols) { + const auto ProtobufMeessage = toProtobuf(Sym); + const auto SymToProtobufAndBack = fromProtobuf(ProtobufMeessage, &Strings); + EXPECT_TRUE(SymToProtobufAndBack.hasValue()); + EXPECT_EQ(toYAML(Sym), toYAML(*SymToProtobufAndBack)); + } +} + +TEST(RemoteMarshallingTest, ReferenceSerialization) { + TestTU TU; + TU.HeaderCode = R"( + int foo(); + int GlobalVariable = 42; + class Foo { + public: + Foo(); + + char Symbol = 'S'; + }; + template + T getT() { return T(); } + )"; + TU.Code = R"( + int foo() { + ++GlobalVariable; + + Foo foo = Foo(); + if (foo.Symbol - 'a' == 42) { + foo.Symbol = 'b'; + } + + const auto bar = getT(); + } + )"; + const auto References = TU.headerRefs(); + llvm::BumpPtrAllocator Arena; + llvm::UniqueStringSaver Strings(Arena); + // Sanity check: there are more than 5 references available. + EXPECT_GE(References.numRefs(), 5UL); + for (const auto &SymbolWithRefs : References) { + for (const auto &Ref : SymbolWithRefs.second) { + const auto RefToProtobufAndBack = fromProtobuf(toProtobuf(Ref), &Strings); + EXPECT_TRUE(RefToProtobufAndBack.hasValue()); + EXPECT_EQ(toYAML(Ref), toYAML(*RefToProtobufAndBack)); + } + } +} // namespace + +} // namespace +} // namespace remote +} // namespace clangd +} // namespace clang -- 2.7.4