[modules] Do not cache invalid state for modules that we attempted to load.

[vsapsai]

Summary

Partially reverts 0a2be46cfdb698fefcc860a56b47dde0884d5335 as it turned
out to cause redundant module rebuilds in multi-process incremental builds.
When a module was getting out of date, all compilation processes started at the
same time were marking it as ToBuild. So each process was building the same
module instead of checking if it was built by someone else and using that
result. In addition to the work duplication, contention on the same .pcm file
wasn't making builds faster.

Note that for a single-process build this change would cause redundant module
reads and validations. But reading a module is faster than building it and
multi-process builds are more common than single-process. So I'm willing to
make such a trade-off.

rdar://problem/54395127

[vsapsai]

Anecdotal build time measurements before and after the change. First row is a clean build, subsequent rows are incremental builds.

Revision	Before change	After change	Change (after - before)	Relative change
5da385fb56c	27:40	28:20	+40s	+2.41%
d4e006e8446	30:50	28:29	-141s	-7.62%
77d049d0c65	0:28	0:28	0s	0%
1b9ef3bbb59	0:10	0:11	+1s	+10%
ab411801b82	11:34	11:15	-19s	-2.74%

Cannot claim huge build time improvements but seems like there are no real regressions.

clang/lib/Serialization/ModuleManager.cpp

183–185

Made this change because if we don't have a valid module but opened a corresponding .pcm file earlier, there is a high chance that .pcm file was rebuilt.

[merge_guards_bot]

Unit tests: pass. 61863 tests passed, 0 failed and 781 were skipped.

clang-tidy: unknown.

clang-format: pass.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[dexonsmith]

LGTM.

clang/lib/Serialization/ModuleManager.cpp

183–185

Please add a comment in the code explaining that.

[rsmith]

clang/lib/Serialization/ModuleManager.cpp

183–185

This change is proving really bad for us. This prevents using mmap for loading module files, and instead forces the entire file to be loaded every time. Please revert.

[dexonsmith]

clang/lib/Serialization/ModuleManager.cpp

183–185

Can we limit the revert to explicit vs. implicit module builds? The scenario Volodymyr was defending against is implicit-only.

[vsapsai]

clang/lib/Serialization/ModuleManager.cpp

183–185

Richard, can you please tell what is your modules configuration and how you are invoking clang? For implicit builds loading a module instead of mmap is still better than building a module. But for explicit modules I see there is no need to use isVolatile as modules aren't changing all the time.

[dblaikie]

clang/lib/Serialization/ModuleManager.cpp

183–185

Richard/Google use explicit modules, if that's the particular parameter you're asking about.

So, yes, for Google's needs a solution that allows mmap to continue to be used for explicit modules would suffice, I believe.

