manmanren created this revision.
manmanren added reviewers: benlangmuir, rsmith.
manmanren added a subscriber: cfe-commits.
With implicit modules, it is hard to debug issues that depend on state of the
module cache before the clang invocation. The state of the module cache can be
changed by other parallel clang invocations that share the module cache.
We build a module by spawning a child thread. Even when the parent thread
already validated some modules and stored the FileEntries, the child thread can
invalidate those modules, causing use-after-free for the parent thread.
Since this issue is time-sensitive, it is hard to reproduce with a release
compiler. With an assert or an ASAN'ed compiler the chance of reproducing is
even smaller. This patch tries to diagnose the use-after-free in the compiler,
by passing up the list of invalidated modules from the child thread and
emitting a hard error in the parent thread.
Another option is to pass down the list of validated modules from the parent
thread, the child thread can emit a warning and not invalidate those modules.
I am open to other suggestions as well.
https://reviews.llvm.org/D25916
Files:
include/clang/Basic/DiagnosticCommonKinds.td
include/clang/Serialization/ModuleManager.h
lib/Frontend/CompilerInstance.cpp
lib/Serialization/ModuleManager.cpp
test/Modules/Inputs/system-out-of-date/X.h
test/Modules/Inputs/system-out-of-date/Y.h
test/Modules/Inputs/system-out-of-date/Z.h
test/Modules/Inputs/system-out-of-date/module.map
test/Modules/system-out-of-date-test.m
Index: test/Modules/system-out-of-date-test.m
===================================================================
--- test/Modules/system-out-of-date-test.m
+++ test/Modules/system-out-of-date-test.m
@@ -0,0 +1,14 @@
+// RUN: rm -rf %t
+// RUN: echo '@import X;' | \
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN: -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date \
+// RUN: -fsyntax-only -x objective-c -
+
+// We have an version built with different diagnostic options.
+// RUN: not %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t -I %S/Inputs/system-out-of-date -Wnon-modular-include-in-framework-module -Werror=non-modular-include-in-framework-module %s -fsyntax-only 2>&1 | FileCheck %s
+
+@import X;
+
+#import <Z.h>
+// CHECK: fatal error: module 'Y' is invalidated by its child
+// CHECK: fatal error: could not build module 'Z'
Index: test/Modules/Inputs/system-out-of-date/module.map
===================================================================
--- test/Modules/Inputs/system-out-of-date/module.map
+++ test/Modules/Inputs/system-out-of-date/module.map
@@ -0,0 +1,12 @@
+module X [system] {
+ header "X.h" // imports Y
+ export *
+}
+module Y {
+ header "Y.h"
+ export *
+}
+module Z {
+ header "Z.h" // imports Y
+ export *
+}
Index: test/Modules/Inputs/system-out-of-date/Z.h
===================================================================
--- test/Modules/Inputs/system-out-of-date/Z.h
+++ test/Modules/Inputs/system-out-of-date/Z.h
@@ -0,0 +1 @@
+#import <Y.h>
Index: test/Modules/Inputs/system-out-of-date/Y.h
===================================================================
--- test/Modules/Inputs/system-out-of-date/Y.h
+++ test/Modules/Inputs/system-out-of-date/Y.h
@@ -0,0 +1 @@
+//empty
Index: test/Modules/Inputs/system-out-of-date/X.h
===================================================================
--- test/Modules/Inputs/system-out-of-date/X.h
+++ test/Modules/Inputs/system-out-of-date/X.h
@@ -0,0 +1 @@
+#import <Y.h>
Index: lib/Serialization/ModuleManager.cpp
===================================================================
--- lib/Serialization/ModuleManager.cpp
+++ lib/Serialization/ModuleManager.cpp
@@ -182,6 +182,29 @@
return NewlyLoaded;
}
+// Check to see if the parent thread is holding on to an entry that the child
+// thread invalidated.
+std::string ModuleManager::checkForInvalidateEntries(
+ const ModuleMap *modMapInParent, ModuleManager *parentManager) {
+ for (auto E : InvalidatedModules) {
+ if (parentManager->lookup(E.first))
+ return E.second;
+
+ if (modMapInParent) {
+ if (Module *mod = modMapInParent->findModule(E.second)) {
+ if (mod->getASTFile() == E.first)
+ return E.second;
+ }
+ }
+ }
+ return "";
+}
+
+void ModuleManager::mergeInvalidatedEntries(ModuleManager *child) {
+ for (auto E : child->InvalidatedModules)
+ InvalidatedModules[E.first] = E.second;
+}
+
void ModuleManager::removeModules(
ModuleIterator first, ModuleIterator last,
llvm::SmallPtrSetImpl<ModuleFile *> &LoadedSuccessfully,
@@ -228,8 +251,10 @@
// Files that didn't make it through ReadASTCore successfully will be
// rebuilt (or there was an error). Invalidate them so that we can load the
// new files that will be renamed over the old ones.
- if (LoadedSuccessfully.count(*victim) == 0)
+ if (LoadedSuccessfully.count(*victim) == 0) {
+ InvalidatedModules[(*victim)->File] = (*victim)->ModuleName;
FileMgr.invalidateCache((*victim)->File);
+ }
delete *victim;
}
Index: lib/Frontend/CompilerInstance.cpp
===================================================================
--- lib/Frontend/CompilerInstance.cpp
+++ lib/Frontend/CompilerInstance.cpp
@@ -1060,6 +1060,21 @@
diag::remark_module_build_done)
<< Module->Name;
+ if (ImportingInstance.getModuleManager() && Instance.getModuleManager()) {
+ serialization::ModuleManager &ParentManager =
+ ImportingInstance.getModuleManager()->getModuleManager();
+ serialization::ModuleManager &ChildManager =
+ Instance.getModuleManager()->getModuleManager();
+
+ // Copy the invalidated modules to the parent thread.
+ ParentManager.mergeInvalidatedEntries(&ChildManager);
+ std::string Invalid = ChildManager.checkForInvalidateEntries(
+ &ModMap, &ParentManager);
+ if (!Invalid.empty())
+ Instance.getDiagnostics().Report(ImportLoc,
+ diag::err_module_invalidated_by_child) << Invalid;
+ }
+
// Delete the temporary module map file.
// FIXME: Even though we're executing under crash protection, it would still
// be nice to do this with RemoveFileOnSignal when we can. However, that
Index: include/clang/Serialization/ModuleManager.h
===================================================================
--- include/clang/Serialization/ModuleManager.h
+++ include/clang/Serialization/ModuleManager.h
@@ -46,6 +46,9 @@
/// \brief All loaded modules, indexed by name.
llvm::DenseMap<const FileEntry *, ModuleFile *> Modules;
+ /// \brief Modules that are invalidated by the child threads.
+ llvm::DenseMap<const FileEntry *, std::string> InvalidatedModules;
+
/// \brief FileManager that handles translating between filenames and
/// FileEntry *.
FileManager &FileMgr;
@@ -280,6 +283,16 @@
time_t ExpectedModTime,
const FileEntry *&File);
+ /// In the context of a parent thread spawn a child thread to build a module,
+ /// the child thread's module manager checks to see if any successfully loaded
+ /// module in the parent thread is invalidated here. It returns the name of
+ /// the module if such module exists.
+ std::string checkForInvalidateEntries(const ModuleMap *modMapInParent,
+ ModuleManager *parentManager);
+
+ /// Pull invalidated entries from the child manager.
+ void mergeInvalidatedEntries(ModuleManager *childManager);
+
/// \brief View the graphviz representation of the module graph.
void viewGraph();
};
Index: include/clang/Basic/DiagnosticCommonKinds.td
===================================================================
--- include/clang/Basic/DiagnosticCommonKinds.td
+++ include/clang/Basic/DiagnosticCommonKinds.td
@@ -96,6 +96,8 @@
DefaultFatal;
def err_module_prebuilt : Error<
"error in loading module '%0' from prebuilt module path">, DefaultFatal;
+def err_module_invalidated_by_child : Error<
+ "module '%0' is invalidated by a child thread">, 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">;
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
