use std::atomic<> to protect variables being accessed by multiple threads

[sbest]

Summary

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.

[sbest]

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.

[tfiala]

Submitted here:

svn commit
Sending        include/lldb/Core/Communication.h
Sending        include/lldb/Core/ConnectionFileDescriptor.h
Sending        include/lldb/Target/Process.h
Sending        source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
Sending        source/Plugins/Process/POSIX/ProcessPOSIX.cpp
Sending        source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
Sending        source/Target/Process.cpp
Sending        source/lldb-log.cpp
Transmitting file data ........
Committed revision 217818.

include/lldb/Core/Communication.h

 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...

include/lldb/Core/ConnectionFileDescriptor.h

 
     Pipe m_pipe;
     Mutex m_mutex;
-    bool m_shutting_down;       // This marks that we are shutting down so if we get woken up from
+    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);

include/lldb/Target/Process.h

     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

source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp

 
     case ProcessMessage::eLimboMessage:
     case ProcessMessage::eExitMessage:
-        m_exit_status = message.GetExitStatus();
-        SetExitStatus(m_exit_status, NULL);
+        SetExitStatus(message.GetExitStatus(), NULL);
         break;
 
     case ProcessMessage::eSignalMessage:

source/Plugins/Process/POSIX/ProcessPOSIX.cpp

         // 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);

source/Plugins/Process/gdb-remote/ProcessGDBRemote.h

     
     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;

source/Target/Process.cpp

     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),

 int
 Process::GetExitStatus ()
 {
+    Mutex::Locker locker (m_exit_status_mutex);
+
     if (m_public_state.GetValue() == eStateExited)
         return m_exit_status;
     return -1;

 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;

         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 ();
 

source/lldb-log.cpp

 // 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 ()