[clang] 3220855 - [Modules] Do not remove failed modules after the control block phase
Volodymyr Sapsai via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 17 16:47:31 PDT 2021
Author: Ben Barham
Date: 2021-08-17T16:46:51-07:00
New Revision: 32208555af26c48f3df845a10b049c8eb74e2eb3
URL: https://github.com/llvm/llvm-project/commit/32208555af26c48f3df845a10b049c8eb74e2eb3
DIFF: https://github.com/llvm/llvm-project/commit/32208555af26c48f3df845a10b049c8eb74e2eb3.diff
LOG: [Modules] Do not remove failed modules after the control block phase
Reading modules first reads each control block in the chain and then all
AST blocks.
The first phase is intended to find recoverable errors, eg. an out of
date or missing module. If any error occurs during this phase, it is
safe to remove all modules in the chain as no references to them will
exist.
While reading the AST blocks, however, various fields in ASTReader are
updated with references to the module. Removing modules at this point
can cause dangling pointers which can be accessed later. These would be
otherwise harmless, eg. a binary search over `GlobalSLocEntryMap` may
access a failed module that could error, but shouldn't crash. Do not
remove modules in this phase, regardless of failures.
Since this is the case, it also doesn't make sense to return OutOfDate
during this phase, so remove the two cases where this happens.
When they were originally added these checks would return a failure when
the serialized and current path didn't match up. That was updated to an
OutOfDate as it was found to be hit when using VFS and overriding the
umbrella. Later on the path was changed to instead be the name as
written in the module file, resolved using the serialized base
directory. At this point the check is really only comparing the name of
the umbrella and only works for frameworks since those don't include
`Headers/` in the name (which means the resolved path will never exist)
Given all that, it seems safe to ignore this case entirely for now.
This makes the handling of an umbrella header/directory the same as
regular headers, which also don't check for differences in the path
caused by VFS.
Resolves rdar://79329355
Differential Revision: https://reviews.llvm.org/D107690
Added:
clang/test/VFS/module-header-mismatches.m
Modified:
clang/lib/Serialization/ASTReader.cpp
Removed:
clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
clang/test/VFS/umbrella-mismatch.m
################################################################################
diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp
index 83bade9941b3d..b8c4889b10f9d 100644
--- a/clang/lib/Serialization/ASTReader.cpp
+++ b/clang/lib/Serialization/ASTReader.cpp
@@ -4240,8 +4240,11 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
PreviousGeneration = incrementGeneration(*ContextObj);
unsigned NumModules = ModuleMgr.size();
- auto removeModulesAndReturn = [&](ASTReadResult ReadResult) {
- assert(ReadResult && "expected to return error");
+ SmallVector<ImportedModule, 4> Loaded;
+ if (ASTReadResult ReadResult =
+ ReadASTCore(FileName, Type, ImportLoc,
+ /*ImportedBy=*/nullptr, Loaded, 0, 0, ASTFileSignature(),
+ ClientLoadCapabilities)) {
ModuleMgr.removeModules(ModuleMgr.begin() + NumModules,
PP.getLangOpts().Modules
? &PP.getHeaderSearchInfo().getModuleMap()
@@ -4252,22 +4255,6 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
GlobalIndex.reset();
ModuleMgr.setGlobalIndex(nullptr);
return ReadResult;
- };
-
- SmallVector<ImportedModule, 4> Loaded;
- switch (ASTReadResult ReadResult =
- ReadASTCore(FileName, Type, ImportLoc,
- /*ImportedBy=*/nullptr, Loaded, 0, 0,
- ASTFileSignature(), ClientLoadCapabilities)) {
- case Failure:
- case Missing:
- case OutOfDate:
- case VersionMismatch:
- case ConfigurationMismatch:
- case HadErrors:
- return removeModulesAndReturn(ReadResult);
- case Success:
- break;
}
// Here comes stuff that we only do once the entire chain is loaded.
@@ -4279,18 +4266,18 @@ ASTReader::ASTReadResult ASTReader::ReadAST(StringRef FileName,
// Read the AST block.
if (ASTReadResult Result = ReadASTBlock(F, ClientLoadCapabilities))
- return removeModulesAndReturn(Result);
+ return Failure;
// The AST block should always have a definition for the main module.
if (F.isModule() && !F.DidReadTopLevelSubmodule) {
Error(diag::err_module_file_missing_top_level_submodule, F.FileName);
- return removeModulesAndReturn(Failure);
+ return Failure;
}
// Read the extension blocks.
while (!SkipCursorToBlock(F.Stream, EXTENSION_BLOCK_ID)) {
if (ASTReadResult Result = ReadExtensionBlock(F))
- return removeModulesAndReturn(Result);
+ return Failure;
}
// Once read, set the ModuleFile bit base offset and update the size in
@@ -5605,17 +5592,20 @@ ASTReader::ReadSubmoduleBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
}
case SUBMODULE_UMBRELLA_HEADER: {
+ // FIXME: This doesn't work for framework modules as `Filename` is the
+ // name as written in the module file and does not include
+ // `Headers/`, so this path will never exist.
std::string Filename = std::string(Blob);
ResolveImportedPath(F, Filename);
if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
- if (!CurrentModule->getUmbrellaHeader())
+ if (!CurrentModule->getUmbrellaHeader()) {
// FIXME: NameAsWritten
ModMap.setUmbrellaHeader(CurrentModule, *Umbrella, Blob, "");
- else if (CurrentModule->getUmbrellaHeader().Entry != *Umbrella) {
- if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
- Error("mismatched umbrella headers in submodule");
- return OutOfDate;
}
+ // Note that it's too late at this point to return out of date if the
+ // name from the PCM doesn't match up with the one in the module map,
+ // but also quite unlikely since we will have already checked the
+ // modification time and size of the module map file itself.
}
break;
}
@@ -5639,16 +5629,13 @@ ASTReader::ReadSubmoduleBlock(ModuleFile &F, unsigned ClientLoadCapabilities) {
break;
case SUBMODULE_UMBRELLA_DIR: {
+ // See comments in SUBMODULE_UMBRELLA_HEADER
std::string Dirname = std::string(Blob);
ResolveImportedPath(F, Dirname);
if (auto Umbrella = PP.getFileManager().getDirectory(Dirname)) {
- if (!CurrentModule->getUmbrellaDir())
+ if (!CurrentModule->getUmbrellaDir()) {
// FIXME: NameAsWritten
ModMap.setUmbrellaDir(CurrentModule, *Umbrella, Blob, "");
- else if (CurrentModule->getUmbrellaDir().Entry != *Umbrella) {
- if ((ClientLoadCapabilities & ARR_OutOfDate) == 0)
- Error("mismatched umbrella directories in submodule");
- return OutOfDate;
}
}
break;
diff --git a/clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h b/clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
deleted file mode 100644
index 375d3ea2a0449..0000000000000
--- a/clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h
+++ /dev/null
@@ -1 +0,0 @@
- at import Foo;
diff --git a/clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap b/clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
deleted file mode 100644
index 55be29e800193..0000000000000
--- a/clang/test/VFS/Inputs/UsesFoo.framework/Modules/module.modulemap
+++ /dev/null
@@ -1,4 +0,0 @@
-framework module UsesFoo {
- umbrella header "UsesFoo.h"
- export *
-}
diff --git a/clang/test/VFS/module-header-mismatches.m b/clang/test/VFS/module-header-mismatches.m
new file mode 100644
index 0000000000000..f4e77bd555e6f
--- /dev/null
+++ b/clang/test/VFS/module-header-mismatches.m
@@ -0,0 +1,86 @@
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: sed -e "s;TEST_DIR;%/t;g" %t/sed-overlay.yaml > %t/overlay.yaml
+
+// These tests first build with an overlay such that the header is resolved
+// to %t/other/Mismatch.h. They then build again with the header resolved
+// to the one in their directory.
+//
+// This should cause a rebuild if the contents is
diff erent (and thus multiple
+// PCMs), but this currently isn't the case. We should at least not error,
+// since this does happen in real projects (with a
diff erent copy of the same
+// file).
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -ivfsoverlay %t/overlay.yaml -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/header-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/hf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/df-mcp -ivfsoverlay %t/overlay.yaml -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/hf-mcp -F %t/dir-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/df-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -ivfsoverlay %t/overlay.yaml -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/nf-mcp -F %t/norm-frameworks -fsyntax-only -verify %t/use.m
+// RUN: find %t/nf-mcp -name "Mismatch-*.pcm" | count 1
+
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -ivfsoverlay %t/overlay.yaml -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/m-mcp -I %t/mod -fsyntax-only -verify %t/use.m
+// RUN: find %t/m-mcp -name "Mismatch-*.pcm" | count 1
+
+//--- use.m
+// expected-no-diagnostics
+ at import Mismatch;
+
+//--- header-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- header-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- dir-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ umbrella "someheaders"
+}
+//--- dir-frameworks/Mismatch.framework/someheaders/Mismatch.h
+
+//--- norm-frameworks/Mismatch.framework/Modules/module.modulemap
+framework module Mismatch {
+ header "Mismatch.h"
+}
+//--- norm-frameworks/Mismatch.framework/Headers/Mismatch.h
+
+//--- mod/module.modulemap
+module Mismatch {
+ umbrella header "Mismatch.h"
+}
+//--- mod/Mismatch.h
+
+//--- other/Mismatch.h
+
+//--- sed-overlay.yaml
+{
+ 'version': 0,
+ 'roots': [
+ { 'name': 'TEST_DIR', 'type': 'directory',
+ 'contents': [
+ { 'name': 'header-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+ },
+ { 'name': 'dir-frameworks/Mismatch.framework/someheaders',
+ 'type': 'directory',
+ 'external-contents': 'TEST_DIR/others'
+ },
+ { 'name': 'norm-frameworks/Mismatch.framework/Headers/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+ },
+ { 'name': 'mod/Mismatch.h',
+ 'type': 'file',
+ 'external-contents': 'TEST_DIR/other/Mismatch.h'
+ }
+ ]
+ }
+ ]
+}
+
diff --git a/clang/test/VFS/umbrella-mismatch.m b/clang/test/VFS/umbrella-mismatch.m
deleted file mode 100644
index 8167a21f485bb..0000000000000
--- a/clang/test/VFS/umbrella-mismatch.m
+++ /dev/null
@@ -1,7 +0,0 @@
-// RUN: rm -rf %t
-// RUN: sed -e "s;INPUT_DIR;%/S/Inputs;g" -e "s;OUT_DIR;%/S/Inputs;g" %S/Inputs/vfsoverlay.yaml > %t.yaml
-
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -ivfsoverlay %t.yaml -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// RUN: %clang_cc1 -Werror -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -F %S/Inputs -fsyntax-only %s -Wno-atimport-in-framework-header -verify
-// expected-no-diagnostics
- at import UsesFoo;
More information about the cfe-commits
mailing list