Page MenuHomePhabricator

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

Authored by sbest on Sep 10 2014, 4:38 PM.

Details

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

Diff Detail

Event Timeline

sbest updated this revision to Diff 13564.Sep 10 2014, 4:38 PM
sbest retitled this revision from to use std::atomic<> to protect variables being accessed by multiple threads.
sbest updated this object.
sbest edited the test plan for this revision. (Show Details)
sbest added a reviewer: tfiala.
sbest added subscribers: tfiala, Unknown Object (MLST).
sbest updated this revision to Diff 13649.Sep 12 2014, 11:14 AM

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 accepted this revision.Sep 15 2014, 1:16 PM
tfiala edited edge metadata.
This revision is now accepted and ready to land.Sep 15 2014, 1:16 PM
tfiala closed this revision.Sep 15 2014, 1:17 PM

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.