[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

[Alexandre Ganea via Phabricator via cfe-commits]

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,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D69959.228276.patch
Type: text/x-patch
Size: 2027 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191107/2324601e/attachment.bin>