[clang-tools-extra] r365123 - [clangd] Make HadErrors part of background index's internal state
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 4 06:44:47 PDT 2019
Hi Kadir,
Your change is causing a build failure on our internal linux build bot running gcc 5.4:
FAILED: CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/lib/ccache/g++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/extra/clangd -I/home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd -I/home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/include -Itools/clang/include -Iinclude -I/home/siadmin/jenkins/w/opensource/opensource_build/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wno-maybe-uninitialized -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -O3 -UNDEBUG -fno-exceptions -fno-rtti -MMD -MT tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o -MF tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o.d -o tools/clang/tools/extra/clangd/CMakeFiles/obj.clangDaemon.dir/ClangdLSPServer.cpp.o -c /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.cpp
In file included from /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdServer.h:24:0,
from /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.h:12,
from /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.cpp:9:
/home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/index/Background.h:99:24: error: array must be initialized with a brace-enclosed initializer
FileDigest Digest{0};
^
Can you please take a look?
Douglas Yung
-----Original Message-----
From: cfe-commits <cfe-commits-bounces at lists.llvm.org> On Behalf Of Kadir Cetinkaya via cfe-commits
Sent: Thursday, July 4, 2019 2:52
To: cfe-commits at lists.llvm.org
Subject: [clang-tools-extra] r365123 - [clangd] Make HadErrors part of background index's internal state
Author: kadircet
Date: Thu Jul 4 02:52:12 2019
New Revision: 365123
URL: http://llvm.org/viewvc/llvm-project?rev=365123&view=rev
Log:
[clangd] Make HadErrors part of background index's internal state
Reviewers: sammccall
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D64147
Modified:
clang-tools-extra/trunk/clangd/index/Background.cpp
clang-tools-extra/trunk/clangd/index/Background.h
clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
Modified: clang-tools-extra/trunk/clangd/index/Background.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=365123&r1=365122&r2=365123&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Jul 4
+++ 02:52:12 2019
@@ -101,28 +101,6 @@ IncludeGraph getSubGraph(const URI &U, c
return IG;
}
-// Creates a filter to not collect index results from files with unchanged -// digests.
-// \p FileDigests contains file digests for the current indexed files.
-decltype(SymbolCollector::Options::FileFilter)
-createFileFilter(const llvm::StringMap<FileDigest> &FileDigests) {
- return [&FileDigests](const SourceManager &SM, FileID FID) {
- const auto *F = SM.getFileEntryForID(FID);
- if (!F)
- return false; // Skip invalid files.
- auto AbsPath = getCanonicalPath(F, SM);
- if (!AbsPath)
- return false; // Skip files without absolute path.
- auto Digest = digestFile(SM, FID);
- if (!Digest)
- return false;
- auto D = FileDigests.find(*AbsPath);
- if (D != FileDigests.end() && D->second == Digest)
- return false; // Skip files that haven't changed.
- return true;
- };
-}
-
// We cannot use vfs->makeAbsolute because Cmd.FileName is either absolute or // relative to Cmd.Directory, which might not be the same as current working // directory.
@@ -274,12 +252,12 @@ void BackgroundIndex::enqueueTask(Task T }
/// Given index results from a TU, only update symbols coming from files that -/// are different or missing from than \p DigestsSnapshot. Also stores new index -/// information on IndexStorage.
-void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,
- const llvm::StringMap<FileDigest> &DigestsSnapshot,
- BackgroundIndexStorage *IndexStorage,
- bool HadErrors) {
+/// are different or missing from than \p ShardVersionsSnapshot. Also
+stores new /// index information on IndexStorage.
+void BackgroundIndex::update(
+ llvm::StringRef MainFile, IndexFileIn Index,
+ const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,
+ BackgroundIndexStorage *IndexStorage, bool HadErrors) {
// Partition symbols/references into files.
struct File {
llvm::DenseSet<const Symbol *> Symbols; @@ -291,18 +269,14 @@ void BackgroundIndex::update(llvm::Strin
URIToFileCache URICache(MainFile);
for (const auto &IndexIt : *Index.Sources) {
const auto &IGN = IndexIt.getValue();
- // In case of failures, we only store main file of TU. That way we can still
- // get symbols from headers if some other TU can compile them. Note that
- // sources do not contain any information regarding missing headers, since
- // we don't even know what absolute path they should fall in.
- // FIXME: Also store contents from other files whenever the current contents
- // for those files are missing or if they had errors before.
- if (HadErrors && !(IGN.Flags & IncludeGraphNode::SourceFlag::IsTU))
- continue;
+ // Note that sources do not contain any information regarding missing
+ // headers, since we don't even know what absolute path they should fall in.
const auto AbsPath = URICache.resolve(IGN.URI);
- const auto DigestIt = DigestsSnapshot.find(AbsPath);
- // File has different contents.
- if (DigestIt == DigestsSnapshot.end() || DigestIt->getValue() != IGN.Digest)
+ const auto DigestIt = ShardVersionsSnapshot.find(AbsPath);
+ // File has different contents, or indexing was successfull this time.
+ if (DigestIt == ShardVersionsSnapshot.end() ||
+ DigestIt->getValue().Digest != IGN.Digest ||
+ (DigestIt->getValue().HadErrors && !HadErrors))
Files.try_emplace(AbsPath).first->getValue().Digest = IGN.Digest;
}
// This map is used to figure out where to store relations.
@@ -385,13 +359,17 @@ void BackgroundIndex::update(llvm::Strin
}
{
- std::lock_guard<std::mutex> Lock(DigestsMu);
+ std::lock_guard<std::mutex> Lock(ShardVersionsMu);
auto Hash = FileIt.second.Digest;
- // Skip if file is already up to date.
- auto DigestIt = IndexedFileDigests.try_emplace(Path);
- if (!DigestIt.second && DigestIt.first->second == Hash)
+ auto DigestIt = ShardVersions.try_emplace(Path);
+ ShardVersion &SV = DigestIt.first->second;
+ // Skip if file is already up to date, unless previous index was broken
+ // and this one is not.
+ if (!DigestIt.second && SV.Digest == Hash && SV.HadErrors &&
+ !HadErrors)
continue;
- DigestIt.first->second = Hash;
+ SV.Digest = Hash;
+ SV.HadErrors = HadErrors;
+
// This can override a newer version that is added in another thread, if
// this thread sees the older version but finishes later. This should be
// rare in practice.
@@ -439,11 +417,11 @@ llvm::Error BackgroundIndex::index(tooli
return llvm::errorCodeToError(Buf.getError());
auto Hash = digest(Buf->get()->getBuffer());
- // Take a snapshot of the digests to avoid locking for each file in the TU.
- llvm::StringMap<FileDigest> DigestsSnapshot;
+ // Take a snapshot of the versions to avoid locking for each file in the TU.
+ llvm::StringMap<ShardVersion> ShardVersionsSnapshot;
{
- std::lock_guard<std::mutex> Lock(DigestsMu);
- DigestsSnapshot = IndexedFileDigests;
+ std::lock_guard<std::mutex> Lock(ShardVersionsMu);
+ ShardVersionsSnapshot = ShardVersions;
}
vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash)); @@ -463,7 +441,26 @@ llvm::Error BackgroundIndex::index(tooli
"Couldn't build compiler instance");
SymbolCollector::Options IndexOpts;
- IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);
+ // Creates a filter to not collect index results from files with
+ unchanged // digests.
+ IndexOpts.FileFilter = [&ShardVersionsSnapshot](const SourceManager &SM,
+ FileID FID) {
+ const auto *F = SM.getFileEntryForID(FID);
+ if (!F)
+ return false; // Skip invalid files.
+ auto AbsPath = getCanonicalPath(F, SM);
+ if (!AbsPath)
+ return false; // Skip files without absolute path.
+ auto Digest = digestFile(SM, FID);
+ if (!Digest)
+ return false;
+ auto D = ShardVersionsSnapshot.find(*AbsPath);
+ if (D != ShardVersionsSnapshot.end() && D->second.Digest == Digest &&
+ !D->second.HadErrors)
+ return false; // Skip files that haven't changed, without errors.
+ return true;
+ };
+
IndexFileIn Index;
auto Action = createStaticIndexingAction(
IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); }, @@ -503,7 +500,7 @@ llvm::Error BackgroundIndex::index(tooli
for (auto &It : *Index.Sources)
It.second.Flags |= IncludeGraphNode::SourceFlag::HadErrors;
}
- update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage,
+ update(AbsolutePath, std::move(Index), ShardVersionsSnapshot,
+ IndexStorage,
HadErrors);
if (BuildIndexPeriodMs > 0)
@@ -524,6 +521,7 @@ BackgroundIndex::loadShard(const tooling
std::unique_ptr<IndexFileIn> Shard;
FileDigest Digest = {};
bool CountReferences = false;
+ bool HadErrors = false;
};
std::vector<ShardInfo> IntermediateSymbols;
// Make sure we don't have duplicate elements in the queue. Keys are absolute @@ -586,6 +584,8 @@ BackgroundIndex::loadShard(const tooling
SI.Digest = I.getValue().Digest;
SI.CountReferences =
I.getValue().Flags & IncludeGraphNode::SourceFlag::IsTU;
+ SI.HadErrors =
+ I.getValue().Flags & IncludeGraphNode::SourceFlag::HadErrors;
IntermediateSymbols.push_back(std::move(SI));
// Check if the source needs re-indexing.
// Get the digest, skip it if file doesn't exist.
@@ -604,7 +604,7 @@ BackgroundIndex::loadShard(const tooling
}
// Load shard information into background-index.
{
- std::lock_guard<std::mutex> Lock(DigestsMu);
+ std::lock_guard<std::mutex> Lock(ShardVersionsMu);
// This can override a newer version that is added in another thread,
// if this thread sees the older version but finishes later. This
// should be rare in practice.
@@ -620,7 +620,10 @@ BackgroundIndex::loadShard(const tooling
SI.Shard->Relations
? llvm::make_unique<RelationSlab>(std::move(*SI.Shard->Relations))
: nullptr;
- IndexedFileDigests[SI.AbsolutePath] = SI.Digest;
+ ShardVersion &SV = ShardVersions[SI.AbsolutePath];
+ SV.Digest = SI.Digest;
+ SV.HadErrors = SI.HadErrors;
+
IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS),
std::move(RelS), SI.CountReferences);
}
Modified: clang-tools-extra/trunk/clangd/index/Background.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=365123&r1=365122&r2=365123&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Background.h (original)
+++ clang-tools-extra/trunk/clangd/index/Background.h Thu Jul 4
+++ 02:52:12 2019
@@ -12,6 +12,7 @@
#include "Context.h"
#include "FSProvider.h"
#include "GlobalCompilationDatabase.h"
+#include "SourceCode.h"
#include "Threading.h"
#include "index/FileIndex.h"
#include "index/Index.h"
@@ -93,11 +94,17 @@ public:
static void preventThreadStarvationInTests();
private:
+ /// Represents the state of a single file when indexing was performed.
+ struct ShardVersion {
+ FileDigest Digest{0};
+ bool HadErrors = false;
+ };
+
/// Given index results from a TU, only update symbols coming from files with
- /// different digests than \p DigestsSnapshot. Also stores new index
+ /// different digests than \p ShardVersionsSnapshot. Also stores new
+ index
/// information on IndexStorage.
void update(llvm::StringRef MainFile, IndexFileIn Index,
- const llvm::StringMap<FileDigest> &DigestsSnapshot,
+ const llvm::StringMap<ShardVersion>
+ &ShardVersionsSnapshot,
BackgroundIndexStorage *IndexStorage, bool HadErrors);
// configuration
@@ -115,8 +122,8 @@ private:
std::condition_variable IndexCV;
FileSymbols IndexedSymbols;
- llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.
- std::mutex DigestsMu;
+ llvm::StringMap<ShardVersion> ShardVersions; // Key is absolute file path.
+ std::mutex ShardVersionsMu;
BackgroundIndexStorage::Factory IndexStorageFactory;
struct Source {
Modified: clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp?rev=365123&r1=365122&r2=365123&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp
+++ Thu Jul 4 02:52:12 2019
@@ -524,18 +524,41 @@ TEST_F(BackgroundIndexTest, Uncompilable
CDB.setCompileCommand(testPath("build/../A.cc"), Cmd);
ASSERT_TRUE(Idx.blockUntilIdleForTest());
- // Make sure we only store the shard for main file.
- EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc")));
- auto Shard = MSS.loadShard(testPath("A.cc"));
- EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
- EXPECT_THAT(Shard->Sources->keys(),
- UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",
- "unittest:///B.h"));
-
- EXPECT_THAT(Shard->Sources->lookup("unittest:///A.cc"), HadErrors());
- // FIXME: We should also persist headers while marking them with errors.
- EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), Not(HadErrors()));
- EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), Not(HadErrors()));
+ EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h"),
+ testPath("B.h"),
+ testPath("C.h")));
+
+ {
+ auto Shard = MSS.loadShard(testPath("A.cc"));
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
+ EXPECT_THAT(Shard->Sources->keys(),
+ UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",
+ "unittest:///B.h"));
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///A.cc"),
+ HadErrors()); }
+
+ {
+ auto Shard = MSS.loadShard(testPath("A.h"));
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));
+ EXPECT_THAT(Shard->Sources->keys(),
+ UnorderedElementsAre("unittest:///A.h"));
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"),
+ HadErrors()); }
+
+ {
+ auto Shard = MSS.loadShard(testPath("B.h"));
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("asdf")));
+ EXPECT_THAT(Shard->Sources->keys(),
+ UnorderedElementsAre("unittest:///B.h", "unittest:///C.h"));
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"),
+ HadErrors()); }
+
+ {
+ auto Shard = MSS.loadShard(testPath("C.h"));
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre());
+ EXPECT_THAT(Shard->Sources->keys(),
+ UnorderedElementsAre("unittest:///C.h"));
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///C.h"),
+ HadErrors()); }
}
TEST_F(BackgroundIndexTest, CmdLineHash) {
_______________________________________________
cfe-commits mailing list
cfe-commits at lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list