[clang] bbdb0c7 - [clang][modules] Respect "-fmodule-name=" when serializing included files into a PCH

[Jan Svoboda via cfe-commits]

Author: Jan Svoboda
Date: 2023-08-10T10:11:43-07:00
New Revision: bbdb0c7e4496b145a5e9354f951eec272695049d

URL: https://github.com/llvm/llvm-project/commit/bbdb0c7e4496b145a5e9354f951eec272695049d
DIFF: https://github.com/llvm/llvm-project/commit/bbdb0c7e4496b145a5e9354f951eec272695049d.diff

LOG: [clang][modules] Respect "-fmodule-name=" when serializing included files into a PCH

Clang writes the set of textually included files into AST files, so that importers know to avoid including those files again and instead deserialize their contents from the AST on-demand.

Logic for determining the set of included files files only considers headers that are either non-modular or that are modular but with `HeaderFileInfo::isCompilingModuleHeader` set. Logic for computing that bit is different than the one that determines whether to include a header textually with the "-fmodule-name=Mod" option. That can lead to header from module "Mod" being included textually in a PCH, but be omitted in the serialized set of included files. This can then allow such header to be textually included from importer of the PCH, wreaking havoc.

This patch fixes that by aligning the logic for computing `HeaderFileInfo::isCompilingModuleHeader` with the logic for deciding whether to include modular header textually.

As far as I can tell, this bug has been in Clang for forever. It got accidentally "fixed" by D114095 (that changed the logic for determining the set of included files) and got broken again in D155131 (which is essentially a revert of the former).

rdar://113520515

Reviewed By: benlangmuir

Differential Revision: https://reviews.llvm.org/D157559

Added: 
    clang/test/Modules/pch-with-module-name-import-twice.c

Modified: 
    clang/lib/Lex/ModuleMap.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/Lex/ModuleMap.cpp b/clang/lib/Lex/ModuleMap.cpp
index 5a1b0a918caab1..f8b767e1b5eb80 100644
--- a/clang/lib/Lex/ModuleMap.cpp
+++ b/clang/lib/Lex/ModuleMap.cpp
@@ -1268,8 +1268,7 @@ void ModuleMap::addHeader(Module *Mod, Module::Header Header,
   HeaderList.push_back(KH);
   Mod->Headers[headerRoleToKind(Role)].push_back(Header);
 
-  bool isCompilingModuleHeader =
-      LangOpts.isCompilingModule() && Mod->getTopLevelModule() == SourceModule;
+  bool isCompilingModuleHeader = Mod->isForBuilding(LangOpts);
   if (!Imported || isCompilingModuleHeader) {
     // When we import HeaderFileInfo, the external source is expected to
     // set the isModuleHeader flag itself.

diff  --git a/clang/test/Modules/pch-with-module-name-import-twice.c b/clang/test/Modules/pch-with-module-name-import-twice.c
new file mode 100644
index 00000000000000..67932a4ba46aa6
--- /dev/null
+++ b/clang/test/Modules/pch-with-module-name-import-twice.c
@@ -0,0 +1,19 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// This test checks that headers that are part of a module named by
+// -fmodule-name= don't get included again if previously included from a PCH.
+
+//--- include/module.modulemap
+module Mod { header "Mod.h" }
+//--- include/Mod.h
+struct Symbol {};
+//--- pch.h
+#import "Mod.h"
+//--- tu.c
+#import "Mod.h" // expected-no-diagnostics
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -fmodule-name=Mod -I %t/include \
+// RUN:   -emit-pch -x c-header %t/pch.h -o %t/pch.pch
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache -fimplicit-module-maps -fmodule-name=Mod -I %t/include \
+// RUN:   -fsyntax-only %t/tu.c -include-pch %t/pch.pch -verify