[Modules] Do not remove failed modules after the control block phase

[bnbarham]

Summary

Reading modules first reads each control block in the chain and then all
AST blocks.

The first phase is intended to find recoverable errors, eg. an out of
date or missing module. If any error occurs during this phase, it is
safe to remove all modules in the chain as no references to them will
exist.

While reading the AST blocks, however, various fields in ASTReader are
updated with references to the module. Removing modules at this point
can cause dangling pointers which can be accessed later. These would be
otherwise harmless, eg. a binary search over GlobalSLocEntryMap may
access a failed module that could error, but shouldn't crash. Do not
remove modules in this phase, regardless of failures.

Since this is the case, it also doesn't make sense to return OutOfDate
during this phase, so remove the two cases where this happens.

When they were originally added these checks would return a failure when
the serialized and current path didn't match up. That was updated to an
OutOfDate as it was found to be hit when using VFS and overriding the
umbrella. Later on the path was changed to instead be the name as
written in the module file, resolved using the serialized base
directory. At this point the check is really only comparing the name of
the umbrella and only works for frameworks since those don't include
Headers/ in the name (which means the resolved path will never exist)

Given all that, it seems safe to ignore this case entirely for now.
This makes the handling of an umbrella header/directory the same as
regular headers, which also don't check for differences in the path
caused by VFS.

Resolves rdar://79329355

[bnbarham]

I have another change to update the post-control-block functions to llvm::Error instead to hopefully make the distinction more clear as well. Let me know if you think it belongs in this PR, otherwise I'll open another once this one is in.

[vsapsai]

Made the first review pass and return Failure makes sense to me as recovery isn't the best idea at this point. Still want to check more thoroughly if the removed code for SUBMODULE_UMBRELLA_HEADER and SUBMODULE_UMBRELLA_DIR has any load-bearing side-effects.

Have no opinion on updating post-control-block functions to llvm::Error.

clang/test/VFS/module-header-mismatches.m

3

Didn't know about split-file, that's nice.

clang/test/VFS/umbrella-mismatch.m

4

Are you deleting this test case because it is subsumed by module-header-mismatches.m?

Also if it makes sense to delete this test, need to do more cleanup because UsesFoo.framework and Foo.framework seem to be used only from this test. Though I'm not 100% sure.

[bnbarham]

clang/test/VFS/umbrella-mismatch.m

4

Yep, the check in this one is the same as the header-frameworks.

Foo.framework is used by subframework-symlink.m. But UsesFoo.framework isn't used by anything else, thanks.

[bnbarham]

Removed the now unused UsesFoo.framework in the tests

[vsapsai]

Why are you not just changing return OutOfDate to return Failure for SUBMODULE_UMBRELLA_HEADER and SUBMODULE_UMBRELLA_DIR? I have no strong opinion on this but it feels more in line with the rest of the changes. Also I've tried to do that and nothing seems to be broken.

And for the rest of the code for these records I think it is worth keeping ModMap.setUmbrellaHeader and ModMap.setUmbrellaDir with preceding preparation and checks. The code in ModuleMap seems to be complicated enough and stateful, so removing those setters can break some stuff. It is possible everything will be fine but I don't want to chance that unless required.

[bnbarham]

Changed to keep setting the umbrella header/directory with a FIXME that it currently doesn't work for framework modules.

[vsapsai]

Looks good to me. The only nitpicky thing is I don't remember if the code style requires braces around multi-line case blocks or not.

[bnbarham]

Forgot to add the braces back in for the case statements

[yaron.keren]

clang/lib/Serialization/ASTReader.cpp

4268

Result is unused now.

4279

Same here.

[vsapsai]

clang/lib/Serialization/ASTReader.cpp

4268

Thanks for pointing it out. Are there any bots failing because of that now? Asking if should have a small urgent fix or if can wait for https://reviews.llvm.org/D108268 to land.

[yaron.keren]

clang/lib/Serialization/ASTReader.cpp

4268

Just a compiler warning, could wait.

clang/lib/Serialization/ASTReader.cpp

ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
   Deserializing AnASTFile(this);
 
   // Bump the generation number.
   unsigned PreviousGeneration = 0;
   if (ContextObj)
     PreviousGeneration = incrementGeneration(*ContextObj);
 
   unsigned NumModules = ModuleMgr.size();
-  auto removeModulesAndReturn = [&](ASTReadResult ReadResult) {
-    assert(ReadResult && "expected to return error");
+  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);
 
     // 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;
