diff --git a/clang/lib/Serialization/ASTReader.cpp b/clang/lib/Serialization/ASTReader.cpp --- a/clang/lib/Serialization/ASTReader.cpp +++ b/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 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 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 @@ -5605,17 +5592,20 @@ } 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()) // 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 @@ 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()) // 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 --- a/clang/test/VFS/Inputs/UsesFoo.framework/Headers/UsesFoo.h +++ /dev/null @@ -1 +0,0 @@ -@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 --- 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 --- /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 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' + } + ] + } + ] +} + diff --git a/clang/test/VFS/umbrella-mismatch.m b/clang/test/VFS/umbrella-mismatch.m deleted file mode 100644 --- 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 -@import UsesFoo;