kbobyrev created this revision.
kbobyrev added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous,
MaskRay, ilya-biryukov.
Herald added a project: clang.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D83826
Files:
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;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
