[modules] Fix error "malformed or corrupted AST file: 'SourceLocation remap refers to unknown module...'".

[vsapsai]

Summary

When a framework can be found at a new location, all references to it in
the module cache become outdated. When we try to load such outdated .pcm
file, we shouldn't change any already loaded and processed modules.

If Module has ASTFile, it means we've read its AST block already and
it is too late to undo that. If ASTFile is None, there is no value
in setting it to None again. So we don't reset ASTFile in
ModuleManager::removeModules at all.

rdar://97216258

[ChuanqiXu]

clang/lib/Serialization/ModuleManager.cpp

284–286

minor: we can remove the { here.

[bnbarham]

LGTM.

When we try to load such outdated .pcm file, we shouldn't change any already loaded and processed modules.

Or to put another way - we can't remove these modules because they may already have references... anywhere.

[vsapsai]

Thanks for the review!

clang/lib/Serialization/ModuleManager.cpp

284–286

Done in actual commit.

clang/include/clang/Basic/Module.h

public:
 
   /// The serialized AST file for this module, if one was created.
   OptionalFileEntryRefDegradesToFileEntryPtr getASTFile() const {
     return getTopLevelModule()->ASTFile;
   }
 
   /// Set the serialized AST file for the top-level module of this module.
   void setASTFile(Optional<FileEntryRef> File) {
-    assert((!File || !getASTFile() || getASTFile() == File) &&
-           "file path changed");
+    assert((!getASTFile() || getASTFile() == File) && "file path changed");
     getTopLevelModule()->ASTFile = File;
   }
 
   /// Retrieve the directory for which this module serves as the
   /// umbrella.
   DirectoryName getUmbrellaDir() const;
 
   /// Retrieve the header that serves as the umbrella header for this

clang/include/clang/Serialization/ModuleManager.h

AddModuleResult addModule(StringRef FileName, ModuleKind Type,
                             ModuleFile *ImportedBy, unsigned Generation,
                             off_t ExpectedSize, time_t ExpectedModTime,
                             ASTFileSignature ExpectedSignature,
                             ASTFileSignatureReader ReadSignature,
                             ModuleFile *&Module,
                             std::string &ErrorStr);
 
   /// Remove the modules starting from First (to the end).
-  void removeModules(ModuleIterator First, ModuleMap *modMap);
+  void removeModules(ModuleIterator First);
 
   /// Add an in-memory buffer the list of known buffers
   void addInMemoryBuffer(StringRef FileName,
                          std::unique_ptr<llvm::MemoryBuffer> Buffer);
 
   /// Set the global module index.
   void setGlobalIndex(GlobalModuleIndex *Index);
 

clang/lib/Serialization/ASTReader.cpp

if (ContextObj)
     PreviousGeneration = incrementGeneration(*ContextObj);
 
   unsigned NumModules = ModuleMgr.size();
   SmallVector<ImportedModule, 4> Loaded;
   if (ASTReadResult ReadResult =
           ReadASTCore(FileName, Type, ImportLoc,
                       /*ImportedBy=*/nullptr, Loaded, 0, 0, ASTFileSignature(),
                       ClientLoadCapabilities)) {
-    ModuleMgr.removeModules(ModuleMgr.begin() + NumModules,
-                            PP.getLangOpts().Modules
-                                ? &PP.getHeaderSearchInfo().getModuleMap()
-                                : nullptr);
+    ModuleMgr.removeModules(ModuleMgr.begin() + NumModules);
 
     // If we find that any modules are unusable, the global index is going
     // to be out-of-date. Just remove it.
     GlobalIndex.reset();
     ModuleMgr.setGlobalIndex(nullptr);
     return ReadResult;
   }
 

clang/lib/Serialization/ModuleManager.cpp

if (!NewModule->isModule())
     PCHChain.push_back(NewModule.get());
   if (!ImportedBy)
     Roots.push_back(NewModule.get());
 
   Chain.push_back(std::move(NewModule));
   return NewlyLoaded;
 }
 
-void ModuleManager::removeModules(ModuleIterator First, ModuleMap *modMap) {
+void ModuleManager::removeModules(ModuleIterator First) {
   auto Last = end();
   if (First == Last)
     return;
 
   // Explicitly clear VisitOrder since we might not notice it is stale.
   VisitOrder.clear();
 
   // Collect the set of module file pointers that we'll be removing.
void ModuleManager::removeModules(ModuleIterator First) {
   // Remove the modules from the PCH chain.
   for (auto I = First; I != Last; ++I) {
     if (!I->isModule()) {
       PCHChain.erase(llvm::find(PCHChain, &*I), PCHChain.end());
       break;
     }
   }
 
-  // Delete the modules and erase them from the various structures.
+  // Delete the modules.
   for (ModuleIterator victim = First; victim != Last; ++victim) {
     Modules.erase(victim->File);
-
-    if (modMap) {
-      StringRef ModuleName = victim->ModuleName;
-      if (Module *mod = modMap->findModule(ModuleName)) {
-        mod->setASTFile(None);
-      }
-    }
   }
 
-  // Delete the modules.
   Chain.erase(Chain.begin() + (First - begin()), Chain.end());
 }
 
 void
 ModuleManager::addInMemoryBuffer(StringRef FileName,
                                  std::unique_ptr<llvm::MemoryBuffer> Buffer) {
   const FileEntry *Entry =
       FileMgr.getVirtualFile(FileName, Buffer->getBufferSize(), 0);

clang/test/Modules/dependent-module-different-location.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+//
+// At first build Stable.pcm that references Movable.framework from StableFrameworks.
+// RUN: %clang_cc1 -fsyntax-only -F %t/JustBuilt -F %t/StableFrameworks %t/prepopulate-module-cache.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+//
+// Now add Movable.framework to JustBuilt.
+// RUN: mkdir %t/JustBuilt
+// RUN: cp -r %t/StableFrameworks/Movable.framework %t/JustBuilt/Movable.framework
+//
+// Load Movable.pcm at first for JustBuilt location and then in the same TU try to load transitively for StableFrameworks location.
+// RUN: %clang_cc1 -fsyntax-only -F %t/JustBuilt -F %t/StableFrameworks %t/trigger-error.m \
+// RUN:            -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/modules.cache
+
+// Test the case when a dependent module is found in a different location, so
+// module cache has outdated information. <rdar://97216258>
+
+//--- StableFrameworks/Movable.framework/Headers/Movable.h
+// empty
+
+//--- StableFrameworks/Movable.framework/Modules/module.modulemap
+framework module Movable {
+  header "Movable.h"
+  export *
+}
+
+
+//--- StableFrameworks/Stable.framework/Headers/Stable.h
+#import <Movable/Movable.h>
+
+//--- StableFrameworks/Stable.framework/Modules/module.modulemap
+framework module Stable {
+  header "Stable.h"
+  export *
+}
+
+
+//--- prepopulate-module-cache.m
+#import <Stable/Stable.h>
+
+//--- trigger-error.m
+#import <Movable/Movable.h>
+#import <Stable/Stable.h>