simark created this revision.
Herald added subscribers: cfe-commits, jkorous, MaskRay, ioeric, ilya-biryukov.
When compile_commands.json contains some source files expressed as
relative paths, we can get duplicate responses to findDefinitions. The
responses only differ by the URI, which are different versions of the
same file:
"result": [
{
...
"uri":
"file:///home/emaisin/src/ls-interact/cpp-test/build/../src/first.h"
},
{
...
"uri": "file:///home/emaisin/src/ls-interact/cpp-test/src/first.h"
}
]
In getAbsoluteFilePath, we try to obtain the realpath of the FileEntry
by calling tryGetRealPathName. However, this doesn't work for
FileEntries that don't actually open the file, because their
RealPathName field is never populated.
I changed getAbsoluteFilePath so that if tryGetRealPathName returns an
empty string, we try to find the real path using VFS.getRealPath. This
requires to pass the VFS all the way from ClangdServer.
I tried to write a test case similar to
TEST(GoToDefinition, RelPathsInCompileCommand)
that would trigger this issue, but I wasn't able to. No matter what I
do, the paths are returned by the FileManager as already normalized /
without the "..". There must be something different in the test case
than in real execution that I don't see.
Repro instructions are in bug #37963, though I can include them in the
commit message if you prefer.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48687
Files:
clangd/ClangdServer.cpp
clangd/XRefs.cpp
clangd/XRefs.h
unittests/clangd/TestTU.cpp
unittests/clangd/TestTU.h
unittests/clangd/XRefsTests.cpp
Index: unittests/clangd/XRefsTests.cpp
===================================================================
--- unittests/clangd/XRefsTests.cpp
+++ unittests/clangd/XRefsTests.cpp
@@ -114,8 +114,9 @@
TU.HeaderCode = SymbolHeader.code();
auto Index = TU.index();
auto runFindDefinitionsWithIndex = [&Index](const Annotations &Main) {
- auto AST = TestTU::withCode(Main.code()).build();
- return clangd::findDefinitions(AST, Main.point(), Index.get());
+ IntrusiveRefCntPtr<vfs::FileSystem> FS;
+ auto AST = TestTU::withCode(Main.code()).build(&FS);
+ return clangd::findDefinitions(AST, Main.point(), *FS, Index.get());
};
Annotations Test(R"cpp(// only declaration in AST.
@@ -301,11 +302,12 @@
};
for (const char *Test : Tests) {
Annotations T(Test);
- auto AST = TestTU::withCode(T.code()).build();
+ IntrusiveRefCntPtr<vfs::FileSystem> FS;
+ auto AST = TestTU::withCode(T.code()).build(&FS);
std::vector<Matcher<Location>> ExpectedLocations;
for (const auto &R : T.ranges())
ExpectedLocations.push_back(RangeIs(R));
- EXPECT_THAT(findDefinitions(AST, T.point()),
+ EXPECT_THAT(findDefinitions(AST, T.point(), *FS),
ElementsAreArray(ExpectedLocations))
<< Test;
}
Index: unittests/clangd/TestTU.h
===================================================================
--- unittests/clangd/TestTU.h
+++ unittests/clangd/TestTU.h
@@ -44,7 +44,7 @@
std::string HeaderCode;
std::string HeaderFilename = "TestTU.h";
- ParsedAST build() const;
+ ParsedAST build(IntrusiveRefCntPtr<vfs::FileSystem> *OutFS = nullptr) const;
SymbolSlab headerSymbols() const;
std::unique_ptr<SymbolIndex> index() const;
};
Index: unittests/clangd/TestTU.cpp
===================================================================
--- unittests/clangd/TestTU.cpp
+++ unittests/clangd/TestTU.cpp
@@ -19,7 +19,7 @@
namespace clangd {
using namespace llvm;
-ParsedAST TestTU::build() const {
+ParsedAST TestTU::build(IntrusiveRefCntPtr<vfs::FileSystem> *OutFS) const {
std::string FullFilename = testPath(Filename),
FullHeaderName = testPath(HeaderFilename);
std::vector<const char *> Cmd = {"clang", FullFilename.c_str()};
@@ -29,11 +29,12 @@
Cmd.push_back("-include");
Cmd.push_back(FullHeaderName.c_str());
}
- auto AST = ParsedAST::Build(
- createInvocationFromCommandLine(Cmd), nullptr,
- MemoryBuffer::getMemBufferCopy(Code),
- std::make_shared<PCHContainerOperations>(),
- buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}}));
+ auto FS = buildTestFS({{FullFilename, Code}, {FullHeaderName, HeaderCode}});
+ if (OutFS)
+ *OutFS = FS;
+ auto AST = ParsedAST::Build(createInvocationFromCommandLine(Cmd), nullptr,
+ MemoryBuffer::getMemBufferCopy(Code),
+ std::make_shared<PCHContainerOperations>(), FS);
if (!AST.hasValue()) {
ADD_FAILURE() << "Failed to build code:\n" << Code;
llvm_unreachable("Failed to build TestTU!");
Index: clangd/XRefs.h
===================================================================
--- clangd/XRefs.h
+++ clangd/XRefs.h
@@ -24,6 +24,7 @@
/// Get definition of symbol at a specified \p Pos.
std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+ const vfs::FileSystem &VFS,
const SymbolIndex *Index = nullptr);
/// Returns highlights for all usages of a symbol at \p Pos.
Index: clangd/XRefs.cpp
===================================================================
--- clangd/XRefs.cpp
+++ clangd/XRefs.cpp
@@ -174,11 +174,17 @@
return {DeclMacrosFinder.takeDecls(), DeclMacrosFinder.takeMacroInfos()};
}
-llvm::Optional<std::string>
-getAbsoluteFilePath(const FileEntry *F, const SourceManager &SourceMgr) {
+llvm::Optional<std::string> getAbsoluteFilePath(const FileEntry *F,
+ const SourceManager &SourceMgr,
+ const vfs::FileSystem &VFS) {
SmallString<64> FilePath = F->tryGetRealPathName();
- if (FilePath.empty())
- FilePath = F->getName();
+
+ if (FilePath.empty()) {
+ if (VFS.getRealPath(F->getName(), FilePath) != std::error_code()) {
+ FilePath = F->getName();
+ }
+ }
+
if (!llvm::sys::path::is_absolute(FilePath)) {
if (!SourceMgr.getFileManager().makeAbsolutePath(FilePath)) {
log("Could not turn relative path to absolute: " + FilePath);
@@ -188,8 +194,9 @@
return FilePath.str().str();
}
-llvm::Optional<Location>
-makeLocation(ParsedAST &AST, const SourceRange &ValSourceRange) {
+llvm::Optional<Location> makeLocation(ParsedAST &AST,
+ const SourceRange &ValSourceRange,
+ const vfs::FileSystem &VFS) {
const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
const LangOptions &LangOpts = AST.getASTContext().getLangOpts();
SourceLocation LocStart = ValSourceRange.getBegin();
@@ -205,7 +212,7 @@
Range R = {Begin, End};
Location L;
- auto FilePath = getAbsoluteFilePath(F, SourceMgr);
+ auto FilePath = getAbsoluteFilePath(F, SourceMgr, VFS);
if (!FilePath) {
log("failed to get path!");
return llvm::None;
@@ -227,6 +234,7 @@
} // namespace
std::vector<Location> findDefinitions(ParsedAST &AST, Position Pos,
+ const vfs::FileSystem &VFS,
const SymbolIndex *Index) {
const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
SourceLocation SourceLocationBeg =
@@ -247,7 +255,7 @@
for (auto Item : Symbols.Macros) {
auto Loc = Item.Info->getDefinitionLoc();
- auto L = makeLocation(AST, SourceRange(Loc, Loc));
+ auto L = makeLocation(AST, SourceRange(Loc, Loc), VFS);
if (L)
Result.push_back(*L);
}
@@ -290,7 +298,7 @@
auto &Candidate = ResultCandidates[Key];
auto Loc = findNameLoc(D);
- auto L = makeLocation(AST, SourceRange(Loc, Loc));
+ auto L = makeLocation(AST, SourceRange(Loc, Loc), VFS);
// The declaration in the identified symbols is a definition if possible
// otherwise it is declaration.
bool IsDef = GetDefinition(D) == D;
@@ -309,7 +317,7 @@
std::string HintPath;
const FileEntry *FE =
SourceMgr.getFileEntryForID(SourceMgr.getMainFileID());
- if (auto Path = getAbsoluteFilePath(FE, SourceMgr))
+ if (auto Path = getAbsoluteFilePath(FE, SourceMgr, VFS))
HintPath = *Path;
// Query the index and populate the empty slot.
Index->lookup(
Index: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ clangd/ClangdServer.cpp
@@ -305,7 +305,8 @@
llvm::Expected<InputsAndAST> InpAST) {
if (!InpAST)
return CB(InpAST.takeError());
- CB(clangd::findDefinitions(InpAST->AST, Pos, Index));
+ CB(clangd::findDefinitions(InpAST->AST, Pos, *FSProvider.getFileSystem(),
+ Index));
};
WorkScheduler.runWithAST("Definitions", File, Bind(Action, std::move(CB)));
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
