[Lldb-commits] [PATCH] use std::atomic<> to protect variables being accessed by multiple threads

[jingham at apple.com]

This seems like an incomplete solution to the problem that the thread sanitizer might have uncovered.  

In the case of m_shutting_down, this flag is advisory, and really means don't start any new work because I am going to shut down.  Then there's a mutex to make sure that the work of kicking anybody who is actually reading off the read thread and shutting it down is all protected by a mutex.  So putting the atomic around this seems fine, though not strictly necessary.

But for instance, in the case of m_exit_status, if somebody is really getting in to read the exit status while Process::SetExitStatus is in flight, then they get the wrong exit string (since IT may not have been set yet.)  BTW, it also seems kind of weird that ProcessPosix & ProcessFreeBSD do:

            m_exit_status = message.GetExitStatus();
            SetExitStatus(m_exit_status, NULL);

Should they really be setting the ivar and then calling the function that's supposed to set the ivar?

Similarly, if somebody really is accessing g_log_enabled while it might be being set in lldb_private::DisableLog then they might also be getting log bits which are in the process of being updated, so making the g_log_enabled atomic won't really solve this problem.  If you are convinced that can't happen then again, the atomic is formally good but not really necessary.  Otherwise it just shuts up the thread sanitizer without actually fixing the problem...

Jim



> On Sep 10, 2014, at 4:39 PM, Shawn Best <sbest at blueshiftinc.com> wrote:
> 
> 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.
> 
> http://reviews.llvm.org/D5302
> 
> Files:
>  include/lldb/Core/Communication.h
>  include/lldb/Core/ConnectionFileDescriptor.h
>  include/lldb/Target/Process.h
>  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
>  source/lldb-log.cpp
> <D5302.13564.patch>_______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits