From 57a2eaf3c1aa1a08e9b474b873064e075c52daef Mon Sep 17 00:00:00 2001 From: Rumeet Dhindsa Date: Tue, 10 Mar 2020 10:59:26 -0700 Subject: [PATCH] Revert "[modules] Do not cache invalid state for modules that we attempted to load." As per comment on https://reviews.llvm.org/D72860, it is suggested to revert this change in the meantime, since it has introduced regression. This reverts commit 83f4c3af021cd5322ea10fd1c4e839874c1dae49. --- .../clang/Serialization/InMemoryModuleCache.h | 42 +++++++++++--- clang/lib/Serialization/ASTReader.cpp | 2 +- clang/lib/Serialization/ASTWriter.cpp | 2 +- clang/lib/Serialization/InMemoryModuleCache.cpp | 29 +++++++--- clang/lib/Serialization/ModuleManager.cpp | 11 ++-- .../Modules/Inputs/implicit-invalidate-chain/A.h | 2 + .../Modules/Inputs/implicit-invalidate-chain/B.h | 2 + .../Modules/Inputs/implicit-invalidate-chain/C.h | 2 + .../implicit-invalidate-chain/module.modulemap | 3 + clang/test/Modules/implicit-invalidate-chain.c | 67 ++++++++++++++++++++++ clang/unittests/Frontend/FrontendActionTest.cpp | 6 +- .../Serialization/InMemoryModuleCacheTest.cpp | 38 +++++++----- 12 files changed, 169 insertions(+), 37 deletions(-) create mode 100644 clang/test/Modules/Inputs/implicit-invalidate-chain/A.h create mode 100644 clang/test/Modules/Inputs/implicit-invalidate-chain/B.h create mode 100644 clang/test/Modules/Inputs/implicit-invalidate-chain/C.h create mode 100644 clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap create mode 100644 clang/test/Modules/implicit-invalidate-chain.c diff --git a/clang/include/clang/Serialization/InMemoryModuleCache.h b/clang/include/clang/Serialization/InMemoryModuleCache.h index c4814c4..7b5b5c1 100644 --- a/clang/include/clang/Serialization/InMemoryModuleCache.h +++ b/clang/include/clang/Serialization/InMemoryModuleCache.h @@ -45,35 +45,61 @@ class InMemoryModuleCache : public llvm::RefCountedBase { llvm::StringMap PCMs; public: + /// There are four states for a PCM. It must monotonically increase. + /// + /// 1. Unknown: the PCM has neither been read from disk nor built. + /// 2. Tentative: the PCM has been read from disk but not yet imported or + /// built. It might work. + /// 3. ToBuild: the PCM read from disk did not work but a new one has not + /// been built yet. + /// 4. Final: indicating that the current PCM was either built in this + /// process or has been successfully imported. + enum State { Unknown, Tentative, ToBuild, Final }; + + /// Get the state of the PCM. + State getPCMState(llvm::StringRef Filename) const; + /// Store the PCM under the Filename. /// - /// \pre PCM for the same Filename shouldn't be in cache already. + /// \pre state is Unknown + /// \post state is Tentative /// \return a reference to the buffer as a convenience. llvm::MemoryBuffer &addPCM(llvm::StringRef Filename, std::unique_ptr Buffer); - /// Store a final PCM under the Filename. + /// Store a just-built PCM under the Filename. /// - /// \pre PCM for the same Filename shouldn't be in cache already. + /// \pre state is Unknown or ToBuild. + /// \pre state is not Tentative. /// \return a reference to the buffer as a convenience. - llvm::MemoryBuffer &addFinalPCM(llvm::StringRef Filename, + llvm::MemoryBuffer &addBuiltPCM(llvm::StringRef Filename, std::unique_ptr Buffer); - /// Try to remove a PCM from the cache. No effect if it is Final. + /// Try to remove a buffer from the cache. No effect if state is Final. /// - /// \return false on success. - bool tryToRemovePCM(llvm::StringRef Filename); + /// \pre state is Tentative/Final. + /// \post Tentative => ToBuild or Final => Final. + /// \return false on success, i.e. if Tentative => ToBuild. + bool tryToDropPCM(llvm::StringRef Filename); /// Mark a PCM as final. + /// + /// \pre state is Tentative or Final. + /// \post state is Final. void finalizePCM(llvm::StringRef Filename); - /// Get a pointer to the PCM if it exists; else nullptr. + /// Get a pointer to the pCM if it exists; else nullptr. llvm::MemoryBuffer *lookupPCM(llvm::StringRef Filename) const; /// Check whether the PCM is final and has been shown to work. /// /// \return true iff state is Final. bool isPCMFinal(llvm::StringRef Filename) const; + + /// Check whether the PCM is waiting to be built. + /// + /// \return true iff state is ToBuild. + bool shouldBuildPCM(llvm::StringRef Filename) const; }; } // end namespace clang diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp index b5ca1e1..63c817b 100644 --- a/clang/lib/Serialization/ASTReader.cpp +++ b/clang/lib/Serialization/ASTReader.cpp @@ -4502,7 +4502,7 @@ ASTReader::ReadASTCore(StringRef FileName, if (ShouldFinalizePCM) MC.finalizePCM(FileName); else - MC.tryToRemovePCM(FileName); + MC.tryToDropPCM(FileName); }); ModuleFile &F = *M; BitstreamCursor &Stream = F.Stream; diff --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp index bf893c7..1278841 100644 --- a/clang/lib/Serialization/ASTWriter.cpp +++ b/clang/lib/Serialization/ASTWriter.cpp @@ -4317,7 +4317,7 @@ ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef, WritingAST = false; if (ShouldCacheASTInMemory) { // Construct MemoryBuffer and update buffer manager. - ModuleCache.addFinalPCM(OutputFile, + ModuleCache.addBuiltPCM(OutputFile, llvm::MemoryBuffer::getMemBufferCopy( StringRef(Buffer.begin(), Buffer.size()))); } diff --git a/clang/lib/Serialization/InMemoryModuleCache.cpp b/clang/lib/Serialization/InMemoryModuleCache.cpp index 68d9411..d35fa2a 100644 --- a/clang/lib/Serialization/InMemoryModuleCache.cpp +++ b/clang/lib/Serialization/InMemoryModuleCache.cpp @@ -11,6 +11,16 @@ using namespace clang; +InMemoryModuleCache::State +InMemoryModuleCache::getPCMState(llvm::StringRef Filename) const { + auto I = PCMs.find(Filename); + if (I == PCMs.end()) + return Unknown; + if (I->second.IsFinal) + return Final; + return I->second.Buffer ? Tentative : ToBuild; +} + llvm::MemoryBuffer & InMemoryModuleCache::addPCM(llvm::StringRef Filename, std::unique_ptr Buffer) { @@ -20,11 +30,11 @@ InMemoryModuleCache::addPCM(llvm::StringRef Filename, } llvm::MemoryBuffer & -InMemoryModuleCache::addFinalPCM(llvm::StringRef Filename, +InMemoryModuleCache::addBuiltPCM(llvm::StringRef Filename, std::unique_ptr Buffer) { auto &PCM = PCMs[Filename]; assert(!PCM.IsFinal && "Trying to override finalized PCM?"); - assert(!PCM.Buffer && "Already has a non-final PCM"); + assert(!PCM.Buffer && "Trying to override tentative PCM?"); PCM.Buffer = std::move(Buffer); PCM.IsFinal = true; return *PCM.Buffer; @@ -39,21 +49,24 @@ InMemoryModuleCache::lookupPCM(llvm::StringRef Filename) const { } bool InMemoryModuleCache::isPCMFinal(llvm::StringRef Filename) const { - auto I = PCMs.find(Filename); - if (I == PCMs.end()) - return false; - return I->second.IsFinal; + return getPCMState(Filename) == Final; +} + +bool InMemoryModuleCache::shouldBuildPCM(llvm::StringRef Filename) const { + return getPCMState(Filename) == ToBuild; } -bool InMemoryModuleCache::tryToRemovePCM(llvm::StringRef Filename) { +bool InMemoryModuleCache::tryToDropPCM(llvm::StringRef Filename) { auto I = PCMs.find(Filename); assert(I != PCMs.end() && "PCM to remove is unknown..."); auto &PCM = I->second; + assert(PCM.Buffer && "PCM to remove is scheduled to be built..."); + if (PCM.IsFinal) return true; - PCMs.erase(I); + PCM.Buffer.reset(); return false; } diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp index 9b326d2..2656220 100644 --- a/clang/lib/Serialization/ModuleManager.cpp +++ b/clang/lib/Serialization/ModuleManager.cpp @@ -163,7 +163,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, // Load the contents of the module if (std::unique_ptr Buffer = lookupBuffer(FileName)) { // The buffer was already provided for us. - NewModule->Buffer = &ModuleCache->addFinalPCM(FileName, std::move(Buffer)); + NewModule->Buffer = &ModuleCache->addBuiltPCM(FileName, std::move(Buffer)); // Since the cached buffer is reused, it is safe to close the file // descriptor that was opened while stat()ing the PCM in // lookupModuleFile() above, it won't be needed any longer. @@ -173,6 +173,11 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, NewModule->Buffer = Buffer; // As above, the file descriptor is no longer needed. Entry->closeFile(); + } else if (getModuleCache().shouldBuildPCM(FileName)) { + // Report that the module is out of date, since we tried (and failed) to + // import it earlier. + Entry->closeFile(); + return OutOfDate; } else { // Open the AST file. llvm::ErrorOr> Buf((std::error_code())); @@ -180,9 +185,7 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type, Buf = llvm::MemoryBuffer::getSTDIN(); } else { // Get a buffer of the file and close the file descriptor when done. - // The file is volatile because in a parallel build we expect multiple - // compiler processes to use the same module file rebuilding it if needed. - Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/true); + Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false); } if (!Buf) { diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/A.h b/clang/test/Modules/Inputs/implicit-invalidate-chain/A.h new file mode 100644 index 0000000..2b9dab8 --- /dev/null +++ b/clang/test/Modules/Inputs/implicit-invalidate-chain/A.h @@ -0,0 +1,2 @@ +// A +#include "B.h" diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/B.h b/clang/test/Modules/Inputs/implicit-invalidate-chain/B.h new file mode 100644 index 0000000..a2711d4 --- /dev/null +++ b/clang/test/Modules/Inputs/implicit-invalidate-chain/B.h @@ -0,0 +1,2 @@ +// B +#include "C.h" diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/C.h b/clang/test/Modules/Inputs/implicit-invalidate-chain/C.h new file mode 100644 index 0000000..231e35c --- /dev/null +++ b/clang/test/Modules/Inputs/implicit-invalidate-chain/C.h @@ -0,0 +1,2 @@ +// C +#include "D.h" diff --git a/clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap b/clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap new file mode 100644 index 0000000..f5b6360 --- /dev/null +++ b/clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap @@ -0,0 +1,3 @@ +module A { header "A.h" } +module B { header "B.h" } +module C { header "C.h" } diff --git a/clang/test/Modules/implicit-invalidate-chain.c b/clang/test/Modules/implicit-invalidate-chain.c new file mode 100644 index 0000000..f7727a6 --- /dev/null +++ b/clang/test/Modules/implicit-invalidate-chain.c @@ -0,0 +1,67 @@ +// RUN: rm -rf %t1 %t2 %t-include +// RUN: mkdir %t-include +// RUN: echo 'module D { header "D.h" }' >> %t-include/module.modulemap + +// Run with -verify, which onliy gets remarks from the main TU. +// +// RUN: echo '#define D 0' > %t-include/D.h +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t1 \ +// RUN: -fdisable-module-hash -fsyntax-only \ +// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \ +// RUN: -Rmodule-build -Rmodule-import %s +// RUN: echo '#define D 11' > %t-include/D.h +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t1 \ +// RUN: -fdisable-module-hash -fsyntax-only \ +// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \ +// RUN: -Rmodule-build -Rmodule-import -verify %s + +// Run again, using FileCheck to check remarks from the module builds. This is +// the key test: after the first attempt to import an out-of-date 'D', all the +// modules have been invalidated and are not imported again until they are +// rebuilt. +// +// RUN: echo '#define D 0' > %t-include/D.h +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t2 \ +// RUN: -fdisable-module-hash -fsyntax-only \ +// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \ +// RUN: -Rmodule-build -Rmodule-import %s +// RUN: echo '#define D 11' > %t-include/D.h +// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t2 \ +// RUN: -fdisable-module-hash -fsyntax-only \ +// RUN: -I%S/Inputs/implicit-invalidate-chain -I%t-include \ +// RUN: -Rmodule-build -Rmodule-import %s 2>&1 |\ +// RUN: FileCheck %s -implicit-check-not "remark:" + +#include "A.h" // \ + expected-remark-re{{importing module 'A' from '{{.*[/\\]}}A.pcm'}} \ + expected-remark-re{{importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm'}} \ + expected-remark-re{{importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'}} \ + expected-remark-re{{importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'}} \ + expected-remark-re{{building module 'A' as '{{.*[/\\]}}A.pcm'}} \ + expected-remark{{finished building module 'A'}} \ + expected-remark-re{{importing module 'A' from '{{.*[/\\]}}A.pcm'}} \ + expected-remark-re{{importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm'}} \ + expected-remark-re{{importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm'}} \ + expected-remark-re{{importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm'}} +// CHECK: remark: importing module 'A' from '{{.*[/\\]}}A.pcm' +// CHECK: remark: importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm' +// CHECK: remark: importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm' +// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm' +// CHECK: remark: building module 'A' +// CHECK: remark: building module 'B' +// CHECK: remark: building module 'C' +// CHECK: remark: building module 'D' +// CHECK: remark: finished building module 'D' +// CHECK: remark: importing module 'D' from '{{.*[/\\]}}D.pcm' +// CHECK: remark: finished building module 'C' +// CHECK: remark: importing module 'C' from '{{.*[/\\]}}C.pcm' +// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm' +// CHECK: remark: finished building module 'B' +// CHECK: remark: importing module 'B' from '{{.*[/\\]}}B.pcm' +// CHECK: remark: importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm' +// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm' +// CHECK: remark: finished building module 'A' +// CHECK: remark: importing module 'A' from '{{.*[/\\]}}A.pcm' +// CHECK: remark: importing module 'B' into 'A' from '{{.*[/\\]}}B.pcm' +// CHECK: remark: importing module 'C' into 'B' from '{{.*[/\\]}}C.pcm' +// CHECK: remark: importing module 'D' into 'C' from '{{.*[/\\]}}D.pcm' diff --git a/clang/unittests/Frontend/FrontendActionTest.cpp b/clang/unittests/Frontend/FrontendActionTest.cpp index 97dacfb..bdc0af4 100644 --- a/clang/unittests/Frontend/FrontendActionTest.cpp +++ b/clang/unittests/Frontend/FrontendActionTest.cpp @@ -285,9 +285,11 @@ TEST(GeneratePCHFrontendAction, CacheGeneratedPCH) { // Check whether the PCH was cached. if (ShouldCache) - EXPECT_TRUE(Compiler.getModuleCache().isPCMFinal(PCHFilename)); + EXPECT_EQ(InMemoryModuleCache::Final, + Compiler.getModuleCache().getPCMState(PCHFilename)); else - EXPECT_EQ(nullptr, Compiler.getModuleCache().lookupPCM(PCHFilename)); + EXPECT_EQ(InMemoryModuleCache::Unknown, + Compiler.getModuleCache().getPCMState(PCHFilename)); } } diff --git a/clang/unittests/Serialization/InMemoryModuleCacheTest.cpp b/clang/unittests/Serialization/InMemoryModuleCacheTest.cpp index 7a5a3d4..ed5e153 100644 --- a/clang/unittests/Serialization/InMemoryModuleCacheTest.cpp +++ b/clang/unittests/Serialization/InMemoryModuleCacheTest.cpp @@ -24,11 +24,12 @@ std::unique_ptr getBuffer(int I) { TEST(InMemoryModuleCacheTest, initialState) { InMemoryModuleCache Cache; - EXPECT_EQ(nullptr, Cache.lookupPCM("B")); + EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B")); EXPECT_FALSE(Cache.isPCMFinal("B")); + EXPECT_FALSE(Cache.shouldBuildPCM("B")); #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST - EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown"); + EXPECT_DEATH(Cache.tryToDropPCM("B"), "PCM to remove is unknown"); EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown"); #endif } @@ -39,33 +40,37 @@ TEST(InMemoryModuleCacheTest, addPCM) { InMemoryModuleCache Cache; EXPECT_EQ(RawB, &Cache.addPCM("B", std::move(B))); + EXPECT_EQ(InMemoryModuleCache::Tentative, Cache.getPCMState("B")); EXPECT_EQ(RawB, Cache.lookupPCM("B")); EXPECT_FALSE(Cache.isPCMFinal("B")); + EXPECT_FALSE(Cache.shouldBuildPCM("B")); #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM"); - EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)), - "Already has a non-final PCM"); + EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)), + "Trying to override tentative PCM"); #endif } -TEST(InMemoryModuleCacheTest, addFinalPCM) { +TEST(InMemoryModuleCacheTest, addBuiltPCM) { auto B = getBuffer(1); auto *RawB = B.get(); InMemoryModuleCache Cache; - EXPECT_EQ(RawB, &Cache.addFinalPCM("B", std::move(B))); + EXPECT_EQ(RawB, &Cache.addBuiltPCM("B", std::move(B))); + EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B")); EXPECT_EQ(RawB, Cache.lookupPCM("B")); EXPECT_TRUE(Cache.isPCMFinal("B")); + EXPECT_FALSE(Cache.shouldBuildPCM("B")); #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM"); - EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)), + EXPECT_DEATH(Cache.addBuiltPCM("B", getBuffer(2)), "Trying to override finalized PCM"); #endif } -TEST(InMemoryModuleCacheTest, tryToRemovePCM) { +TEST(InMemoryModuleCacheTest, tryToDropPCM) { auto B1 = getBuffer(1); auto B2 = getBuffer(2); auto *RawB1 = B1.get(); @@ -73,22 +78,27 @@ TEST(InMemoryModuleCacheTest, tryToRemovePCM) { ASSERT_NE(RawB1, RawB2); InMemoryModuleCache Cache; + EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B")); EXPECT_EQ(RawB1, &Cache.addPCM("B", std::move(B1))); - EXPECT_FALSE(Cache.tryToRemovePCM("B")); + EXPECT_FALSE(Cache.tryToDropPCM("B")); EXPECT_EQ(nullptr, Cache.lookupPCM("B")); + EXPECT_EQ(InMemoryModuleCache::ToBuild, Cache.getPCMState("B")); EXPECT_FALSE(Cache.isPCMFinal("B")); + EXPECT_TRUE(Cache.shouldBuildPCM("B")); #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST - EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown"); - EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown"); + EXPECT_DEATH(Cache.addPCM("B", getBuffer(2)), "Already has a PCM"); + EXPECT_DEATH(Cache.tryToDropPCM("B"), + "PCM to remove is scheduled to be built"); + EXPECT_DEATH(Cache.finalizePCM("B"), "Trying to finalize a dropped PCM"); #endif // Add a new one. - EXPECT_EQ(RawB2, &Cache.addFinalPCM("B", std::move(B2))); + EXPECT_EQ(RawB2, &Cache.addBuiltPCM("B", std::move(B2))); EXPECT_TRUE(Cache.isPCMFinal("B")); // Can try to drop again, but this should error and do nothing. - EXPECT_TRUE(Cache.tryToRemovePCM("B")); + EXPECT_TRUE(Cache.tryToDropPCM("B")); EXPECT_EQ(RawB2, Cache.lookupPCM("B")); } @@ -97,10 +107,12 @@ TEST(InMemoryModuleCacheTest, finalizePCM) { auto *RawB = B.get(); InMemoryModuleCache Cache; + EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B")); EXPECT_EQ(RawB, &Cache.addPCM("B", std::move(B))); // Call finalize. Cache.finalizePCM("B"); + EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B")); EXPECT_TRUE(Cache.isPCMFinal("B")); } -- 2.7.4