aganea created this revision.
aganea added reviewers: STL_MSFT, rnk, dexonsmith.
aganea added a project: clang.
Herald added subscribers: cfe-commits, arphaman.
This is an attempt to fix `clang/test/Index/crash-recovery-modules.m` when
compiling a build with the MSVC STL & _ITERATOR_DEBUG_LEVEL == 2 (meaning a
DEBUG build)
The problem is related to the STL implementation, not the compiler.
I do run sometimes the Debug build with LLVM_ENABLE_ASSERTIONS, it brings up
interesting issues (see rG007d173e2e0c29903bc17a9d5108f531a6f2ea4d
<https://reviews.llvm.org/rG007d173e2e0c29903bc17a9d5108f531a6f2ea4d>)
The problem
-----------
Before, the test used to freeze with this callstack:
F10658035: freeze_crash-recovery-modules_debug.PNG
<https://reviews.llvm.org/F10658035>
A lock was taken by another thread that died since.
The test forcibly triggers a crash with the help of `#pragma clang __debug
crash`. This would hit `clang/lib/Lex/Pragma.cpp, L1040`.
However the call was made through a `CrashRecoveryContext`, which is
essentially a try/catch, so we get back at
`clang/lib/Frontend/CompilerInstance.cpp, L1152`:
F10658114: freeze_crash-recovery-modules_debug 2.PNG
<https://reviews.llvm.org/F10658114>
However, later on at the end of this function, `CompilerInvocation` is
destroyed, which in turn triggers destruction of `FrontendOptions`, which
eventually triggers the destruction of its `Inputs` member.
The problem here is that `Inputs` was being iterated just before the #pragma
crash occurred. This left STL's internal linked list (for the `Inputs` vector)
in a stale state, pointing to a stack variable that doesn't exist anymore (now
that we got out of `CrashRecoveryContext`). When `Inputs` is being destroyed,
the code in `C:\Program Files (x86)\Microsoft Visual
Studio\2017\Professional\VC\Tools\MSVC\14.16.27023\include\xutility` tried to
clear that linked list. But now we have nodes pointing to stack memory pages
that were already un-commited.
F10658173: freeze_crash-recovery-modules_debug 3.PNG
<https://reviews.llvm.org/F10658173>
All this ends up to a second crash which happens to leave the _LOCK_DEBUG mutex
locked. The crash itself is protected by another `CrashRecoveryContext` invoked
by `clang/tools/libclang/CIndex.cpp, L3624` so the program doesn't crash. But
it now ends in a stale state, and the _LOCK_DEBUG mutex is locked forever,
which leads to the freeze in the top screenshot.
The fix
-------
Ideally, this would require some kind of TLS or stack frame mechanism in the
MSVC STL, to unwind the linked list in case of a crash.
Our patch takes a shortcut and does that manually, by simply saving the state
of the vector's debug iterator linked list, and restoring it after (a possible)
crash.
I could disable the test with something like `UNSUPPORTED: windows-msvc, debug`
(we don't have debug) but I thought this test is valuable.
Please let me know what approach you would like me to take. If that's the way
to go, I'll move these two functions (`Save/ApplyIteratorDebug`) to a more
appropriate place.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D69959
Files:
clang/lib/Frontend/CompilerInstance.cpp
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1031,6 +1031,39 @@
return LangOpts.CPlusPlus ? Language::CXX : Language::C;
}
+template <typename T>
+void SaveIteratorDebug(T &Container, SmallVectorImpl<void *> &Saver) {
+#if _ITERATOR_DEBUG_LEVEL == 2
+ std::_Container_proxy *C = Container._Myproxy();
+ if (!C)
+ return;
+ std::_Lockit _Lock(_LOCK_DEBUG);
+ for (std::_Iterator_base12 *P = C->_Myfirstiter; P != nullptr;
+ P = P->_Mynextiter) {
+ Saver.push_back(P);
+ }
+#endif
+}
+
+// NOTE: This doesn't try to clear out the nodes that weren't there before,
+// because they might have an address further down the (old) stack, and the OS
+// might have freed those virtual pages already, which would cause a GPF.
+template <typename T>
+void ApplyIteratorDebug(T &Container, const SmallVectorImpl<void *> &Saver) {
+#if _ITERATOR_DEBUG_LEVEL == 2
+ std::_Container_proxy *C = Container._Myproxy();
+ if (!C)
+ return;
+ std::_Lockit _Lock(_LOCK_DEBUG);
+ std::_Iterator_base12 **P = &C->_Myfirstiter;
+ for (auto S : Saver) {
+ *P = (std::_Iterator_base12 *)S;
+ P = &(*P)->_Mynextiter;
+ }
+ *P = nullptr;
+#endif
+}
+
/// Compile a module file for the given module, using the options
/// provided by the importing compiler instance. Returns true if the module
/// was built without errors.
@@ -1139,6 +1172,9 @@
PreBuildStep(Instance);
+ SmallVector<void*, 4> Iters;
+ SaveIteratorDebug(Instance.getFrontendOpts().Inputs, Iters);
+
// Execute the action to actually build the module in-place. Use a separate
// thread so that we get a stack large enough.
llvm::CrashRecoveryContext CRC;
@@ -1149,6 +1185,8 @@
},
DesiredStackSize);
+ ApplyIteratorDebug(Instance.getFrontendOpts().Inputs, Iters);
+
PostBuildStep(Instance);
ImportingInstance.getDiagnostics().Report(ImportLoc,
Index: clang/lib/Frontend/CompilerInstance.cpp
===================================================================
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -1031,6 +1031,39 @@
return LangOpts.CPlusPlus ? Language::CXX : Language::C;
}
+template <typename T>
+void SaveIteratorDebug(T &Container, SmallVectorImpl<void *> &Saver) {
+#if _ITERATOR_DEBUG_LEVEL == 2
+ std::_Container_proxy *C = Container._Myproxy();
+ if (!C)
+ return;
+ std::_Lockit _Lock(_LOCK_DEBUG);
+ for (std::_Iterator_base12 *P = C->_Myfirstiter; P != nullptr;
+ P = P->_Mynextiter) {
+ Saver.push_back(P);
+ }
+#endif
+}
+
+// NOTE: This doesn't try to clear out the nodes that weren't there before,
+// because they might have an address further down the (old) stack, and the OS
+// might have freed those virtual pages already, which would cause a GPF.
+template <typename T>
+void ApplyIteratorDebug(T &Container, const SmallVectorImpl<void *> &Saver) {
+#if _ITERATOR_DEBUG_LEVEL == 2
+ std::_Container_proxy *C = Container._Myproxy();
+ if (!C)
+ return;
+ std::_Lockit _Lock(_LOCK_DEBUG);
+ std::_Iterator_base12 **P = &C->_Myfirstiter;
+ for (auto S : Saver) {
+ *P = (std::_Iterator_base12 *)S;
+ P = &(*P)->_Mynextiter;
+ }
+ *P = nullptr;
+#endif
+}
+
/// Compile a module file for the given module, using the options
/// provided by the importing compiler instance. Returns true if the module
/// was built without errors.
@@ -1139,6 +1172,9 @@
PreBuildStep(Instance);
+ SmallVector<void*, 4> Iters;
+ SaveIteratorDebug(Instance.getFrontendOpts().Inputs, Iters);
+
// Execute the action to actually build the module in-place. Use a separate
// thread so that we get a stack large enough.
llvm::CrashRecoveryContext CRC;
@@ -1149,6 +1185,8 @@
},
DesiredStackSize);
+ ApplyIteratorDebug(Instance.getFrontendOpts().Inputs, Iters);
+
PostBuildStep(Instance);
ImportingInstance.getDiagnostics().Report(ImportLoc,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
