[Lldb-commits] [lldb] r253086 - Add a null check against the ThreadPlan

[Jim Ingham via lldb-commits]

Thanks.  When I first did the thread plan stuff I wasn't sure whether I would have a use for an empty plan stack, so I didn't rigorously go through and assert everywhere that might happen.  I had some idea for using an empty plan stack which was apparently so kooky that not only did I never implement it but I can't even reconstruct what it was now.  Anyway, one of those cleanup activities that you never seem to get to has been to go back and assert in all the places that might end up making the plan stack NULL (except for the bit where we empty out the plan stack & then prime it with the ThreadPlanNull which we use as an overt signal that the thread is defunct.

Jim

> On Nov 13, 2015, at 1:52 PM, Zachary Turner <zturner at google.com> wrote:
> 
> I'll change it to an assert and next time it happens I'll dig a little more.  I've only seen this on about 1 or 2 out of 100 runs of the test suite so I don't know how it happens.
> 
> On Fri, Nov 13, 2015 at 1:44 PM Jim Ingham <jingham at apple.com> wrote:
> GetCurrentPlan should never return NULL.  The Thread constructor pushes the Base thread plan onto the stack, and pop won't remove the Base plan.  When we destroy the thread (in ThreadDestroy) we clear the plan stack and then push a ThreadPlanNull onto the plan stack.  So if you are seeing a GetCurrentPlan returning NULL that means somebody is using it while it is in the process of being created or destroyed.  The former shouldn't be possible, so you must be accessing it while it is in mid-destroy.
> 
> I'd rather not put more code in various places that checks for things that should never be true, or if you are doing this check you should assert so we can figure out how this is happening.
> 
> Note, ThreadDestroy doesn't lock itself against uses of the thread.  That hasn't been necessary but if you are seeing crashes here, it might be worth doing that.
> 
> Jim
> 
> 
> > On Nov 13, 2015, at 1:28 PM, Zachary Turner via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> >
> > Author: zturner
> > Date: Fri Nov 13 15:28:53 2015
> > New Revision: 253086
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=253086&view=rev
> > Log:
> > Add a null check against the ThreadPlan
> >
> > I'm seeing some cases where the ThreadPlan is null.  It could
> > be a sign of a valid race condition, but at least we shouldn't
> > crash.
> >
> > Modified:
> >    lldb/trunk/source/Target/ThreadList.cpp
> >
> > Modified: lldb/trunk/source/Target/ThreadList.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Target/ThreadList.cpp?rev=253086&r1=253085&r2=253086&view=diff
> > ==============================================================================
> > --- lldb/trunk/source/Target/ThreadList.cpp (original)
> > +++ lldb/trunk/source/Target/ThreadList.cpp Fri Nov 13 15:28:53 2015
> > @@ -538,7 +538,7 @@ ThreadList::WillResume ()
> >
> >     for (pos = m_threads.begin(); pos != end; ++pos)
> >     {
> > -        if ((*pos)->GetResumeState() != eStateSuspended &&
> > +        if ((*pos)->GetResumeState() != eStateSuspended && (*pos)->GetCurrentPlan() &&
> >                  (*pos)->GetCurrentPlan()->StopOthers())
> >         {
> >             if ((*pos)->IsOperatingSystemPluginThread() && !(*pos)->GetBackingThread())
> >
> >
> > _______________________________________________
> > lldb-commits mailing list
> > lldb-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
>