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

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added a project: All.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, ilya-biryukov.
Herald added a project: clang.

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!


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D138193

Files:
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/PCH/preamble-modules.cpp


Index: clang/test/PCH/preamble-modules.cpp
===================================================================
--- clang/test/PCH/preamble-modules.cpp
+++ clang/test/PCH/preamble-modules.cpp
@@ -5,6 +5,9 @@
 // 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.
Index: clang/lib/Serialization/ASTWriter.cpp
===================================================================
--- clang/lib/Serialization/ASTWriter.cpp
+++ clang/lib/Serialization/ASTWriter.cpp
@@ -2641,16 +2641,33 @@
   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;
 
-  auto *Top = Mod->getTopLevelModule();
-  if (Top != WritingModule &&
-      (getLangOpts().CompilingPCH ||
-       !Top->fullModuleNameIs(StringRef(getLangOpts().CurrentModule))))
-    return 0;
+  // 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)
+      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++;
 }
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D138193.476068.patch
Type: text/x-patch
Size: 2398 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20221117/2ee1f06a/attachment.bin>