Use Resume not PrivateResume when asynchronously continuing after the start at stop

[jingham]

Summary

In the auto-continue after we stop at the start point when running the debugger asynchronously, we use PrivateResume to continue. But that doesn't update the stop locker state, which is currently at "stopped". That means the stop locker wasn't doing its job to prevent you from running expressions while the target is running.

I fixed this, and added a test for this case to make sure we get a proper error back. This patch requires (and adds the test for):

https://reviews.llvm.org/D144664

[stella.stamenova]

Looks like the new test is failing on the windows lldb bot: https://lab.llvm.org/buildbot/#/builders/83/builds/29680. The buildbot was already red because of TestCommandScript.py, so you might have missed this.

[mib]

lldb/test/API/python_api/run_locker/TestRunLocker.py

25

@jingham, you're testing the same thing twice. You probably wanted to set the stop_at_entry argument to True here.

lldb/source/API/SBFrame.cpp

 
 #include "lldb/lldb-types.h"
 
 #include "Utils.h"
 #include "lldb/Core/Address.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Core/ValueObjectVariable.h"
+#include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Symbol/Block.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/Variable.h"
if (frame && target) {
     options.SetFetchDynamicValue(fetch_dynamic_value);
     options.SetUnwindOnError(true);
     options.SetIgnoreBreakpoints(true);
     if (target->GetLanguage() != eLanguageTypeUnknown)
       options.SetLanguage(target->GetLanguage());
     else
       options.SetLanguage(frame->GetLanguage());
     return EvaluateExpression(expr, options);
+  } else {
+    Status error;
+    error.SetErrorString("can't evaluate expressions when the "
+                           "process is running.");
+    ValueObjectSP error_val_sp = ValueObjectConstResult::Create(nullptr, error);
+    result.SetSP(error_val_sp, false);
   }
   return result;
 }
 
 SBValue
 SBFrame::EvaluateExpression(const char *expr,
                             lldb::DynamicValueType fetch_dynamic_value) {
   LLDB_INSTRUMENT_VA(this, expr, fetch_dynamic_value);
if (expr == nullptr || expr[0] == '\0') {
     return expr_result;
   }
 
   ValueObjectSP expr_value_sp;
 
   std::unique_lock<std::recursive_mutex> lock;
   ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
 
-
   StackFrame *frame = nullptr;
   Target *target = exe_ctx.GetTargetPtr();
   Process *process = exe_ctx.GetProcessPtr();
 
   if (target && process) {
     Process::StopLocker stop_locker;
     if (stop_locker.TryLock(&process->GetRunLock())) {
       frame = exe_ctx.GetFramePtr();
       if (frame) {
         std::unique_ptr<llvm::PrettyStackTraceFormat> stack_trace;
         if (target->GetDisplayExpressionsInCrashlogs()) {
           StreamString frame_description;
           frame->DumpUsingSettingsFormat(&frame_description);
           stack_trace = std::make_unique<llvm::PrettyStackTraceFormat>(
               "SBFrame::EvaluateExpression (expr = \"%s\", fetch_dynamic_value "
               "= %u) %s",
               expr, options.GetFetchDynamicValue(),
               frame_description.GetData());
         }
 
         target->EvaluateExpression(expr, frame, expr_value_sp, options.ref());
         expr_result.SetSP(expr_value_sp, options.GetFetchDynamicValue());
       }
+    } else {
+      Status error;
+      error.SetErrorString("can't evaluate expressions when the "
+                           "process is running.");
+      expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
+      expr_result.SetSP(expr_value_sp, false);
     }
+  } else {
+      Status error;
+      error.SetErrorString("sbframe object is not valid.");
+      expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
+      expr_result.SetSP(expr_value_sp, false);
   }
 
+  if (expr_result.GetError().Success())
     LLDB_LOGF(expr_log,
               "** [SBFrame::EvaluateExpression] Expression result is "
               "%s, summary %s **",
               expr_result.GetValue(), expr_result.GetSummary());
+  else
+    LLDB_LOGF(expr_log,
+              "** [SBFrame::EvaluateExpression] Expression evaluation failed: "
+              "%s **",
+              expr_result.GetError().GetCString());
 
   return expr_result;
 }
 
 bool SBFrame::IsInlined() {
   LLDB_INSTRUMENT_VA(this);
 
   return static_cast<const SBFrame *>(this)->IsInlined();

lldb/source/API/SBTarget.cpp

lldb::SBValue SBTarget::EvaluateExpression(const char *expr,
   if (target_sp) {
     if (expr == nullptr || expr[0] == '\0') {
       return expr_result;
     }
 
     std::lock_guard<std::recursive_mutex> guard(target_sp->GetAPIMutex());
     ExecutionContext exe_ctx(m_opaque_sp.get());
 
-
     frame = exe_ctx.GetFramePtr();
     Target *target = exe_ctx.GetTargetPtr();
+    Process *process = exe_ctx.GetProcessPtr();
 
     if (target) {
+      // If we have a process, make sure to lock the runlock:
+      if (process) {
+        Process::StopLocker stop_locker;
+        if (stop_locker.TryLock(&process->GetRunLock())) {
           target->EvaluateExpression(expr, frame, expr_value_sp, options.ref());
+        } else {
+          Status error;
+          error.SetErrorString("can't evaluate expressions when the "
+                               "process is running.");
+          expr_value_sp = ValueObjectConstResult::Create(nullptr, error);
+        }
+      } else {
+        target->EvaluateExpression(expr, frame, expr_value_sp, options.ref());
+      }
 
       expr_result.SetSP(expr_value_sp, options.GetFetchDynamicValue());
     }
   }
   LLDB_LOGF(expr_log,
             "** [SBTarget::EvaluateExpression] Expression result is "
             "%s, summary %s **",
             expr_result.GetValue(), expr_result.GetSummary());

lldb/source/Target/Target.cpp

case eStateStopped: {
     if (launch_info.GetFlags().Test(eLaunchFlagStopAtEntry))
       break;
     if (synchronous_execution)
       // Now we have handled the stop-from-attach, and we are just
       // switching to a synchronous resume.  So we should switch to the
       // SyncResume hijacker.
       m_process_sp->ResumeSynchronous(stream);
     else
-      error = m_process_sp->PrivateResume();
+      error = m_process_sp->Resume();
     if (!error.Success()) {
       Status error2;
       error2.SetErrorStringWithFormat(
           "process resume at entry point failed: %s", error.AsCString());
       error = error2;
     }
   } break;
   case eStateExited: {

lldb/test/API/python_api/run_locker/Makefile

+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules

lldb/test/API/python_api/run_locker/TestRunLocker.py

+"""
+Test that the run locker really does work to keep
+us from running SB API that should only be run
+while stopped.  This test is mostly concerned with
+what happens between launch and first stop.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestRunLocker(TestBase):
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    def test_run_locker(self):
+        """Test that the run locker is set correctly when we launch"""
+        self.build()
+        self.runlocker_test(False)
+
+    def test_run_locker_stop_at_entry(self):
+        """Test that the run locker is set correctly when we launch"""
+        self.build()
+        self.runlocker_test(False)
+
+    def setUp(self):
+        # Call super's setUp().
+        TestBase.setUp(self)
+        self.main_source_file = lldb.SBFileSpec("main.c")
+
+    def runlocker_test(self, stop_at_entry):
+        """The code to stop at entry handles events slightly differently, so 
+           we test both versions of process launch."""
+
+        target = lldbutil.run_to_breakpoint_make_target(self)
+        
+        launch_info = target.GetLaunchInfo()
+        if stop_at_entry:
+            flags = launch_info.GetFlags()
+            launch_info.SetFlags(flags | lldb.eLaunchFlagStopAtEntry)
+
+        error = lldb.SBError()
+        # We are trying to do things when the process is running, so
+        # we have to run the debugger asynchronously.
+        self.dbg.SetAsync(True)
+        
+        listener = lldb.SBListener("test-run-lock-listener")
+        launch_info.SetListener(listener)
+        process = target.Launch(launch_info, error)
+        self.assertSuccess(error, "Launched the process")
+        
+        event = lldb.SBEvent()
+        
+        event_result = listener.WaitForEvent(10, event)
+        self.assertTrue(event_result, "timed out waiting for launch")
+        state_type = lldb.SBProcess.GetStateFromEvent(event)
+        # We don't always see a launching...
+        if state_type == lldb.eStateLaunching:
+            event_result = listener.WaitForEvent(10, event)
+            self.assertTrue(event_result, "Timed out waiting for running after launching")
+            state_type = lldb.SBProcess.GetStateFromEvent(event)
+            
+        self.assertState(state_type, lldb.eStateRunning, "Didn't get a running event")
+        
+        # We aren't checking the entry state, but just making sure
+        # the running state is set properly if we continue in this state.
+        
+        if stop_at_entry:
+            event_result = listener.WaitForEvent(10, event)
+            self.assertTrue(event_result, "Timed out waiting for stop at entry stop")
+            state_type = lldb.SBProcess.GetStateFromEvent(event)
+            self.assertState(state_type, eStateStopped, "Stop at entry stopped")
+            process.Continue()
+
+        # Okay, now the process is running, make sure we can't do things
+        # you aren't supposed to do while running, and that we get some
+        # actual error:
+        val = target.EvaluateExpression("SomethingToCall()")
+        error = val.GetError()
+        self.assertTrue(error.Fail(), "Failed to run expression")
+        print(f"Got Error: {error.GetCString()}")
+        self.assertIn("can't evaluate expressions when the process is running", error.GetCString(), "Stopped by stop locker")
+
+        # This should also fail if we try to use the script interpreter directly:
+        interp = self.dbg.GetCommandInterpreter()
+        result = lldb.SBCommandReturnObject()
+        ret = interp.HandleCommand("script var = lldb.frame.EvaluateExpression('SomethingToCall()'); var.GetError().GetCString()", result)
+        self.assertIn("can't evaluate expressions when the process is running", result.GetOutput())

lldb/test/API/python_api/run_locker/main.c

+#include <unistd.h>
+
+int
+SomethingToCall() {
+  return 100;
+}
+
+int
+main()
+{
+  while (1) {
+    sleep(1);
+  }
+  return SomethingToCall();
+}