<div dir="ltr"><a href="https://reviews.llvm.org/rL365140" class="cremed">https://reviews.llvm.org/rL365140</a> should fix it, please let me know if it doesnt.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Jul 4, 2019 at 3:45 PM <<a href="mailto:douglas.yung@sony.com">douglas.yung@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Kadir,<br>
<br>
Your change is causing a build failure on our internal linux build bot running gcc 5.4:<br>
<br>
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<br>
In file included from /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdServer.h:24:0,<br>
from /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.h:12,<br>
from /home/siadmin/jenkins/w/opensource/opensource_build/llvm/tools/clang/tools/extra/clangd/ClangdLSPServer.cpp:9:<br>
/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<br>
FileDigest Digest{0};<br>
^<br>
<br>
Can you please take a look?<br>
<br>
Douglas Yung<br>
<br>
-----Original Message-----<br>
From: cfe-commits <<a href="mailto:cfe-commits-bounces@lists.llvm.org" target="_blank">cfe-commits-bounces@lists.llvm.org</a>> On Behalf Of Kadir Cetinkaya via cfe-commits<br>
Sent: Thursday, July 4, 2019 2:52<br>
To: <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
Subject: [clang-tools-extra] r365123 - [clangd] Make HadErrors part of background index's internal state<br>
<br>
Author: kadircet<br>
Date: Thu Jul 4 02:52:12 2019<br>
New Revision: 365123<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=365123&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=365123&view=rev</a><br>
Log:<br>
[clangd] Make HadErrors part of background index's internal state<br>
<br>
Reviewers: sammccall<br>
<br>
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits<br>
<br>
Tags: #clang<br>
<br>
Differential Revision: <a href="https://reviews.llvm.org/D64147" rel="noreferrer" target="_blank">https://reviews.llvm.org/D64147</a><br>
<br>
Modified:<br>
clang-tools-extra/trunk/clangd/index/Background.cpp<br>
clang-tools-extra/trunk/clangd/index/Background.h<br>
clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp<br>
<br>
Modified: clang-tools-extra/trunk/clangd/index/Background.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=365123&r1=365122&r2=365123&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.cpp?rev=365123&r1=365122&r2=365123&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/index/Background.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/index/Background.cpp Thu Jul 4 <br>
+++ 02:52:12 2019<br>
@@ -101,28 +101,6 @@ IncludeGraph getSubGraph(const URI &U, c<br>
return IG;<br>
}<br>
<br>
-// Creates a filter to not collect index results from files with unchanged -// digests.<br>
-// \p FileDigests contains file digests for the current indexed files.<br>
-decltype(SymbolCollector::Options::FileFilter)<br>
-createFileFilter(const llvm::StringMap<FileDigest> &FileDigests) {<br>
- return [&FileDigests](const SourceManager &SM, FileID FID) {<br>
- const auto *F = SM.getFileEntryForID(FID);<br>
- if (!F)<br>
- return false; // Skip invalid files.<br>
- auto AbsPath = getCanonicalPath(F, SM);<br>
- if (!AbsPath)<br>
- return false; // Skip files without absolute path.<br>
- auto Digest = digestFile(SM, FID);<br>
- if (!Digest)<br>
- return false;<br>
- auto D = FileDigests.find(*AbsPath);<br>
- if (D != FileDigests.end() && D->second == Digest)<br>
- return false; // Skip files that haven't changed.<br>
- return true;<br>
- };<br>
-}<br>
-<br>
// 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.<br>
@@ -274,12 +252,12 @@ void BackgroundIndex::enqueueTask(Task T }<br>
<br>
/// 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.<br>
-void BackgroundIndex::update(llvm::StringRef MainFile, IndexFileIn Index,<br>
- const llvm::StringMap<FileDigest> &DigestsSnapshot,<br>
- BackgroundIndexStorage *IndexStorage,<br>
- bool HadErrors) {<br>
+/// are different or missing from than \p ShardVersionsSnapshot. Also <br>
+stores new /// index information on IndexStorage.<br>
+void BackgroundIndex::update(<br>
+ llvm::StringRef MainFile, IndexFileIn Index,<br>
+ const llvm::StringMap<ShardVersion> &ShardVersionsSnapshot,<br>
+ BackgroundIndexStorage *IndexStorage, bool HadErrors) {<br>
// Partition symbols/references into files.<br>
struct File {<br>
llvm::DenseSet<const Symbol *> Symbols; @@ -291,18 +269,14 @@ void BackgroundIndex::update(llvm::Strin<br>
URIToFileCache URICache(MainFile);<br>
for (const auto &IndexIt : *Index.Sources) {<br>
const auto &IGN = IndexIt.getValue();<br>
- // In case of failures, we only store main file of TU. That way we can still<br>
- // get symbols from headers if some other TU can compile them. Note that<br>
- // sources do not contain any information regarding missing headers, since<br>
- // we don't even know what absolute path they should fall in.<br>
- // FIXME: Also store contents from other files whenever the current contents<br>
- // for those files are missing or if they had errors before.<br>
- if (HadErrors && !(IGN.Flags & IncludeGraphNode::SourceFlag::IsTU))<br>
- continue;<br>
+ // Note that sources do not contain any information regarding missing<br>
+ // headers, since we don't even know what absolute path they should fall in.<br>
const auto AbsPath = URICache.resolve(IGN.URI);<br>
- const auto DigestIt = DigestsSnapshot.find(AbsPath);<br>
- // File has different contents.<br>
- if (DigestIt == DigestsSnapshot.end() || DigestIt->getValue() != IGN.Digest)<br>
+ const auto DigestIt = ShardVersionsSnapshot.find(AbsPath);<br>
+ // File has different contents, or indexing was successfull this time.<br>
+ if (DigestIt == ShardVersionsSnapshot.end() ||<br>
+ DigestIt->getValue().Digest != IGN.Digest ||<br>
+ (DigestIt->getValue().HadErrors && !HadErrors))<br>
Files.try_emplace(AbsPath).first->getValue().Digest = IGN.Digest;<br>
}<br>
// This map is used to figure out where to store relations.<br>
@@ -385,13 +359,17 @@ void BackgroundIndex::update(llvm::Strin<br>
}<br>
<br>
{<br>
- std::lock_guard<std::mutex> Lock(DigestsMu);<br>
+ std::lock_guard<std::mutex> Lock(ShardVersionsMu);<br>
auto Hash = FileIt.second.Digest;<br>
- // Skip if file is already up to date.<br>
- auto DigestIt = IndexedFileDigests.try_emplace(Path);<br>
- if (!DigestIt.second && DigestIt.first->second == Hash)<br>
+ auto DigestIt = ShardVersions.try_emplace(Path);<br>
+ ShardVersion &SV = DigestIt.first->second;<br>
+ // Skip if file is already up to date, unless previous index was broken<br>
+ // and this one is not.<br>
+ if (!DigestIt.second && SV.Digest == Hash && SV.HadErrors && <br>
+ !HadErrors)<br>
continue;<br>
- DigestIt.first->second = Hash;<br>
+ SV.Digest = Hash;<br>
+ SV.HadErrors = HadErrors;<br>
+<br>
// This can override a newer version that is added in another thread, if<br>
// this thread sees the older version but finishes later. This should be<br>
// rare in practice.<br>
@@ -439,11 +417,11 @@ llvm::Error BackgroundIndex::index(tooli<br>
return llvm::errorCodeToError(Buf.getError());<br>
auto Hash = digest(Buf->get()->getBuffer());<br>
<br>
- // Take a snapshot of the digests to avoid locking for each file in the TU.<br>
- llvm::StringMap<FileDigest> DigestsSnapshot;<br>
+ // Take a snapshot of the versions to avoid locking for each file in the TU.<br>
+ llvm::StringMap<ShardVersion> ShardVersionsSnapshot;<br>
{<br>
- std::lock_guard<std::mutex> Lock(DigestsMu);<br>
- DigestsSnapshot = IndexedFileDigests;<br>
+ std::lock_guard<std::mutex> Lock(ShardVersionsMu);<br>
+ ShardVersionsSnapshot = ShardVersions;<br>
}<br>
<br>
vlog("Indexing {0} (digest:={1})", Cmd.Filename, llvm::toHex(Hash)); @@ -463,7 +441,26 @@ llvm::Error BackgroundIndex::index(tooli<br>
"Couldn't build compiler instance");<br>
<br>
SymbolCollector::Options IndexOpts;<br>
- IndexOpts.FileFilter = createFileFilter(DigestsSnapshot);<br>
+ // Creates a filter to not collect index results from files with <br>
+ unchanged // digests.<br>
+ IndexOpts.FileFilter = [&ShardVersionsSnapshot](const SourceManager &SM,<br>
+ FileID FID) {<br>
+ const auto *F = SM.getFileEntryForID(FID);<br>
+ if (!F)<br>
+ return false; // Skip invalid files.<br>
+ auto AbsPath = getCanonicalPath(F, SM);<br>
+ if (!AbsPath)<br>
+ return false; // Skip files without absolute path.<br>
+ auto Digest = digestFile(SM, FID);<br>
+ if (!Digest)<br>
+ return false;<br>
+ auto D = ShardVersionsSnapshot.find(*AbsPath);<br>
+ if (D != ShardVersionsSnapshot.end() && D->second.Digest == Digest &&<br>
+ !D->second.HadErrors)<br>
+ return false; // Skip files that haven't changed, without errors.<br>
+ return true;<br>
+ };<br>
+<br>
IndexFileIn Index;<br>
auto Action = createStaticIndexingAction(<br>
IndexOpts, [&](SymbolSlab S) { Index.Symbols = std::move(S); }, @@ -503,7 +500,7 @@ llvm::Error BackgroundIndex::index(tooli<br>
for (auto &It : *Index.Sources)<br>
It.second.Flags |= IncludeGraphNode::SourceFlag::HadErrors;<br>
}<br>
- update(AbsolutePath, std::move(Index), DigestsSnapshot, IndexStorage,<br>
+ update(AbsolutePath, std::move(Index), ShardVersionsSnapshot, <br>
+ IndexStorage,<br>
HadErrors);<br>
<br>
if (BuildIndexPeriodMs > 0)<br>
@@ -524,6 +521,7 @@ BackgroundIndex::loadShard(const tooling<br>
std::unique_ptr<IndexFileIn> Shard;<br>
FileDigest Digest = {};<br>
bool CountReferences = false;<br>
+ bool HadErrors = false;<br>
};<br>
std::vector<ShardInfo> IntermediateSymbols;<br>
// Make sure we don't have duplicate elements in the queue. Keys are absolute @@ -586,6 +584,8 @@ BackgroundIndex::loadShard(const tooling<br>
SI.Digest = I.getValue().Digest;<br>
SI.CountReferences =<br>
I.getValue().Flags & IncludeGraphNode::SourceFlag::IsTU;<br>
+ SI.HadErrors =<br>
+ I.getValue().Flags & IncludeGraphNode::SourceFlag::HadErrors;<br>
IntermediateSymbols.push_back(std::move(SI));<br>
// Check if the source needs re-indexing.<br>
// Get the digest, skip it if file doesn't exist.<br>
@@ -604,7 +604,7 @@ BackgroundIndex::loadShard(const tooling<br>
}<br>
// Load shard information into background-index.<br>
{<br>
- std::lock_guard<std::mutex> Lock(DigestsMu);<br>
+ std::lock_guard<std::mutex> Lock(ShardVersionsMu);<br>
// This can override a newer version that is added in another thread,<br>
// if this thread sees the older version but finishes later. This<br>
// should be rare in practice.<br>
@@ -620,7 +620,10 @@ BackgroundIndex::loadShard(const tooling<br>
SI.Shard->Relations<br>
? llvm::make_unique<RelationSlab>(std::move(*SI.Shard->Relations))<br>
: nullptr;<br>
- IndexedFileDigests[SI.AbsolutePath] = SI.Digest;<br>
+ ShardVersion &SV = ShardVersions[SI.AbsolutePath];<br>
+ SV.Digest = SI.Digest;<br>
+ SV.HadErrors = SI.HadErrors;<br>
+<br>
IndexedSymbols.update(SI.AbsolutePath, std::move(SS), std::move(RS),<br>
std::move(RelS), SI.CountReferences);<br>
}<br>
<br>
Modified: clang-tools-extra/trunk/clangd/index/Background.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=365123&r1=365122&r2=365123&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Background.h?rev=365123&r1=365122&r2=365123&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/index/Background.h (original)<br>
+++ clang-tools-extra/trunk/clangd/index/Background.h Thu Jul 4 <br>
+++ 02:52:12 2019<br>
@@ -12,6 +12,7 @@<br>
#include "Context.h"<br>
#include "FSProvider.h"<br>
#include "GlobalCompilationDatabase.h"<br>
+#include "SourceCode.h"<br>
#include "Threading.h"<br>
#include "index/FileIndex.h"<br>
#include "index/Index.h"<br>
@@ -93,11 +94,17 @@ public:<br>
static void preventThreadStarvationInTests();<br>
<br>
private:<br>
+ /// Represents the state of a single file when indexing was performed.<br>
+ struct ShardVersion {<br>
+ FileDigest Digest{0};<br>
+ bool HadErrors = false;<br>
+ };<br>
+<br>
/// Given index results from a TU, only update symbols coming from files with<br>
- /// different digests than \p DigestsSnapshot. Also stores new index<br>
+ /// different digests than \p ShardVersionsSnapshot. Also stores new <br>
+ index<br>
/// information on IndexStorage.<br>
void update(llvm::StringRef MainFile, IndexFileIn Index,<br>
- const llvm::StringMap<FileDigest> &DigestsSnapshot,<br>
+ const llvm::StringMap<ShardVersion> <br>
+ &ShardVersionsSnapshot,<br>
BackgroundIndexStorage *IndexStorage, bool HadErrors);<br>
<br>
// configuration<br>
@@ -115,8 +122,8 @@ private:<br>
std::condition_variable IndexCV;<br>
<br>
FileSymbols IndexedSymbols;<br>
- llvm::StringMap<FileDigest> IndexedFileDigests; // Key is absolute file path.<br>
- std::mutex DigestsMu;<br>
+ llvm::StringMap<ShardVersion> ShardVersions; // Key is absolute file path.<br>
+ std::mutex ShardVersionsMu;<br>
<br>
BackgroundIndexStorage::Factory IndexStorageFactory;<br>
struct Source {<br>
<br>
Modified: clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp?rev=365123&r1=365122&r2=365123&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp?rev=365123&r1=365122&r2=365123&view=diff</a><br>
==============================================================================<br>
--- clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp (original)<br>
+++ clang-tools-extra/trunk/clangd/unittests/BackgroundIndexTests.cpp <br>
+++ Thu Jul 4 02:52:12 2019<br>
@@ -524,18 +524,41 @@ TEST_F(BackgroundIndexTest, Uncompilable<br>
CDB.setCompileCommand(testPath("build/../A.cc"), Cmd);<br>
ASSERT_TRUE(Idx.blockUntilIdleForTest());<br>
<br>
- // Make sure we only store the shard for main file.<br>
- EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc")));<br>
- auto Shard = MSS.loadShard(testPath("A.cc"));<br>
- EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));<br>
- EXPECT_THAT(Shard->Sources->keys(),<br>
- UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",<br>
- "unittest:///B.h"));<br>
-<br>
- EXPECT_THAT(Shard->Sources->lookup("unittest:///A.cc"), HadErrors());<br>
- // FIXME: We should also persist headers while marking them with errors.<br>
- EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), Not(HadErrors()));<br>
- EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), Not(HadErrors()));<br>
+ EXPECT_THAT(Storage.keys(), ElementsAre(testPath("A.cc"), testPath("A.h"),<br>
+ testPath("B.h"), <br>
+ testPath("C.h")));<br>
+<br>
+ {<br>
+ auto Shard = MSS.loadShard(testPath("A.cc"));<br>
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));<br>
+ EXPECT_THAT(Shard->Sources->keys(),<br>
+ UnorderedElementsAre("unittest:///A.cc", "unittest:///A.h",<br>
+ "unittest:///B.h"));<br>
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///A.cc"), <br>
+ HadErrors()); }<br>
+<br>
+ {<br>
+ auto Shard = MSS.loadShard(testPath("A.h"));<br>
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("foo")));<br>
+ EXPECT_THAT(Shard->Sources->keys(),<br>
+ UnorderedElementsAre("unittest:///A.h"));<br>
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///A.h"), <br>
+ HadErrors()); }<br>
+<br>
+ {<br>
+ auto Shard = MSS.loadShard(testPath("B.h"));<br>
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre(Named("asdf")));<br>
+ EXPECT_THAT(Shard->Sources->keys(),<br>
+ UnorderedElementsAre("unittest:///B.h", "unittest:///C.h"));<br>
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///B.h"), <br>
+ HadErrors()); }<br>
+<br>
+ {<br>
+ auto Shard = MSS.loadShard(testPath("C.h"));<br>
+ EXPECT_THAT(*Shard->Symbols, UnorderedElementsAre());<br>
+ EXPECT_THAT(Shard->Sources->keys(),<br>
+ UnorderedElementsAre("unittest:///C.h"));<br>
+ EXPECT_THAT(Shard->Sources->lookup("unittest:///C.h"), <br>
+ HadErrors()); }<br>
}<br>
<br>
TEST_F(BackgroundIndexTest, CmdLineHash) {<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div>