[Frontend] Only compile modules if not already finalized

[bnbarham]

Summary

It was possible to re-add a module to a shared in-memory module cache
when search paths are changed. This can eventually cause a crash if the
original module is referenced after this occurs.

1. Module A depends on B
2. B exists in two paths C and D
3. First run only has C on the search path, finds A and B and loads them
4. Second run adds D to the front of the search path. A is loaded and contains a reference to the already compiled module from C. But searching finds the module from D instead, causing a mismatch
5. B and the modules that depend on it are considered out of date and thus rebuilt
6. The recompiled module A is added to the in-memory cache, freeing the previously inserted one

This can never occur from a regular clang process, but is very easy to
do through the API - whether through the use of a shared case or just
running multiple compilations from a single CompilerInstance. Update
the compilation to return early if a module is already finalized so that
the pre-condition in the in-memory module cache holds.

Resolves rdar://78180255

[vsapsai]

This problem shouldn't happen based on module-level name hashing but for some reason it is happening. If I'm not mistaken, InMemoryModuleCache uses .pcm file name as the key. But that file name should contain a hash part that is based on the modulemap path. But frameworks M in "frameworks" and "frameworks2" should have different paths for modulemaps and should have different .pcm names, therefore no conflicts in InMemoryModuleCache.

I see the value in not crashing but looks like there might be a problem somewhere else.

Also it is more of my personal pet peeve. InMemoryModuleCache should simplify memory management for modules but in practice it simplifies creating dangling references. I'm not asking to address that, so if you feel like my requests get too close to that direction, it's good to push back.

[bnbarham]

In D105328#2861028, @vsapsai wrote:

This problem shouldn't happen based on module-level name hashing but for some reason it is happening. If I'm not mistaken, InMemoryModuleCache uses .pcm file name as the key. But that file name should contain a hash part that is based on the modulemap path. But frameworks M in "frameworks" and "frameworks2" should have different paths for modulemaps and should have different .pcm names, therefore no conflicts in InMemoryModuleCache.

Ah sorry, looks like I had my modules slightly confused. The underlying problem is that B is considered out of date due to the check on line 2920, ie. the one that creates the err_imported_module_relocated diagnostic. That causes A to be re-compiled as a module it depends on is out of date. The location of A hasn't changed, so it keeps the same .pcm path.

B is also re-compiled, but as you point out that's perfectly fine since it'll get a different path in the cache.

EDIT: Replace A with Top and B with M for the corresponding modules in the test. Not sure why I used different names in the summary and test...

I'll update the comments in the test + the review summary. Sorry for the confusion.

I see the value in not crashing but looks like there might be a problem somewhere else.

For what it's worth, even if there was another cause I'd still like to get this change in - writing over a module in the cache causes all sorts of different crashes that are hard to diagnose. I'd be *very* surprised if there weren't more issues like this, so at least we'll get an actual error rather than random crashes.

Also it is more of my personal pet peeve. InMemoryModuleCache should simplify memory management for modules but in practice it simplifies creating dangling references. I'm not asking to address that, so if you feel like my requests get too close to that direction, it's good to push back.

Heh, I find this a nice summary of how I feel about it after trying to find the root cause of these crashes!

[bnbarham]

Updated the test comment with the actual cause (ie. Top being recompiled and inserted, not M).

[vsapsai]

clang/unittests/Serialization/ModuleCacheTest.cpp

126–127

Haven't rechecked the code more carefully but had an idea is that if we want to allow InMemoryModuleCache reuse between multiple CompilerInstance, safer API would be to transfer ModuleCache ownership to the new CompilerInstance and maybe make all records in the cache purgeable. But that's applicable only if ModuleCache reuse is important.

[bnbarham]

clang/unittests/Serialization/ModuleCacheTest.cpp

126–127

Every module compilation runs in a separate thread with a separate CompilerInstance (but shared cache). So ownership would have to be transferred back to the original instance once complete. I might be misunderstanding what you mean here though.

Another point to note is that eg. Swift doesn't actually create another CompilerInstance, it's just re-used across module loading and CompilerInstance::loadModule is called directly. I went with this way in the test as otherwise we'd need to inline most of ExecuteAction and have a custom FrontendAction.

[vsapsai]

clang/lib/Frontend/CompilerInstance.cpp

