Skip to content

Commit 3e8689b

Browse files
benlangmuirbenlangmuir
committed
[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 (cherry picked from commit e06a91c)
1 parent 85a7598 commit 3e8689b

File tree

4 files changed

+182
-7
lines changed

4 files changed

+182
-7
lines changed

‎clang/include/clang/Serialization/ASTReader.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -961,8 +961,14 @@ class ASTReader
961961

962962
private:
963963
/// A list of modules that were imported by precompiled headers or
964-
/// any other non-module AST file.
965-
SmallVector<ImportedSubmodule, 2> ImportedModules;
964+
/// any other non-module AST file and have not yet been made visible. If a
965+
/// module is made visible in the ASTReader, it will be transfered to
966+
/// \c PendingImportedModulesSema.
967+
SmallVector<ImportedSubmodule, 2> PendingImportedModules;
968+
969+
/// A list of modules that were imported by precompiled headers or
970+
/// any other non-module AST file and have not yet been made visible for Sema.
971+
SmallVector<ImportedSubmodule, 2> PendingImportedModulesSema;
966972
//@}
967973

968974
/// The system include root to be used when loading the

‎clang/lib/Serialization/ASTReader.cpp

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3711,7 +3711,7 @@ llvm::Error ASTReader::ReadASTBlock(ModuleFile &F,
37113711
unsigned GlobalID = getGlobalSubmoduleID(F, Record[I++]);
37123712
SourceLocation Loc = ReadSourceLocation(F, Record, I);
37133713
if (GlobalID) {
3714-
ImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
3714+
PendingImportedModules.push_back(ImportedSubmodule(GlobalID, Loc));
37153715
if (DeserializationListener)
37163716
DeserializationListener->ModuleImportRead(GlobalID, Loc);
37173717
}
@@ -4426,8 +4426,8 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
44264426
UnresolvedModuleRefs.clear();
44274427

44284428
if (Imported)
4429-
Imported->append(ImportedModules.begin(),
4430-
ImportedModules.end());
4429+
Imported->append(PendingImportedModules.begin(),
4430+
PendingImportedModules.end());
44314431

44324432
// FIXME: How do we load the 'use'd modules? They may not be submodules.
44334433
// Might be unnecessary as use declarations are only used to build the
@@ -5044,7 +5044,7 @@ void ASTReader::InitializeContext() {
50445044

50455045
// Re-export any modules that were imported by a non-module AST file.
50465046
// FIXME: This does not make macro-only imports visible again.
5047-
for (auto &Import : ImportedModules) {
5047+
for (auto &Import : PendingImportedModules) {
50485048
if (Module *Imported = getSubmodule(Import.ID)) {
50495049
makeModuleVisible(Imported, Module::AllVisible,
50505050
/*ImportLoc=*/Import.ImportLoc);
@@ -5054,6 +5054,10 @@ void ASTReader::InitializeContext() {
50545054
// nullptr here, we do the same later, in UpdateSema().
50555055
}
50565056
}
5057+
5058+
// Hand off these modules to Sema.
5059+
PendingImportedModulesSema.append(PendingImportedModules);
5060+
PendingImportedModules.clear();
50575061
}
50585062

50595063
void ASTReader::finalizeForWriting() {
@@ -8086,13 +8090,14 @@ void ASTReader::UpdateSema() {
80868090
}
80878091

80888092
// For non-modular AST files, restore visiblity of modules.
8089-
for (auto &Import : ImportedModules) {
8093+
for (auto &Import : PendingImportedModulesSema) {
80908094
if (Import.ImportLoc.isInvalid())
80918095
continue;
80928096
if (Module *Imported = getSubmodule(Import.ID)) {
80938097
SemaObj->makeModuleVisible(Imported, Import.ImportLoc);
80948098
}
80958099
}
8100+
PendingImportedModulesSema.clear();
80968101
}
80978102

80988103
IdentifierInfo *ASTReader::get(StringRef Name) {

‎clang/test/ClangScanDeps/modules-pch-imports.c

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
// Check that a module from -fmodule-name= does not accidentally pick up extra
2+
// dependencies that come from a PCH.
3+
4+
// RUN: rm -rf %t
5+
// RUN: split-file %s %t
6+
// RUN: sed "s|DIR|%/t|g" %t/cdb.json.template > %t/cdb.json
7+
// RUN: sed "s|DIR|%/t|g" %t/cdb_pch.json.template > %t/cdb_pch.json
8+
9+
// Scan PCH
10+
// RUN: clang-scan-deps -compilation-database %t/cdb_pch.json \
11+
// RUN: -format experimental-full -mode preprocess-dependency-directives \
12+
// RUN: > %t/deps_pch.json
13+
14+
// Build PCH
15+
// RUN: %deps-to-rsp %t/deps_pch.json --module-name A > %t/A.rsp
16+
// RUN: %deps-to-rsp %t/deps_pch.json --module-name B > %t/B.rsp
17+
// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
18+
// RUN: %clang @%t/A.rsp
19+
// RUN: %clang @%t/B.rsp
20+
// RUN: %clang @%t/pch.rsp
21+
22+
// Scan TU with PCH
23+
// RUN: clang-scan-deps -compilation-database %t/cdb.json \
24+
// RUN: -format experimental-full -mode preprocess-dependency-directives \
25+
// RUN: > %t/deps.json
26+
27+
// RUN: cat %t/deps.json | sed 's:\\\\\?:/:g' | FileCheck %s -DPREFIX=%/t
28+
29+
// Verify that the only modular import in C is E and not the unrelated modules
30+
// A or B that come from the PCH.
31+
32+
// CHECK: {
33+
// CHECK-NEXT: "modules": [
34+
// CHECK-NEXT: {
35+
// CHECK: "clang-module-deps": [
36+
// CHECK-NEXT: {
37+
// CHECK: "module-name": "E"
38+
// CHECK: }
39+
// CHECK-NEXT: ]
40+
// CHECK: "clang-modulemap-file": "[[PREFIX]]/module.modulemap"
41+
// CHECK: "command-line": [
42+
// CHECK-NOT: "-fmodule-file=
43+
// CHECK: "-fmodule-file={{(E=)?}}[[PREFIX]]/{{.*}}/E-{{.*}}.pcm"
44+
// CHECK-NOT: "-fmodule-file=
45+
// CHECK: ]
46+
// CHECK: "name": "C"
47+
// CHECK: }
48+
49+
50+
//--- cdb_pch.json.template
51+
[{
52+
"file": "DIR/prefix.h",
53+
"directory": "DIR",
54+
"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"
55+
}]
56+
57+
//--- cdb.json.template
58+
[{
59+
"file": "DIR/tu.c",
60+
"directory": "DIR",
61+
"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"
62+
}]
63+
64+
//--- module.modulemap
65+
module A { header "A.h" export * }
66+
module B { header "B.h" export * }
67+
module C { header "C.h" export * }
68+
module D { header "D.h" export * }
69+
module E { header "E.h" export * }
70+
71+
//--- A.h
72+
#pragma once
73+
struct A { int x; };
74+
75+
//--- B.h
76+
#pragma once
77+
#include "A.h"
78+
struct B { struct A a; };
79+
80+
//--- C.h
81+
#pragma once
82+
#include "E.h"
83+
struct C { struct E e; };
84+
85+
//--- D.h
86+
#pragma once
87+
#include "C.h"
88+
struct D { struct C c; };
89+
90+
//--- E.h
91+
#pragma once
92+
struct E { int y; };
93+
94+
//--- prefix.h
95+
#include "B.h"
96+
97+
//--- tu.c
98+
// C.h is first included textually due to -fmodule-name=C.
99+
#include "C.h"
100+
// importing D pulls in a modular import of C; it's this build of C that we
101+
// are verifying above
102+
#include "D.h"
103+
104+
void tu(void) {
105+
struct A a;
106+
struct B b;
107+
struct C c;
108+
}

‎clang/test/Modules/submodule-visibility-pch.c

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// Verify that the use of a PCH does not accidentally make modules from the PCH
2+
// visible to submodules when using -fmodules-local-submodule-visibility
3+
// and -fmodule-name to trigger a textual include.
4+
5+
// RUN: rm -rf %t
6+
// RUN: split-file %s %t
7+
8+
// First check that it works with a header
9+
10+
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
11+
// RUN: -fmodules-local-submodule-visibility -fimplicit-module-maps \
12+
// RUN: -fmodule-name=Mod \
13+
// RUN: %t/tu.c -include %t/prefix.h -I %t -verify
14+
15+
// Now with a PCH
16+
17+
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
18+
// RUN: -fmodules-local-submodule-visibility -fimplicit-module-maps \
19+
// RUN: -x c-header %t/prefix.h -emit-pch -o %t/prefix.pch -I %t
20+
21+
// RUN: %clang_cc1 -fmodules -fmodules-cache-path=%t/cache \
22+
// RUN: -fmodules-local-submodule-visibility -fimplicit-module-maps \
23+
// RUN: -fmodule-name=Mod \
24+
// RUN: %t/tu.c -include-pch %t/prefix.pch -I %t -verify
25+
26+
//--- module.modulemap
27+
module ModViaPCH { header "ModViaPCH.h" }
28+
module ModViaInclude { header "ModViaInclude.h" }
29+
module Mod { header "Mod.h" }
30+
module SomeOtherMod { header "SomeOtherMod.h" }
31+
32+
//--- ModViaPCH.h
33+
#define ModViaPCH 1
34+
35+
//--- ModViaInclude.h
36+
#define ModViaInclude 2
37+
38+
//--- SomeOtherMod.h
39+
// empty
40+
41+
//--- Mod.h
42+
#include "SomeOtherMod.h"
43+
#ifdef ModViaPCH
44+
#error "Visibility violation ModViaPCH"
45+
#endif
46+
#ifdef ModViaInclude
47+
#error "Visibility violation ModViaInclude"
48+
#endif
49+
50+
//--- prefix.h
51+
#include "ModViaPCH.h"
52+
53+
//--- tu.c
54+
#include "ModViaInclude.h"
55+
#include "Mod.h"
56+
// expected-no-diagnostics

0 commit comments

Comments
 (0)