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 @@ -146,16 +146,16 @@ 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}", + if (!Message.has_info() || !Message.has_canonical_declaration()) { + elog("Cannot convert Symbol from protobuf (missing info, definition or " + "declaration): {0}", 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 {0} given protobuf: {1}", ID.takeError(), Message.ShortDebugString()); return llvm::None; } @@ -163,14 +163,18 @@ 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; + if (Message.has_definition()) { + auto Definition = fromProtobuf(Message.definition(), Strings, IndexRoot); + if (Definition) + Result.Definition = *Definition; + } auto Declaration = fromProtobuf(Message.canonical_declaration(), Strings, IndexRoot); - if (!Declaration) + if (!Declaration) { + elog("Cannot convert Symbol from protobuf (invalid declaration): {0}", + Message.ShortDebugString()); return llvm::None; + } Result.CanonicalDeclaration = *Declaration; Result.References = Message.references(); Result.Origin = static_cast(Message.origin()); @@ -184,6 +188,9 @@ auto SerializedHeader = fromProtobuf(Header, Strings, IndexRoot); if (SerializedHeader) Result.IncludeHeaders.push_back(*SerializedHeader); + else + elog("Cannot convert HeaderWithIncludes from protobuf: {0}", + Header.ShortDebugString()); } Result.Flags = static_cast(Message.flags()); return Result; @@ -193,13 +200,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): {0}", + 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): {0}", + Message.ShortDebugString()); return llvm::None; + } Result.Location = *Location; Result.Kind = static_cast(Message.kind()); return Result; @@ -243,18 +254,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) {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; + 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()); @@ -266,8 +286,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: {0}", + Header.IncludeHeader); continue; + } auto *NextHeader = Result.add_headers(); *NextHeader = *Serialized; } @@ -275,14 +298,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 convert Reference to potobuf (invalid location) {0}: {1}", + From, From.Location); + return llvm::None; + } + *Result.mutable_location() = *Location; return Result; } @@ -292,8 +318,10 @@ 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 (RelativePath.empty()) { + elog("Remote index client got empty path from server"); + return llvm::None; + } if (llvm::sys::path::is_absolute(RelativePath)) { elog("Remote index client got absolute path from server: {0}", RelativePath); @@ -326,7 +354,7 @@ 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\" {0}: {r}", 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, @@ -57,11 +58,16 @@ "clangd/unittests/remote/MarshallingTests.cpp", Strings)); + // Can't have empty paths. + *Serialized->mutable_location()->mutable_file_path() = std::string(); + Deserialized = fromProtobuf(*Serialized, &Strings, LocalIndexPrefix); + 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(), ""); + EXPECT_FALSE(Serialized); // Can not use URIs with scheme different from "file". auto UnittestURI = @@ -70,7 +76,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 +137,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 +166,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 +200,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 +255,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;