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, <[email protected]> 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 <[email protected]> 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 > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits > > -- Todd Fiala | Software Engineer | [email protected] | 650-943-3180
_______________________________________________ lldb-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
