kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay,
ilya-biryukov.
Herald added a project: clang.
Clangd is returning current working directory for overriden commands.
This can cause inconsistencies between:
- header and the main files, as OverlayCDB only contains entries for the main
files it direct any queries for the headers to the base, creating a discrepancy
between the two.
- different clangd instances, as the results will be different depending on the
timing of execution of the query and override of the command. hence clangd
might see two different project infos for the same file between different
invocations.
- editors and the way user has invoked it, as current working directory of
clangd will depend on those, hence even when there's no underlying base CWD
might change depending on the editor, or the directory user has started the
editor in.
This patch gets rid of that discrepency by always directing queries to base or
returning llvm::None in absence of it.
For a sample bug see https://reviews.llvm.org/D83099#2154185.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D83934
Files:
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/clangd/GlobalCompilationDatabase.h
clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -313,9 +313,22 @@
llvm::sys::path::append(File, "blabla", "..", "a.cc");
EXPECT_TRUE(DB.getCompileCommand(File));
- EXPECT_TRUE(DB.getProjectInfo(File));
+ EXPECT_FALSE(DB.getProjectInfo(File));
}
+TEST_F(OverlayCDBTest, GetProjectInfo) {
+ OverlayCDB DB(Base.get());
+ Path File = testPath("foo.cc");
+ Path Header = testPath("foo.h");
+
+ EXPECT_EQ(DB.getProjectInfo(File)->SourceRoot, testRoot());
+ EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
+
+ // Shouldn't change after an override.
+ DB.setCompileCommand(File, tooling::CompileCommand());
+ EXPECT_EQ(DB.getProjectInfo(File)->SourceRoot, testRoot());
+ EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.h
===================================================================
--- clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -119,7 +119,6 @@
getQueryDriverDatabase(llvm::ArrayRef<std::string> QueryDriverGlobs,
std::unique_ptr<GlobalCompilationDatabase> Base);
-
/// Wraps another compilation database, and supports overriding the commands
/// using an in-memory mapping.
class OverlayCDB : public GlobalCompilationDatabase {
@@ -134,6 +133,10 @@
llvm::Optional<tooling::CompileCommand>
getCompileCommand(PathRef File) const override;
tooling::CompileCommand getFallbackCommand(PathRef File) const override;
+ /// Note that project info is gathered purely from the inner compilation
+ /// database. This is to ensure consistency, as a translation unit with an
+ /// overriden command might yield a different project compared to headers
+ /// included in it (or before and after the override arrives).
llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override;
/// Sets or clears the compilation command for a particular file.
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===================================================================
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -298,15 +298,8 @@
}
llvm::Optional<ProjectInfo> OverlayCDB::getProjectInfo(PathRef File) const {
- {
- std::lock_guard<std::mutex> Lock(Mutex);
- auto It = Commands.find(removeDots(File));
- if (It != Commands.end())
- return ProjectInfo{};
- }
if (Base)
return Base->getProjectInfo(File);
-
return llvm::None;
}
} // namespace clangd
Index: clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
+++ clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp
@@ -313,9 +313,22 @@
llvm::sys::path::append(File, "blabla", "..", "a.cc");
EXPECT_TRUE(DB.getCompileCommand(File));
- EXPECT_TRUE(DB.getProjectInfo(File));
+ EXPECT_FALSE(DB.getProjectInfo(File));
}
+TEST_F(OverlayCDBTest, GetProjectInfo) {
+ OverlayCDB DB(Base.get());
+ Path File = testPath("foo.cc");
+ Path Header = testPath("foo.h");
+
+ EXPECT_EQ(DB.getProjectInfo(File)->SourceRoot, testRoot());
+ EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
+
+ // Shouldn't change after an override.
+ DB.setCompileCommand(File, tooling::CompileCommand());
+ EXPECT_EQ(DB.getProjectInfo(File)->SourceRoot, testRoot());
+ EXPECT_EQ(DB.getProjectInfo(Header)->SourceRoot, testRoot());
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.h
===================================================================
--- clang-tools-extra/clangd/GlobalCompilationDatabase.h
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.h
@@ -119,7 +119,6 @@
getQueryDriverDatabase(llvm::ArrayRef<std::string> QueryDriverGlobs,
std::unique_ptr<GlobalCompilationDatabase> Base);
-
/// Wraps another compilation database, and supports overriding the commands
/// using an in-memory mapping.
class OverlayCDB : public GlobalCompilationDatabase {
@@ -134,6 +133,10 @@
llvm::Optional<tooling::CompileCommand>
getCompileCommand(PathRef File) const override;
tooling::CompileCommand getFallbackCommand(PathRef File) const override;
+ /// Note that project info is gathered purely from the inner compilation
+ /// database. This is to ensure consistency, as a translation unit with an
+ /// overriden command might yield a different project compared to headers
+ /// included in it (or before and after the override arrives).
llvm::Optional<ProjectInfo> getProjectInfo(PathRef File) const override;
/// Sets or clears the compilation command for a particular file.
Index: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
===================================================================
--- clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
+++ clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
@@ -298,15 +298,8 @@
}
llvm::Optional<ProjectInfo> OverlayCDB::getProjectInfo(PathRef File) const {
- {
- std::lock_guard<std::mutex> Lock(Mutex);
- auto It = Commands.find(removeDots(File));
- if (It != Commands.end())
- return ProjectInfo{};
- }
if (Base)
return Base->getProjectInfo(File);
-
return llvm::None;
}
} // namespace clangd
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
