[lldb-dev] Potential race conditions identified by ThreadSanitizer

[Alexander Potapenko]

On Fri, Sep 19, 2014 at 6:46 PM, Todd Fiala <tfiala at google.com> wrote:

> Hey Alexander,
>
> We totally agree.  We're in the process of getting internal build bots
> that look for this.  We've been working with Chandler on getting an
> externally-visible buildbot infrastructure up.
>
> For the time being, there is a lot of noise in the tsan reports.  Since we
> won't be able to just focus on squelching all of those at once, we're
> likely going to try to filter the tsan output to have it indicate new
> occurrences, and get to fixing the more important ones as we find them.
>
> You can possibly make use of ThreadSanitizer's suppressions mechanism, see
https://code.google.com/p/thread-sanitizer/wiki/Suppressions

That's the long-term plan, at least.
>
> -Todd
>
> On Fri, Sep 19, 2014 at 7:09 AM, Alexander Potapenko <glider at google.com>
> wrote:
>
>> Hi Shawn,
>>
>> This is really great that you've committed your time to fix some these
>> issues. But our experience shows that without regular testing new bugs
>> will appear in the codebase soon.
>> Perhaps a good idea is to add a buildbot running lldb tests under
>> ThreadSanitizer, if the lldb team is interested in investing into
>> that.
>>
>> Regarding the race reports that are "not an issue in practice", such
>> cases still violate the C++ Standard, so nobody knows when they're
>> going to break.
>>
>> HTH,
>> Alex.
>>
>> On Tue, Sep 16, 2014 at 5:39 AM, Shawn Best <sbest at blueshiftinc.com>
>> wrote:
>> > Hi,
>> >
>> >
>> > I have recently built lldb with an automated tool “ThreadSanitizer( or
>> > tsan)” designed to detect race  conditions and similar concurrency
>> problems.
>> > It turned up a number of potential issues.  Some may be real, others are
>> > probably not an issue in practice.  I submitted a patch which fixed a
>> few,
>> > either using a mutex, or std::atomic< >.  The process of manually
>> analyzing
>> > each warning involves a lot of time consuming legwork.  In many cases,
>> the
>> > work to quiet the warning would add code complexity and runtime hit
>> that may
>> > not be worth it.
>> >
>> >
>> > I need to back-burner this for a bit and move on tow work on something
>> > different.  I thought I would post some of my findings here as there may
>> > still be some real bugs in there.
>> >
>> >
>> > 1.  ==========================
>> >
>> >
>> > Timer.cpp: g_depth : this is a class static variable used in
>> constructor and
>> > destructor.  Use of a timer object from different threads will have a
>> race.
>> >
>> >
>> > CommandInterpreter::HandleCommand() uses Timer scopedTimer;  // called
>> from
>> > main
>> >
>> >
>> > Disassembler::FindPlugin() uses scoped_timer, called from event handler
>> > thread
>> >
>> >
>> > 2. ===========================
>> >
>> >
>> > Editline.cpp/.h : Editline::Hide(), and other functions use
>> m_getting_line
>> > from eventhandler thread
>> >
>> >
>> > EditLine::GetLine() called from main thread via IOHandlerEditLine::Run()
>> >
>> >
>> > 3. ===========================
>> >
>> >
>> > IOHandler::Activate(), Deactivate() accesses m_active, called in
>> > Debugger::ExecuteIOHandlers() in main thread,
>> >
>> > Debugger::PopIOHandler()  can also call those functions from event
>> handler
>> > thread
>> >
>> >
>> > 4. ===================
>> >
>> >
>> > IOHandlerProcessSTDIO::OpenPipes() will read/write to m_pipe from main
>> > thread
>> >
>> > IOHandlerProcessSTDIO::Cancel() will call m_pipe.Write() from event
>> handler
>> > thread
>> >
>> >
>> > 5. =======================
>> >
>> > Process.h:  m_private_state_thread - accessed from multiple places from
>> main
>> > thread
>> >
>> > Process::RunThreadPlan(),
>> >
>> > Process::StartPrivateStateThread(),
>> >
>> > Process::ControlPrivateStateThread()
>> >
>> > Process::PrivateStateThreadIsValid()
>> >
>> >
>> > and from internal state thread:
>> >
>> > Process::RunPrivateStateThread() clears this value when exiting. I don’t
>> > think this is safe in all possible cases
>> >
>> >
>> > 6. ===========================
>> >
>> >
>> > There is a whole class of problems related to resources created in
>> Process,
>> > and used by multiple threads. TSAN complains when the ~Process()
>> destructor
>> > is called and destroys those resources.  From what I can tell, the
>> threads
>> > are typically shut down and not using those resources at that point,
>> but if
>> > a thread were to shut itself down asynchronously, I think it is
>> possible for
>> > the resources to get destroyed without a formal interlock on those
>> events.
>> >
>> >
>> > Things like:
>> >
>> > m_public_run_lock.SetStopped();
>> >
>> > m_private_state_control_wait.SetValue (true, eBroadcastAlways);
>> >
>> > m_private_state_thread.Reset();
>> >
>> > called in private state thread
>> >
>> >
>> > In ~Process(), if m_private_state_thread null, it will not call
>> > ControlPrivateStateThread() to shut the thread down.
>> >
>> >
>> > 7. ============================
>> >
>> > Similar to point 5:
>> >
>> >
>> > ProcessGDBRemote::AsyncThread(), can clear m_async_thread when shutting
>> > down, while it is access many places from the main thread.
>> >
>> >
>> > 8. ===========================
>> >
>> > Communication::m_read_thread_enabled   is read from ReadThread(), but
>> > modified from main()
>> >
>> >
>> > 9. ============================
>> >
>> >
>> > ProcessGDBRemote::DoDestroy() will call a Socket::Close() from main
>> thread.
>> >
>> >
>> > An error condition in reading a socket in the async thread will call
>> > Disconnect(), accessing the same socket variable
>> >
>> >
>> >
>> > _______________________________________________
>> > lldb-dev mailing list
>> > lldb-dev at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>> >
>>
>>
>>
>> --
>> Alexander Potapenko
>> Software Engineer
>> Google Moscow
>>
>> _______________________________________________
>> lldb-dev mailing list
>> lldb-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>>
>
>
>
> --
> Todd Fiala | Software Engineer | tfiala at google.com | 650-943-3180
>



-- 
Alexander Potapenko
Software Engineer
Google Moscow
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140919/b51538df/attachment.html>