(not to in any way derail that from being addressed - I thought implicit modules weren't race-y - they're hashed, etc, so they shouldn't collide/be overwritten? Seems like a loss in performance to have to copy the whole thing into memory rather than using mmap, if that could be avoided by more precise hashing/collision avoidance?)

[vsapsai]

clang/lib/Serialization/ModuleManager.cpp

183–185

I need a specific command-line invocation to test with. If tests in clang/test/Modules/explicit* are relevant to Google's setup, I can use those as an example. It's just they were touched last time in 2016.

To discuss racy-ness, that is kinda inherent problem for incremental builds. If a module becomes invalid, all of its users will try to get an up-to-date version of it. And they can try to build that module themselves or check and load already rebuilt module. Unfortunately, I don't see how different hashing can help with it, after all multiple compiler invocations do want the same .pcm file.

[rsmith]

clang/lib/Serialization/ModuleManager.cpp

183–185

Preventing the use of mmap seems unacceptable in general -- for either implicit or explicit modules. Anything that forces compile time to be linear in the total size of transitive modules used is not reasonable. To really see the problems here you'll need a real world testcase with thousands to tens of thousands of modules, which I can't reasonably give you.

There should be ways we can avoid problems with racyness. Perhaps we could switch to a content-addressed module file store?
In any case, this patch has introduced a regression, so we should revert this change while we work out what to do.

clang/include/clang/Serialization/InMemoryModuleCache.h

struct PCM {
     PCM(std::unique_ptr<llvm::MemoryBuffer> Buffer)
         : Buffer(std::move(Buffer)) {}
   };
 
   /// Cache of buffers.
   llvm::StringMap<PCM> 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 state is Unknown
-  /// \post state is Tentative
+  /// \pre PCM for the same Filename shouldn't be in cache already.
   /// \return a reference to the buffer as a convenience.
   llvm::MemoryBuffer &addPCM(llvm::StringRef Filename,
                              std::unique_ptr<llvm::MemoryBuffer> Buffer);
 
-  /// Store a just-built PCM under the Filename.
+  /// Store a final PCM under the Filename.
   ///
-  /// \pre state is Unknown or ToBuild.
-  /// \pre state is not Tentative.
+  /// \pre PCM for the same Filename shouldn't be in cache already.
   /// \return a reference to the buffer as a convenience.
-  llvm::MemoryBuffer &addBuiltPCM(llvm::StringRef Filename,
+  llvm::MemoryBuffer &addFinalPCM(llvm::StringRef Filename,
                                   std::unique_ptr<llvm::MemoryBuffer> Buffer);
 
-  /// Try to remove a buffer from the cache.  No effect if state is Final.
+  /// Try to remove a PCM from the cache.  No effect if it is Final.
   ///
-  /// \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);
+  /// \return false on success.
+  bool tryToRemovePCM(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
 
 #endif // LLVM_CLANG_SERIALIZATION_INMEMORYMODULECACHE_H

clang/lib/Serialization/ASTReader.cpp

ASTReader::ReadASTCore(StringRef FileName,
   assert(M && "Missing module file");
 
   bool ShouldFinalizePCM = false;
   auto FinalizeOrDropPCM = llvm::make_scope_exit([&]() {
     auto &MC = getModuleManager().getModuleCache();
     if (ShouldFinalizePCM)
       MC.finalizePCM(FileName);
     else
-      MC.tryToDropPCM(FileName);
+      MC.tryToRemovePCM(FileName);
   });
   ModuleFile &F = *M;
   BitstreamCursor &Stream = F.Stream;
   Stream = BitstreamCursor(PCHContainerRdr.ExtractPCH(*F.Buffer));
   F.SizeInBits = F.Buffer->getBufferSize() * 8;
 
   // Sniff for the signature.
   if (llvm::Error Err = doesntStartWithASTFileMagic(Stream)) {

clang/lib/Serialization/ASTWriter.cpp

ASTFileSignature ASTWriter::WriteAST(Sema &SemaRef,
   Context = nullptr;
   PP = nullptr;
   this->WritingModule = nullptr;
   this->BaseDirectory.clear();
 
   WritingAST = false;
   if (ShouldCacheASTInMemory) {
     // Construct MemoryBuffer and update buffer manager.
-    ModuleCache.addBuiltPCM(OutputFile,
+    ModuleCache.addFinalPCM(OutputFile,
                             llvm::MemoryBuffer::getMemBufferCopy(
                                 StringRef(Buffer.begin(), Buffer.size())));
   }
   return Signature;
 }
 
 template<typename Vector>
 static void AddLazyVectorDecls(ASTWriter &Writer, Vector &Vec,

clang/lib/Serialization/InMemoryModuleCache.cpp

 //===- InMemoryModuleCache.cpp - Cache for loaded memory buffers ----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/Serialization/InMemoryModuleCache.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 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<llvm::MemoryBuffer> Buffer) {
   auto Insertion = PCMs.insert(std::make_pair(Filename, std::move(Buffer)));
   assert(Insertion.second && "Already has a PCM");
   return *Insertion.first->second.Buffer;
 }
 
 llvm::MemoryBuffer &
-InMemoryModuleCache::addBuiltPCM(llvm::StringRef Filename,
+InMemoryModuleCache::addFinalPCM(llvm::StringRef Filename,
                                  std::unique_ptr<llvm::MemoryBuffer> Buffer) {
   auto &PCM = PCMs[Filename];
   assert(!PCM.IsFinal && "Trying to override finalized PCM?");
-  assert(!PCM.Buffer && "Trying to override tentative PCM?");
+  assert(!PCM.Buffer && "Already has a non-final PCM");
   PCM.Buffer = std::move(Buffer);
   PCM.IsFinal = true;
   return *PCM.Buffer;
 }
 
 llvm::MemoryBuffer *
 InMemoryModuleCache::lookupPCM(llvm::StringRef Filename) const {
   auto I = PCMs.find(Filename);
   if (I == PCMs.end())
     return nullptr;
   return I->second.Buffer.get();
 }
 
 bool InMemoryModuleCache::isPCMFinal(llvm::StringRef Filename) const {
-  return getPCMState(Filename) == Final;
-}
-
-bool InMemoryModuleCache::shouldBuildPCM(llvm::StringRef Filename) const {
-  return getPCMState(Filename) == ToBuild;
+  auto I = PCMs.find(Filename);
+  if (I == PCMs.end())
+    return false;
+  return I->second.IsFinal;
 }
 
-bool InMemoryModuleCache::tryToDropPCM(llvm::StringRef Filename) {
+bool InMemoryModuleCache::tryToRemovePCM(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;
 
-  PCM.Buffer.reset();
+  PCMs.erase(I);
   return false;
 }
 
 void InMemoryModuleCache::finalizePCM(llvm::StringRef Filename) {
   auto I = PCMs.find(Filename);
   assert(I != PCMs.end() && "PCM to finalize is unknown...");
 
   auto &PCM = I->second;
   assert(PCM.Buffer && "Trying to finalize a dropped PCM...");
   PCM.IsFinal = true;
 }

clang/lib/Serialization/ModuleManager.cpp

if (NewModule->Kind == MK_ImplicitModule) {
     if (!FileMgr.getNoncachedStatValue(TimestampFilename, Status))
       NewModule->InputFilesValidationTimestamp =
           llvm::sys::toTimeT(Status.getLastModificationTime());
   }
 
   // Load the contents of the module
   if (std::unique_ptr<llvm::MemoryBuffer> Buffer = lookupBuffer(FileName)) {
     // The buffer was already provided for us.
-    NewModule->Buffer = &ModuleCache->addBuiltPCM(FileName, std::move(Buffer));
+    NewModule->Buffer = &ModuleCache->addFinalPCM(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.
     Entry->closeFile();
   } else if (llvm::MemoryBuffer *Buffer =
                  getModuleCache().lookupPCM(FileName)) {
     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<std::unique_ptr<llvm::MemoryBuffer>> Buf((std::error_code()));
     if (FileName == "-") {
       Buf = llvm::MemoryBuffer::getSTDIN();
     } else {
       // Get a buffer of the file and close the file descriptor when done.
-      Buf = FileMgr.getBufferForFile(NewModule->File, /*isVolatile=*/false);
+      // 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);
     }
 
     if (!Buf) {
       ErrorStr = Buf.getError().message();
       return Missing;
     }
 
     NewModule->Buffer = &getModuleCache().addPCM(FileName, std::move(*Buf));

clang/test/Modules/Inputs/implicit-invalidate-chain/A.h

-// A
-#include "B.h"

clang/test/Modules/Inputs/implicit-invalidate-chain/B.h

-// B
-#include "C.h"

clang/test/Modules/Inputs/implicit-invalidate-chain/C.h

-// C
-#include "D.h"

clang/test/Modules/Inputs/implicit-invalidate-chain/module.modulemap

-module A { header "A.h" }
-module B { header "B.h" }
-module C { header "C.h" }

clang/test/Modules/implicit-invalidate-chain.c

-// 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'

clang/unittests/Frontend/FrontendActionTest.cpp

for (bool ShouldCache : {false, true}) {
     Compiler.setInvocation(std::move(Invocation));
     Compiler.createDiagnostics();
 
     GeneratePCHAction TestAction;
     ASSERT_TRUE(Compiler.ExecuteAction(TestAction));
 
     // Check whether the PCH was cached.
     if (ShouldCache)
-      EXPECT_EQ(InMemoryModuleCache::Final,
-                Compiler.getModuleCache().getPCMState(PCHFilename));
+      EXPECT_TRUE(Compiler.getModuleCache().isPCMFinal(PCHFilename));
     else
-      EXPECT_EQ(InMemoryModuleCache::Unknown,
-                Compiler.getModuleCache().getPCMState(PCHFilename));
+      EXPECT_EQ(nullptr, Compiler.getModuleCache().lookupPCM(PCHFilename));
   }
 }
 
 } // anonymous namespace

clang/unittests/Serialization/InMemoryModuleCacheTest.cpp

std::unique_ptr<MemoryBuffer> getBuffer(int I) {
   SmallVector<char, 8> Bytes;
   raw_svector_ostream(Bytes) << "data:" << I;
   return MemoryBuffer::getMemBuffer(StringRef(Bytes.data(), Bytes.size()), "",
                                     /* RequiresNullTerminator = */ false);
 }
 
 TEST(InMemoryModuleCacheTest, initialState) {
   InMemoryModuleCache Cache;
-  EXPECT_EQ(InMemoryModuleCache::Unknown, Cache.getPCMState("B"));
+  EXPECT_EQ(nullptr, Cache.lookupPCM("B"));
   EXPECT_FALSE(Cache.isPCMFinal("B"));
-  EXPECT_FALSE(Cache.shouldBuildPCM("B"));
 
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH(Cache.tryToDropPCM("B"), "PCM to remove is unknown");
+  EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
   EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
 #endif
 }
 
 TEST(InMemoryModuleCacheTest, addPCM) {
   auto B = getBuffer(1);
   auto *RawB = B.get();
 
   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.addBuiltPCM("B", getBuffer(2)),
-               "Trying to override tentative PCM");
+  EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
+               "Already has a non-final PCM");
 #endif
 }
 
-TEST(InMemoryModuleCacheTest, addBuiltPCM) {
+TEST(InMemoryModuleCacheTest, addFinalPCM) {
   auto B = getBuffer(1);
   auto *RawB = B.get();
 
   InMemoryModuleCache Cache;
-  EXPECT_EQ(RawB, &Cache.addBuiltPCM("B", std::move(B)));
-  EXPECT_EQ(InMemoryModuleCache::Final, Cache.getPCMState("B"));
+  EXPECT_EQ(RawB, &Cache.addFinalPCM("B", std::move(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.addBuiltPCM("B", getBuffer(2)),
+  EXPECT_DEATH(Cache.addFinalPCM("B", getBuffer(2)),
                "Trying to override finalized PCM");
 #endif
 }
 
-TEST(InMemoryModuleCacheTest, tryToDropPCM) {
+TEST(InMemoryModuleCacheTest, tryToRemovePCM) {
   auto B1 = getBuffer(1);
   auto B2 = getBuffer(2);
   auto *RawB1 = B1.get();
   auto *RawB2 = B2.get();
   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.tryToDropPCM("B"));
+  EXPECT_FALSE(Cache.tryToRemovePCM("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.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");
+  EXPECT_DEATH(Cache.tryToRemovePCM("B"), "PCM to remove is unknown");
+  EXPECT_DEATH(Cache.finalizePCM("B"), "PCM to finalize is unknown");
 #endif
 
   // Add a new one.
-  EXPECT_EQ(RawB2, &Cache.addBuiltPCM("B", std::move(B2)));
+  EXPECT_EQ(RawB2, &Cache.addFinalPCM("B", std::move(B2)));
   EXPECT_TRUE(Cache.isPCMFinal("B"));
 
   // Can try to drop again, but this should error and do nothing.
-  EXPECT_TRUE(Cache.tryToDropPCM("B"));
+  EXPECT_TRUE(Cache.tryToRemovePCM("B"));
   EXPECT_EQ(RawB2, Cache.lookupPCM("B"));
 }
 
 TEST(InMemoryModuleCacheTest, finalizePCM) {
   auto B = getBuffer(1);
   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"));
 }
 
 } // namespace