From eef162c330b02fdc53ec33e5549635be5c5911fa Mon Sep 17 00:00:00 2001 From: Kirill Bobyrev Date: Tue, 21 Jul 2020 11:04:53 +0200 Subject: [PATCH] [clangd] Don't send invalid messages from remote index Summary: Remote server should not send messages that are invalid and will cause problems on the client side. The client should not be affected by server's failures whenever possible. Also add more error messages and logs. Reviewers: sammccall Reviewed By: sammccall Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D83826 --- clang-tools-extra/clangd/index/remote/Client.cpp | 27 +- .../index/remote/marshalling/Marshalling.cpp | 341 +++++++++++---------- .../clangd/index/remote/marshalling/Marshalling.h | 116 ++++--- .../clangd/index/remote/server/Server.cpp | 25 +- .../clangd/unittests/remote/MarshallingTests.cpp | 147 +++++---- 5 files changed, 362 insertions(+), 294 deletions(-) diff --git a/clang-tools-extra/clangd/index/remote/Client.cpp b/clang-tools-extra/clangd/index/remote/Client.cpp index b4688319307..35ce84068f4 100644 --- a/clang-tools-extra/clangd/index/remote/Client.cpp +++ b/clang-tools-extra/clangd/index/remote/Client.cpp @@ -29,16 +29,6 @@ class IndexClient : public clangd::SymbolIndex { using StreamingCall = std::unique_ptr> ( remote::SymbolIndex::Stub::*)(grpc::ClientContext *, const RequestT &); - template - RequestT serializeRequest(ClangdRequestT Request) const { - return toProtobuf(Request); - } - - template <> - FuzzyFindRequest serializeRequest(clangd::FuzzyFindRequest Request) const { - return toProtobuf(Request, ProjectRoot); - } - template bool streamRPC(ClangdRequestT Request, @@ -46,24 +36,23 @@ class IndexClient : public clangd::SymbolIndex { CallbackT Callback) const { bool FinalResult = false; trace::Span Tracer(RequestT::descriptor()->name()); - const auto RPCRequest = serializeRequest(Request); + const auto RPCRequest = ProtobufMarshaller->toProtobuf(Request); grpc::ClientContext Context; std::chrono::system_clock::time_point Deadline = std::chrono::system_clock::now() + DeadlineWaitingTime; Context.set_deadline(Deadline); auto Reader = (Stub.get()->*RPCCall)(&Context, RPCRequest); - llvm::BumpPtrAllocator Arena; - llvm::UniqueStringSaver Strings(Arena); ReplyT Reply; while (Reader->Read(&Reply)) { if (!Reply.has_stream_result()) { FinalResult = Reply.final_result(); continue; } - auto Response = - fromProtobuf(Reply.stream_result(), &Strings, ProjectRoot); - if (!Response) + auto Response = ProtobufMarshaller->fromProtobuf(Reply.stream_result()); + if (!Response) { elog("Received invalid {0}", ReplyT::descriptor()->name()); + continue; + } Callback(*Response); } SPAN_ATTACH(Tracer, "status", Reader->Finish().ok()); @@ -74,7 +63,9 @@ public: IndexClient( std::shared_ptr Channel, llvm::StringRef ProjectRoot, std::chrono::milliseconds DeadlineTime = std::chrono::milliseconds(1000)) - : Stub(remote::SymbolIndex::NewStub(Channel)), ProjectRoot(ProjectRoot), + : Stub(remote::SymbolIndex::NewStub(Channel)), + ProtobufMarshaller(new Marshaller(/*RemoteIndexRoot=*/"", + /*LocalIndexRoot=*/ProjectRoot)), DeadlineWaitingTime(DeadlineTime) {} void lookup(const clangd::LookupRequest &Request, @@ -105,7 +96,7 @@ public: private: std::unique_ptr Stub; - std::string ProjectRoot; + std::unique_ptr ProtobufMarshaller; // Each request will be terminated if it takes too long. std::chrono::milliseconds DeadlineWaitingTime; }; diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp index db895bf039b..059c42ee0ea 100644 --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp @@ -30,101 +30,28 @@ 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; -} - -llvm::Optional -fromProtobuf(const SymbolLocation &Message, llvm::UniqueStringSaver *Strings, - llvm::StringRef IndexRoot) { - clangd::SymbolLocation Location; - auto URIString = relativePathToURI(Message.file_path(), IndexRoot); - if (!URIString) - return llvm::None; - Location.FileURI = Strings->save(*URIString).begin(); - Location.Start = fromProtobuf(Message.start()); - Location.End = fromProtobuf(Message.end()); - return Location; -} - -llvm::Optional -toProtobuf(const clangd::SymbolLocation &Location, llvm::StringRef IndexRoot) { - remote::SymbolLocation Result; - auto RelativePath = uriToRelativePath(Location.FileURI, IndexRoot); - if (!RelativePath) - return llvm::None; - *Result.mutable_file_path() = *RelativePath; - *Result.mutable_start() = toProtobuf(Location.Start); - *Result.mutable_end() = toProtobuf(Location.End); - return Result; -} - -llvm::Optional -toProtobuf(const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader, - llvm::StringRef IndexRoot) { - HeaderWithReferences Result; - Result.set_references(IncludeHeader.References); - const std::string Header = IncludeHeader.IncludeHeader.str(); - if (isLiteralInclude(Header)) { - Result.set_header(Header); - return Result; +Marshaller::Marshaller(llvm::StringRef RemoteIndexRoot, + llvm::StringRef LocalIndexRoot) + : Strings(Arena) { + if (!RemoteIndexRoot.empty()) { + assert(llvm::sys::path::is_absolute(RemoteIndexRoot)); + assert(RemoteIndexRoot == + llvm::sys::path::convert_to_slash(RemoteIndexRoot)); + assert(RemoteIndexRoot.endswith(llvm::sys::path::get_separator())); + this->RemoteIndexRoot = RemoteIndexRoot.str(); } - auto RelativePath = uriToRelativePath(Header, IndexRoot); - if (!RelativePath) - return llvm::None; - Result.set_header(*RelativePath); - return Result; -} - -llvm::Optional -fromProtobuf(const HeaderWithReferences &Message, - llvm::UniqueStringSaver *Strings, llvm::StringRef IndexRoot) { - std::string Header = Message.header(); - if (Header.front() != '<' && Header.front() != '"') { - auto URIString = relativePathToURI(Header, IndexRoot); - if (!URIString) - return llvm::None; - Header = *URIString; + if (!LocalIndexRoot.empty()) { + assert(llvm::sys::path::is_absolute(LocalIndexRoot)); + assert(LocalIndexRoot == llvm::sys::path::convert_to_slash(LocalIndexRoot)); + assert(LocalIndexRoot.endswith(llvm::sys::path::get_separator())); + this->LocalIndexRoot = LocalIndexRoot.str(); } - return clangd::Symbol::IncludeHeaderWithReferences{Strings->save(Header), - Message.references()}; + assert(!RemoteIndexRoot.empty() || !LocalIndexRoot.empty()); } -} // namespace - -clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request, - llvm::StringRef IndexRoot) { +clangd::FuzzyFindRequest +Marshaller::fromProtobuf(const FuzzyFindRequest *Request) { + assert(LocalIndexRoot); clangd::FuzzyFindRequest Result; Result.Query = Request->query(); for (const auto &Scope : Request->scopes()) @@ -134,7 +61,7 @@ clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request, Result.Limit = Request->limit(); Result.RestrictForCodeCompletion = Request->restricted_for_code_completion(); for (const auto &Path : Request->proximity_paths()) { - llvm::SmallString<256> LocalPath = llvm::StringRef(IndexRoot); + llvm::SmallString<256> LocalPath = llvm::StringRef(*LocalIndexRoot); llvm::sys::path::append(LocalPath, Path); Result.ProximityPaths.push_back(std::string(LocalPath)); } @@ -143,34 +70,35 @@ clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request, return Result; } -llvm::Optional fromProtobuf(const Symbol &Message, - llvm::UniqueStringSaver *Strings, - llvm::StringRef IndexRoot) { - if (!Message.has_info() || !Message.has_definition() || - !Message.has_canonical_declaration()) { - elog("Cannot convert Symbol from Protobuf: {0}", - Message.ShortDebugString()); +llvm::Optional Marshaller::fromProtobuf(const Symbol &Message) { + if (!Message.has_info() || !Message.has_canonical_declaration()) { + elog("Cannot convert Symbol from protobuf (missing info, definition or " + "declaration): {0}", + Message.DebugString()); return llvm::None; } clangd::Symbol Result; auto ID = SymbolID::fromStr(Message.id()); if (!ID) { - elog("Cannot parse SymbolID {0} given Protobuf: {1}", ID.takeError(), - Message.ShortDebugString()); + elog("Cannot parse SymbolID {0} given protobuf: {1}", ID.takeError(), + Message.DebugString()); return llvm::None; } Result.ID = *ID; Result.SymInfo = fromProtobuf(Message.info()); Result.Name = Message.name(); Result.Scope = Message.scope(); - auto Definition = fromProtobuf(Message.definition(), Strings, IndexRoot); - if (!Definition) - return llvm::None; - Result.Definition = *Definition; - auto Declaration = - fromProtobuf(Message.canonical_declaration(), Strings, IndexRoot); - if (!Declaration) + if (Message.has_definition()) { + auto Definition = fromProtobuf(Message.definition()); + if (Definition) + Result.Definition = *Definition; + } + auto Declaration = fromProtobuf(Message.canonical_declaration()); + if (!Declaration) { + elog("Cannot convert Symbol from protobuf (invalid declaration): {0}", + Message.DebugString()); return llvm::None; + } Result.CanonicalDeclaration = *Declaration; Result.References = Message.references(); Result.Origin = static_cast(Message.origin()); @@ -181,39 +109,44 @@ llvm::Optional fromProtobuf(const Symbol &Message, Result.ReturnType = Message.return_type(); Result.Type = Message.type(); for (const auto &Header : Message.headers()) { - auto SerializedHeader = fromProtobuf(Header, Strings, IndexRoot); + auto SerializedHeader = fromProtobuf(Header); if (SerializedHeader) Result.IncludeHeaders.push_back(*SerializedHeader); + else + elog("Cannot convert HeaderWithIncludes from protobuf: {0}", + Header.DebugString()); } Result.Flags = static_cast(Message.flags()); return Result; } -llvm::Optional fromProtobuf(const Ref &Message, - llvm::UniqueStringSaver *Strings, - llvm::StringRef IndexRoot) { +llvm::Optional Marshaller::fromProtobuf(const Ref &Message) { if (!Message.has_location()) { - elog("Cannot convert Ref from Protobuf: {}", Message.ShortDebugString()); + elog("Cannot convert Ref from protobuf (missing location): {0}", + Message.DebugString()); return llvm::None; } clangd::Ref Result; - auto Location = fromProtobuf(Message.location(), Strings, IndexRoot); - if (!Location) + auto Location = fromProtobuf(Message.location()); + if (!Location) { + elog("Cannot convert Ref from protobuf (invalid location): {0}", + Message.DebugString()); return llvm::None; + } Result.Location = *Location; Result.Kind = static_cast(Message.kind()); return Result; } -LookupRequest toProtobuf(const clangd::LookupRequest &From) { +LookupRequest Marshaller::toProtobuf(const clangd::LookupRequest &From) { LookupRequest RPCRequest; for (const auto &SymbolID : From.IDs) RPCRequest.add_ids(SymbolID.str()); return RPCRequest; } -FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From, - llvm::StringRef IndexRoot) { +FuzzyFindRequest Marshaller::toProtobuf(const clangd::FuzzyFindRequest &From) { + assert(RemoteIndexRoot); FuzzyFindRequest RPCRequest; RPCRequest.set_query(From.Query); for (const auto &Scope : From.Scopes) @@ -224,7 +157,8 @@ FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From, RPCRequest.set_restricted_for_code_completion(From.RestrictForCodeCompletion); for (const auto &Path : From.ProximityPaths) { llvm::SmallString<256> RelativePath = llvm::StringRef(Path); - if (llvm::sys::path::replace_path_prefix(RelativePath, IndexRoot, "")) + if (llvm::sys::path::replace_path_prefix(RelativePath, *RemoteIndexRoot, + "")) RPCRequest.add_proximity_paths(llvm::sys::path::convert_to_slash( RelativePath, llvm::sys::path::Style::posix)); } @@ -233,7 +167,7 @@ FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From, return RPCRequest; } -RefsRequest toProtobuf(const clangd::RefsRequest &From) { +RefsRequest Marshaller::toProtobuf(const clangd::RefsRequest &From) { RefsRequest RPCRequest; for (const auto &ID : From.IDs) RPCRequest.add_ids(ID.str()); @@ -243,18 +177,28 @@ RefsRequest toProtobuf(const clangd::RefsRequest &From) { return RPCRequest; } -Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot) { +llvm::Optional Marshaller::toProtobuf(const clangd::Symbol &From) { Symbol Result; Result.set_id(From.ID.str()); *Result.mutable_info() = toProtobuf(From.SymInfo); Result.set_name(From.Name.str()); - auto Definition = toProtobuf(From.Definition, IndexRoot); - if (Definition) + if (*From.Definition.FileURI) { + auto Definition = toProtobuf(From.Definition); + if (!Definition) { + elog("Can not convert Symbol to protobuf (invalid definition) {0}: {1}", + From, From.Definition); + return llvm::None; + } *Result.mutable_definition() = *Definition; + } Result.set_scope(From.Scope.str()); - auto Declaration = toProtobuf(From.CanonicalDeclaration, IndexRoot); - if (Declaration) - *Result.mutable_canonical_declaration() = *Declaration; + auto Declaration = toProtobuf(From.CanonicalDeclaration); + if (!Declaration) { + elog("Can not convert Symbol to protobuf (invalid declaration) {0}: {1}", + From, From.CanonicalDeclaration); + return llvm::None; + } + *Result.mutable_canonical_declaration() = *Declaration; Result.set_references(From.References); Result.set_origin(static_cast(From.Origin)); Result.set_signature(From.Signature.str()); @@ -265,9 +209,12 @@ Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot) { Result.set_return_type(From.ReturnType.str()); Result.set_type(From.Type.str()); for (const auto &Header : From.IncludeHeaders) { - auto Serialized = toProtobuf(Header, IndexRoot); - if (!Serialized) + auto Serialized = toProtobuf(Header); + if (!Serialized) { + elog("Can not convert IncludeHeaderWithReferences to protobuf: {0}", + Header.IncludeHeader); continue; + } auto *NextHeader = Result.add_headers(); *NextHeader = *Serialized; } @@ -275,50 +222,38 @@ Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot) { return Result; } -// FIXME(kirillbobyrev): A reference without location is invalid. -// llvm::Optional here and on the server side? -Ref toProtobuf(const clangd::Ref &From, llvm::StringRef IndexRoot) { +llvm::Optional Marshaller::toProtobuf(const clangd::Ref &From) { Ref Result; Result.set_kind(static_cast(From.Kind)); - auto Location = toProtobuf(From.Location, IndexRoot); - if (Location) - *Result.mutable_location() = *Location; + auto Location = toProtobuf(From.Location); + if (!Location) { + elog("Can not convert Reference to protobuf (invalid location) {0}: {1}", + From, From.Location); + return llvm::None; + } + *Result.mutable_location() = *Location; return Result; } -llvm::Optional relativePathToURI(llvm::StringRef RelativePath, - llvm::StringRef IndexRoot) { +llvm::Optional +Marshaller::relativePathToURI(llvm::StringRef RelativePath) { + assert(LocalIndexRoot); assert(RelativePath == llvm::sys::path::convert_to_slash( RelativePath, llvm::sys::path::Style::posix)); - assert(IndexRoot == llvm::sys::path::convert_to_slash(IndexRoot)); - assert(IndexRoot.endswith(llvm::sys::path::get_separator())); - if (RelativePath.empty()) - return std::string(); - if (llvm::sys::path::is_absolute(RelativePath)) { - elog("Remote index client got absolute path from server: {0}", - RelativePath); + if (RelativePath.empty()) { return llvm::None; } - if (llvm::sys::path::is_relative(IndexRoot)) { - elog("Remote index client got a relative path as index root: {0}", - IndexRoot); + if (llvm::sys::path::is_absolute(RelativePath)) { return llvm::None; } - llvm::SmallString<256> FullPath = IndexRoot; + llvm::SmallString<256> FullPath = llvm::StringRef(*LocalIndexRoot); llvm::sys::path::append(FullPath, RelativePath); auto Result = URI::createFile(FullPath); return Result.toString(); } -llvm::Optional uriToRelativePath(llvm::StringRef URI, - llvm::StringRef IndexRoot) { - assert(IndexRoot.endswith(llvm::sys::path::get_separator())); - assert(IndexRoot == llvm::sys::path::convert_to_slash(IndexRoot)); - assert(!IndexRoot.empty()); - if (llvm::sys::path::is_relative(IndexRoot)) { - elog("Index root {0} is not absolute path", IndexRoot); - return llvm::None; - } +llvm::Optional Marshaller::uriToRelativePath(llvm::StringRef URI) { + assert(RemoteIndexRoot); auto ParsedURI = URI::parse(URI); if (!ParsedURI) { elog("Remote index got bad URI from client {0}: {1}", URI, @@ -326,14 +261,10 @@ llvm::Optional uriToRelativePath(llvm::StringRef URI, return llvm::None; } if (ParsedURI->scheme() != "file") { - elog("Remote index got URI with scheme other than \"file\" {0}: {1}", URI, - ParsedURI->scheme()); return llvm::None; } llvm::SmallString<256> Result = ParsedURI->body(); - if (!llvm::sys::path::replace_path_prefix(Result, IndexRoot, "")) { - elog("Can not get relative path from the URI {0} given the index root {1}", - URI, IndexRoot); + if (!llvm::sys::path::replace_path_prefix(Result, *RemoteIndexRoot, "")) { return llvm::None; } // Make sure the result has UNIX slashes. @@ -341,6 +272,94 @@ llvm::Optional uriToRelativePath(llvm::StringRef URI, llvm::sys::path::Style::posix); } +clangd::SymbolLocation::Position +Marshaller::fromProtobuf(const Position &Message) { + clangd::SymbolLocation::Position Result; + Result.setColumn(static_cast(Message.column())); + Result.setLine(static_cast(Message.line())); + return Result; +} + +Position +Marshaller::toProtobuf(const clangd::SymbolLocation::Position &Position) { + remote::Position Result; + Result.set_column(Position.column()); + Result.set_line(Position.line()); + return Result; +} + +clang::index::SymbolInfo Marshaller::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 Marshaller::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; +} + +llvm::Optional +Marshaller::fromProtobuf(const SymbolLocation &Message) { + clangd::SymbolLocation Location; + auto URIString = relativePathToURI(Message.file_path()); + if (!URIString) + return llvm::None; + Location.FileURI = Strings.save(*URIString).begin(); + Location.Start = fromProtobuf(Message.start()); + Location.End = fromProtobuf(Message.end()); + return Location; +} + +llvm::Optional +Marshaller::toProtobuf(const clangd::SymbolLocation &Location) { + remote::SymbolLocation Result; + auto RelativePath = uriToRelativePath(Location.FileURI); + if (!RelativePath) + return llvm::None; + *Result.mutable_file_path() = *RelativePath; + *Result.mutable_start() = toProtobuf(Location.Start); + *Result.mutable_end() = toProtobuf(Location.End); + return Result; +} + +llvm::Optional Marshaller::toProtobuf( + const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader) { + HeaderWithReferences Result; + Result.set_references(IncludeHeader.References); + const std::string Header = IncludeHeader.IncludeHeader.str(); + if (isLiteralInclude(Header)) { + Result.set_header(Header); + return Result; + } + auto RelativePath = uriToRelativePath(Header); + if (!RelativePath) + return llvm::None; + Result.set_header(*RelativePath); + return Result; +} + +llvm::Optional +Marshaller::fromProtobuf(const HeaderWithReferences &Message) { + std::string Header = Message.header(); + if (Header.front() != '<' && Header.front() != '"') { + auto URIString = relativePathToURI(Header); + if (!URIString) + return llvm::None; + Header = *URIString; + } + return clangd::Symbol::IncludeHeaderWithReferences{Strings.save(Header), + Message.references()}; +} + } // namespace remote } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h index 86cc8fa272f..9129cff24db 100644 --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h @@ -5,31 +5,6 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // //===----------------------------------------------------------------------===// -// -// Marshalling provides translation between native clangd types into the -// Protobuf-generated classes. Most translations are 1-to-1 and wrap variables -// into appropriate Protobuf types. -// -// A notable exception is URI translation. Because paths to files are different -// on indexing machine and client machine -// ("/remote/machine/projects/llvm-project/llvm/include/HelloWorld.h" versus -// "/usr/local/username/llvm-project/llvm/include/HelloWorld.h"), they need to -// be converted appropriately. Remote machine strips the prefix from the -// absolute path and passes paths relative to the project root over the wire -// ("include/HelloWorld.h" in this example). The indexed project root is passed -// to the remote server. Client receives this relative path and constructs a URI -// that points to the relevant file in the filesystem. The relative path is -// appended to IndexRoot to construct the full path and build the final URI. -// -// Index root is the absolute path to the project and includes a trailing slash. -// The relative path passed over the wire has unix slashes. -// -// toProtobuf() functions serialize native clangd types and strip IndexRoot from -// the file paths specific to indexing machine. fromProtobuf() functions -// deserialize clangd types and translate relative paths into machine-native -// URIs. -// -//===----------------------------------------------------------------------===// #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_INDEX_REMOTE_MARSHALLING_H @@ -43,33 +18,76 @@ namespace clang { namespace clangd { namespace remote { -clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request, - llvm::StringRef IndexRoot); -llvm::Optional fromProtobuf(const Symbol &Message, - llvm::UniqueStringSaver *Strings, - llvm::StringRef IndexRoot); -llvm::Optional fromProtobuf(const Ref &Message, - llvm::UniqueStringSaver *Strings, - llvm::StringRef IndexRoot); +// Marshalling provides an interface for translattion between native clangd +// types into the Protobuf-generated classes. Most translations are 1-to-1 and +// wrap variables into appropriate Protobuf types. +// +/// A notable exception is URI translation. Because paths to files are different +/// on indexing machine and client machine +/// ("/remote/machine/projects/llvm-project/llvm/include/HelloWorld.h" versus +/// "/usr/local/username/llvm-project/llvm/include/HelloWorld.h"), they need to +/// be converted appropriately. Remote machine strips the prefix +/// (RemoteIndexRoot) from the absolute path and passes paths relative to the +/// project root over the wire ("include/HelloWorld.h" in this example). The +/// indexed project root is passed to the remote server. Client receives this +/// relative path and constructs a URI that points to the relevant file in the +/// filesystem. The relative path is appended to LocalIndexRoot to construct the +/// full path and build the final URI. +class Marshaller { +public: + Marshaller() = delete; + Marshaller(llvm::StringRef RemoteIndexRoot, llvm::StringRef LocalIndexRoot); + + clangd::FuzzyFindRequest fromProtobuf(const FuzzyFindRequest *Request); + llvm::Optional fromProtobuf(const Symbol &Message); + llvm::Optional fromProtobuf(const Ref &Message); + + /// toProtobuf() functions serialize native clangd types and strip IndexRoot + /// from the file paths specific to indexing machine. fromProtobuf() functions + /// deserialize clangd types and translate relative paths into machine-native + /// URIs. + LookupRequest toProtobuf(const clangd::LookupRequest &From); + FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From); + RefsRequest toProtobuf(const clangd::RefsRequest &From); + + llvm::Optional toProtobuf(const clangd::Ref &From); + llvm::Optional toProtobuf(const clangd::Symbol &From); -LookupRequest toProtobuf(const clangd::LookupRequest &From); -FuzzyFindRequest toProtobuf(const clangd::FuzzyFindRequest &From, - llvm::StringRef IndexRoot); -RefsRequest toProtobuf(const clangd::RefsRequest &From); + /// Translates \p RelativePath into the absolute path and builds URI for the + /// user machine. This translation happens on the client side with the + /// \p RelativePath received from remote index server and \p IndexRoot is + /// provided by the client. + /// + /// The relative path passed over the wire has unix slashes. + llvm::Optional relativePathToURI(llvm::StringRef RelativePath); + /// Translates a URI from the server's backing index to a relative path + /// suitable to send over the wire to the client. + llvm::Optional uriToRelativePath(llvm::StringRef URI); -Ref toProtobuf(const clangd::Ref &From, llvm::StringRef IndexRoot); -Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot); +private: + clangd::SymbolLocation::Position fromProtobuf(const Position &Message); + Position toProtobuf(const clangd::SymbolLocation::Position &Position); + clang::index::SymbolInfo fromProtobuf(const SymbolInfo &Message); + SymbolInfo toProtobuf(const clang::index::SymbolInfo &Info); + llvm::Optional + fromProtobuf(const SymbolLocation &Message); + llvm::Optional + toProtobuf(const clangd::SymbolLocation &Location); + llvm::Optional + toProtobuf(const clangd::Symbol::IncludeHeaderWithReferences &IncludeHeader); + llvm::Optional + fromProtobuf(const HeaderWithReferences &Message); -/// Translates \p RelativePath into the absolute path and builds URI for the -/// user machine. This translation happens on the client side with the -/// \p RelativePath received from remote index server and \p IndexRoot is -/// provided by the client. -llvm::Optional relativePathToURI(llvm::StringRef RelativePath, - llvm::StringRef IndexRoot); -/// Translates a URI from the server's backing index to a relative path suitable -/// to send over the wire to the client. -llvm::Optional uriToRelativePath(llvm::StringRef URI, - llvm::StringRef IndexRoot); + /// RemoteIndexRoot and LocalIndexRoot are absolute paths to the project (on + /// remote and local machine respectively) and include a trailing slash. One + /// of them can be missing (if the machines are different they don't know each + /// other's specifics and will only do one-way translation), but both can not + /// be missing at the same time. + llvm::Optional RemoteIndexRoot; + llvm::Optional LocalIndexRoot; + llvm::BumpPtrAllocator Arena; + llvm::UniqueStringSaver Strings; +}; } // namespace remote } // namespace clangd diff --git a/clang-tools-extra/clangd/index/remote/server/Server.cpp b/clang-tools-extra/clangd/index/remote/server/Server.cpp index fecd72806cb..07b1c736b67 100644 --- a/clang-tools-extra/clangd/index/remote/server/Server.cpp +++ b/clang-tools-extra/clangd/index/remote/server/Server.cpp @@ -50,7 +50,9 @@ public: : Index(std::move(Index)) { llvm::SmallString<256> NativePath = IndexRoot; llvm::sys::path::native(NativePath); - IndexedProjectRoot = std::string(NativePath); + ProtobufMarshaller = std::unique_ptr(new Marshaller( + /*RemoteIndexRoot=*/llvm::StringRef(NativePath), + /*LocalIndexRoot=*/"")); } private: @@ -65,9 +67,11 @@ private: Req.IDs.insert(*SID); } Index->lookup(Req, [&](const clangd::Symbol &Sym) { + auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym); + if (!SerializedSymbol) + return; LookupReply NextMessage; - *NextMessage.mutable_stream_result() = - toProtobuf(Sym, IndexedProjectRoot); + *NextMessage.mutable_stream_result() = *SerializedSymbol; Reply->Write(NextMessage); }); LookupReply LastMessage; @@ -79,11 +83,13 @@ private: grpc::Status FuzzyFind(grpc::ServerContext *Context, const FuzzyFindRequest *Request, grpc::ServerWriter *Reply) override { - const auto Req = fromProtobuf(Request, IndexedProjectRoot); + const auto Req = ProtobufMarshaller->fromProtobuf(Request); bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol &Sym) { + auto SerializedSymbol = ProtobufMarshaller->toProtobuf(Sym); + if (!SerializedSymbol) + return; FuzzyFindReply NextMessage; - *NextMessage.mutable_stream_result() = - toProtobuf(Sym, IndexedProjectRoot); + *NextMessage.mutable_stream_result() = *SerializedSymbol; Reply->Write(NextMessage); }); FuzzyFindReply LastMessage; @@ -102,8 +108,11 @@ private: Req.IDs.insert(*SID); } bool HasMore = Index->refs(Req, [&](const clangd::Ref &Reference) { + auto SerializedRef = ProtobufMarshaller->toProtobuf(Reference); + if (!SerializedRef) + return; RefsReply NextMessage; - *NextMessage.mutable_stream_result() = toProtobuf(Reference, IndexRoot); + *NextMessage.mutable_stream_result() = *SerializedRef; Reply->Write(NextMessage); }); RefsReply LastMessage; @@ -113,7 +122,7 @@ private: } std::unique_ptr Index; - std::string IndexedProjectRoot; + std::unique_ptr ProtobufMarshaller; }; void runServer(std::unique_ptr Index, diff --git a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp index a14ff13150d..6cc08144bef 100644 --- a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp +++ b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp @@ -13,6 +13,7 @@ #include "index/Serialization.h" #include "index/Symbol.h" #include "index/SymbolID.h" +#include "index/SymbolLocation.h" #include "index/remote/marshalling/Marshalling.h" #include "clang/Index/IndexSymbol.h" #include "llvm/ADT/SmallString.h" @@ -39,29 +40,35 @@ const char *testPathURI(llvm::StringRef Path, TEST(RemoteMarshallingTest, URITranslation) { llvm::BumpPtrAllocator Arena; llvm::UniqueStringSaver Strings(Arena); + Marshaller ProtobufMarshaller( + testPath("remote/machine/projects/llvm-project/"), + testPath("home/my-projects/llvm-project/")); clangd::Ref Original; Original.Location.FileURI = testPathURI("remote/machine/projects/llvm-project/clang-tools-extra/" "clangd/unittests/remote/MarshallingTests.cpp", Strings); - auto Serialized = - toProtobuf(Original, testPath("remote/machine/projects/llvm-project/")); - EXPECT_EQ(Serialized.location().file_path(), + auto Serialized = ProtobufMarshaller.toProtobuf(Original); + EXPECT_TRUE(Serialized); + EXPECT_EQ(Serialized->location().file_path(), "clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp"); - const std::string LocalIndexPrefix = testPath("local/machine/project/"); - auto Deserialized = fromProtobuf(Serialized, &Strings, - testPath("home/my-projects/llvm-project/")); + auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); EXPECT_TRUE(Deserialized); - EXPECT_EQ(Deserialized->Location.FileURI, - testPathURI("home/my-projects/llvm-project/clang-tools-extra/" - "clangd/unittests/remote/MarshallingTests.cpp", - Strings)); + EXPECT_STREQ(Deserialized->Location.FileURI, + testPathURI("home/my-projects/llvm-project/clang-tools-extra/" + "clangd/unittests/remote/MarshallingTests.cpp", + Strings)); + + // Can't have empty paths. + *Serialized->mutable_location()->mutable_file_path() = std::string(); + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + EXPECT_FALSE(Deserialized); clangd::Ref WithInvalidURI; - // Invalid URI results in empty path. + // Invalid URI results in serialization failure. WithInvalidURI.Location.FileURI = "This is not a URI"; - Serialized = toProtobuf(WithInvalidURI, testPath("home/")); - EXPECT_EQ(Serialized.location().file_path(), ""); + Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI); + EXPECT_FALSE(Serialized); // Can not use URIs with scheme different from "file". auto UnittestURI = @@ -69,15 +76,15 @@ TEST(RemoteMarshallingTest, URITranslation) { EXPECT_TRUE(bool(UnittestURI)); WithInvalidURI.Location.FileURI = Strings.save(UnittestURI->toString()).begin(); - Serialized = toProtobuf(WithInvalidURI, testPath("project/lib/")); - EXPECT_EQ(Serialized.location().file_path(), ""); + Serialized = ProtobufMarshaller.toProtobuf(WithInvalidURI); + EXPECT_FALSE(Serialized); + // Paths transmitted over the wire can not be absolute, they have to be + // relative. Ref WithAbsolutePath; *WithAbsolutePath.mutable_location()->mutable_file_path() = "/usr/local/user/home/HelloWorld.cpp"; - Deserialized = fromProtobuf(WithAbsolutePath, &Strings, LocalIndexPrefix); - // Paths transmitted over the wire can not be absolute, they have to be - // relative. + Deserialized = ProtobufMarshaller.fromProtobuf(WithAbsolutePath); EXPECT_FALSE(Deserialized); } @@ -128,48 +135,63 @@ TEST(RemoteMarshallingTest, SymbolSerialization) { Sym.Flags = clangd::Symbol::SymbolFlag::IndexedForCodeCompletion; + Marshaller ProtobufMarshaller(testPath("home/"), testPath("home/")); + // Check that symbols are exactly the same if the path to indexed project is // the same on indexing machine and the client. - auto Serialized = toProtobuf(Sym, testPath("home/")); - auto Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/")); + auto Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_TRUE(Serialized); + auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); EXPECT_TRUE(Deserialized); EXPECT_EQ(toYAML(Sym), toYAML(*Deserialized)); // Serialized paths are relative and have UNIX slashes. - EXPECT_EQ(convert_to_slash(Serialized.definition().file_path(), + EXPECT_EQ(convert_to_slash(Serialized->definition().file_path(), llvm::sys::path::Style::posix), - Serialized.definition().file_path()); + Serialized->definition().file_path()); EXPECT_TRUE( - llvm::sys::path::is_relative(Serialized.definition().file_path())); + llvm::sys::path::is_relative(Serialized->definition().file_path())); + + // Missing definition is OK. + Sym.Definition = clangd::SymbolLocation(); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_TRUE(Serialized); + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + EXPECT_TRUE(Deserialized); + + // Relative path is absolute. + *Serialized->mutable_canonical_declaration()->mutable_file_path() = + convert_to_slash("/path/to/Declaration.h"); + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); + EXPECT_FALSE(Deserialized); // Fail with an invalid URI. Location.FileURI = "Not A URI"; Sym.Definition = Location; - Serialized = toProtobuf(Sym, testPath("home/")); - Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/")); - EXPECT_FALSE(Deserialized); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_FALSE(Serialized); // Schemes other than "file" can not be used. auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest"); EXPECT_TRUE(bool(UnittestURI)); Location.FileURI = Strings.save(UnittestURI->toString()).begin(); Sym.Definition = Location; - Serialized = toProtobuf(Sym, testPath("home/")); - Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/")); - EXPECT_FALSE(Deserialized); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_FALSE(Serialized); // Passing root that is not prefix of the original file path. Location.FileURI = testPathURI("home/File.h", Strings); Sym.Definition = Location; // Check that the symbol is valid and passing the correct path works. - Serialized = toProtobuf(Sym, testPath("home/")); - Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/")); + Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_TRUE(Serialized); + Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); EXPECT_TRUE(Deserialized); - EXPECT_EQ(Deserialized->Definition.FileURI, - testPathURI("home/File.h", Strings)); + EXPECT_STREQ(Deserialized->Definition.FileURI, + testPathURI("home/File.h", Strings)); // Fail with a wrong root. - Serialized = toProtobuf(Sym, testPath("nothome/")); - Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/")); - EXPECT_FALSE(Deserialized); + Marshaller WrongMarshaller(testPath("nothome/"), testPath("home/")); + Serialized = WrongMarshaller.toProtobuf(Sym); + EXPECT_FALSE(Serialized); } TEST(RemoteMarshallingTest, RefSerialization) { @@ -188,9 +210,12 @@ TEST(RemoteMarshallingTest, RefSerialization) { "llvm-project/llvm/clang-tools-extra/clangd/Protocol.h", Strings); Ref.Location = Location; - auto Serialized = toProtobuf(Ref, testPath("llvm-project/")); - auto Deserialized = - fromProtobuf(Serialized, &Strings, testPath("llvm-project/")); + Marshaller ProtobufMarshaller(testPath("llvm-project/"), + testPath("llvm-project/")); + + auto Serialized = ProtobufMarshaller.toProtobuf(Ref); + EXPECT_TRUE(Serialized); + auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); EXPECT_TRUE(Deserialized); EXPECT_EQ(toYAML(Ref), toYAML(*Deserialized)); } @@ -242,10 +267,13 @@ TEST(RemoteMarshallingTest, IncludeHeaderURIs) { InvalidHeaders.end()); Sym.IncludeHeaders = AllHeaders; - auto Serialized = toProtobuf(Sym, convert_to_slash("/")); - EXPECT_EQ(static_cast(Serialized.headers_size()), + Marshaller ProtobufMarshaller(convert_to_slash("/"), convert_to_slash("/")); + + auto Serialized = ProtobufMarshaller.toProtobuf(Sym); + EXPECT_EQ(static_cast(Serialized->headers_size()), ValidHeaders.size()); - auto Deserialized = fromProtobuf(Serialized, &Strings, convert_to_slash("/")); + EXPECT_TRUE(Serialized); + auto Deserialized = ProtobufMarshaller.fromProtobuf(*Serialized); EXPECT_TRUE(Deserialized); Sym.IncludeHeaders = ValidHeaders; @@ -257,35 +285,38 @@ TEST(RemoteMarshallingTest, FuzzyFindRequestSerialization) { Request.ProximityPaths = {testPath("remote/Header.h"), testPath("remote/subdir/OtherHeader.h"), testPath("notremote/File.h"), "Not a Path."}; - auto Serialized = toProtobuf(Request, testPath("remote/")); + Marshaller ProtobufMarshaller(testPath("remote/"), testPath("home/")); + auto Serialized = ProtobufMarshaller.toProtobuf(Request); EXPECT_EQ(Serialized.proximity_paths_size(), 2); - auto Deserialized = fromProtobuf(&Serialized, testPath("home/")); + auto Deserialized = ProtobufMarshaller.fromProtobuf(&Serialized); EXPECT_THAT(Deserialized.ProximityPaths, testing::ElementsAre(testPath("home/Header.h"), testPath("home/subdir/OtherHeader.h"))); } TEST(RemoteMarshallingTest, RelativePathToURITranslation) { - EXPECT_TRUE(relativePathToURI("lib/File.cpp", testPath("home/project/"))); + Marshaller ProtobufMarshaller(/*RemoteIndexRoot=*/"", + /*LocalIndexRoot=*/testPath("home/project/")); + EXPECT_TRUE(ProtobufMarshaller.relativePathToURI("lib/File.cpp")); // RelativePath can not be absolute. - EXPECT_FALSE(relativePathToURI("/lib/File.cpp", testPath("home/project/"))); - // IndexRoot has to be absolute path. - EXPECT_FALSE(relativePathToURI("lib/File.cpp", "home/project/")); + EXPECT_FALSE(ProtobufMarshaller.relativePathToURI("/lib/File.cpp")); + // RelativePath can not be empty. + EXPECT_FALSE(ProtobufMarshaller.relativePathToURI(std::string())); } TEST(RemoteMarshallingTest, URIToRelativePathTranslation) { llvm::BumpPtrAllocator Arena; llvm::UniqueStringSaver Strings(Arena); - EXPECT_TRUE( - uriToRelativePath(testPathURI("home/project/lib/File.cpp", Strings), - testPath("home/project/"))); - // IndexRoot has to be absolute path. - EXPECT_FALSE(uriToRelativePath( - testPathURI("home/project/lib/File.cpp", Strings), "home/project/")); - // IndexRoot has to be be a prefix of the file path. - EXPECT_FALSE( - uriToRelativePath(testPathURI("home/project/lib/File.cpp", Strings), - testPath("home/other/project/"))); + Marshaller ProtobufMarshaller(/*RemoteIndexRoot=*/testPath("remote/project/"), + /*LocalIndexRoot=*/""); + EXPECT_TRUE(ProtobufMarshaller.uriToRelativePath( + testPathURI("remote/project/lib/File.cpp", Strings))); + // RemoteIndexRoot has to be be a prefix of the file path. + Marshaller WrongMarshaller( + /*RemoteIndexRoot=*/testPath("remote/other/project/"), + /*LocalIndexRoot=*/""); + EXPECT_FALSE(WrongMarshaller.uriToRelativePath( + testPathURI("remote/project/lib/File.cpp", Strings))); } } // namespace -- 2.11.0