1063

Can we get in infinite loop with AllowPCMWithCompilerErrors = true? Specifically, I'm thinking about the scenario

1. compileModuleAndReadAST obtains a file lock and calls compileModule
2. compileModule calls compileModuleImpl
3. Module is finalized but AllowPCMWithCompilerErrors is true, so compileModuleImpl returns true
4. compileModule returns true
5. compileModuleAndReadAST tries to read AST because compilation was successful
6. AST is out of date, so compileModuleAndReadAST decides to try again, goto 1

Haven't tried to reproduce it locally but even if this scenario is impossible, a corresponding test case can be useful.

clang/lib/Serialization/ASTReader.cpp

2926

I'm thinking if in case of finalized modules diagnostic messages are good enough. One concern is it won't be clear why a module wasn't rebuilt. It can be already confusing for precompiled headers and I'm afraid we won't be able to detect isPCMFinal code path without a debugger. Though I don't know how bad that would be in practice.

Another concern is that retrying a compilation should succeed as for a new process we have a new InMemoryModuleCache and isPCMFinal should return false. So we might have non-deterministic behavior and some of the valid error messages can seem to be non-deterministic and not reliable. I was thinking about adding a note in case we are dealing with isPCMFinal to distinguish these cases but not sure it is a good idea.

5932

Based on the rest of the code in clang, the expectation for diagnose... methods is to emit diagnostic in some cases. Personally, I'm often confused what true/false means for these methods, so I'm thinking about renaming the method to something like isRecoverableOutOfDateModule, canRecoverOutOfDateModule or some such. Feel free to pick a name you believe is appropriate, mine are just examples.

clang/unittests/Serialization/ModuleCacheTest.cpp

126–127

Thanks for the explanation. I haven't considered how the API is used right now, so please discard my earlier comment.

[bnbarham]

clang/lib/Frontend/CompilerInstance.cpp

1063

Nice catch, that does seem likely - I'll see if I can add a test for this.

clang/lib/Serialization/ASTReader.cpp

2926

I'm thinking if in case of finalized modules diagnostic messages are good enough. One concern is it won't be clear why a module wasn't rebuilt. It can be already confusing for precompiled headers and I'm afraid we won't be able to detect isPCMFinal code path without a debugger. Though I don't know how bad that would be in practice.

The error messages will mention a module in the module cache, which would be the main way to tell. We could add a note here as you suggest below, but I'm not quite sure what it would say... something about there being two modules with the same name?

Another concern is that retrying a compilation should succeed as for a new process we have a new InMemoryModuleCache and isPCMFinal should return false. So we might have non-deterministic behavior and some of the valid error messages can seem to be non-deterministic and not reliable. I was thinking about adding a note in case we are dealing with isPCMFinal to distinguish these cases but not sure it is a good idea.

The errors should be deterministic I believe. If one process has the issue then a new one will have the issue as well. For what it's worth, I don't think these crashes are possible from the clang frontend. They require messing around with search paths such that between two compilations in the same process, different modules are found.

5932

Fair enough, canRecoverOutOfDateModule sounds reasonable to me. Or maybe canRecoverFromOutOfDate?

[bnbarham]

clang/lib/Frontend/CompilerInstance.cpp

1063

It doesn't end up causing an infinite recursion as false will end up being returned from compileModuleAndReadAST, but in that case there's no point returning true from compileModuleImpl in the first place so I've changed it anyway. Have also added that as a test case, just to make sure.

clang/lib/Serialization/ASTReader.cpp

2926

I've added an extra note "this is generally caused by modules with the same name found in multiple paths". So the diagnostics would now be:

/.../test.m:2:10: fatal error: module 'M' was built in directory '/.../frameworks/M.framework' but now resides in directory 'frameworks2/M.framework'
        @import Top;
         ^
/.../test.m:2:10: note: imported by module 'Top' in '/path/to/module/cache/LGSY9KSYAKN1/Top-1MZE9QJ1AHENQ.pcm
/.../test.m:2:10: note: this is generally caused by modules with the same name found in multiple paths

I was thinking of mentioning the finalized module, but that's really just a side effect of the error that should have already been output (ie. module 'M' was built in... in this case). So the existence of this note should help point users to the problem + make which path caused the error obvious when looking at the code.

5932

I went with the latter.

