[clang-tools-extra] r365123 - [clangd] Make HadErrors part of background index's internal state
Kadir Çetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Thu Jul 4 06:48:07 PDT 2019
https://reviews.llvm.org/rL365140 should fix it, please let me know if it
doesnt.
On Thu, Jul 4, 2019 at 3:45 PM <douglas.yung at sony.com> wrote:
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190704/665a99bc/attachment-0001.html>
More information about the cfe-commits
mailing list