[Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process
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 ChannelMap;
static llvm::ManagedStatic g_channel_map;
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 Disable
Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process
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 ChannelMap;
>static llvm::ManagedStatic g_channel_map;
>
>
>
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process
On 15 October 2017 at 23:04, Zachary Turner wrote: > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it just > release at the end of the function? The thing is, it is unable to acquire the lock in the first place, because the mutex is already "locked". So, the sequence of events is process 1, thread 1: acquire lock process 1, thread 2: fork(), process 2 is created process 1 thread 1: release lock everything goes well from now on in process 1... process 2, thread one (and only). acquire lock: BANG. Process 2 is deadlocked because there is noone to release the lock there. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process
what are we using fork for, out of curiosity? It seems pretty hard to make this work. Logging isn’t the only thing in the program that might be holding a mutex, so it seems like this could still happen. Seems like mixing fork and mutexes is just a fundamentally bad idea, unless I’m missing something (which i very well could be) On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath wrote: > On 15 October 2017 at 23:04, Zachary Turner wrote: > > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it > just > > release at the end of the function? > > > The thing is, it is unable to acquire the lock in the first place, > because the mutex is already "locked". So, the sequence of events is > process 1, thread 1: acquire lock > process 1, thread 2: fork(), process 2 is created > process 1 thread 1: release lock > everything goes well from now on in process 1... > process 2, thread one (and only). acquire lock: > BANG. > Process 2 is deadlocked because there is noone to release the lock there. > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process
On 15 October 2017 at 23:59, Zachary Turner wrote: > what are we using fork for, out of curiosity? It seems pretty hard to make > this work. Logging isn’t the only thing in the program that might be > holding a mutex, so it seems like this could still happen. > > Seems like mixing fork and mutexes is just a fundamentally bad idea, unless > I’m missing something (which i very well could be) We are using fork() (+exec()) to start a new process, which is kinda hard to avoid, since we're in the business of running processes. :) In general, mixing fork and anything is a bad idea and code that runs between fork() and exec() needs to be written with a very steady hand. (there usually isn't that much of it -- our function is 100 lines long). Generally, one tries to use purely c library functions here, but we also use some low level lldb functions, mainly for decoding ProcessLaunchInfo struct. Some of them probably do some logging (I haven't looked at which ones, but this log disabling code has been there for ages). Normally, it is expected that the author of this code will make sure that all code he uses is safe to call in this funky state. However, logging is a bit special in that in that logging calls can crop up anywhere and we expect them to be safe to use from any context. So even if we made sure these functions do no logging that, that can easily change in the future. PS: One way we could encapsulate this better would be to use the pthread_atfork() call to register callback. That way, all of this nastiness could be hidden in the Log class. (However, I don't think this is particularly bad either, as ProcessLauncher is one of the lowest layers anyway) > > On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath wrote: >> >> On 15 October 2017 at 23:04, Zachary Turner wrote: >> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it >> > just >> > release at the end of the function? >> >> >> The thing is, it is unable to acquire the lock in the first place, >> because the mutex is already "locked". So, the sequence of events is >> process 1, thread 1: acquire lock >> process 1, thread 2: fork(), process 2 is created >> process 1 thread 1: release lock >> everything goes well from now on in process 1... >> process 2, thread one (and only). acquire lock: >> BANG. >> Process 2 is deadlocked because there is noone to release the lock there. On 15 October 2017 at 23:59, Zachary Turner wrote: > what are we using fork for, out of curiosity? It seems pretty hard to make > this work. Logging isn’t the only thing in the program that might be > holding a mutex, so it seems like this could still happen. > > Seems like mixing fork and mutexes is just a fundamentally bad idea, unless > I’m missing something (which i very well could be) > > On Sun, Oct 15, 2017 at 3:15 PM Pavel Labath wrote: >> >> On 15 October 2017 at 23:04, Zachary Turner wrote: >> > Doesn’t DisableAllLogChannels acquire a scoped lock? If so wouldn’t it >> > just >> > release at the end of the function? >> >> >> The thing is, it is unable to acquire the lock in the first place, >> because the mutex is already "locked". So, the sequence of events is >> process 1, thread 1: acquire lock >> process 1, thread 2: fork(), process 2 is created >> process 1 thread 1: release lock >> everything goes well from now on in process 1... >> process 2, thread one (and only). acquire lock: >> BANG. >> Process 2 is deadlocked because there is noone to release the lock there. ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
