jansvoboda11 created this revision.
jansvoboda11 added a reviewer: benlangmuir.
Herald added a subscriber: ributzka.
Herald added a project: All.
jansvoboda11 requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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 ommited 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 <https://reviews.llvm.org/D114095> (that
changed the logic for determining the set of included files) and got broken
again in D155131 <https://reviews.llvm.org/D155131> (which is essentially a
revert of the former).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D157559
Files:
clang/lib/Lex/ModuleMap.cpp
clang/test/Modules/pch-with-module-name-import-twice.c
Index: clang/test/Modules/pch-with-module-name-import-twice.c
===================================================================
--- /dev/null
+++ 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
Index: clang/lib/Lex/ModuleMap.cpp
===================================================================
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1268,8 +1268,7 @@
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.
Index: clang/test/Modules/pch-with-module-name-import-twice.c
===================================================================
--- /dev/null
+++ 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
Index: clang/lib/Lex/ModuleMap.cpp
===================================================================
--- clang/lib/Lex/ModuleMap.cpp
+++ clang/lib/Lex/ModuleMap.cpp
@@ -1268,8 +1268,7 @@
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.
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
