Skip to content

Commit 67fe9c0

Browse files
committed
[Frontend] Only compile modules if not already finalized
It was possible to re-add a module to a shared in-memory module cache when search paths are changed. This can eventually cause a crash if the original module is referenced after this occurs. 1. Module A depends on B 2. B exists in two paths C and D 3. First run only has C on the search path, finds A and B and loads them 4. Second run adds D to the front of the search path. A is loaded and contains a reference to the already compiled module from C. But searching finds the module from D instead, causing a mismatch 5. B and the modules that depend on it are considered out of date and thus rebuilt 6. The recompiled module A is added to the in-memory cache, freeing the previously inserted one This can never occur from a regular clang process, but is very easy to do through the API - whether through the use of a shared case or just running multiple compilations from a single `CompilerInstance`. Update the compilation to return early if a module is already finalized so that the pre-condition in the in-memory module cache holds. Resolves rdar://78180255 Differential Revision: https://reviews.llvm.org/D105328
1 parent c4e395e commit 67fe9c0

File tree

7 files changed

+220
-11
lines changed

7 files changed

+220
-11
lines changed

clang/include/clang/Basic/DiagnosticCommonKinds.td

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,8 @@ def err_module_cycle : Error<"cyclic dependency in module '%0': %1">,
111111
DefaultFatal;
112112
def err_module_prebuilt : Error<
113113
"error in loading module '%0' from prebuilt module path">, DefaultFatal;
114+
def err_module_rebuild_finalized : Error<
115+
"cannot rebuild module '%0' as it is already finalized">, DefaultFatal;
114116
def note_pragma_entered_here : Note<"#pragma entered here">;
115117
def note_decl_hiding_tag_type : Note<
116118
"%1 %0 is hidden by a non-type declaration of %0 here">;

clang/include/clang/Basic/DiagnosticSerializationKinds.td

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,9 @@ def err_module_file_not_module : Error<
6969
"AST file '%0' was not built as a module">, DefaultFatal;
7070
def err_module_file_missing_top_level_submodule : Error<
7171
"module file '%0' is missing its top-level submodule">, DefaultFatal;
72+
def note_module_file_conflict : Note<
73+
"this is generally caused by modules with the same name found in multiple "
74+
"paths">;
7275

7376
def remark_module_import : Remark<
7477
"importing module '%0'%select{| into '%3'}2 from '%1'">,

clang/include/clang/Serialization/ASTReader.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,6 +1396,9 @@ class ASTReader
13961396
llvm::iterator_range<PreprocessingRecord::iterator>
13971397
getModulePreprocessedEntities(ModuleFile &Mod) const;
13981398

1399+
bool canRecoverFromOutOfDate(StringRef ModuleFileName,
1400+
unsigned ClientLoadCapabilities);
1401+
13991402
public:
14001403
class ModuleDeclIterator
14011404
: public llvm::iterator_adaptor_base<

