kbobyrev updated this revision to Diff 278008.
kbobyrev added a comment.
Also do not attempt to use callback on unserialized messages.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83826/new/
https://reviews.llvm.org/D83826
Files:
clang-tools-extra/clangd/index/remote/Client.cpp
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
clang-tools-extra/clangd/index/remote/server/Server.cpp
clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
Index: clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/remote/MarshallingTests.cpp
+++ 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<size_t>(Serialized.headers_size()),
+ EXPECT_EQ(static_cast<size_t>(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;
Index: clang-tools-extra/clangd/index/remote/server/Server.cpp
===================================================================
--- clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ 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<FuzzyFindReply> *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;
Index: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
===================================================================
--- clang-tools-extra/clangd/index/remote/marshalling/Marshalling.h
+++ 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<Ref> toProtobuf(const clangd::Ref &From,
+ llvm::StringRef IndexRoot);
+llvm::Optional<Symbol> 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
Index: clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
===================================================================
--- clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
+++ clang-tools-extra/clangd/index/remote/marshalling/Marshalling.cpp
@@ -155,7 +155,7 @@
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 +164,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: {0}",
+ 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: {0}",
+ Message.ShortDebugString());
return llvm::None;
+ }
Result.CanonicalDeclaration = *Declaration;
Result.References = Message.references();
Result.Origin = static_cast<clangd::SymbolOrigin>(Message.origin());
@@ -184,6 +190,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<clangd::Symbol::SymbolFlag>(Message.flags());
return Result;
@@ -198,8 +207,11 @@
}
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<clangd::RefKind>(Message.kind());
return Result;
@@ -243,18 +255,27 @@
return RPCRequest;
}
-Symbol toProtobuf(const clangd::Symbol &From, llvm::StringRef IndexRoot) {
+llvm::Optional<Symbol> 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}", 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}", From,
+ From.CanonicalDeclaration);
+ return llvm::None;
+ }
+ *Result.mutable_canonical_declaration() = *Declaration;
Result.set_references(From.References);
Result.set_origin(static_cast<uint32_t>(From.Origin));
Result.set_signature(From.Signature.str());
@@ -266,8 +287,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 +299,16 @@
return Result;
}
-// FIXME(kirillbobyrev): A reference without location is invalid.
-// llvm::Optional<Ref> here and on the server side?
-Ref toProtobuf(const clangd::Ref &From, llvm::StringRef IndexRoot) {
+llvm::Optional<Ref> toProtobuf(const clangd::Ref &From,
+ llvm::StringRef IndexRoot) {
Ref Result;
Result.set_kind(static_cast<uint32_t>(From.Kind));
auto Location = toProtobuf(From.Location, IndexRoot);
- if (Location)
- *Result.mutable_location() = *Location;
+ if (!Location) {
+ elog("Can not convet Reference from Potobuf - invalid location: {0}", From);
+ return llvm::None;
+ }
+ *Result.mutable_location() = *Location;
return Result;
}
Index: clang-tools-extra/clangd/index/remote/Client.cpp
===================================================================
--- clang-tools-extra/clangd/index/remote/Client.cpp
+++ 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());
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
