diff --git a/clang-tools-extra/clangd/index/remote/Client.cpp b/clang-tools-extra/clangd/index/remote/Client.cpp --- a/clang-tools-extra/clangd/index/remote/Client.cpp +++ b/clang-tools-extra/clangd/index/remote/Client.cpp @@ -62,8 +62,10 @@ } auto Response = fromProtobuf(Reply.stream_result(), &Strings, ProjectRoot); - if (!Response) + if (!Response) { elog("Received invalid {0}", ReplyT::descriptor()->name()); + continue; + } Callback(*Response); } SPAN_ATTACH(Tracer, "status", Reader->Finish().ok()); diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h @@ -57,8 +57,10 @@ llvm::StringRef IndexRoot); RefsRequest toProtobuf(const clangd::RefsRequest &From); -Ref toProtobuf(const clangd::Ref &From, llvm::StringRef IndexRoot); -Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot); +llvm::Optional toProtobuf(const clangd::Ref &From, + llvm::StringRef IndexRoot); +llvm::Optional toProtobuf(const clangd::Symbol &From, + llvm::StringRef IndexRoot); /// Translates \p RelativePath into the absolute path and builds URI for the /// user machine. This translation happens on the client side with the diff --git a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp --- a/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp +++ b/clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp @@ -148,14 +148,15 @@ llvm::StringRef IndexRoot) { if (!Message.has_info() || !Message.has_definition() || !Message.has_canonical_declaration()) { - elog("Cannot convert Symbol from Protobuf: {0}", + elog("Cannot convert Symbol from Protobuf (missing info, definition or " + "declaration): {1}", Message.ShortDebugString()); return llvm::None; } clangd::Symbol Result; auto ID = SymbolID::fromStr(Message.id()); if (!ID) { - elog("Cannot parse SymbolID {0} given Protobuf: {1}", ID.takeError(), + elog("Cannot parse SymbolID {1} given Protobuf: {2}", ID.takeError(), Message.ShortDebugString()); return llvm::None; } @@ -164,13 +165,19 @@ Result.Name = Message.name(); Result.Scope = Message.scope(); auto Definition = fromProtobuf(Message.definition(), Strings, IndexRoot); - if (!Definition) + if (!Definition) { + elog("Cannot convert Symbol from Protobuf (invalid definition): {1}", + Message.ShortDebugString()); return llvm::None; + } Result.Definition = *Definition; auto Declaration = fromProtobuf(Message.canonical_declaration(), Strings, IndexRoot); - if (!Declaration) + if (!Declaration) { + elog("Cannot convert Symbol from Protobuf (invalid declaration): {1}", + Message.ShortDebugString()); return llvm::None; + } Result.CanonicalDeclaration = *Declaration; Result.References = Message.references(); Result.Origin = static_cast(Message.origin()); @@ -184,6 +191,9 @@ auto SerializedHeader = fromProtobuf(Header, Strings, IndexRoot); if (SerializedHeader) Result.IncludeHeaders.push_back(*SerializedHeader); + else + elog("Cannot convert HeaderWithIncludes from Protobuf: {1}", + Header.ShortDebugString()); } Result.Flags = static_cast(Message.flags()); return Result; @@ -193,13 +203,17 @@ llvm::UniqueStringSaver *Strings, llvm::StringRef IndexRoot) { if (!Message.has_location()) { - elog("Cannot convert Ref from Protobuf: {}", Message.ShortDebugString()); + elog("Cannot convert Ref from Protobuf (missing location): {1}", + Message.ShortDebugString()); return llvm::None; } clangd::Ref Result; auto Location = fromProtobuf(Message.location(), Strings, IndexRoot); - if (!Location) + if (!Location) { + elog("Cannot convert Ref from Protobuf (invalid location): {1}", + Message.ShortDebugString()); return llvm::None; + } Result.Location = *Location; Result.Kind = static_cast(Message.kind()); return Result; @@ -243,18 +257,27 @@ return RPCRequest; } -Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot) { +llvm::Optional toProtobuf(const clangd::Symbol &From, + llvm::StringRef IndexRoot) { 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) - *Result.mutable_definition() = *Definition; + if (!Definition) { + elog("Can not convert Symbol to Protobuf (invalid definition) {1}: {2}", + 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; + if (!Declaration) { + elog("Can not convert Symbol to Protobuf (invalid declaration) {1}: {2}", + 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()); @@ -266,8 +289,11 @@ Result.set_type(From.Type.str()); for (const auto &Header : From.IncludeHeaders) { auto Serialized = toProtobuf(Header, IndexRoot); - if (!Serialized) + if (!Serialized) { + elog("Can not convert IncludeHeaderWithReferences to Protobuf: {1}", + Header.IncludeHeader); continue; + } auto *NextHeader = Result.add_headers(); *NextHeader = *Serialized; } @@ -275,14 +301,17 @@ 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 toProtobuf(const clangd::Ref &From, + llvm::StringRef IndexRoot) { Ref Result; Result.set_kind(static_cast(From.Kind)); auto Location = toProtobuf(From.Location, IndexRoot); - if (Location) - *Result.mutable_location() = *Location; + if (!Location) { + elog("Can not convet Reference to Potobuf (invalid location) {1}: {2}", + From, From.Location); + return llvm::None; + } + *Result.mutable_location() = *Location; return Result; } @@ -295,12 +324,12 @@ if (RelativePath.empty()) return std::string(); if (llvm::sys::path::is_absolute(RelativePath)) { - elog("Remote index client got absolute path from server: {0}", + elog("Remote index client got absolute path from server: {1}", RelativePath); return llvm::None; } if (llvm::sys::path::is_relative(IndexRoot)) { - elog("Remote index client got a relative path as index root: {0}", + elog("Remote index client got a relative path as index root: {1}", IndexRoot); return llvm::None; } @@ -316,17 +345,17 @@ 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); + elog("Index root {1} is not absolute path", IndexRoot); return llvm::None; } auto ParsedURI = URI::parse(URI); if (!ParsedURI) { - elog("Remote index got bad URI from client {0}: {1}", URI, + elog("Remote index got bad URI from client {1}: {2}", URI, ParsedURI.takeError()); return llvm::None; } if (ParsedURI->scheme() != "file") { - elog("Remote index got URI with scheme other than \"file\" {0}: {1}", URI, + elog("Remote index got URI with scheme other than \"file\" {1}: {2}", URI, ParsedURI->scheme()); return llvm::None; } diff --git a/clang-tools-extra/clangd/index/remote/server/Server.cpp b/clang-tools-extra/clangd/index/remote/server/Server.cpp --- a/clang-tools-extra/clangd/index/remote/server/Server.cpp +++ b/clang-tools-extra/clangd/index/remote/server/Server.cpp @@ -65,9 +65,11 @@ Req.IDs.insert(*SID); } Index->lookup(Req, [&](const clangd::Symbol &Sym) { + auto SerializedSymbol = toProtobuf(Sym, IndexedProjectRoot); + if (!SerializedSymbol) + return; LookupReply NextMessage; - *NextMessage.mutable_stream_result() = - toProtobuf(Sym, IndexedProjectRoot); + *NextMessage.mutable_stream_result() = *SerializedSymbol; Reply->Write(NextMessage); }); LookupReply LastMessage; @@ -81,9 +83,11 @@ grpc::ServerWriter *Reply) override { const auto Req = fromProtobuf(Request, IndexedProjectRoot); bool HasMore = Index->fuzzyFind(Req, [&](const clangd::Symbol &Sym) { + auto SerializedSymbol = toProtobuf(Sym, IndexedProjectRoot); + if (!SerializedSymbol) + return; FuzzyFindReply NextMessage; - *NextMessage.mutable_stream_result() = - toProtobuf(Sym, IndexedProjectRoot); + *NextMessage.mutable_stream_result() = *SerializedSymbol; Reply->Write(NextMessage); }); FuzzyFindReply LastMessage; @@ -102,8 +106,11 @@ Req.IDs.insert(*SID); } bool HasMore = Index->refs(Req, [&](const clangd::Ref &Reference) { + auto SerializedRef = toProtobuf(Reference, IndexedProjectRoot); + if (!SerializedRef) + return; RefsReply NextMessage; - *NextMessage.mutable_stream_result() = toProtobuf(Reference, IndexRoot); + *NextMessage.mutable_stream_result() = *SerializedRef; Reply->Write(NextMessage); }); RefsReply LastMessage; diff --git a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp --- a/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp +++ b/clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp @@ -46,10 +46,11 @@ Strings); auto Serialized = toProtobuf(Original, testPath("remote/machine/projects/llvm-project/")); - EXPECT_EQ(Serialized.location().file_path(), + 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, + auto Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/my-projects/llvm-project/")); EXPECT_TRUE(Deserialized); EXPECT_EQ(Deserialized->Location.FileURI, @@ -58,10 +59,10 @@ Strings)); 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(), ""); + EXPECT_FALSE(Serialized); // Can not use URIs with scheme different from "file". auto UnittestURI = @@ -70,7 +71,7 @@ WithInvalidURI.Location.FileURI = Strings.save(UnittestURI->toString()).begin(); Serialized = toProtobuf(WithInvalidURI, testPath("project/lib/")); - EXPECT_EQ(Serialized.location().file_path(), ""); + EXPECT_FALSE(Serialized); Ref WithAbsolutePath; *WithAbsolutePath.mutable_location()->mutable_file_path() = @@ -131,22 +132,28 @@ // 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/")); + EXPECT_TRUE(Serialized); + auto Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/")); 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())); + + // Relative path is absolute. + *Serialized->mutable_canonical_declaration()->mutable_file_path() = + convert_to_slash("/path/to/Declaration.h"); + Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/")); + 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); + EXPECT_FALSE(Serialized); // Schemes other than "file" can not be used. auto UnittestURI = URI::create(testPath("home/SomePath.h"), "unittest"); @@ -154,22 +161,21 @@ Location.FileURI = Strings.save(UnittestURI->toString()).begin(); Sym.Definition = Location; Serialized = toProtobuf(Sym, testPath("home/")); - Deserialized = fromProtobuf(Serialized, &Strings, testPath("home/")); - EXPECT_FALSE(Deserialized); + 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/")); + EXPECT_TRUE(Serialized); + Deserialized = fromProtobuf(*Serialized, &Strings, testPath("home/")); EXPECT_TRUE(Deserialized); EXPECT_EQ(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); + EXPECT_FALSE(Serialized); } TEST(RemoteMarshallingTest, RefSerialization) { @@ -189,8 +195,9 @@ Ref.Location = Location; auto Serialized = toProtobuf(Ref, testPath("llvm-project/")); + EXPECT_TRUE(Serialized); auto Deserialized = - fromProtobuf(Serialized, &Strings, testPath("llvm-project/")); + fromProtobuf(*Serialized, &Strings, testPath("llvm-project/")); EXPECT_TRUE(Deserialized); EXPECT_EQ(toYAML(Ref), toYAML(*Deserialized)); } @@ -243,9 +250,11 @@ Sym.IncludeHeaders = AllHeaders; auto Serialized = toProtobuf(Sym, convert_to_slash("/")); - EXPECT_EQ(static_cast(Serialized.headers_size()), + EXPECT_EQ(static_cast(Serialized->headers_size()), ValidHeaders.size()); - auto Deserialized = fromProtobuf(Serialized, &Strings, convert_to_slash("/")); + EXPECT_TRUE(Serialized); + auto Deserialized = + fromProtobuf(*Serialized, &Strings, convert_to_slash("/")); EXPECT_TRUE(Deserialized); Sym.IncludeHeaders = ValidHeaders;