Hey Todd, sure thing. I haven't written a test before and I'm out of time tonight but I can take a look over the weekend or early next week, I'm looking into another issue with a SIGSTOP leaking out on detach as well so would you be okay if I apply this small patch now and then write a test that covers both this case and the detach case for when I submit a patch for that?
Cheers, Andrew On Fri, Mar 21, 2014 at 8:04 PM, Todd Fiala <[email protected]> wrote: > Hey Andrew, > > Any chance you can put a test into lldb to verify the fix? I'm doing some > work on (essentially currently) parallel code (implementations of > NativeProcessProtocol/NativeThreadProtocol) for Linux that will be used in > lldb-gdbserver and I'd love us to have tests that verify we don't gunk that > up or regress it in the future. > > > On Fri, Mar 21, 2014 at 11:59 AM, <[email protected]> wrote: > >> Indeed, setting the resume signal explicitly on the thread on stop was >> not the right thing to do. You don't want to clear the thread's resume >> signal out in Thread::ShouldStop because somebody might be trying to resume >> the thread with a hand-provided signal, and you wouldn't want to interrupt >> that. But the thread itself should let the StopInfo carry the stop signal >> information. >> >> Jim >> >> On Mar 21, 2014, at 11:35 AM, Andrew MacPherson <[email protected]> >> wrote: >> >> > Actually it looks like those calls may be redundant now as it's being >> handled by StopInfo. This patch gets attach resume/interrupt working for me >> on Linux. >> > >> > >> > On Fri, Mar 21, 2014 at 7:21 PM, Andrew MacPherson < >> [email protected]> wrote: >> > Thanks Jim, LinuxSignals.cpp had SIGSTOP marked as suppress = false >> unlike UnixSignals.cpp which had it correctly marked suppress = true. With >> that fixed though the SIGSTOP is still getting set due to the call to >> SetResumeSignal() in POSIXThread::SignalDeliveredNotify() it looks like. >> Should this code also be using the signals table to decide whether to >> suppress it? >> > >> > Thanks again. >> > >> > >> > >> > On Fri, Mar 21, 2014 at 6:58 PM, <[email protected]> wrote: >> > Sorry, that's StopInfoSignal::WillResume. >> > >> > Jim >> > >> > On Mar 21, 2014, at 10:54 AM, [email protected] wrote: >> > >> > > Somebody is not paying attention to the "process handle" settings. >> Normally SIGSTOP is set not to pass: >> > > >> > > (lldb) process handle SIGSTOP >> > > NAME PASS STOP NOTIFY >> > > ========== ===== ===== ====== >> > > SIGSTOP false true true >> > > >> > > This check should be done in Process::WillResume: >> > > >> > > virtual void >> > > WillResume (lldb::StateType resume_state) >> > > { >> > > ThreadSP thread_sp (m_thread_wp.lock()); >> > > if (thread_sp) >> > > { >> > > if >> (thread_sp->GetProcess()->GetUnixSignals().GetShouldSuppress(m_value) == >> false) >> > > thread_sp->SetResumeSignal(m_value); >> > > } >> > > } >> > > >> > > I wonder why this isn't happening in your case? >> > > >> > > Jim >> > > >> > > >> > > On Mar 21, 2014, at 10:46 AM, Andrew MacPherson < >> [email protected]> wrote: >> > > >> > >> Currently when attaching to a running process on Linux, a SIGSTOP >> signal is (incorrectly I think) injected into the inferior on resume. This >> can be reproduced by simply launching any process and then in a separate >> terminal doing: >> > >> >> > >> sudo lldb -p <pid> >> > >> c >> > >> process interrupt >> > >> c >> > >> >> > >> On the second continue the SIGSTOP that was used to stop the process >> is injected into the inferior by being passed to PTRACE_CONT, resulting in >> the process going into a stop state outside the control of LLDB. The >> SIGSTOP comes from the SetResumeSignal() call in POSIXThread and StopInfo. >> > >> >> > >> I can't think of any reason why a SIGSTOP should ever be injected >> into the inferior so as a test I simply prevented it from ever happening: >> > >> >> > >> diff --git a/source/Plugins/Process/Linux/ProcessMonitor.cpp >> b/source/Plugins/Process/Linux/ProcessMonitor.cpp >> > >> index 3dec6de..3079379 100644 >> > >> --- a/source/Plugins/Process/Linux/ProcessMonitor.cpp >> > >> +++ b/source/Plugins/Process/Linux/ProcessMonitor.cpp >> > >> @@ -2209,6 +2209,9 @@ ProcessMonitor::Resume(lldb::tid_t tid, >> uint32_t signo) >> > >> bool result; >> > >> Log *log (ProcessPOSIXLog::GetLogIfAllCategoriesSet >> (POSIX_LOG_PROCESS)); >> > >> >> > >> + if (signo == SIGSTOP) >> > >> + signo = eResumeSignalNone; >> > >> + >> > >> if (log) >> > >> log->Printf ("ProcessMonitor::%s() resuming thread = %" >> PRIu64 " with signal %s", __FUNCTION__, tid, >> > >> >> m_process->GetUnixSignals().GetSignalAsCString (signo)); >> > >> >> > >> This resolves the issue and doesn't cause any other problems that I >> can find but almost certainly isn't the proper fix. My main concern is that >> all of the resume signal code is shared with other OSes which I'm guessing >> treat this differently. >> > >> >> > >> Any thoughts as to what the proper fix here might be? >> > >> >> > >> Thanks, >> > >> Andrew >> > >> _______________________________________________ >> > >> lldb-dev mailing list >> > >> [email protected] >> > >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev >> > > >> > > _______________________________________________ >> > > lldb-dev mailing list >> > > [email protected] >> > > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev >> > >> > >> > >> > <SIGSTOP-resume.patch> >> >> _______________________________________________ >> lldb-dev mailing list >> [email protected] >> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev >> > > > > -- > Todd Fiala | Software Engineer | [email protected] | 650-943-3180 >
_______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
