[lldb-dev] Potential race conditions identified by ThreadSanitizer

[Todd Fiala]

Thanks, Alexander.

On Fri, Sep 19, 2014 at 7:51 AM, Alexander Potapenko <glider at google.com>
wrote:

>
>
> 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
>



-- 
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-dev/attachments/20140919/1571e9ab/attachment.html>