Hi Jim, thanks for the feedback.
I agree these may fall into the category of 'formally correct, but not strictly
necessary.' One good thing is it makes clear the intention these variables are
being directly accessed from multiple threads.
Something I noticed about thread sanitizer is that it will complain if 2
threads access the same memory without a synchronization mechanism between
them, even if those accesses are separated by a good amount of time (or some
other kind of logic).
Looking at closer at exit status, the problem, I think I should add a mutex to
Process::SetExitStatus() , GetExitStatus(), GetExitDescription(). This should
prevent an exit code and exit string potentially not matching.
Yes, ProcessPosix, ProcessFreeBSD directly setting the variable, then calling a
function to do so, looks a little weird, I've cleaned that up.
As far as I can tell, if someone were to turn logging on/off in the middle of a
function dumping log information... the worst that would happen is it would not
start/stop a bit late. The typical usage pattern will check the flag once at
the beginning of the function and keep the pointer for the duration of the
function. I don't think that would be anything a user would notice.
Shawn.
http://reviews.llvm.org/D5302
Files:
include/lldb/Core/Communication.h
include/lldb/Core/ConnectionFileDescriptor.h
include/lldb/Target/Process.h
source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
source/Plugins/Process/POSIX/ProcessPOSIX.cpp
source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
source/Target/Process.cpp
source/lldb-log.cpp
Index: include/lldb/Core/Communication.h
===================================================================
--- include/lldb/Core/Communication.h
+++ include/lldb/Core/Communication.h
@@ -352,7 +352,7 @@
protected:
lldb::ConnectionSP m_connection_sp; ///< The connection that is current in use by this communications class.
HostThread m_read_thread; ///< The read thread handle in case we need to cancel the thread.
- bool m_read_thread_enabled;
+ std::atomic<bool> m_read_thread_enabled;
std::string m_bytes; ///< A buffer to cache bytes read in the ReadThread function.
Mutex m_bytes_mutex; ///< A mutex to protect multi-threaded access to the cached bytes.
Mutex m_write_mutex; ///< Don't let multiple threads write at the same time...
Index: include/lldb/Core/ConnectionFileDescriptor.h
===================================================================
--- include/lldb/Core/ConnectionFileDescriptor.h
+++ include/lldb/Core/ConnectionFileDescriptor.h
@@ -106,8 +106,8 @@
Pipe m_pipe;
Mutex m_mutex;
- bool m_shutting_down; // This marks that we are shutting down so if we get woken up from
- // BytesAvailable to disconnect, we won't try to read again.
+ std::atomic<bool> m_shutting_down; // This marks that we are shutting down so if we get woken up from
+ // BytesAvailable to disconnect, we won't try to read again.
bool m_waiting_for_accept;
private:
DISALLOW_COPY_AND_ASSIGN (ConnectionFileDescriptor);
Index: include/lldb/Target/Process.h
===================================================================
--- include/lldb/Target/Process.h
+++ include/lldb/Target/Process.h
@@ -3050,6 +3050,7 @@
std::map<uint64_t, uint32_t> m_thread_id_to_index_id_map;
int m_exit_status; ///< The exit status of the process, or -1 if not set.
std::string m_exit_string; ///< A textual description of why a process exited.
+ Mutex m_exit_status_mutex; ///< Mutex so m_exit_status m_exit_string can be safely accessed from multiple threads
Mutex m_thread_mutex;
ThreadList m_thread_list_real; ///< The threads for this process as are known to the protocol we are debugging with
ThreadList m_thread_list; ///< The threads for this process as the user will see them. This is usually the same as
Index: source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
===================================================================
--- source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
+++ source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
@@ -258,8 +258,7 @@
case ProcessMessage::eLimboMessage:
case ProcessMessage::eExitMessage:
- m_exit_status = message.GetExitStatus();
- SetExitStatus(m_exit_status, NULL);
+ SetExitStatus(message.GetExitStatus(), NULL);
break;
case ProcessMessage::eSignalMessage:
Index: source/Plugins/Process/POSIX/ProcessPOSIX.cpp
===================================================================
--- source/Plugins/Process/POSIX/ProcessPOSIX.cpp
+++ source/Plugins/Process/POSIX/ProcessPOSIX.cpp
@@ -434,8 +434,7 @@
// FIXME: I'm not sure we need to do this.
if (message.GetTID() == GetID())
{
- m_exit_status = message.GetExitStatus();
- SetExitStatus(m_exit_status, NULL);
+ SetExitStatus(message.GetExitStatus(), NULL);
}
else if (!IsAThreadRunning())
SetPrivateState(eStateStopped);
Index: source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
===================================================================
--- source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
+++ source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
@@ -328,7 +328,7 @@
lldb_private::Flags m_flags; // Process specific flags (see eFlags enums)
GDBRemoteCommunicationClient m_gdb_comm;
- lldb::pid_t m_debugserver_pid;
+ std::atomic<lldb::pid_t> m_debugserver_pid;
StringExtractorGDBRemote m_last_stop_packet;
lldb_private::Mutex m_last_stop_packet_mutex;
GDBRemoteDynamicRegisterInfo m_register_info;
Index: source/Target/Process.cpp
===================================================================
--- source/Target/Process.cpp
+++ source/Target/Process.cpp
@@ -688,6 +688,7 @@
m_thread_id_to_index_id_map (),
m_exit_status (-1),
m_exit_string (),
+ m_exit_status_mutex(),
m_thread_mutex (Mutex::eMutexTypeRecursive),
m_thread_list_real (this),
m_thread_list (this),
@@ -1186,6 +1187,8 @@
int
Process::GetExitStatus ()
{
+ Mutex::Locker locker (m_exit_status_mutex);
+
if (m_public_state.GetValue() == eStateExited)
return m_exit_status;
return -1;
@@ -1195,6 +1198,8 @@
const char *
Process::GetExitDescription ()
{
+ Mutex::Locker locker (m_exit_status_mutex);
+
if (m_public_state.GetValue() == eStateExited && !m_exit_string.empty())
return m_exit_string.c_str();
return NULL;
@@ -1219,11 +1224,16 @@
return false;
}
- m_exit_status = status;
- if (cstr)
- m_exit_string = cstr;
- else
- m_exit_string.clear();
+ // use a mutex to protect the status and string during updating
+ {
+ Mutex::Locker locker (m_exit_status_mutex);
+
+ m_exit_status = status;
+ if (cstr)
+ m_exit_string = cstr;
+ else
+ m_exit_string.clear();
+ }
DidExit ();
Index: source/lldb-log.cpp
===================================================================
--- source/lldb-log.cpp
+++ source/lldb-log.cpp
@@ -27,7 +27,7 @@
// that will construct the static g_lob_sp the first time this function is
// called.
-static bool g_log_enabled = false;
+static std::atomic<bool> g_log_enabled {false};
static Log * g_log = NULL;
static Log *
GetLog ()
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
