Hi Jim, Thanks for the feedback.
I thought about the multithreading model you mentioned, and that's part of why I tried to distribute the handling of this so much. I think the ThreadList handler could add some sort of check to see if each thread stops when the process stops, but that would put me back in the position of needing to know which thread initiated the "stop" and SetPrivateState doesn't currently seem to know that. I wonder if Process::SetPrivateState itself will even make sense when the whole process doesn't share a state. It seems like I'm in a bit of a quandary here. My other patch lets the POSIX process plug-in handle the thread state, which is nice for the POSIX stuff but leaves other platforms to find their own solution. This second patch puts it in the common code, but makes assumptions about what's going to happen. I pushed the actual setting of the thread state down to the Thread object so that subclasses could intercept it if they needed to do something more. I'm happy to do it either way. The second patch seems marginally better to me, but I'm willing to defer to your judgment. I'm also happy to take a third stab at it if you see another way forward. -Andy -----Original Message----- From: [email protected] [mailto:[email protected]] Sent: Tuesday, May 07, 2013 5:51 PM To: Kaylor, Andrew Cc: [email protected]; Greg Clayton Subject: Re: Thread state At some point in the future (though not right away) we will want to allow a running mode where we have some threads that we keep alive and others we stop. At that point, the part of this patch where you do: +void +ThreadList::DidStop () +{ + Mutex::Locker locker(GetMutex()); + collection::iterator pos, end = m_threads.end(); + for (pos = m_threads.begin(); pos != end; ++pos) + { + // Notify threads that the process just stopped. + ThreadSP thread_sp(*pos); + if (StateIsRunningState(thread_sp->GetState())) + thread_sp->DidStop (); + } +} + will have been the wrong thing to do. There's lots of other code around that assumes that a "process did stop" event is meaningful, which in the case of 'keep-alive' debugging it obviously isn't. So OTOH, adding another is no worse than what is there already, but OTO I'd rather not add more if there's some way around it. It's really the responsibility of the plugins to say who did and didn't stop when the process got an interesting state changed message. Jim On May 7, 2013, at 5:29 PM, "Kaylor, Andrew" <[email protected]> wrote: > Hi Jim, > > What do you think of the attached patch? I've verified that this make the > existing test for thread state after a breakpoint pass on Linux, and I expect > it will also work on Darwin. > > The patch basically assumes that all threads that were previously running > became stopped when SetPrivateState is called. This isn't currently true on > Linux, but we want it to be true and this is a step in that direction. > > -Andy > > -----Original Message----- > From: [email protected] [mailto:[email protected]] > Sent: Thursday, May 02, 2013 1:27 PM > To: Kaylor, Andrew > Cc: [email protected]; Greg Clayton > Subject: Re: Thread state > > > On May 2, 2013, at 12:28 PM, "Kaylor, Andrew" <[email protected]> wrote: > >>> But I am not sure what you mean by "multi-threaded debugging"? Are you >>> talking about having the process (or at least some of its threads) stay >>> running while some of the threads stay stopped? >> >> Unfortunately, we're still trying to sort out the opposite problem. >> Currently on Linux when we hit a breakpoint all the other threads continue >> running and we're going to need to stop them manually. This does still put >> us in the position of having to handle potential incoming thread/process >> events while we're trying to stop everything. I've got some ideas for how >> to deal with that, but nothing in place. > > Ack, what a pain... > >> >> I prototyped an implementation which stops the threads from the >> ProcessMonitor callback and waits there for the stop notification, but it >> didn't handle the case where something other than what it's waiting for >> happens. I do have some tests which cause other things to happen in that >> time frame. >> >> The problem with setting the thread state in SetPrivateState is that by the >> time I get there I don't know which thread caused the stop, and in my >> current situation that's the only thread that's actually stopped (on Linux). >> I might be able to use a local variable farther upstream to track threads >> as I'm stopping them and then set all the threads to stopped in >> SetPrivateState, but that would result in threads being incorrectly marked >> as stopped on Linux until we get the code working to stop all threads. That >> might not be any worse than the current behavior though. > > lldb will handle the notion of many threads having stopped "for reasons" when > the process stops. This actually happens on Mac OS X quite frequently when > you have lots of threads. So if you can manage it, maybe it is best to hold > off on calling SetPrivateState till you've stopped all the threads > internally, then record in each thread why it stopped (either no reason if > you just managed to stop it without anything interesting happening, or > whatever the actual stop reason is if something else happened) and only then > set the Private state to stopped, which will cause all the generic event > handling to occur. > >> >> >> BTW, I committed a test for thread state under >> 'test/functionalities/thread/state' earlier this week. In addition to >> showing the failure to mark threads as stopped after a breakpoint, this test >> currently shows an inability to resume a process after it has been stopped >> with 'process interrupt'. Both of these cases fail on both Mac and Linux. >> The problem with 'process interrupt' seems to be because the public run lock >> isn't being unlocked when 'process interrupt' is used to stop the process. > > I'll take a look at this. > > Jim > > >> >> -Andy >> >> -----Original Message----- >> From: [email protected] [mailto:[email protected]] >> Sent: Thursday, May 02, 2013 12:08 PM >> To: Kaylor, Andrew >> Cc: [email protected]; Greg Clayton >> Subject: Re: Thread state >> >> This seems like the sort of thing that if it can be done generically it >> should. WillStop does seem too late. Process::SetPrivateState is the call >> that triggers notifying the rest of the lldb world that the process has >> stopped, so you definitely want to do it before that. >> >> In your suggested patch you are almost always doing it right before you call >> SetPrivateState, which suggests to me that probably it should be done there. >> If there is Process Plugin specific knowledge to figure out the thread >> state, you may need some virtual method in the plugin to do that, which you >> call from SetPrivateState. >> >> But I am not sure what you mean by "multi-threaded debugging"? Are you >> talking about having the process (or at least some of its threads) stay >> running while some of the threads stay stopped? If you are going to start >> thinking along those lines than a static call on thread to set it's state >> isn't going to work. After all, you are going to get some notification from >> the target that a target has stopped. So you set that thread's state to >> stopped, and then send the event to the generic execution control. While >> you're processing that, another thread stops, so you change its state and >> send another event. But the processing of the first event is only mid-way >> through, so now it is dealing with a thread state that changed out from >> under it. No good. >> >> If you really want to do "keep-alive" debugging, then which thread(s) >> participated in the stop needs to be recorded in the process event, and >> handled from there. >> >> Jim >> >> >> >> >> On May 2, 2013, at 11:21 AM, "Kaylor, Andrew" <[email protected]> >> wrote: >> >>> Ping. >>> >>> From: [email protected] [mailto:[email protected]] On >>> Behalf Of Kaylor, Andrew >>> Sent: Monday, April 29, 2013 3:55 PM >>> To: [email protected]; Jim Ingham; Greg Clayton >>> Subject: [lldb-dev] Thread state >>> >>> In preparation for getting multithreaded debugging working in LLDB on Linux >>> I'm trying to get the thread state in lldb::Thread objects to be kept >>> up-to-date in some reasonable fashion. I recently added a preliminary test >>> that checks the thread state of a single-threaded program and to my >>> surprise that test fails even on Darwin platforms. The test initially >>> fails because threads aren't marked as stopped when a breakpoint is hit in >>> the thread. >>> >>> I realize Process objects have both a private and a public state and that >>> the latter doesn't always correspond to the actual state of the inferior >>> process, and if I'm not mistaken there are some transitory times when the >>> private state also doesn't match the inferior's actual state. I've also >>> seen that Thread objects maintain a 'state' (which I take to be analogous >>> to the Process' private state) and a 'resume_state' (which I believe is the >>> state the thread should go into after a resume operation). I'm stating all >>> of this here so that if there's an error in my understanding of the design >>> it might be easier to spot. >>> >>> I've been specifically trying to get the Thread state to be correctly >>> updated when the inferior stops. I've found two ways of doing this: >>> >>> 1. Have Thread::WillStop call Thread::SetState(eStateStopped). >>> 2. Have the ProcessPOSIX::SendMessage call Thread::SetState for the >>> thread associated with the event. >>> >>> Option 1 is pretty straightforward, but it feels like it might be happening >>> too late in the overall flow. >>> >>> Option 2 only solves the problem for POSIX platforms, but it feels more >>> consistent with the current design. For the record, >>> ProcessPOSIX::SendMessage is called by the Linux/FreeBSD ProcessMonitor >>> callback function after they've figured out what a signal/trap from the >>> inferior means. This potential solution is represented in the attached >>> patch. >>> >>> The reason I care about the thread state is that I'm going to need to >>> manually stop background threads when something like a breakpoint happens >>> and bad things will happen if I try to stop a thread that's already >>> stopped. The ProcessPOSIX::SendMessage method seems like a good candidate >>> for where to stop the other threads, and so that's why I'm leaning toward >>> Option 2. >>> >>> However, I'm not certain I completely understand the existing design in all >>> of the related areas, so I thought I ought to step back and ask for >>> feedback at this point. >>> >>> Comments? Suggestions? >>> >>> Thank, >>> Andy >>> >>> <thread-state.patch> > > <thread-stop-state.patch> _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
