bnbarham created this revision.
bnbarham added reviewers: vsapsai, dexonsmith.
Herald added a subscriber: kristof.beyls.
bnbarham requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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 OutOfDate when
the serialized and current path didn't match up. This is not the same as
the existing check, which is now different since only the name as
written is serialized. This means the OutOfDate condition is really only
checking for an umbrella with a different *name* (assuming it exists in
the base directory). It is also skipped completely for frameworks since
these don't include `Headers/` in the name, causing no file entry to be
found.
Given all this, 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
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D107690
Files:
clang/lib/Serialization/ASTReader.cpp
clang/test/VFS/module-header-mismatches.m
clang/test/VFS/umbrella-mismatch.m
Index: clang/test/VFS/umbrella-mismatch.m
===================================================================
--- 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
-@import UsesFoo;
Index: clang/test/VFS/module-header-mismatches.m
===================================================================
--- /dev/null
+++ 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 different (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 different 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
+@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'
+ }
+ ]
+ }
+ ]
+}
+
Index: clang/lib/Serialization/ASTReader.cpp
===================================================================
--- clang/lib/Serialization/ASTReader.cpp
+++ clang/lib/Serialization/ASTReader.cpp
@@ -4240,8 +4240,11 @@
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 @@
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 @@
// 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
@@ -5604,21 +5591,23 @@
break;
}
- case SUBMODULE_UMBRELLA_HEADER: {
- std::string Filename = std::string(Blob);
- ResolveImportedPath(F, Filename);
- if (auto Umbrella = PP.getFileManager().getFile(Filename)) {
- 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;
- }
- }
+ case SUBMODULE_UMBRELLA_HEADER:
+ // The filename here is the name as written in the modulemap (or the name
+ // that would be written in the case of inferred modules).
+ //
+ // 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 its modification
+ // time and size.
+ //
+ // Another possibility is that the header itself was resolved to a
+ // different path (eg. through the use of a VFS), but again, it's too
+ // late to return out of date here anyway.
+ //
+ // FIXME: Add a warning if name/path don't match up? Note that the name
+ // and relative name ignore VFS at the moment (see implementation
+ // of `ModuleMap::findHeader`)
break;
- }
case SUBMODULE_HEADER:
case SUBMODULE_EXCLUDED_HEADER:
@@ -5638,21 +5627,9 @@
CurrentModule->addTopHeaderFilename(Blob);
break;
- case SUBMODULE_UMBRELLA_DIR: {
- std::string Dirname = std::string(Blob);
- ResolveImportedPath(F, Dirname);
- if (auto Umbrella = PP.getFileManager().getDirectory(Dirname)) {
- 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;
- }
- }
+ case SUBMODULE_UMBRELLA_DIR:
+ // See comment in SUBMODULE_UMBRELLA_HEADER
break;
- }
case SUBMODULE_METADATA: {
F.BaseSubmoduleID = getTotalNumSubmodules();
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
