Continue after process exit

[Honsik]

Summary

When process exits, and SBProcess.Continue() is executed, timeout occurs (5secs). Instead error message "Process is not alive" is returned. Added test for this message.

Fix of breakpoint case sensitivity test on Linux.

[jingham]

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.

[Honsik]

I tried to put this check in PrivateResume, but its not that simple because of the public run lock that is acquired in Resume and ResumeSynchronous. I am not that sure if it is safe to always unclock the lock inside PrivateResume.

[zturner]

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.

[jingham]

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()

<lldb.SBError; proxy of <Swig Object of type 'lldb::SBError *' at 0x10b9e7c00> >
Process 64883 exited with status = 0 (0x00000000)

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.

[Honsik]

OK I have modified the PrivateResume to add the check and release the public running lock.

The "Resume timed out" is caused by waiting for debugging thread events, but they won't ever come, because process is already dead.

[jingham]

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.

[Honsik]

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.

[jingham]

I marked a comment left over from a previous draft of the patch that isn't needed. Other than that, this looks fine.

source/Target/Process.cpp

3561–3562

Is this comment needed anymore?

[jingham]

More precisely, the "Public running lock..." part of the comment.

[zturner]

Petr, is this one ok to go in?

[Honsik]

Yes please, with that comment change jingham has mentioned.

Do you want me to create new diff?

[zturner]

I can remove that comment for you, no worries.

[Eugene.Zelenko]

Looks like patch was not committed.

packages/Python/lldbsuite/test/functionalities/breakpoint/breakpoint_case_sensitivity/TestBreakpointCaseSensitivity.py

         self.case_sensitivity_breakpoint(True)
 
     @skipIf(oslist=['windows']) # Skip for windows platforms
-    @expectedFailureAll() # Failing for unknown reason on non-Windows platforms.
     def test_breakpoint_doesnt_match_file_with_different_case(self):
         """Set breakpoint on file, shouldn't match files with different case on POSIX systems"""
         self.build()

                                                                    lldb.SBFileSpec(file))
         else:    
             breakpoint = self.target.BreakpointCreateByLocation(file, self.line)
+       
+        # breakpoint should be always created
+        self.assertTrue(breakpoint, VALID_BREAKPOINT + desc)
         
-        self.assertEqual(breakpoint and breakpoint.GetNumLocations() == 1,
-                    should_hit,
+        # check number of locations
+        self.assertEqual(breakpoint.GetNumLocations(),
+                    int(should_hit),
                     VALID_BREAKPOINT + desc)
+        
 
         # Get the breakpoint location from breakpoint after we verified that,
         # indeed, it has one location.
         location = breakpoint.GetLocationAtIndex(0)
-        self.assertEqual(location and location.IsEnabled(),
+        # location has to be converted to bool. If no location is present, GetLocationAtIndex returns None.
+        # Because None and xxxx = None, test will always fail (None != False and None != True)
+        self.assertEqual(bool(location) and location.IsEnabled(),
                     should_hit,
                     VALID_BREAKPOINT_LOCATION + desc)
         

             # check the breakpoint was not hit
             self.assertEqual(lldb.eStateExited, process.GetState())
             self.assertEqual(breakpoint.GetHitCount(), 0)
-
-        # let process finish
-        process.Continue()
         
         # cleanup
         self.target.BreakpointDelete(breakpoint.GetID())
+        # let process finish
+        process.Continue()
+        
 No newline at end of file

packages/Python/lldbsuite/test/python_api/process/TestProcessAPI.py

         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

source/Target/Process.cpp

     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

         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();

                     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())
     {