Author: tfiala Date: Mon Sep 15 15:07:33 2014 New Revision: 217818 URL: http://llvm.org/viewvc/llvm-project?rev=217818&view=rev Log: use std::atomic<> to protect variables being accessed by multiple threads
There are several places where multiple threads are accessing the same variables simultaneously without any kind of protection. I propose using std::atomic<> to make it safer. I did a special build of lldb, using the google tool 'thread sanitizer' which identified many cases of multiple threads accessing the same memory. std::atomic is low overhead and does not use any locks for simple types such as int/bool. See http://reviews.llvm.org/D5302 for more details. Change by Shawn Best. Modified: lldb/trunk/include/lldb/Core/Communication.h lldb/trunk/include/lldb/Core/ConnectionFileDescriptor.h lldb/trunk/include/lldb/Target/Process.h lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIX.cpp lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h lldb/trunk/source/Target/Process.cpp lldb/trunk/source/lldb-log.cpp Modified: lldb/trunk/include/lldb/Core/Communication.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/Communication.h?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/include/lldb/Core/Communication.h (original) +++ lldb/trunk/include/lldb/Core/Communication.h Mon Sep 15 15:07:33 2014 @@ -352,7 +352,7 @@ private: 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... Modified: lldb/trunk/include/lldb/Core/ConnectionFileDescriptor.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Core/ConnectionFileDescriptor.h?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/include/lldb/Core/ConnectionFileDescriptor.h (original) +++ lldb/trunk/include/lldb/Core/ConnectionFileDescriptor.h Mon Sep 15 15:07:33 2014 @@ -106,8 +106,8 @@ protected: 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); Modified: lldb/trunk/include/lldb/Target/Process.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Process.h?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/include/lldb/Target/Process.h (original) +++ lldb/trunk/include/lldb/Target/Process.h Mon Sep 15 15:07:33 2014 @@ -3050,6 +3050,7 @@ protected: 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 Modified: lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp (original) +++ lldb/trunk/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp Mon Sep 15 15:07:33 2014 @@ -258,8 +258,7 @@ ProcessFreeBSD::SendMessage(const Proces case ProcessMessage::eLimboMessage: case ProcessMessage::eExitMessage: - m_exit_status = message.GetExitStatus(); - SetExitStatus(m_exit_status, NULL); + SetExitStatus(message.GetExitStatus(), NULL); break; case ProcessMessage::eSignalMessage: Modified: lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIX.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIX.cpp?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIX.cpp (original) +++ lldb/trunk/source/Plugins/Process/POSIX/ProcessPOSIX.cpp Mon Sep 15 15:07:33 2014 @@ -434,8 +434,7 @@ ProcessPOSIX::SendMessage(const ProcessM // 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); Modified: lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h (original) +++ lldb/trunk/source/Plugins/Process/gdb-remote/ProcessGDBRemote.h Mon Sep 15 15:07:33 2014 @@ -328,7 +328,7 @@ protected: 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; Modified: lldb/trunk/source/Target/Process.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/Process.cpp?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/source/Target/Process.cpp (original) +++ lldb/trunk/source/Target/Process.cpp Mon Sep 15 15:07:33 2014 @@ -688,6 +688,7 @@ Process::Process(Target &target, Listene 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 @@ Process::IsRunning () const 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 @@ Process::GetExitStatus () 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 @@ Process::SetExitStatus (int status, cons 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 (); Modified: lldb/trunk/source/lldb-log.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/lldb-log.cpp?rev=217818&r1=217817&r2=217818&view=diff ============================================================================== --- lldb/trunk/source/lldb-log.cpp (original) +++ lldb/trunk/source/lldb-log.cpp Mon Sep 15 15:07:33 2014 @@ -27,7 +27,7 @@ using namespace lldb_private; // 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 lldb-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits