[lldb-dev] Potential race conditions identified by ThreadSanitizer

[Todd Fiala]

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.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20140919/2233801a/attachment.html>