[vsapsai]

clang/lib/Frontend/CompilerInstance.cpp

1063

Thanks for investigating it and adding a test case.

And appreciate that the error message mentions the module name. It is so hard to work with errors like "cannot rebuild this module".

clang/lib/Serialization/ASTReader.cpp

2926

I like the note. Think it gives a good hint at what's wrong and we don't need to improve it further now.

The errors should be deterministic I believe. If one process has the issue then a new one will have the issue as well. For what it's worth, I don't think these crashes are possible from the clang frontend. They require messing around with search paths such that between two compilations in the same process, different modules are found.

Thanks for the explanation. I was thinking about triggering the error not through API but through clang frontend. But I see that through API the error should be deterministic.

5932

I think with the current naming we need to flip the conditions to opposite. Currently,

return !(ClientLoadCapabilities & ARR_OutOfDate) ||
       getModuleManager().getModuleCache().isPCMFinal(ModuleFileName);

corresponds to cannotRecoverFromOutOfDate. But to avoid double negations it is better to have canRecoverFromOutOfDate (with (ClientLoadCapabilities & ARR_OutOfDate) && !isPCMFinal(ModuleFileName)) and call it as !canRecoverFromOutOfDate when necessary.

[bnbarham]

clang/lib/Frontend/CompilerInstance.cpp

1063

Note that this error is really just a fallback - it will be silenced by the previous fatal errors in ASTReader.

clang/lib/Serialization/ASTReader.cpp

2859–2860

The failing test was because this note was being output when if OutOfDate wasn't in Capabilities. It should only get output when OutOfDate can be handled (ie. it'd be recompiled) *and* the module is already finalized.

5932

Argh, sorry - just did a find + replace without thinking. Have flipped that function and its calls.

[vsapsai]

