Doesn’t DisableAllLogChannels acquire a scoped lock?  If so wouldn’t it just release at the end of the function?<br><div class="gmail_quote"><div dir="ltr">On Sun, Oct 15, 2017 at 2:42 PM Pavel Labath via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">labath created this revision.<br>
<br>
We had a bug where if we had forked (in the ProcessLauncherPosixFork)<br>
while another thread was writing a log message, we would deadlock. This<br>
happened because the fork child inherited the locked log rwmutex, which<br>
would never get unlocked. This meant the child got stuck trying to<br>
disable all log channels.<br>
<br>
The bug existed for a while but only started being apparent after<br>
<a href="https://reviews.llvm.org/D37930" rel="noreferrer" target="_blank">https://reviews.llvm.org/D37930</a>, which started using ThreadLauncher (which uses logging) instead<br>
of std::thread (which does not) for launching TaskPool threads.<br>
<br>
The fix is to provide a lock free way to disable logging, which should<br>
be used in this case, and this case only.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D38938" rel="noreferrer" target="_blank">https://reviews.llvm.org/D38938</a><br>
<br>
Files:<br>
  include/lldb/Utility/Log.h<br>
  source/Host/posix/ProcessLauncherPosixFork.cpp<br>
  source/Utility/Log.cpp<br>
<br>
<br>
Index: source/Utility/Log.cpp<br>
===================================================================<br>
--- source/Utility/Log.cpp<br>
+++ source/Utility/Log.cpp<br>
@@ -90,7 +90,10 @@<br>
<br>
 void Log::Disable(uint32_t flags) {<br>
   llvm::sys::ScopedWriter lock(m_mutex);<br>
+  DisableUnlocked(flags);<br>
+}<br>
<br>
+void Log::DisableUnlocked(uint32_t flags) {<br>
   uint32_t mask = m_mask.fetch_and(~flags, std::memory_order_relaxed);<br>
   if (!(mask & ~flags)) {<br>
     m_stream_sp.reset();<br>
@@ -241,6 +244,11 @@<br>
     entry.second.Disable(UINT32_MAX);<br>
 }<br>
<br>
+void Log::DisableAllLogChannelsUnlocked() {<br>
+  for (auto &entry : *g_channel_map)<br>
+    entry.second.DisableUnlocked(UINT32_MAX);<br>
+}<br>
+<br>
 void Log::ListAllLogChannels(llvm::raw_ostream &stream) {<br>
   if (g_channel_map->empty()) {<br>
     stream << "No logging channels are currently registered.\n";<br>
Index: source/Host/posix/ProcessLauncherPosixFork.cpp<br>
===================================================================<br>
--- source/Host/posix/ProcessLauncherPosixFork.cpp<br>
+++ source/Host/posix/ProcessLauncherPosixFork.cpp<br>
@@ -97,7 +97,7 @@<br>
                                               const ProcessLaunchInfo &info) {<br>
   // First, make sure we disable all logging. If we are logging to stdout, our<br>
   // logs can be mistaken for inferior output.<br>
-  Log::DisableAllLogChannels();<br>
+  Log::DisableAllLogChannelsUnlocked();<br>
<br>
   // Do not inherit setgid powers.<br>
   if (setgid(getgid()) != 0)<br>
Index: include/lldb/Utility/Log.h<br>
===================================================================<br>
--- include/lldb/Utility/Log.h<br>
+++ include/lldb/Utility/Log.h<br>
@@ -116,6 +116,11 @@<br>
<br>
   static void DisableAllLogChannels();<br>
<br>
+  // Only use if no other threads are executing and calling the locking version<br>
+  // is known to be unsafe (for example, this happens right after forking, if we<br>
+  // have forked while another thread was holding the log mutex)).<br>
+  static void DisableAllLogChannelsUnlocked();<br>
+<br>
   static void ListAllLogChannels(llvm::raw_ostream &stream);<br>
<br>
   //------------------------------------------------------------------<br>
@@ -184,6 +189,7 @@<br>
               uint32_t options, uint32_t flags);<br>
<br>
   void Disable(uint32_t flags);<br>
+  void DisableUnlocked(uint32_t flags);<br>
<br>
   typedef llvm::StringMap<Log> ChannelMap;<br>
   static llvm::ManagedStatic<ChannelMap> g_channel_map;<br>
<br>
<br>
</blockquote></div>