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