Have 1 punctuation nit (that I'm not sure about), the rest looks good.

clang/lib/Serialization/ASTReader.cpp

2854

I don't remember recommended LLVM style or if clang-tidy would complain about it but ... & ... && ... can be unclear regarding the priority of operations. Personally, I would do (... & ...) && ... but I don't know what is the rule, so not insisting.

[bnbarham]

clang/lib/Serialization/ASTReader.cpp

2854

I don't feel particularly strongly about it either way, so have just updated to add the parentheses :).

Also renamed recompileFinalized to recompilingFinalized as I thought that made more sense after coming back to it.

[bnbarham]

I don't have commit access. @vsapsai or @akyrtzi would you mind committing this? The test failures seem unrelated to me.

clang/include/clang/Basic/DiagnosticCommonKinds.td

def remark_module_lock_timeout : Remark<
   "timed out waiting to acquire lock file for module '%0'">, InGroup<ModuleBuild>;
 def err_module_shadowed : Error<"import of shadowed module '%0'">, DefaultFatal;
 def err_module_build_shadowed_submodule : Error<
   "build a shadowed submodule '%0'">, DefaultFatal;
 def err_module_cycle : Error<"cyclic dependency in module '%0': %1">,
   DefaultFatal;
 def err_module_prebuilt : Error<
   "error in loading module '%0' from prebuilt module path">, DefaultFatal;
+def err_module_rebuild_finalized : Error<
+  "cannot rebuild module '%0' as it is already finalized">, DefaultFatal;
 def note_pragma_entered_here : Note<"#pragma entered here">;
 def note_decl_hiding_tag_type : Note<
   "%1 %0 is hidden by a non-type declaration of %0 here">;
 def err_attribute_not_type_attr : Error<
   "%0 attribute cannot be applied to types">;
 def err_enum_template : Error<"enumeration cannot be a template">;
 
 def warn_cxx20_compat_consteval : Warning<

clang/include/clang/Basic/DiagnosticSerializationKinds.td

 def err_ast_file_invalid : Error<
   "file '%1' is not a valid precompiled %select{PCH|module|AST}0 file">, DefaultFatal;
 def note_module_file_imported_by : Note<
   "imported by %select{|module '%2' in }1'%0'">;
 def err_module_file_not_module : Error<
   "AST file '%0' was not built as a module">, DefaultFatal;
 def err_module_file_missing_top_level_submodule : Error<
   "module file '%0' is missing its top-level submodule">, DefaultFatal;
+def note_module_file_conflict : Note<
+  "this is generally caused by modules with the same name found in multiple "
+  "paths">;
 
 def remark_module_import : Remark<
   "importing module '%0'%select{| into '%3'}2 from '%1'">,
   InGroup<ModuleImport>;
 
 def err_imported_module_not_found : Error<
     "module '%0' in AST file '%1' %select{(imported by AST file '%2') |}4"
     "is not defined in any loaded module map file; "

clang/include/clang/Serialization/ASTReader.h

private:
   std::pair<ModuleFile *, unsigned>
     getModulePreprocessedEntity(unsigned GlobalIndex);
 
   /// Returns (begin, end) pair for the preprocessed entities of a
   /// particular module.
   llvm::iterator_range<PreprocessingRecord::iterator>
   getModulePreprocessedEntities(ModuleFile &Mod) const;
 
+  bool canRecoverFromOutOfDate(StringRef ModuleFileName,
+                               unsigned ClientLoadCapabilities);
+
 public:
   class ModuleDeclIterator
       : public llvm::iterator_adaptor_base<
             ModuleDeclIterator, const serialization::LocalDeclID *,
             std::random_access_iterator_tag, const Decl *, ptrdiff_t,
             const Decl *, const Decl *> {
     ASTReader *Reader = nullptr;
     ModuleFile *Mod = nullptr;

clang/lib/Frontend/CompilerInstance.cpp

compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
                   StringRef ModuleName, FrontendInputFile Input,
                   StringRef OriginalModuleMapFile, StringRef ModuleFileName,
                   llvm::function_ref<void(CompilerInstance &)> PreBuildStep =
                       [](CompilerInstance &) {},
                   llvm::function_ref<void(CompilerInstance &)> PostBuildStep =
                       [](CompilerInstance &) {}) {
   llvm::TimeTraceScope TimeScope("Module Compile", ModuleName);
 
+  // Never compile a module that's already finalized - this would cause the
+  // existing module to be freed, causing crashes if it is later referenced
+  if (ImportingInstance.getModuleCache().isPCMFinal(ModuleFileName)) {
+    ImportingInstance.getDiagnostics().Report(
+        ImportLoc, diag::err_module_rebuild_finalized)
+        << ModuleName;
+    return false;
+  }
+
   // Construct a compiler invocation for creating this module.
   auto Invocation =
       std::make_shared<CompilerInvocation>(ImportingInstance.getInvocation());
 
   PreprocessorOptions &PPOpts = Invocation->getPreprocessorOpts();
 
   // For any options that aren't intended to affect how a module is built,
   // reset them to their default values.

clang/lib/Serialization/ASTReader.cpp

case METADATA: {
         return VersionMismatch;
       }
 
       bool hasErrors = Record[6];
       if (hasErrors && !DisableValidation) {
         // If requested by the caller and the module hasn't already been read
         // or compiled, mark modules on error as out-of-date.
         if ((ClientLoadCapabilities & ARR_TreatModuleWithErrorsAsOutOfDate) &&
-            !ModuleMgr.getModuleCache().isPCMFinal(F.FileName))
+            canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
           return OutOfDate;
 
         if (!AllowASTWithCompilerErrors) {
           Diag(diag::err_pch_with_compiler_errors);
           return HadErrors;
         }
       }
       if (hasErrors) {
case IMPORTS: {
           Capabilities &= ~ARR_Missing;
 
         // Load the AST file.
         auto Result = ReadASTCore(ImportedFile, ImportedKind, ImportLoc, &F,
                                   Loaded, StoredSize, StoredModTime,
                                   StoredSignature, Capabilities);
 
         // If we diagnosed a problem, produce a backtrace.
-        if (isDiagnosedResult(Result, Capabilities))
+        bool recompilingFinalized =
+            Result == OutOfDate && (Capabilities & ARR_OutOfDate) &&
+            getModuleManager().getModuleCache().isPCMFinal(F.FileName);
+        if (isDiagnosedResult(Result, Capabilities) || recompilingFinalized)
           Diag(diag::note_module_file_imported_by)
               << F.FileName << !F.ModuleName.empty() << F.ModuleName;
+        if (recompilingFinalized)
+          Diag(diag::note_module_file_conflict);
 
         switch (Result) {
         case Failure: return Failure;
           // If we have to ignore the dependency, we'll have to ignore this too.
         case Missing:
         case OutOfDate: return OutOfDate;
         case VersionMismatch: return VersionMismatch;
         case ConfigurationMismatch: return ConfigurationMismatch;
case MODULE_DIRECTORY: {
         // If we're implicitly loading a module, the base directory can't
         // change between the build and use.
         // Don't emit module relocation error if we have -fno-validate-pch
         if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
                   DisableValidationForModuleKind::Module) &&
             F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) {
           auto BuildDir = PP.getFileManager().getDirectory(Blob);
           if (!BuildDir || *BuildDir != M->Directory) {
-            if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+            if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
               Diag(diag::err_imported_module_relocated)
                   << F.ModuleName << Blob << M->Directory->getName();
             return OutOfDate;
           }
         }
         F.BaseDirectory = std::string(M->Directory->getName());
       } else {
         F.BaseDirectory = std::string(Blob);
if (F.Kind == MK_ImplicitModule && ModuleMgr.begin()->Kind != MK_MainFile) {
     // module map file that we've already loaded.
     Module *M = PP.getHeaderSearchInfo().lookupModule(F.ModuleName);
     auto &Map = PP.getHeaderSearchInfo().getModuleMap();
     const FileEntry *ModMap = M ? Map.getModuleMapFileForUniquing(M) : nullptr;
     // Don't emit module relocation error if we have -fno-validate-pch
     if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
               DisableValidationForModuleKind::Module) &&
         !ModMap) {
-      if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) {
+      if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) {
         if (auto ASTFE = M ? M->getASTFile() : None) {
           // This module was defined by an imported (explicit) module.
           Diag(diag::err_module_file_conflict) << F.ModuleName << F.FileName
                                                << ASTFE->getName();
         } else {
           // This module was built with a different module map.
           Diag(diag::err_imported_module_not_found)
               << F.ModuleName << F.FileName
if (F.Kind == MK_ImplicitModule && ModuleMgr.begin()->Kind != MK_MainFile) {
 
     // Check the primary module map file.
     auto StoredModMap = FileMgr.getFile(F.ModuleMapPath);
     if (!StoredModMap || *StoredModMap != ModMap) {
       assert(ModMap && "found module is missing module map file");
       assert((ImportedBy || F.Kind == MK_ImplicitModule) &&
              "top-level import should be verified");
       bool NotImported = F.Kind == MK_ImplicitModule && !ImportedBy;
-      if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+      if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
         Diag(diag::err_imported_module_modmap_changed)
             << F.ModuleName << (NotImported ? F.FileName : ImportedBy->FileName)
             << ModMap->getName() << F.ModuleMapPath << NotImported;
       return OutOfDate;
     }
 
     llvm::SmallPtrSet<const FileEntry *, 1> AdditionalStoredMaps;
     for (unsigned I = 0, N = Record[Idx++]; I < N; ++I) {
       // FIXME: we should use input files rather than storing names.
       std::string Filename = ReadPath(F, Record, Idx);
-      auto F = FileMgr.getFile(Filename, false, false);
-      if (!F) {
-        if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+      auto SF = FileMgr.getFile(Filename, false, false);
+      if (!SF) {
+        if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
           Error("could not find file '" + Filename +"' referenced by AST file");
         return OutOfDate;
       }
-      AdditionalStoredMaps.insert(*F);
+      AdditionalStoredMaps.insert(*SF);
     }
 
     // Check any additional module map files (e.g. module.private.modulemap)
     // that are not in the pcm.
     if (auto *AdditionalModuleMaps = Map.getAdditionalModuleMapFiles(M)) {
       for (const FileEntry *ModMap : *AdditionalModuleMaps) {
         // Remove files that match
         // Note: SmallPtrSet::erase is really remove
         if (!AdditionalStoredMaps.erase(ModMap)) {
-          if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+          if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
             Diag(diag::err_module_different_modmap)
               << F.ModuleName << /*new*/0 << ModMap->getName();
           return OutOfDate;
         }
       }
     }
 
     // Check any additional module map files that are in the pcm, but not
     // found in header search. Cases that match are already removed.
     for (const FileEntry *ModMap : AdditionalStoredMaps) {
-      if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
+      if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
         Diag(diag::err_module_different_modmap)
           << F.ModuleName << /*not new*/1 << ModMap->getName();
       return OutOfDate;
     }
   }
 
   if (Listener)
     Listener->ReadModuleMapFile(F.ModuleMapPath);
ASTReader::getModulePreprocessedEntities(ModuleFile &Mod) const {
   if (PreprocessingRecord *PPRec = PP.getPreprocessingRecord())
     return PPRec->getIteratorsForLoadedRange(Mod.BasePreprocessedEntityID,
                                              Mod.NumPreprocessedEntities);
 
   return llvm::make_range(PreprocessingRecord::iterator(),
                           PreprocessingRecord::iterator());
 }
 
+bool ASTReader::canRecoverFromOutOfDate(StringRef ModuleFileName,
+                                        unsigned int ClientLoadCapabilities) {
+  return ClientLoadCapabilities & ARR_OutOfDate &&
+         !getModuleManager().getModuleCache().isPCMFinal(ModuleFileName);
+}
+
 llvm::iterator_range<ASTReader::ModuleDeclIterator>
 ASTReader::getModuleFileLevelDecls(ModuleFile &Mod) {
   return llvm::make_range(
       ModuleDeclIterator(this, &Mod, Mod.FileSortedDecls),
       ModuleDeclIterator(this, &Mod,
                          Mod.FileSortedDecls + Mod.NumFileSortedDecls));
 }
 

clang/unittests/Serialization/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BitReader
   BitstreamReader
   Support
   )
 
 add_clang_unittest(SerializationTests
   InMemoryModuleCacheTest.cpp
+  ModuleCacheTest.cpp
   )
 
 clang_target_link_libraries(SerializationTests
   PRIVATE
   clangAST
   clangBasic
+  clangFrontend
   clangLex
   clangSema
   clangSerialization
   )

clang/unittests/Serialization/ModuleCacheTest.cpp

+//===- unittests/Serialization/ModuleCacheTest.cpp - CI tests -------------===//
+//
+// 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/Basic/FileManager.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class ModuleCacheTest : public ::testing::Test {
+  void SetUp() override {
+    ASSERT_FALSE(sys::fs::createUniqueDirectory("modulecache-test", TestDir));
+
+    ModuleCachePath = SmallString<256>(TestDir);
+    sys::path::append(ModuleCachePath, "mcp");
+    ASSERT_FALSE(sys::fs::create_directories(ModuleCachePath));
+  }
+
+  void TearDown() override { sys::fs::remove_directories(TestDir); }
+
+public:
+  SmallString<256> TestDir;
+  SmallString<256> ModuleCachePath;
+
+  void addFile(StringRef Path, StringRef Contents) {
+    ASSERT_FALSE(sys::path::is_absolute(Path));
+
+    SmallString<256> AbsPath(TestDir);
+    sys::path::append(AbsPath, Path);
+
+    std::error_code EC;
+    ASSERT_FALSE(
+        sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath)));
+    llvm::raw_fd_ostream OS(AbsPath, EC);
+    ASSERT_FALSE(EC);
+    OS << Contents;
+  }
+
+  void addDuplicateFrameworks() {
+    addFile("test.m", R"cpp(
+        @import Top;
+    )cpp");
+
+    addFile("frameworks/Top.framework/Headers/top.h", R"cpp(
+        @import M;
+    )cpp");
+    addFile("frameworks/Top.framework/Modules/module.modulemap", R"cpp(
+        framework module Top [system] {
+          header "top.h"
+          export *
+        }
+    )cpp");
+
+    addFile("frameworks/M.framework/Headers/m.h", R"cpp(
+        void foo();
+    )cpp");
+    addFile("frameworks/M.framework/Modules/module.modulemap", R"cpp(
+        framework module M [system] {
+          header "m.h"
+          export *
+        }
+    )cpp");
+
+    addFile("frameworks2/M.framework/Headers/m.h", R"cpp(
+        void foo();
+    )cpp");
+    addFile("frameworks2/M.framework/Modules/module.modulemap", R"cpp(
+        framework module M [system] {
+          header "m.h"
+          export *
+        }
+    )cpp");
+  }
+};
+
+TEST_F(ModuleCacheTest, CachedModuleNewPath) {
+  addDuplicateFrameworks();
+
+  SmallString<256> MCPArg("-fmodules-cache-path=");
+  MCPArg.append(ModuleCachePath);
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions());
+
+  // First run should pass with no errors
+  const char *Args[] = {"clang",        "-fmodules",          "-Fframeworks",
+                        MCPArg.c_str(), "-working-directory", TestDir.c_str(),
+                        "test.m"};
+  std::shared_ptr<CompilerInvocation> Invocation =
+      createInvocationFromCommandLine(Args, Diags);
+  ASSERT_TRUE(Invocation);
+  CompilerInstance Instance;
+  Instance.setDiagnostics(Diags.get());
+  Instance.setInvocation(Invocation);
+  SyntaxOnlyAction Action;
+  ASSERT_TRUE(Instance.ExecuteAction(Action));
+  ASSERT_FALSE(Diags->hasErrorOccurred());
+
+  // Now add `frameworks2` to the search path. `Top.pcm` will have a reference
+  // to the `M` from `frameworks`, but a search will find the `M` from
+  // `frameworks2` - causing a mismatch and it to be considered out of date.
+  //
+  // Normally this would be fine - `M` and the modules it depends on would be
+  // rebuilt. However, since we have a shared module cache and thus an already
+  // finalized `Top`, recompiling `Top` will cause the existing module to be
+  // removed from the cache, causing possible crashed if it is ever used.
+  //
+  // Make sure that an error occurs instead.
+  const char *Args2[] = {"clang",         "-fmodules",    "-Fframeworks2",
+                         "-Fframeworks",  MCPArg.c_str(), "-working-directory",
+                         TestDir.c_str(), "test.m"};
+  std::shared_ptr<CompilerInvocation> Invocation2 =
+      createInvocationFromCommandLine(Args2, Diags);
+  ASSERT_TRUE(Invocation2);
+  CompilerInstance Instance2(Instance.getPCHContainerOperations(),
+                             &Instance.getModuleCache());
+  Instance2.setDiagnostics(Diags.get());
+  Instance2.setInvocation(Invocation2);
+  SyntaxOnlyAction Action2;
+  ASSERT_FALSE(Instance2.ExecuteAction(Action2));
+  ASSERT_TRUE(Diags->hasErrorOccurred());
+}
+
+TEST_F(ModuleCacheTest, CachedModuleNewPathAllowErrors) {
+  addDuplicateFrameworks();
+
+  SmallString<256> MCPArg("-fmodules-cache-path=");
+  MCPArg.append(ModuleCachePath);
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions());
+
+  // First run should pass with no errors
+  const char *Args[] = {"clang",        "-fmodules",          "-Fframeworks",
+                        MCPArg.c_str(), "-working-directory", TestDir.c_str(),
+                        "test.m"};
+  std::shared_ptr<CompilerInvocation> Invocation =
+      createInvocationFromCommandLine(Args, Diags);
+  ASSERT_TRUE(Invocation);
+  CompilerInstance Instance;
+  Instance.setDiagnostics(Diags.get());
+  Instance.setInvocation(Invocation);
+  SyntaxOnlyAction Action;
+  ASSERT_TRUE(Instance.ExecuteAction(Action));
+  ASSERT_FALSE(Diags->hasErrorOccurred());
+
+  // Same as `CachedModuleNewPath` but while allowing errors. This is a hard
+  // failure where the module wasn't created, so it should still fail.
+  const char *Args2[] = {
+      "clang",         "-fmodules",    "-Fframeworks2",
+      "-Fframeworks",  MCPArg.c_str(), "-working-directory",
+      TestDir.c_str(), "-Xclang",      "-fallow-pcm-with-compiler-errors",
+      "test.m"};
+  std::shared_ptr<CompilerInvocation> Invocation2 =
+      createInvocationFromCommandLine(Args2, Diags);
+  ASSERT_TRUE(Invocation2);
+  CompilerInstance Instance2(Instance.getPCHContainerOperations(),
+                             &Instance.getModuleCache());
+  Instance2.setDiagnostics(Diags.get());
+  Instance2.setInvocation(Invocation2);
+  SyntaxOnlyAction Action2;
+  ASSERT_FALSE(Instance2.ExecuteAction(Action2));
+  ASSERT_TRUE(Diags->hasErrorOccurred());
+}
+
+} // anonymous namespace