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 <[email protected]> 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
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
