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

[Todd Fiala]

Hey Jim,

Just for clarification, is this a LGTM?  Or are you wanting Shawn to spend
more time looking at the bigger picture behind some of the locking?

-Todd

On Fri, Sep 12, 2014 at 12:37 PM, <jingham at apple.com> wrote:

> That looks okay.  Thanks for looking at this.  Seems to me that if you're
> only synchronization is locking access to some flag, then you're either
> doing something fairly weak (like the log example) or missing some real
> synchronization...  So it's worth viewing the necessity of the change with
> skepticism.
>
> Jim
>
> > On Sep 12, 2014, at 11:15 AM, Shawn Best <sbest at blueshiftinc.com> wrote:
> >
> > 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.
> >
> > http://reviews.llvm.org/D5302
> >
> > Files:
> >  include/lldb/Core/Communication.h
> >  include/lldb/Core/ConnectionFileDescriptor.h
> >  include/lldb/Target/Process.h
> >  source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp
> >  source/Plugins/Process/POSIX/ProcessPOSIX.cpp
> >  source/Plugins/Process/gdb-remote/ProcessGDBRemote.h
> >  source/Target/Process.cpp
> >  source/lldb-log.cpp
> > <D5302.13649.patch>_______________________________________________
> > lldb-commits mailing list
> > lldb-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>


-- 
Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20140915/bb068015/attachment.html>