<div dir="ltr">Oh yeah sure that's fine. When testing the test to make sure it catches the failure, you can just essentially reverse out the patch and then reapply to verify the fix.<div><br></div><div>No rush either, it would just be great to get more tests into the system to catch these things as we discover proper behavior.</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Mar 21, 2014 at 12:09 PM, Andrew MacPherson <span dir="ltr"><<a href="mailto:andrew.macp@gmail.com" target="_blank">andrew.macp@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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?<br>
<br>Cheers,<br>Andrew<br></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Mar 21, 2014 at 8:04 PM, Todd Fiala <span dir="ltr"><<a href="mailto:tfiala@google.com" target="_blank">tfiala@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hey Andrew,<div><br></div><div>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.</div>
</div><div class="gmail_extra"><div><div><br><br><div class="gmail_quote">On Fri, Mar 21, 2014 at 11:59 AM, <span dir="ltr"><<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
Jim<br>
<div><div><br>
On Mar 21, 2014, at 11:35 AM, Andrew MacPherson <<a href="mailto:andrew.macp@gmail.com" target="_blank">andrew.macp@gmail.com</a>> wrote:<br>
<br>
> 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.<br>
><br>
><br>
> On Fri, Mar 21, 2014 at 7:21 PM, Andrew MacPherson <<a href="mailto:andrew.macp@gmail.com" target="_blank">andrew.macp@gmail.com</a>> wrote:<br>
> 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?<br>
><br>
> Thanks again.<br>
><br>
><br>
><br>
> On Fri, Mar 21, 2014 at 6:58 PM, <<a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a>> wrote:<br>
> Sorry, that's StopInfoSignal::WillResume.<br>
><br>
> Jim<br>
><br>
> On Mar 21, 2014, at 10:54 AM, <a href="mailto:jingham@apple.com" target="_blank">jingham@apple.com</a> wrote:<br>
><br>
> > Somebody is not paying attention to the "process handle" settings. Normally SIGSTOP is set not to pass:<br>
> ><br>
> > (lldb) process handle SIGSTOP<br>
> > NAME PASS STOP NOTIFY<br>
> > ========== ===== ===== ======<br>
> > SIGSTOP false true true<br>
> ><br>
> > This check should be done in Process::WillResume:<br>
> ><br>
> > virtual void<br>
> > WillResume (lldb::StateType resume_state)<br>
> > {<br>
> > ThreadSP thread_sp (m_thread_wp.lock());<br>
> > if (thread_sp)<br>
> > {<br>
> > if (thread_sp->GetProcess()->GetUnixSignals().GetShouldSuppress(m_value) == false)<br>
> > thread_sp->SetResumeSignal(m_value);<br>
> > }<br>
> > }<br>
> ><br>
> > I wonder why this isn't happening in your case?<br>
> ><br>
> > Jim<br>
> ><br>
> ><br>
> > On Mar 21, 2014, at 10:46 AM, Andrew MacPherson <<a href="mailto:andrew.macp@gmail.com" target="_blank">andrew.macp@gmail.com</a>> wrote:<br>
> ><br>
> >> 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:<br>
> >><br>
> >> sudo lldb -p <pid><br>
> >> c<br>
> >> process interrupt<br>
> >> c<br>
> >><br>
> >> 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.<br>
> >><br>
> >> 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:<br>
> >><br>
> >> diff --git a/source/Plugins/Process/Linux/ProcessMonitor.cpp b/source/Plugins/Process/Linux/ProcessMonitor.cpp<br>
> >> index 3dec6de..3079379 100644<br>
> >> --- a/source/Plugins/Process/Linux/ProcessMonitor.cpp<br>
> >> +++ b/source/Plugins/Process/Linux/ProcessMonitor.cpp<br>
> >> @@ -2209,6 +2209,9 @@ ProcessMonitor::Resume(lldb::tid_t tid, uint32_t signo)<br>
> >> bool result;<br>
> >> Log *log (ProcessPOSIXLog::GetLogIfAllCategoriesSet (POSIX_LOG_PROCESS));<br>
> >><br>
> >> + if (signo == SIGSTOP)<br>
> >> + signo = eResumeSignalNone;<br>
> >> +<br>
> >> if (log)<br>
> >> log->Printf ("ProcessMonitor::%s() resuming thread = %" PRIu64 " with signal %s", __FUNCTION__, tid,<br>
> >> m_process->GetUnixSignals().GetSignalAsCString (signo));<br>
> >><br>
> >> 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.<br>
> >><br>
> >> Any thoughts as to what the proper fix here might be?<br>
> >><br>
> >> Thanks,<br>
> >> Andrew<br>
> >> _______________________________________________<br>
> >> lldb-dev mailing list<br>
> >> <a href="mailto:lldb-dev@cs.uiuc.edu" target="_blank">lldb-dev@cs.uiuc.edu</a><br>
> >> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
> ><br>
> > _______________________________________________<br>
> > lldb-dev mailing list<br>
> > <a href="mailto:lldb-dev@cs.uiuc.edu" target="_blank">lldb-dev@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
><br>
><br>
><br>
</div></div>> <SIGSTOP-resume.patch><br>
<div><div><br>
_______________________________________________<br>
lldb-dev mailing list<br>
<a href="mailto:lldb-dev@cs.uiuc.edu" target="_blank">lldb-dev@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div></div></div><span><font color="#888888">-- <br><div dir="ltr"><table style="color:rgb(136,136,136);font-family:'Times New Roman'" cellpadding="0" cellspacing="0">
<tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small">
<td style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px" nowrap>Todd Fiala |</td><td style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px" nowrap> Software Engineer |</td>
<td style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px" nowrap> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a> |</td>
<td style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px" nowrap><font color="#1155cc"> <a>650-943-3180</a></font></td></tr></tbody></table><br></div>
</font></span></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr"><table cellspacing="0" cellpadding="0" style="color:rgb(136,136,136);font-family:'Times New Roman'"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small">
<td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Todd Fiala |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td>
<td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tfiala@google.com" style="color:rgb(17,85,204)" target="_blank"><span style="background-color:rgb(255,255,204);color:rgb(34,34,34);background-repeat:initial initial">tfiala@google.com</span></a> |</td>
<td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"><font color="#1155cc"> <a>650-943-3180</a></font></td></tr></tbody></table><br></div>
</div>