[Serialization] Fix crash writing non-module PCH in non-module mode including modular headers

[sammccall]

Summary

Ran into this when trying to turn off modules mode in clangd.

Took me a long time to (mostly) work out the logic behind this function
so documented my understanding, hopefully it's correct!

clang/lib/Serialization/ASTWriter.cpp

Stream.EmitRecordWithBlob(PPESkippedRangeAbbrev, Record,
                               bytes(SerializedSkippedRanges));
   }
 }
 
 unsigned ASTWriter::getLocalOrImportedSubmoduleID(const Module *Mod) {
   if (!Mod)
     return 0;
 
+  // Is there already an ID assigned to this module?
   auto Known = SubmoduleIDs.find(Mod);
   if (Known != SubmoduleIDs.end())
     return Known->second;
 
+  // If Mod is part of a loaded module, its ID is assigned in the ModuleRead
+  // callback. Otherwise we must assign an ID.
+  bool IDAssignedOnImport = [&]{
+    // If we're simulating modules (e.g. -fmodule-map-file without -fmodules)
+    // then no reading happens.
+    if (!getLangOpts().Modules)
+      return false;
+    // If this is a submodule of the module we're building, it's not loaded.
     auto *Top = Mod->getTopLevelModule();
-  if (Top != WritingModule &&
-      (getLangOpts().CompilingPCH ||
-       !Top->fullModuleNameIs(StringRef(getLangOpts().CurrentModule))))
-    return 0;
+    if (Top == WritingModule)
+      return false;
+    // FIXME: This name-based check is similar to above, why is it needed?
+    // When building a module PCH, #includes of same-module headers are textual.
+    if (!getLangOpts().CompilingPCH &&
+        Top->fullModuleNameIs(StringRef(getLangOpts().CurrentModule)))
+      return false;
+    // Unrelated module that must be loaded.
+    return true;
+  }();
 
+  if (IDAssignedOnImport)
+    return 0;
   return SubmoduleIDs[Mod] = NextSubmoduleID++;
 }
 
 unsigned ASTWriter::getSubmoduleID(Module *Mod) {
   unsigned ID = getLocalOrImportedSubmoduleID(Mod);
   // FIXME: This can easily happen, if we have a reference to a submodule that
   // did not result in us loading a module file for that submodule. For
   // instance, a cross-top-level-module 'conflict' declaration will hit this.

clang/test/PCH/preamble-modules.cpp

 // Check that modules included in the preamble remain visible to the rest of the
 // file.
 
 // RUN: rm -rf %t.mcp
 // RUN: %clang_cc1 -emit-pch -o %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
 // RUN: %clang_cc1 -include-pch %t.pch %s -fmodules -fmodule-map-file=%S/Inputs/modules/module.modulemap -fmodules-local-submodule-visibility -fmodules-cache-path=%t.mcp
 
+// Check we can build a PCH with modular includes but no -fmodules. (https://github.com/llvm/llvm-project/issues/59027)
+// RUN: %clang_cc1 -emit-pch -o %t.pch %s -fmodule-map-file=%S/Inputs/modules/module.modulemap
+
 #ifndef MAIN_FILE
 #define MAIN_FILE
 // Premable section.
 #include "Inputs/modules/Foo.h"
 #else
 // Main section.
 MyType foo;
 #endif