Re: [Lldb-commits] [PATCH] D17635: Continue after process exit

[Eugene Zelenko via lldb-commits]

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

[Zachary Turner via lldb-commits]

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

[Zachary Turner via lldb-commits]

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

[Petr Hons via lldb-commits]

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

[Jim Ingham via lldb-commits]

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

[Jim Ingham via lldb-commits]

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

[Petr Hons via lldb-commits]

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

[Jim Ingham via lldb-commits]

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

[Jim Ingham via lldb-commits]

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

[Zachary Turner via lldb-commits]

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

[Petr Hons via lldb-commits]

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

[Jim Ingham via lldb-commits]

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