[Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

[Zachary Turner via lldb-commits]

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 <
reviews at 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;
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20171015/867399be/attachment.html>