Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
Eugene.Zelenko added a subscriber: Eugene.Zelenko. Eugene.Zelenko added a comment. Looks like patch was not committed. https://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
zturner added a comment. I can remove that comment for you, no worries. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
zturner added a comment. Petr, is this one ok to go in? http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
Honsik added a comment. Yes please, with that comment change jingham has mentioned. Do you want me to create new diff? http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
jingham added a comment. More precisely, the "Public running lock..." part of the comment. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
jingham accepted this revision. jingham added a comment. This revision is now accepted and ready to land. I marked a comment left over from a previous draft of the patch that isn't needed. Other than that, this looks fine. Comment at: source/Target/Process.cpp:3561-3562 @@ +3560,4 @@ +Error error; +// Cannot resume already exited process. Public running lock is +// held when PrivateResume is entered +if (!IsAlive()) Is this comment needed anymore? http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
Honsik updated this revision to Diff 50373. Honsik added a comment. Sorry for the delay, I have been on holiday. I have modified the patch as jingham requested. The public run lock is reset back on error in both Resume and ResumeSynchronous methods. http://reviews.llvm.org/D17635 Files: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py packages/Python/lldbsuite/test/python_api/process/TestProcessAPI.py source/Target/Process.cpp Index: source/Target/Process.cpp === --- source/Target/Process.cpp +++ source/Target/Process.cpp @@ -1740,14 +1740,23 @@ Log *log(lldb_private::GetLogIfAnyCategoriesSet (LIBLLDB_LOG_STATE | LIBLLDB_LOG_PROCESS)); if (log) log->Printf("Process::Resume -- locking run lock"); + +Error error; if (!m_public_run_lock.TrySetRunning()) { -Error error("Resume request failed - process still running."); +error.SetErrorString("Resume request failed - process still running."); if (log) log->Printf ("Process::Resume: -- TrySetRunning failed, not resuming."); return error; } -return PrivateResume(); + +error = PrivateResume(); +if (error.Fail()) +{ +// PrivateResume failed, reset the public run lock back +m_public_run_lock.SetStopped(); +} +return error; } Error @@ -1775,6 +1784,11 @@ if (!StateIsStoppedState(state, must_be_alive)) error.SetErrorStringWithFormat("process not in stopped state after synchronous resume: %s", StateAsCString(state)); } +else +{ +// PrivateResume failed, reset the public run lock back +m_public_run_lock.SetStopped(); +} // Undo the hijacking of process events... RestoreProcessEvents(); @@ -3543,7 +3557,16 @@ StateAsCString(m_public_state.GetValue()), StateAsCString(m_private_state.GetValue())); -Error error (WillResume()); +Error error; +// Cannot resume already exited process. Public running lock is +// held when PrivateResume is entered +if (!IsAlive()) +{ +error.SetErrorString("Process is not alive"); +return error; +} + +error = WillResume(); // Tell the process it is about to resume before the thread list if (error.Success()) { Index: packages/Python/lldbsuite/test/python_api/process/TestProcessAPI.py === --- packages/Python/lldbsuite/test/python_api/process/TestProcessAPI.py +++ packages/Python/lldbsuite/test/python_api/process/TestProcessAPI.py @@ -284,3 +284,40 @@ if self.TraceOn() and error.Success(): print("Number of supported hardware watchpoints: %d" % num) +@add_test_categories(['pyapi']) +def test_continue_after_process_exit(self): +"""Test SBProcess.Continue() API after the process exits.""" +self.build() +exe = os.path.join(os.getcwd(), "a.out") +self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET) + +target = self.dbg.CreateTarget(exe) +self.assertTrue(target, VALID_TARGET) + +breakpoint = target.BreakpointCreateByLocation("main.cpp", self.line) +self.assertTrue(breakpoint, VALID_BREAKPOINT) + +# Launch the process, and do not stop at the entry point. +process = target.LaunchSimple (None, None, self.get_process_working_directory()) + +thread = get_stopped_thread(process, lldb.eStopReasonBreakpoint) +self.assertTrue(thread.IsValid(), "There should be a thread stopped due to breakpoint") + +frame = thread.GetFrameAtIndex(0) +le = frame.GetLineEntry() +self.assertTrue(le.IsValid(), "There should be valid line entry at breakpoint") +self.assertEqual(self.line, le.GetLine(), "There should be valid line number") + +# Continue the return out of main +err = process.Continue() +self.assertTrue(err.Success(), "Continue after breakpoint should be valid") + +# At this point, the inferior process should have exited. +self.assertEqual(lldb.eStateExited, process.GetState(), PROCESS_EXITED) + +# Continue after proces exited should fail with good message, try it multiple times +for i in range(2): +err = process.Continue() +self.assertTrue(err.Fail(), "Continue after exit shouldn't be valid") +self.assertIn("Process is not alive", err.GetCString()) + \ No newline at end of file Index: packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py === ---
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
jingham added a comment. I don't think you can manipulate the public run lock in PrivateResume like this. PrivateResume gets run in a bunch of places (like calling functions) that are way below the level the public run lock. You probably need to catch errors from PrivateResume in Resume and release the lock there. That's a little ugly, but it is good to have PrivateResume return an accurate error, so I think putting the check there is right. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
jingham requested changes to this revision. jingham added a reviewer: jingham. jingham added a comment. This revision now requires changes to proceed. I agree with Zachary, it would be better to put it in PrivateResume before the call to WillResume. Having this happen in Process::PrivateResume after taking the run lock is okay, that works correctly on OS X. OTOH, the error reporting isn't correct there: > > > lldb.process.Continue() > > > >Process 64883 exited with status = 0 (0x) > > > error = lldb.process.Continue() > > > > > > > print error > > > > error: Resume timed out. So this definitely needs fixing generically... Process::WillResume only gets called in one place (Process::PrivateResume) so it is fine to just put the check there before calling WillResume. When we have generic bits of work we want to do before or after a plugin method X we often make a virtual "DoX" and have that be the plugin method, and then X is not virtual and does the generic work. But that seems overkill in this case, we just want to make sure the process is alive before calling into the plugins. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
zturner added a comment. It doesn't look like `Process::PrivateResume()` returns an error if the process is alive unless `WillResume()` returns an error, which is up to the individual process implementation. So maybe the short circuit needs to happen there. This isn't really my area though so I'll defer to Jim on this review. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
Honsik added a comment. I tried to put this check in PrivateResume, but its not that simple because of the public RUN lock. I am not that sure if it is safe to always unclock the lock inside PrivateResume. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D17635: Continue after process exit
jingham added a subscriber: jingham. jingham added a comment. It's okay to short-circuit this here, but why was PrivateResume not returning an error when the process was not alive. That error should have gotten propagated to the caller, obviating the need for this short-circuit. http://reviews.llvm.org/D17635 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits