[PATCH] D138193: [Serialization] Fix crash writing non-module PCH in non-module mode including modular headers
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Nov 17 03:13:53 PST 2022
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>
More information about the cfe-commits
mailing list