-  };
-
-  SmallVector<ImportedModule, 4> Loaded;
-  switch (ASTReadResult ReadResult =
-              ReadASTCore(FileName, Type, ImportLoc,
-                          /*ImportedBy=*/nullptr, Loaded, 0, 0,
-                          ASTFileSignature(), ClientLoadCapabilities)) {
-  case Failure:
-  case Missing:
-  case OutOfDate:
-  case VersionMismatch:
-  case ConfigurationMismatch:
-  case HadErrors:
-    return removeModulesAndReturn(ReadResult);
-  case Success:
-    break;
   }
 
   // Here comes stuff that we only do once the entire chain is loaded.
 
   // Load the AST blocks of all of the modules that we loaded.  We can still
   // hit errors parsing the ASTs at this point.
   for (ImportedModule &M : Loaded) {
     ModuleFile &F = *M.Mod;
 
     // Read the AST block.
     if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
-      return removeModulesAndReturn(Result);
+      return Failure;
 
     // The AST block should always have a definition for the main module.
     if (F.isModule() && !F.DidReadTopLevelSubmodule) {
       Error(diag::err_module_file_missing_top_level_submodule, F.FileName);
-      return removeModulesAndReturn(Failure);
+      return Failure;
     }
 
     // Read the extension blocks.
     while (!SkipCursorToBlock(F.Stream, EXTENSION_BLOCK_ID)) {
       if (ASTReadResult Result = ReadExtensionBlock(F))
-        return removeModulesAndReturn(Result);
+        return Failure;
     }
 
     // Once read, set the ModuleFile bit base offset and update the size in
     // bits of all files we've seen.
     F.GlobalBitOffset = TotalModulesSizeInBits;
     TotalModulesSizeInBits += F.SizeInBits;
     GlobalBitOffsetsMap.insert(std::make_pair(F.GlobalBitOffset, &F));
   }
case SUBMODULE_DEFINITION: {
       CurrentModule->MissingHeaders.clear();
       CurrentModule->IsUnimportable =
           ParentModule && ParentModule->IsUnimportable;
       CurrentModule->IsAvailable = !CurrentModule->IsUnimportable;
       break;
     }
 
     case SUBMODULE_UMBRELLA_HEADER: {
+      // FIXME: This doesn't work for framework modules as `Filename` is the
+      //        name as written in the module file and does not include
+      //        `Headers/`, so this path will never exist.
       std::string Filename = std::string(Blob);
       ResolveImportedPath(F, Filename);
       if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
-        if (!CurrentModule->getUmbrellaHeader())
+        if (!CurrentModule->getUmbrellaHeader()) {
           // FIXME: NameAsWritten
           ModMap.setUmbrellaHeader(CurrentModule, *Umbrella, Blob, "");
-        else if (CurrentModule->getUmbrellaHeader().Entry != *Umbrella) {
-          if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
-            Error("mismatched umbrella headers in submodule");
-          return OutOfDate;
         }
+        // Note that it's too late at this point to return out of date if the
+        // name from the PCM doesn't match up with the one in the module map,
+        // but also quite unlikely since we will have already checked the
+        // modification time and size of the module map file itself.
       }
       break;
     }
 
     case SUBMODULE_HEADER:
     case SUBMODULE_EXCLUDED_HEADER:
     case SUBMODULE_PRIVATE_HEADER:
       // We lazily associate headers with their modules via the HeaderInfo table.
       // FIXME: Re-evaluate this section; maybe only store InputFile IDs instead
       // of complete filenames or remove it entirely.
       break;
 
     case SUBMODULE_TEXTUAL_HEADER:
     case SUBMODULE_PRIVATE_TEXTUAL_HEADER:
       // FIXME: Textual headers are not marked in the HeaderInfo table. Load
       // them here.
       break;
 
     case SUBMODULE_TOPHEADER:
       CurrentModule->addTopHeaderFilename(Blob);
       break;
 
     case SUBMODULE_UMBRELLA_DIR: {
+      // See comments in SUBMODULE_UMBRELLA_HEADER
       std::string Dirname = std::string(Blob);
       ResolveImportedPath(F, Dirname);
       if (auto Umbrella = PP.getFileManager().getDirectory(Dirname)) {
-        if (!CurrentModule->getUmbrellaDir())
+        if (!CurrentModule->getUmbrellaDir()) {
           // FIXME: NameAsWritten
           ModMap.setUmbrellaDir(CurrentModule, *Umbrella, Blob, "");
-        else if (CurrentModule->getUmbrellaDir().Entry != *Umbrella) {
-          if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
-            Error("mismatched umbrella directories in submodule");
-          return OutOfDate;
         }
       }
       break;
     }
 
     case SUBMODULE_METADATA: {
       F.BaseSubmoduleID = getTotalNumSubmodules();
       F.LocalNumSubmodules = Record[0];

clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h

-@import Foo;

clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap

-framework module UsesFoo {
-  umbrella header "UsesFoo.h"
-  export *
-}

clang/test/VFS/module-header-mismatches.m

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is different (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a different copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+@import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+  umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+  umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+  header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+  umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+  'version': 0,
+  'roots': [
+    { 'name': 'TEST_DIR', 'type': 'directory',
+      'contents': [
+        { 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+          'type': 'file',
+          'external-contents': 'TEST_DIR/other/Mismatch.h'
+        },
+        { 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+          'type': 'directory',
+          'external-contents': 'TEST_DIR/others'
+        },
+        { 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+          'type': 'file',
+          'external-contents': 'TEST_DIR/other/Mismatch.h'
+        },
+        { 'name': 'mod/Mismatch.h',
+          'type': 'file',
+          'external-contents': 'TEST_DIR/other/Mismatch.h'
+        }
+      ]
+    }
+  ]
+}
+

clang/test/VFS/umbrella-mismatch.m

-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
-@import UsesFoo;