[clang] 31e14f4 - clang/Modules: Sink CompilerInstance::KnownModules into ModuleMap
Duncan P. N. Exon Smith via cfe-commits
cfe-commits at lists.llvm.org
Sun Nov 3 20:35:33 PST 2019
Author: Duncan P. N. Exon Smith
Date: 2019-11-03T19:57:33-08:00
New Revision: 31e14f41a21f9016050a20f07d5da03db2e8c13e
URL: https://github.com/llvm/llvm-project/commit/31e14f41a21f9016050a20f07d5da03db2e8c13e
DIFF: https://github.com/llvm/llvm-project/commit/31e14f41a21f9016050a20f07d5da03db2e8c13e.diff
LOG: clang/Modules: Sink CompilerInstance::KnownModules into ModuleMap
Avoid use-after-frees when FrontendAction::BeginSourceFile is called
twice on the same CompilerInstance by sinking
CompilerInstance::KnownModules into ModuleMap. On the way, rename the
map to CachedModuleLoads. I considered (but rejected) merging this with
ModuleMap::Modules, since that only has top-level modules and this map
includes submodules.
This is an alternative to https://reviews.llvm.org/D58497. Thanks to
nemanjai for the detailed analysis of the problem!
Added:
Modified:
clang/include/clang/Frontend/CompilerInstance.h
clang/include/clang/Lex/ModuleMap.h
clang/lib/Frontend/CompilerInstance.cpp
Removed:
################################################################################
diff --git a/clang/include/clang/Frontend/CompilerInstance.h b/clang/include/clang/Frontend/CompilerInstance.h
index d15bdc4665a3..0ed5c9beac27 100644
--- a/clang/include/clang/Frontend/CompilerInstance.h
+++ b/clang/include/clang/Frontend/CompilerInstance.h
@@ -126,10 +126,6 @@ class CompilerInstance : public ModuleLoader {
std::vector<std::shared_ptr<DependencyCollector>> DependencyCollectors;
- /// The set of top-level modules that has already been loaded,
- /// along with the module map
- llvm::DenseMap<const IdentifierInfo *, Module *> KnownModules;
-
/// The set of top-level modules that has already been built on the
/// fly as part of this overall compilation action.
std::map<std::string, std::string> BuiltModules;
diff --git a/clang/include/clang/Lex/ModuleMap.h b/clang/include/clang/Lex/ModuleMap.h
index 36e97a16223b..3110ead86010 100644
--- a/clang/include/clang/Lex/ModuleMap.h
+++ b/clang/include/clang/Lex/ModuleMap.h
@@ -14,17 +14,18 @@
#ifndef LLVM_CLANG_LEX_MODULEMAP_H
#define LLVM_CLANG_LEX_MODULEMAP_H
+#include "clang/Basic/IdentifierTable.h"
#include "clang/Basic/LangOptions.h"
#include "clang/Basic/Module.h"
#include "clang/Basic/SourceLocation.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/PointerIntPair.h"
-#include "llvm/ADT/StringSet.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSet.h"
#include "llvm/ADT/TinyPtrVector.h"
#include "llvm/ADT/Twine.h"
#include <ctime>
@@ -100,6 +101,10 @@ class ModuleMap {
/// The top-level modules that are known.
llvm::StringMap<Module *> Modules;
+ /// Module loading cache that includes submodules, indexed by IdentifierInfo.
+ /// nullptr is stored for modules that are known to fail to load.
+ llvm::DenseMap<const IdentifierInfo *, Module *> CachedModuleLoads;
+
/// Shadow modules created while building this module map.
llvm::SmallVector<Module*, 2> ShadowModules;
@@ -684,6 +689,16 @@ class ModuleMap {
module_iterator module_begin() const { return Modules.begin(); }
module_iterator module_end() const { return Modules.end(); }
+
+ /// Cache a module load. M might be nullptr.
+ void cacheModuleLoad(const IdentifierInfo &II, Module *M) {
+ CachedModuleLoads[&II] = M;
+ }
+
+ /// Return a cached module load.
+ Module *getCachedModuleLoad(const IdentifierInfo &II) {
+ return CachedModuleLoads.lookup(&II);
+ }
};
} // namespace clang
diff --git a/clang/lib/Frontend/CompilerInstance.cpp b/clang/lib/Frontend/CompilerInstance.cpp
index c409c07ff133..cc3d848c1e02 100644
--- a/clang/lib/Frontend/CompilerInstance.cpp
+++ b/clang/lib/Frontend/CompilerInstance.cpp
@@ -1541,12 +1541,9 @@ bool CompilerInstance::loadModuleFile(StringRef FileName) {
}
void registerAll() {
- for (auto *II : LoadedModules) {
- CI.KnownModules[II] = CI.getPreprocessor()
- .getHeaderSearchInfo()
- .getModuleMap()
- .findModule(II->getName());
- }
+ ModuleMap &MM = CI.getPreprocessor().getHeaderSearchInfo().getModuleMap();
+ for (auto *II : LoadedModules)
+ MM.cacheModuleLoad(*II, MM.findModule(II->getName()));
LoadedModules.clear();
}
@@ -1635,14 +1632,11 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
return LastModuleImportResult;
}
- clang::Module *Module = nullptr;
-
// If we don't already have information on this module, load the module now.
- llvm::DenseMap<const IdentifierInfo *, clang::Module *>::iterator Known
- = KnownModules.find(Path[0].first);
- if (Known != KnownModules.end()) {
- // Retrieve the cached top-level module.
- Module = Known->second;
+ ModuleMap &MM = getPreprocessor().getHeaderSearchInfo().getModuleMap();
+ clang::Module *Module = MM.getCachedModuleLoad(*Path[0].first);
+ if (Module) {
+ // Nothing to do here, we found it.
} else if (ModuleName == getLangOpts().CurrentModule) {
// This is the module we're building.
Module = PP->getHeaderSearchInfo().lookupModule(
@@ -1656,7 +1650,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
// ModuleBuildFailed = true;
// return ModuleLoadResult();
//}
- Known = KnownModules.insert(std::make_pair(Path[0].first, Module)).first;
+ MM.cacheModuleLoad(*Path[0].first, Module);
} else {
// Search for a module with the given name.
Module = PP->getHeaderSearchInfo().lookupModule(ModuleName, true,
@@ -1750,7 +1744,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
getDiagnostics().Report(ModuleNameLoc, diag::err_module_prebuilt)
<< ModuleName;
ModuleBuildFailed = true;
- KnownModules[Path[0].first] = nullptr;
+ MM.cacheModuleLoad(*Path[0].first, nullptr);
return ModuleLoadResult();
}
}
@@ -1764,7 +1758,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
// necessarily even have a module map. Since ReadAST already produces
// diagnostics for these two cases, we simply error out here.
ModuleBuildFailed = true;
- KnownModules[Path[0].first] = nullptr;
+ MM.cacheModuleLoad(*Path[0].first, nullptr);
return ModuleLoadResult();
}
@@ -1809,7 +1803,7 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
"undiagnosed error in compileAndLoadModule");
if (getPreprocessorOpts().FailedModules)
getPreprocessorOpts().FailedModules->addFailed(ModuleName);
- KnownModules[Path[0].first] = nullptr;
+ MM.cacheModuleLoad(*Path[0].first, nullptr);
ModuleBuildFailed = true;
return ModuleLoadResult();
}
@@ -1832,19 +1826,19 @@ CompilerInstance::loadModule(SourceLocation ImportLoc,
ModuleLoader::HadFatalFailure = true;
// FIXME: The ASTReader will already have complained, but can we shoehorn
// that diagnostic information into a more useful form?
- KnownModules[Path[0].first] = nullptr;
+ MM.cacheModuleLoad(*Path[0].first, nullptr);
return ModuleLoadResult();
case ASTReader::Failure:
ModuleLoader::HadFatalFailure = true;
// Already complained, but note now that we failed.
- KnownModules[Path[0].first] = nullptr;
+ MM.cacheModuleLoad(*Path[0].first, nullptr);
ModuleBuildFailed = true;
return ModuleLoadResult();
}
// Cache the result of this top-level module lookup for later.
- Known = KnownModules.insert(std::make_pair(Path[0].first, Module)).first;
+ MM.cacheModuleLoad(*Path[0].first, Module);
}
// If we never found the module, fail.
More information about the cfe-commits
mailing list