Index: clang/include/clang/Basic/DiagnosticCommonKinds.td =================================================================== --- clang/include/clang/Basic/DiagnosticCommonKinds.td +++ clang/include/clang/Basic/DiagnosticCommonKinds.td @@ -111,6 +111,8 @@ DefaultFatal; def err_module_prebuilt : Error< "error in loading module '%0' from prebuilt module path">, DefaultFatal; +def err_module_rebuild_finalized : Error< + "cannot rebuild module '%0' as it is already finalized">, DefaultFatal; def note_pragma_entered_here : Note<"#pragma entered here">; def note_decl_hiding_tag_type : Note< "%1 %0 is hidden by a non-type declaration of %0 here">; Index: clang/include/clang/Serialization/ASTReader.h =================================================================== --- clang/include/clang/Serialization/ASTReader.h +++ clang/include/clang/Serialization/ASTReader.h @@ -1401,6 +1401,9 @@ llvm::iterator_range getModulePreprocessedEntities(ModuleFile &Mod) const; + bool diagnoseOutOfDate(StringRef ModuleFileName, + unsigned ClientLoadCapabilities); + public: class ModuleDeclIterator : public llvm::iterator_adaptor_base< Index: clang/lib/Frontend/CompilerInstance.cpp =================================================================== --- clang/lib/Frontend/CompilerInstance.cpp +++ clang/lib/Frontend/CompilerInstance.cpp @@ -1054,6 +1054,15 @@ [](CompilerInstance &) {}) { llvm::TimeTraceScope TimeScope("Module Compile", ModuleName); + // Never compile a module that's already finalized - this would cause the + // existing module to be freed, causing crashes if it is later referenced + if (ImportingInstance.getModuleCache().isPCMFinal(ModuleFileName)) { + ImportingInstance.getDiagnostics().Report( + ImportLoc, diag::err_module_rebuild_finalized) + << ModuleName; + return ImportingInstance.getFrontendOpts().AllowPCMWithCompilerErrors; + } + // Construct a compiler invocation for creating this module. auto Invocation = std::make_shared(ImportingInstance.getInvocation()); Index: clang/lib/Serialization/ASTReader.cpp =================================================================== --- clang/lib/Serialization/ASTReader.cpp +++ clang/lib/Serialization/ASTReader.cpp @@ -2764,7 +2764,7 @@ // If requested by the caller and the module hasn't already been read // or compiled, mark modules on error as out-of-date. if ((ClientLoadCapabilities & ARR_TreatModuleWithErrorsAsOutOfDate) && - !ModuleMgr.getModuleCache().isPCMFinal(F.FileName)) + !diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) return OutOfDate; if (!AllowASTWithCompilerErrors) { @@ -2850,7 +2850,9 @@ StoredSignature, Capabilities); // If we diagnosed a problem, produce a backtrace. - if (isDiagnosedResult(Result, Capabilities)) + if (isDiagnosedResult(Result, Capabilities) || + (Result == OutOfDate && + diagnoseOutOfDate(F.FileName, Capabilities))) Diag(diag::note_module_file_imported_by) << F.FileName << !F.ModuleName.empty() << F.ModuleName; @@ -2918,7 +2920,7 @@ F.Kind != MK_ExplicitModule && F.Kind != MK_PrebuiltModule) { auto BuildDir = PP.getFileManager().getDirectory(Blob); if (!BuildDir || *BuildDir != M->Directory) { - if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) + if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) Diag(diag::err_imported_module_relocated) << F.ModuleName << Blob << M->Directory->getName(); return OutOfDate; @@ -3928,7 +3930,7 @@ if (!bool(PP.getPreprocessorOpts().DisablePCHOrModuleValidation & DisableValidationForModuleKind::Module) && !ModMap) { - if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) { + if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) { if (auto ASTFE = M ? M->getASTFile() : None) { // This module was defined by an imported (explicit) module. Diag(diag::err_module_file_conflict) << F.ModuleName << F.FileName @@ -3959,7 +3961,7 @@ assert((ImportedBy || F.Kind == MK_ImplicitModule) && "top-level import should be verified"); bool NotImported = F.Kind == MK_ImplicitModule && !ImportedBy; - if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) + if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) Diag(diag::err_imported_module_modmap_changed) << F.ModuleName << (NotImported ? F.FileName : ImportedBy->FileName) << ModMap->getName() << F.ModuleMapPath << NotImported; @@ -3970,13 +3972,13 @@ for (unsigned I = 0, N = Record[Idx++]; I < N; ++I) { // FIXME: we should use input files rather than storing names. std::string Filename = ReadPath(F, Record, Idx); - auto F = FileMgr.getFile(Filename, false, false); - if (!F) { - if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) + auto SF = FileMgr.getFile(Filename, false, false); + if (!SF) { + if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) Error("could not find file '" + Filename +"' referenced by AST file"); return OutOfDate; } - AdditionalStoredMaps.insert(*F); + AdditionalStoredMaps.insert(*SF); } // Check any additional module map files (e.g. module.private.modulemap) @@ -3986,7 +3988,7 @@ // Remove files that match // Note: SmallPtrSet::erase is really remove if (!AdditionalStoredMaps.erase(ModMap)) { - if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) + if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) Diag(diag::err_module_different_modmap) << F.ModuleName << /*new*/0 << ModMap->getName(); return OutOfDate; @@ -3997,7 +3999,7 @@ // Check any additional module map files that are in the pcm, but not // found in header search. Cases that match are already removed. for (const FileEntry *ModMap : AdditionalStoredMaps) { - if ((ClientLoadCapabilities & ARR_OutOfDate) == 0) + if (diagnoseOutOfDate(F.FileName, ClientLoadCapabilities)) Diag(diag::err_module_different_modmap) << F.ModuleName << /*not new*/1 << ModMap->getName(); return OutOfDate; @@ -5924,6 +5926,12 @@ PreprocessingRecord::iterator()); } +bool ASTReader::diagnoseOutOfDate(StringRef ModuleFileName, + unsigned int ClientLoadCapabilities) { + return !(ClientLoadCapabilities & ARR_OutOfDate) || + getModuleManager().getModuleCache().isPCMFinal(ModuleFileName); +} + llvm::iterator_range ASTReader::getModuleFileLevelDecls(ModuleFile &Mod) { return llvm::make_range( Index: clang/unittests/Serialization/CMakeLists.txt =================================================================== --- clang/unittests/Serialization/CMakeLists.txt +++ clang/unittests/Serialization/CMakeLists.txt @@ -6,12 +6,14 @@ add_clang_unittest(SerializationTests InMemoryModuleCacheTest.cpp + ModuleCacheTest.cpp ) clang_target_link_libraries(SerializationTests PRIVATE clangAST clangBasic + clangFrontend clangLex clangSema clangSerialization Index: clang/unittests/Serialization/ModuleCacheTest.cpp =================================================================== --- /dev/null +++ clang/unittests/Serialization/ModuleCacheTest.cpp @@ -0,0 +1,135 @@ +//===- unittests/Serialization/ModuleCacheTest.cpp - CI tests -------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "clang/Basic/FileManager.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Frontend/CompilerInvocation.h" +#include "clang/Frontend/FrontendActions.h" +#include "clang/Lex/HeaderSearch.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/Support/FileSystem.h" +#include "llvm/Support/raw_ostream.h" + +#include "gtest/gtest.h" + +using namespace llvm; +using namespace clang; + +namespace { + +class ModuleCacheTest : public ::testing::Test { + void SetUp() override { + ASSERT_FALSE(sys::fs::createUniqueDirectory("modulecache-test", TestDir)); + + ModuleCachePath = SmallString<256>(TestDir); + sys::path::append(ModuleCachePath, "mcp"); + ASSERT_FALSE(sys::fs::create_directories(ModuleCachePath)); + } + + void TearDown() override { sys::fs::remove_directories(TestDir); } + +public: + SmallString<256> TestDir; + SmallString<256> ModuleCachePath; + + void AddFile(StringRef Path, StringRef Contents) { + ASSERT_FALSE(sys::path::is_absolute(Path)); + + SmallString<256> AbsPath(TestDir); + sys::path::append(AbsPath, Path); + + std::error_code EC; + ASSERT_FALSE( + sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath))); + llvm::raw_fd_ostream OS(AbsPath, EC); + ASSERT_FALSE(EC); + OS << Contents; + } +}; + +TEST_F(ModuleCacheTest, CachedModuleNewPath) { + AddFile("test.m", R"cpp( + @import Top; + )cpp"); + + AddFile("frameworks/Top.framework/Headers/top.h", R"cpp( + @import M; + )cpp"); + AddFile("frameworks/Top.framework/Modules/module.modulemap", R"cpp( + framework module Top [system] { + header "top.h" + export * + } + )cpp"); + + AddFile("frameworks/M.framework/Headers/m.h", R"cpp( + void foo(); + )cpp"); + AddFile("frameworks/M.framework/Modules/module.modulemap", R"cpp( + framework module M [system] { + header "m.h" + export * + } + )cpp"); + + AddFile("frameworks2/M.framework/Headers/m.h", R"cpp( + void foo(); + )cpp"); + AddFile("frameworks2/M.framework/Modules/module.modulemap", R"cpp( + framework module M [system] { + header "m.h" + export * + } + )cpp"); + + SmallString<256> MCPArg("-fmodules-cache-path="); + MCPArg.append(ModuleCachePath); + IntrusiveRefCntPtr Diags = + CompilerInstance::createDiagnostics(new DiagnosticOptions()); + + // First run should pass with no errors + const char *Args[] = {"clang", "-fmodules", "-Fframeworks", + MCPArg.c_str(), "-working-directory", TestDir.c_str(), + "test.m"}; + std::shared_ptr Invocation = + createInvocationFromCommandLine(Args, Diags); + ASSERT_TRUE(Invocation); + CompilerInstance Instance; + Instance.setDiagnostics(Diags.get()); + Instance.setInvocation(Invocation); + SyntaxOnlyAction Action; + ASSERT_TRUE(Instance.ExecuteAction(Action)); + ASSERT_FALSE(Diags->hasErrorOccurred()); + + // Now add frameworks2 to the search path. Top.pcm will have a reference to + // the M from frameworks, but a search will find the M from frameworks2 - + // causing a mismatch. + // + // Normally this would be fine as that module would be set as out of date and + // it would be rebuilt. However, since we have a shared module cache and + // there's *already* a finalized module for `M`, recompiling would cause + // the existing module to be removed from the cache, causing possible crashes + // if it is ever referenced. + // + // Make sure that an error occurs instead. + const char *Args2[] = {"clang", "-fmodules", "-Fframeworks2", + "-Fframeworks", MCPArg.c_str(), "-working-directory", + TestDir.c_str(), "test.m"}; + std::shared_ptr Invocation2 = + createInvocationFromCommandLine(Args2, Diags); + ASSERT_TRUE(Invocation2); + CompilerInstance Instance2(Instance.getPCHContainerOperations(), + &Instance.getModuleCache()); + Instance2.setDiagnostics(Diags.get()); + Instance2.setInvocation(Invocation2); + SyntaxOnlyAction Action2; + ASSERT_FALSE(Instance2.ExecuteAction(Action2)); + ASSERT_TRUE(Diags->hasErrorOccurred()); +} + +} // anonymous namespace