clang/lib/Frontend/CompilerInstance.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1062,6 +1062,15 @@ compileModuleImpl(CompilerInstance &ImportingInstance, SourceLocation ImportLoc,
10621062
[](CompilerInstance &) {}) {
10631063
llvm::TimeTraceScope TimeScope("Module Compile", ModuleName);
10641064

1065+
// Never compile a module that's already finalized - this would cause the
1066+
// existing module to be freed, causing crashes if it is later referenced
1067+
if (ImportingInstance.getModuleCache().isPCMFinal(ModuleFileName)) {
1068+
ImportingInstance.getDiagnostics().Report(
1069+
ImportLoc, diag::err_module_rebuild_finalized)
1070+
<< ModuleName;
1071+
return false;
1072+
}
1073+
10651074
// Construct a compiler invocation for creating this module.
10661075
auto Invocation =
10671076
std::make_shared<CompilerInvocation>(ImportingInstance.getInvocation());

clang/lib/Serialization/ASTReader.cpp

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2747,7 +2747,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
27472747
// If requested by the caller and the module hasn't already been read
27482748
// or compiled, mark modules on error as out-of-date.
27492749
if ((ClientLoadCapabilities & ARR_TreatModuleWithErrorsAsOutOfDate) &&
2750-
!ModuleMgr.getModuleCache().isPCMFinal(F.FileName))
2750+
canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
27512751
return OutOfDate;
27522752

27532753
if (!AllowASTWithCompilerErrors) {
@@ -2833,9 +2833,14 @@ ASTReader::ReadControlBlock(ModuleFile &F,
28332833
StoredSignature, Capabilities);
28342834

28352835
// If we diagnosed a problem, produce a backtrace.
2836-
if (isDiagnosedResult(Result, Capabilities))
2836+
bool recompilingFinalized =
2837+
Result == OutOfDate && (Capabilities & ARR_OutOfDate) &&
2838+
getModuleManager().getModuleCache().isPCMFinal(F.FileName);
2839+
if (isDiagnosedResult(Result, Capabilities) || recompilingFinalized)
28372840
Diag(diag::note_module_file_imported_by)
28382841
<< F.FileName << !F.ModuleName.empty() << F.ModuleName;
2842+
if (recompilingFinalized)
2843+
Diag(diag::note_module_file_conflict);
28392844

28402845
switch (Result) {
28412846
case Failure: return Failure;
@@ -2901,7 +2906,7 @@ ASTReader::ReadControlBlock(ModuleFile &F,
29012906
F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) {
29022907
auto BuildDir = PP.getFileManager().getDirectory(Blob);
29032908
if (!BuildDir || *BuildDir != M->Directory) {
2904-
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
2909+
if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
29052910
Diag(diag::err_imported_module_relocated)
29062911
<< F.ModuleName << Blob << M->Directory->getName();
29072912
return OutOfDate;
@@ -3914,7 +3919,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
39143919
if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation &
39153920
DisableValidationForModuleKind::Module) &&
39163921
!ModMap) {
3917-
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) {
3922+
if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities)) {
39183923
if (auto ASTFE = M ? M->getASTFile() : None) {
39193924
// This module was defined by an imported (explicit) module.
39203925
Diag(diag::err_module_file_conflict) << F.ModuleName << F.FileName
@@ -3945,7 +3950,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
39453950
assert((ImportedBy || F.Kind == MK_ImplicitModule) &&
39463951
"top-level import should be verified");
39473952
bool NotImported = F.Kind == MK_ImplicitModule && !ImportedBy;
3948-
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
3953+
if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
39493954
Diag(diag::err_imported_module_modmap_changed)
39503955
<< F.ModuleName << (NotImported ? F.FileName : ImportedBy->FileName)
39513956
<< ModMap->getName() << F.ModuleMapPath << NotImported;
@@ -3956,13 +3961,13 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
39563961
for (unsigned I = 0, N = Record[Idx++]; I < N; ++I) {
39573962
// FIXME: we should use input files rather than storing names.
39583963
std::string Filename = ReadPath(F, Record, Idx);
3959-
auto F = FileMgr.getFile(Filename, false, false);
3960-
if (!F) {
3961-
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
3964+
auto SF = FileMgr.getFile(Filename, false, false);
3965+
if (!SF) {
3966+
if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
39623967
Error("could not find file '" + Filename +"' referenced by AST file");
39633968
return OutOfDate;
39643969
}
3965-
AdditionalStoredMaps.insert(*F);
3970+
AdditionalStoredMaps.insert(*SF);
39663971
}
39673972

39683973
// Check any additional module map files (e.g. module.private.modulemap)
@@ -3972,7 +3977,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
39723977
// Remove files that match
39733978
// Note: SmallPtrSet::erase is really remove
39743979
if (!AdditionalStoredMaps.erase(ModMap)) {
3975-
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
3980+
if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
39763981
Diag(diag::err_module_different_modmap)
39773982
<< F.ModuleName << /*new*/0 << ModMap->getName();
39783983
return OutOfDate;
@@ -3983,7 +3988,7 @@ ASTReader::ReadModuleMapFileBlock(RecordData &Record, ModuleFile &F,
39833988
// Check any additional module map files that are in the pcm, but not
39843989
// found in header search. Cases that match are already removed.
39853990
for (const FileEntry *ModMap : AdditionalStoredMaps) {
3986-
if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
3991+
if (!canRecoverFromOutOfDate(F.FileName, ClientLoadCapabilities))
39873992
Diag(diag::err_module_different_modmap)
39883993
<< F.ModuleName << /*not new*/1 << ModMap->getName();
39893994
return OutOfDate;
@@ -5952,6 +5957,12 @@ ASTReader::getModulePreprocessedEntities(ModuleFile &Mod) const {
59525957
PreprocessingRecord::iterator());
59535958
}
59545959

5960+
bool ASTReader::canRecoverFromOutOfDate(StringRef ModuleFileName,
5961+
unsigned int ClientLoadCapabilities) {
5962+
return ClientLoadCapabilities & ARR_OutOfDate &&
5963+
!getModuleManager().getModuleCache().isPCMFinal(ModuleFileName);
5964+
}
5965+
59555966
llvm::iterator_range<ASTReader::ModuleDeclIterator>
59565967
ASTReader::getModuleFileLevelDecls(ModuleFile &Mod) {
59575968
return llvm::make_range(

clang/unittests/Serialization/CMakeLists.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,14 @@ set(LLVM_LINK_COMPONENTS
66

77
add_clang_unittest(SerializationTests
88
InMemoryModuleCacheTest.cpp
9+
ModuleCacheTest.cpp
910
)
1011

1112
clang_target_link_libraries(SerializationTests
1213
PRIVATE
1314
clangAST
1415
clangBasic
16+
clangFrontend
1517
clangLex
1618
clangSema
1719
clangSerialization
Lines changed: 179 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,179 @@
1+
//===- unittests/Serialization/ModuleCacheTest.cpp - CI tests -------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
#include "clang/Basic/FileManager.h"
10+
#include "clang/Frontend/CompilerInstance.h"
11+
#include "clang/Frontend/CompilerInvocation.h"
12+
#include "clang/Frontend/FrontendActions.h"
13+
#include "clang/Lex/HeaderSearch.h"
14+
#include "llvm/ADT/SmallString.h"
15+
#include "llvm/Support/FileSystem.h"
16+
#include "llvm/Support/raw_ostream.h"
17+
18+
#include "gtest/gtest.h"
19+
20+
using namespace llvm;
21+
using namespace clang;
22+
23+
namespace {
24+
25+
class ModuleCacheTest : public ::testing::Test {
26+
void SetUp() override {
27+
ASSERT_FALSE(sys::fs::createUniqueDirectory("modulecache-test", TestDir));
28+
29+
ModuleCachePath = SmallString<256>(TestDir);
30+
sys::path::append(ModuleCachePath, "mcp");
31+
ASSERT_FALSE(sys::fs::create_directories(ModuleCachePath));
32+
}
33+
34+
void TearDown() override { sys::fs::remove_directories(TestDir); }
35+
36+
public:
37+
SmallString<256> TestDir;
38+
SmallString<256> ModuleCachePath;
39+
40+
void addFile(StringRef Path, StringRef Contents) {
41+
ASSERT_FALSE(sys::path::is_absolute(Path));
42+
43+
SmallString<256> AbsPath(TestDir);
44+
sys::path::append(AbsPath, Path);
45+
46+
std::error_code EC;
47+
ASSERT_FALSE(
48+
sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath)));
49+
llvm::raw_fd_ostream OS(AbsPath, EC);
50+
ASSERT_FALSE(EC);
51+
OS << Contents;
52+
}
53+
54+
void addDuplicateFrameworks() {
55+
addFile("test.m", R"cpp(
56+
@import Top;
57+
)cpp");
58+
59+
addFile("frameworks/Top.framework/Headers/top.h", R"cpp(
60+
@import M;
61+
)cpp");
62+
addFile("frameworks/Top.framework/Modules/module.modulemap", R"cpp(
63+
framework module Top [system] {
64+
header "top.h"
65+
export *
66+
}
67+
)cpp");
68+
69+
addFile("frameworks/M.framework/Headers/m.h", R"cpp(
70+
void foo();
71+
)cpp");
72+
addFile("frameworks/M.framework/Modules/module.modulemap", R"cpp(
73+
framework module M [system] {
74+
header "m.h"
75+
export *
76+
}
77+
)cpp");
78+
79+
addFile("frameworks2/M.framework/Headers/m.h", R"cpp(
80+
void foo();
81+
)cpp");
82+
addFile("frameworks2/M.framework/Modules/module.modulemap", R"cpp(
83+
framework module M [system] {
84+
header "m.h"
85+
export *
86+
}
87+
)cpp");
88+
}
89+
};
90+
91+
TEST_F(ModuleCacheTest, CachedModuleNewPath) {
92+
addDuplicateFrameworks();
93+
94+
SmallString<256> MCPArg("-fmodules-cache-path=");
95+
MCPArg.append(ModuleCachePath);
96+
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
97+
CompilerInstance::createDiagnostics(new DiagnosticOptions());
98+
99+
// First run should pass with no errors
100+
const char *Args[] = {"clang", "-fmodules", "-Fframeworks",
101+
MCPArg.c_str(), "-working-directory", TestDir.c_str(),
102+
"test.m"};
103+
std::shared_ptr<CompilerInvocation> Invocation =
104+
createInvocationFromCommandLine(Args, Diags);
105+
ASSERT_TRUE(Invocation);
106+
CompilerInstance Instance;
107+
Instance.setDiagnostics(Diags.get());
108+
Instance.setInvocation(Invocation);
109+
SyntaxOnlyAction Action;
110+
ASSERT_TRUE(Instance.ExecuteAction(Action));
111+
ASSERT_FALSE(Diags->hasErrorOccurred());
112+
113+
// Now add `frameworks2` to the search path. `Top.pcm` will have a reference
114+
// to the `M` from `frameworks`, but a search will find the `M` from
115+
// `frameworks2` - causing a mismatch and it to be considered out of date.
116+
//
117+
// Normally this would be fine - `M` and the modules it depends on would be
118+
// rebuilt. However, since we have a shared module cache and thus an already
119+
// finalized `Top`, recompiling `Top` will cause the existing module to be
120+
// removed from the cache, causing possible crashed if it is ever used.
121+
//
122+
// Make sure that an error occurs instead.
123+
const char *Args2[] = {"clang", "-fmodules", "-Fframeworks2",
124+
"-Fframeworks", MCPArg.c_str(), "-working-directory",
125+
TestDir.c_str(), "test.m"};
126+
std::shared_ptr<CompilerInvocation> Invocation2 =
127+
createInvocationFromCommandLine(Args2, Diags);
128+
ASSERT_TRUE(Invocation2);
129+
CompilerInstance Instance2(Instance.getPCHContainerOperations(),
130+
&Instance.getModuleCache());
131+
Instance2.setDiagnostics(Diags.get());
132+
Instance2.setInvocation(Invocation2);
133+
SyntaxOnlyAction Action2;
134+
ASSERT_FALSE(Instance2.ExecuteAction(Action2));
135+
ASSERT_TRUE(Diags->hasErrorOccurred());
136+
}
137+
138+
TEST_F(ModuleCacheTest, CachedModuleNewPathAllowErrors) {
139+
addDuplicateFrameworks();
140+
141+
SmallString<256> MCPArg("-fmodules-cache-path=");
142+
MCPArg.append(ModuleCachePath);
143+
IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
144+
CompilerInstance::createDiagnostics(new DiagnosticOptions());
145+
146+
// First run should pass with no errors
147+
const char *Args[] = {"clang", "-fmodules", "-Fframeworks",
148+
MCPArg.c_str(), "-working-directory", TestDir.c_str(),
149+
"test.m"};
150+
std::shared_ptr<CompilerInvocation> Invocation =
151+
createInvocationFromCommandLine(Args, Diags);
152+
ASSERT_TRUE(Invocation);
153+
CompilerInstance Instance;
154+
Instance.setDiagnostics(Diags.get());
155+
Instance.setInvocation(Invocation);
156+
SyntaxOnlyAction Action;
157+
ASSERT_TRUE(Instance.ExecuteAction(Action));
158+
ASSERT_FALSE(Diags->hasErrorOccurred());
159+
160+
// Same as `CachedModuleNewPath` but while allowing errors. This is a hard
161+
// failure where the module wasn't created, so it should still fail.
162+
const char *Args2[] = {
163+
"clang", "-fmodules", "-Fframeworks2",
164+
"-Fframeworks", MCPArg.c_str(), "-working-directory",
165+
TestDir.c_str(), "-Xclang", "-fallow-pcm-with-compiler-errors",
166+
"test.m"};
167+
std::shared_ptr<CompilerInvocation> Invocation2 =
168+
createInvocationFromCommandLine(Args2, Diags);
169+
ASSERT_TRUE(Invocation2);
170+
CompilerInstance Instance2(Instance.getPCHContainerOperations(),
171+
&Instance.getModuleCache());
172+
Instance2.setDiagnostics(Diags.get());
173+
Instance2.setInvocation(Invocation2);
174+
SyntaxOnlyAction Action2;
175+
ASSERT_FALSE(Instance2.ExecuteAction(Action2));
176+
ASSERT_TRUE(Diags->hasErrorOccurred());
177+
}
178+
179+
} // anonymous namespace

0 commit comments

Comments
 (0)