[clang] e06a91c - [clang][modules] Avoid re-exporting PCH imports on every later module import
Ben Langmuir via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 20 11:29:57 PDT 2023
Author: Ben Langmuir
Date: 2023-04-20T11:29:23-07:00
New Revision: e06a91c5996b039cacd55e6ead0baf14424c740c
URL: https://github.com/llvm/llvm-project/commit/e06a91c5996b039cacd55e6ead0baf14424c740c
DIFF: https://github.com/llvm/llvm-project/commit/e06a91c5996b039cacd55e6ead0baf14424c740c.diff
LOG: [clang][modules] Avoid re-exporting PCH imports on every later module import
We only want to make PCH imports visible once for the the TU, not
repeatedly after every subsequent import. This causes some incorrect
behaviour with submodule visibility, and causes us to get extra module
dependencies in the scanner. So far I have only seen obviously incorrect
behaviour when building with -fmodule-name to cause a submodule to be
textually included when using the PCH, though the old behaviour seems
wrong regardless.
rdar://107449644
Differential Revision: https://reviews.llvm.org/D148176
Added:
clang/test/ClangScanDeps/modules-pch-imports.c
clang/test/Modules/submodule-visibility-pch.c
Modified:
clang/include/clang/Serialization/ASTReader.h
clang/lib/Serialization/ASTReader.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Serialization/ASTReader.h b/clang/include/clang/Serialization/ASTReader.h
index 4f9457d8653e3..1360ee1877c1a 100644
--- a/clang/include/clang/Serialization/ASTReader.h
+++ b/clang/include/clang/Serialization/ASTReader.h
@@ -948,8 +948,14 @@ class ASTReader
private:
/// A list of modules that were imported by precompiled headers or
- /// any other non-module AST file.
- SmallVector<ImportedSubmodule, 2> ImportedModules;
+ /// any other non-module AST file and have not yet been made visible. If a
+ /// module is made visible in the ASTReader, it will be transfered to
+ /// \c PendingImportedModulesSema.
+ SmallVector<ImportedSubmodule, 2> PendingImportedModules;
+
+ /// A list of modules that were imported by precompiled headers or
+ /// any other non-module AST file and have not yet been made visible for Sema.
+ SmallVector<ImportedSubmodule, 2> PendingImportedModulesSema;
//@}
/// The system include root to be used when loading the
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 9dbe56123e40e..b27304deb33c9 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -3728,7 +3728,7 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
SourceLocation Loc = ReadSourceLocation(F, Record, I);
if (GlobalID) {
- ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
+ PendingImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
if (DeserializationListener)
DeserializationListener->ModuleImportRead(GlobalID, Loc);
}
@@ -4445,8 +4445,8 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
UnresolvedModuleRefs.clear();
if (Imported)
- Imported->append(ImportedModules.begin(),
- ImportedModules.end());
+ Imported->append(PendingImportedModules.begin(),
+ PendingImportedModules.end());
// FIXME: How do we load the 'use'd modules? They may not be submodules.
// Might be unnecessary as use declarations are only used to build the
@@ -5050,7 +5050,7 @@ void ASTReader::InitializeContext() {
// Re-export any modules that were imported by a non-module AST file.
// FIXME: This does not make macro-only imports visible again.
- for (auto &Import : ImportedModules) {
+ for (auto &Import : PendingImportedModules) {
if (Module *Imported = getSubmodule(Import.ID)) {
makeModuleVisible(Imported, Module::AllVisible,
/*ImportLoc=*/Import.ImportLoc);
@@ -5060,6 +5060,10 @@ void ASTReader::InitializeContext() {
// nullptr here, we do the same later, in UpdateSema().
}
}
+
+ // Hand off these modules to Sema.
+ PendingImportedModulesSema.append(PendingImportedModules);
+ PendingImportedModules.clear();
}
void ASTReader::finalizeForWriting() {
@@ -8105,13 +8109,14 @@ void ASTReader::UpdateSema() {
}
// For non-modular AST files, restore visiblity of modules.
- for (auto &Import : ImportedModules) {
+ for (auto &Import : PendingImportedModulesSema) {
if (Import.ImportLoc.isInvalid())
continue;
if (Module *Imported = getSubmodule(Import.ID)) {
SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
}
}
+ PendingImportedModulesSema.clear();
}
IdentifierInfo *ASTReader::get(StringRef Name) {
diff --git a/clang/test/ClangScanDeps/modules-pch-imports.c b/clang/test/ClangScanDeps/modules-pch-imports.c
new file mode 100644
index 0000000000000..63c6055efe061
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-pch-imports.c
@@ -0,0 +1,108 @@
+// Check that a module from -fmodule-name= does not accidentally pick up extra
+// dependencies that come from a PCH.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
+// RUN: sed "s|DIR|%/t|g" %t/cdb_pch.json.template > %t/cdb_pch.json
+
+// Scan PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json \
+// RUN: -format experimental-full -mode preprocess-dependency-directives \
+// RUN: > %t/deps_pch.json
+
+// Build PCH
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name A > %t/A.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name B > %t/B.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
+// RUN: %clang @%t/A.rsp
+// RUN: %clang @%t/B.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Scan TU with PCH
+// RUN: clang-scan-deps -compilation-database %t/cdb.json \
+// RUN: -format experimental-full -mode preprocess-dependency-directives \
+// RUN: > %t/deps.json
+
+// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
+
+// Verify that the only modular import in C is E and not the unrelated modules
+// A or B that come from the PCH.
+
+// CHECK: {
+// CHECK-NEXT: "modules": [
+// CHECK-NEXT: {
+// CHECK: "clang-module-deps": [
+// CHECK-NEXT: {
+// CHECK: "module-name": "E"
+// CHECK: }
+// CHECK-NEXT: ]
+// CHECK: "clang-modulemap-file": "[[PREFIX]]/module.modulemap"
+// CHECK: "command-line": [
+// CHECK-NOT: "-fmodule-file=
+// CHECK: "-fmodule-file={{(E=)?}}[[PREFIX]]/{{.*}}/E-{{.*}}.pcm"
+// CHECK-NOT: "-fmodule-file=
+// CHECK: ]
+// CHECK: "name": "C"
+// CHECK: }
+
+
+//--- cdb_pch.json.template
+[{
+ "file": "DIR/prefix.h",
+ "directory": "DIR",
+ "command": "clang -x c-header DIR/prefix.h -o DIR/prefix.h.pch -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- cdb.json.template
+[{
+ "file": "DIR/tu.c",
+ "directory": "DIR",
+ "command": "clang -fsyntax-only DIR/tu.c -include DIR/prefix.h -fmodule-name=C -fmodules -fimplicit-modules -fimplicit-module-maps -fmodules-cache-path=DIR/module-cache"
+}]
+
+//--- module.modulemap
+module A { header "A.h" export * }
+module B { header "B.h" export * }
+module C { header "C.h" export * }
+module D { header "D.h" export * }
+module E { header "E.h" export * }
+
+//--- A.h
+#pragma once
+struct A { int x; };
+
+//--- B.h
+#pragma once
+#include "A.h"
+struct B { struct A a; };
+
+//--- C.h
+#pragma once
+#include "E.h"
+struct C { struct E e; };
+
+//--- D.h
+#pragma once
+#include "C.h"
+struct D { struct C c; };
+
+//--- E.h
+#pragma once
+struct E { int y; };
+
+//--- prefix.h
+#include "B.h"
+
+//--- tu.c
+// C.h is first included textually due to -fmodule-name=C.
+#include "C.h"
+// importing D pulls in a modular import of C; it's this build of C that we
+// are verifying above
+#include "D.h"
+
+void tu(void) {
+ struct A a;
+ struct B b;
+ struct C c;
+}
diff --git a/clang/test/Modules/submodule-visibility-pch.c b/clang/test/Modules/submodule-visibility-pch.c
new file mode 100644
index 0000000000000..648f756fb9713
--- /dev/null
+++ b/clang/test/Modules/submodule-visibility-pch.c
@@ -0,0 +1,56 @@
+// Verify that the use of a PCH does not accidentally make modules from the PCH
+// visible to submodules when using -fmodules-local-submodule-visibility
+// and -fmodule-name to trigger a textual include.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// First check that it works with a header
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN: -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN: -fmodule-name=Mod \
+// RUN: %t/tu.c -include %t/prefix.h -I %t -verify
+
+// Now with a PCH
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN: -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN: -x c-header %t/prefix.h -emit-pch -o %t/prefix.pch -I %t
+
+// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
+// RUN: -fmodules-local-submodule-visibility -fimplicit-module-maps \
+// RUN: -fmodule-name=Mod \
+// RUN: %t/tu.c -include-pch %t/prefix.pch -I %t -verify
+
+//--- module.modulemap
+module ModViaPCH { header "ModViaPCH.h" }
+module ModViaInclude { header "ModViaInclude.h" }
+module Mod { header "Mod.h" }
+module SomeOtherMod { header "SomeOtherMod.h" }
+
+//--- ModViaPCH.h
+#define ModViaPCH 1
+
+//--- ModViaInclude.h
+#define ModViaInclude 2
+
+//--- SomeOtherMod.h
+// empty
+
+//--- Mod.h
+#include "SomeOtherMod.h"
+#ifdef ModViaPCH
+#error "Visibility violation ModViaPCH"
+#endif
+#ifdef ModViaInclude
+#error "Visibility violation ModViaInclude"
+#endif
+
+//--- prefix.h
+#include "ModViaPCH.h"
+
+//--- tu.c
+#include "ModViaInclude.h"
+#include "Mod.h"
+// expected-no-diagnostics
More information about the cfe-commits
mailing list