Handle the case where a thread exits while we were running a function on it

[jingham]

Summary

It is possible that the thread we are running a function on could exit while we are waiting for the function call to return.

We sort of handled that case before, but it really only worked by accident because the ThreadPlan still had a pointer to the Thread, and it hadn't actually gone away when we touched it after stopping and finding that it had exited. Now that ThreadPlans re-look up the thread after each stop, we were handing out a null Thread pointer and crashing.

I moved the checking for vanished threads to the helper routine that handles the stop event, added an expression result of eExpressionThreadVanished and handle it properly in RunThreadPlan. I also added a test using a function that just called pthread_exit. This crashed before these changes, and works correctly after.

[jingham]

clang-format

[jasonmolenda]

I skimmed it, I guess the one question that comes to mind is how this will behave if we an operating system thread provider plugin active, where it may not actually list all of the threads that exist -- where we try to be tolerant of threads disappearing and then re-appearing later -- and the thread running an inferior function call got scheduled out while the function call was still happening. Pretty unlikely scenario, but I thought I'd throw it out there.

lldb/source/Expression/LLVMUserExpression.cpp

235

I think you meant to include the tid in the Printf. Did this produce a warning? We should have decorated the Printf() method in this class with the thing that tells clang this is a printf functoin.

[aprantl]

This looks reasonable based on my limited knowledge.

lldb/source/Target/Process.cpp

4671

Is it useful to both log the message and have an almost similarly-worded diagnostic produced in LLVMUserExpression.cpp?

("Yes" is an acceptable answer)

5534

If you remove the default case and initialize the result_name to "
"<unknown>" at the top, you get a free compiler warning every time when someone adds a new enumerator.

[jingham]

lldb/source/Target/Process.cpp

4671

The diagnostic will go the the user, and end up in the Error in the expression result ValueObject. So that part we have to do. Putting it in the log will make the message show up where it happens in the flow of events we're logging, which is very handy. So IMO it is worthwhile doing the latter as well.

[jingham]

Address review comments

lldb/include/lldb/lldb-enumerations.h

enum ExpressionResults {
   eExpressionCompleted = 0,
   eExpressionSetupError,
   eExpressionParseError,
   eExpressionDiscarded,
   eExpressionInterrupted,
   eExpressionHitBreakpoint,
   eExpressionTimedOut,
   eExpressionResultUnavailable,
-  eExpressionStoppedForDebug
+  eExpressionStoppedForDebug,
+  eExpressionThreadVanished
 };
 
 enum SearchDepth {
   eSearchDepthInvalid = 0,
   eSearchDepthTarget,
   eSearchDepthModule,
   eSearchDepthCompUnit,
   eSearchDepthFunction,

lldb/source/Expression/FunctionCaller.cpp

lldb::ExpressionResults FunctionCaller::ExecuteFunction(
 
   return_value = exe_ctx.GetProcessRef().RunThreadPlan(
       exe_ctx, call_plan_sp, real_options, diagnostic_manager);
 
   if (log) {
     if (return_value != lldb::eExpressionCompleted) {
       LLDB_LOGF(log,
                 "== [FunctionCaller::ExecuteFunction] Execution of \"%s\" "
-                "completed abnormally ==",
-                m_name.c_str());
+                "completed abnormally: %s ==",
+                m_name.c_str(),
+                Process::ExecutionResultAsCString(return_value));
     } else {
       LLDB_LOGF(log,
                 "== [FunctionCaller::ExecuteFunction] Execution of \"%s\" "
                 "completed normally ==",
                 m_name.c_str());
     }
   }
 

lldb/source/Expression/LLVMUserExpression.cpp

LLVMUserExpression::DoExecute(DiagnosticManager &diagnostic_manager,
   } else {
     if (!exe_ctx.HasThreadScope()) {
       diagnostic_manager.Printf(eDiagnosticSeverityError,
                                 "%s called with no thread selected",
                                 __FUNCTION__);
       return lldb::eExpressionSetupError;
     }
 
+    // Store away the thread ID for error reporting, in case it exits
+    // during execution:
+    lldb::tid_t expr_thread_id = exe_ctx.GetThreadRef().GetID();
+
     Address wrapper_address(m_jit_start_addr);
 
     std::vector<lldb::addr_t> args;
 
     if (!AddArguments(exe_ctx, args, struct_address, diagnostic_manager)) {
       diagnostic_manager.Printf(eDiagnosticSeverityError,
                                 "errored out in %s, couldn't AddArguments",
                                 __FUNCTION__);
if (m_can_interpret) {
     } else if (execution_result == lldb::eExpressionStoppedForDebug) {
       diagnostic_manager.PutString(
           eDiagnosticSeverityRemark,
           "Execution was halted at the first instruction of the expression "
           "function because \"debug\" was requested.\n"
           "Use \"thread return -x\" to return to the state before expression "
           "evaluation.");
       return execution_result;
+    } else if (execution_result == lldb::eExpressionThreadVanished) {
+      diagnostic_manager.Printf(
+          eDiagnosticSeverityError,
+          "Couldn't complete execution; the thread "
+          "on which the expression was being run: 0x%" PRIx64
+          " exited during its execution.");
+      return execution_result;
     } else if (execution_result != lldb::eExpressionCompleted) {
       diagnostic_manager.Printf(
           eDiagnosticSeverityError, "Couldn't execute function; result was %s",
           Process::ExecutionResultAsCString(execution_result));
       return execution_result;
     }
   }
 

lldb/source/Target/Process.cpp

GetExpressionTimeout(const EvaluateExpressionOptions &options,
 
   if (!options.GetTimeout())
     return llvm::None;
   else
     return *options.GetTimeout() - GetOneThreadExpressionTimeout(options);
 }
 
 static llvm::Optional<ExpressionResults>
-HandleStoppedEvent(Thread &thread, const ThreadPlanSP &thread_plan_sp,
+HandleStoppedEvent(lldb::tid_t thread_id, const ThreadPlanSP &thread_plan_sp,
                    RestorePlanState &restorer, const EventSP &event_sp,
                    EventSP &event_to_broadcast_sp,
-                   const EvaluateExpressionOptions &options, bool handle_interrupts) {
+                   const EvaluateExpressionOptions &options,
+                   bool handle_interrupts) {
   Log *log = GetLogIfAnyCategoriesSet(LIBLLDB_LOG_STEP | LIBLLDB_LOG_PROCESS);
 
-  ThreadPlanSP plan = thread.GetCompletedPlan();
+  ThreadSP thread_sp = thread_plan_sp->GetTarget()
+                           .GetProcessSP()
+                           ->GetThreadList()
+                           .FindThreadByID(thread_id);
+  if (!thread_sp) {
+    LLDB_LOG(log,
+             "The thread on which we were running the "
+             "expression: tid = {0}, exited while "
+             "the expression was running.",
+             thread_id);
+    return eExpressionThreadVanished;
+  }
+
+  ThreadPlanSP plan = thread_sp->GetCompletedPlan();
   if (plan == thread_plan_sp && plan->PlanSucceeded()) {
     LLDB_LOG(log, "execution completed successfully");
 
     // Restore the plan state so it will get reported as intended when we are
     // done.
     restorer.Clean();
     return eExpressionCompleted;
   }
 
-  StopInfoSP stop_info_sp = thread.GetStopInfo();
+  StopInfoSP stop_info_sp = thread_sp->GetStopInfo();
   if (stop_info_sp && stop_info_sp->GetStopReason() == eStopReasonBreakpoint &&
       stop_info_sp->ShouldNotify(event_sp.get())) {
     LLDB_LOG(log, "stopped for breakpoint: {0}.", stop_info_sp->GetDescription());
     if (!options.DoesIgnoreBreakpoints()) {
       // Restore the plan state and then force Private to false.  We are going
       // to stop because of this plan so we need it to become a public plan or
       // it won't report correctly when we continue to its termination later
       // on.
Process::RunThreadPlan(ExecutionContext &exe_ctx,
 
   Thread *thread = exe_ctx.GetThreadPtr();
   if (thread == nullptr) {
     diagnostic_manager.PutString(eDiagnosticSeverityError,
                                  "RunThreadPlan called with invalid thread.");
     return eExpressionSetupError;
   }
 
+  // Record the thread's id so we can tell when a thread we were using
+  // to run the expression exits during the expression evaluation.
+  lldb::tid_t expr_thread_id = thread->GetID();
+
   // We need to change some of the thread plan attributes for the thread plan
   // runner.  This will restore them when we are done:
 
   RestorePlanState thread_plan_restorer(thread_plan_sp);
 
   // We rely on the thread plan we are running returning "PlanCompleted" if
   // when it successfully completes. For that to be true the plan can't be
   // private - since private plans suppress themselves in the GetCompletedPlan
lldb::EventSP event_to_broadcast_sp;
     ProcessEventHijacker run_thread_plan_hijacker(*this, listener_sp);
 
     if (log) {
       StreamString s;
       thread_plan_sp->GetDescription(&s, lldb::eDescriptionLevelVerbose);
       LLDB_LOGF(log,
                 "Process::RunThreadPlan(): Resuming thread %u - 0x%4.4" PRIx64
                 " to run thread plan \"%s\".",
-                thread->GetIndexID(), thread->GetID(), s.GetData());
+                thread_idx_id, expr_thread_id, s.GetData());
     }
 
     bool got_event;
     lldb::EventSP event_sp;
     lldb::StateType stop_state = lldb::eStateInvalid;
 
     bool before_first_timeout = true; // This is set to false the first time
                                       // that we have to halt the target.
#endif
             stop_state =
                 Process::ProcessEventData::GetStateFromEvent(event_sp.get());
             LLDB_LOGF(log,
                       "Process::RunThreadPlan(): in while loop, got event: %s.",
                       StateAsCString(stop_state));
 
             switch (stop_state) {
             case lldb::eStateStopped: {
-              // We stopped, figure out what we are going to do now.
-              ThreadSP thread_sp =
-                  GetThreadList().FindThreadByIndexID(thread_idx_id);
-              if (!thread_sp) {
-                // Ooh, our thread has vanished.  Unlikely that this was
-                // successful execution...
-                LLDB_LOGF(log,
-                          "Process::RunThreadPlan(): execution completed "
-                          "but our thread (index-id=%u) has vanished.",
-                          thread_idx_id);
-                return_value = eExpressionInterrupted;
-              } else if (Process::ProcessEventData::GetRestartedFromEvent(
-                             event_sp.get())) {
+              if (Process::ProcessEventData::GetRestartedFromEvent(
                 // If we were restarted, we just need to go back up to fetch
                 // another event.
-                if (log) {
-                  LLDB_LOGF(log, "Process::RunThreadPlan(): Got a stop and "
-                                 "restart, so we'll continue waiting.");
-                }
                 keep_going = true;
                 do_resume = false;
                 handle_running_event = true;
               } else {
                 const bool handle_interrupts = true;
                 return_value = *HandleStoppedEvent(
-                    *thread, thread_plan_sp, thread_plan_restorer, event_sp,
-                    event_to_broadcast_sp, options, handle_interrupts);
+                    expr_thread_id, thread_plan_sp, thread_plan_restorer,
+                    event_sp, event_to_broadcast_sp, options,
+                    handle_interrupts);
+                if (return_value == eExpressionThreadVanished)
+                  keep_going = false;
               }
             } break;
 
             case lldb::eStateRunning:
               // This shouldn't really happen, but sometimes we do get two
               // running events without an intervening stop, and in that case
               // we should just go back to waiting for the stop.
               do_resume = false;
#endif
                   continue;
                 }
 
                 // Between the time we initiated the Halt and the time we
                 // delivered it, the process could have already finished its
                 // job.  Check that here:
                 const bool handle_interrupts = false;
                 if (auto result = HandleStoppedEvent(
-                        *thread, thread_plan_sp, thread_plan_restorer, event_sp,
-                        event_to_broadcast_sp, options, handle_interrupts)) {
+                        expr_thread_id, thread_plan_sp, thread_plan_restorer,
+                        event_sp, event_to_broadcast_sp, options,
+                        handle_interrupts)) {
                   return_value = *result;
                   back_to_top = false;
                   break;
                 }
 
                 if (!options.GetTryAllThreads()) {
                   if (log)
                     log->PutCString("Process::RunThreadPlan(): try_all_threads "
if (backup_private_state_thread.IsJoinable()) {
       m_private_state_thread = backup_private_state_thread;
       if (stopper_base_plan_sp) {
         thread->DiscardThreadPlansUpToPlan(stopper_base_plan_sp);
       }
       if (old_state != eStateInvalid)
         m_public_state.SetValueNoLock(old_state);
     }
 
+    // If our thread went away on us, we need to get out of here without
+    // doing any more work.  We don't have to clean up the thread plan, that
+    // will have happened when the Thread was destroyed.
+    if (return_value == eExpressionThreadVanished) {
+      return return_value;
+    }
+
     if (return_value != eExpressionCompleted && log) {
       // Print a backtrace into the log so we can figure out where we are:
       StreamString s;
       s.PutCString("Thread state after unsuccessful completion: \n");
       thread->GetStackFrameStatus(s, 0, UINT32_MAX, true, UINT32_MAX);
       log->PutString(s.GetString());
     }
     // Restore the thread state if we are going to discard the plan execution.
case eExpressionResultUnavailable:
     result_name = "eExpressionResultUnavailable";
     break;
   case eExpressionTimedOut:
     result_name = "eExpressionTimedOut";
     break;
   case eExpressionStoppedForDebug:
     result_name = "eExpressionStoppedForDebug";
     break;
+  case eExpressionThreadVanished:
+    result_name = "eExpressionThreadVanished";
+  default:
+    result_name = "<unknown>";
   }
   return result_name;
 }
 
 void Process::GetStatus(Stream &strm) {
   const StateType state = GetState();
   if (StateIsStoppedState(state, false)) {
     if (state == eStateExited) {

lldb/test/API/functionalities/thread/exit_during_expression/Makefile

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

lldb/test/API/functionalities/thread/exit_during_expression/TestExitDuringExpression.py

+"""
+Make sure that we handle an expression on a thread, if
+the thread exits while the expression is running.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+class TestExitDuringExpression(TestBase):
+
+    mydir = TestBase.compute_mydir(__file__)
+
+    NO_DEBUG_INFO_TESTCASE = True
+
+    @skipIfWindows
+    def test_exit_before_one_thread_unwind(self):
+        """Test the case where we exit within the one thread timeout"""
+        self.exiting_expression_test(True, True)
+
+    @skipIfWindows
+    def test_exit_before_one_thread_no_unwind(self):
+        """Test the case where we exit within the one thread timeout"""
+        self.exiting_expression_test(True, False)
+
+    @skipIfWindows
+    def test_exit_after_one_thread_unwind(self):
+        """Test the case where we exit within the one thread timeout"""
+        self.exiting_expression_test(False, True)
+
+    @skipIfWindows
+    def test_exit_after_one_thread_no_unwind(self):
+        """Test the case where we exit within the one thread timeout"""
+        self.exiting_expression_test(False, False)
+    
+    def setUp(self):
+        TestBase.setUp(self)
+        self.main_source_file = lldb.SBFileSpec("main.c")
+        self.build()
+        
+    def exiting_expression_test(self, before_one_thread_timeout , unwind):
+        """function_to_call sleeps for g_timeout microseconds, then calls pthread_exit.
+           This test calls function_to_call with an overall timeout of 500
+           microseconds, and a one_thread_timeout as passed in.
+           It also sets unwind_on_exit for the call to the unwind passed in.
+           This allows you to have the thread exit either before the one thread
+           timeout is passed. """
+        
+        (target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+                                   "Break here and cause the thread to exit", self.main_source_file)
+
+        # We'll continue to this breakpoint after running our expression:
+        return_bkpt = target.BreakpointCreateBySourceRegex("Break here to make sure the thread exited", self.main_source_file)
+        frame = thread.frames[0]
+        tid = thread.GetThreadID()
+        # Find the timeout:
+        var_options = lldb.SBVariablesOptions()
+        var_options.SetIncludeArguments(False)
+        var_options.SetIncludeLocals(False)
+        var_options.SetIncludeStatics(True)
+        
+        value_list = frame.GetVariables(var_options)
+        g_timeout = value_list.GetFirstValueByName("g_timeout")
+        self.assertTrue(g_timeout.IsValid(), "Found g_timeout")
+        
+        error = lldb.SBError()
+        timeout_value = g_timeout.GetValueAsUnsigned(error)
+        self.assertTrue(error.Success(), "Couldn't get timeout value: %s"%(error.GetCString()))
+        
+        one_thread_timeout = 0
+        if (before_one_thread_timeout):
+            one_thread_timeout = timeout_value * 2
+        else:
+            one_thread_timeout = int(timeout_value / 2)
+
+        options = lldb.SBExpressionOptions()
+        options.SetUnwindOnError(unwind)
+        options.SetOneThreadTimeoutInMicroSeconds(one_thread_timeout)
+        options.SetTimeoutInMicroSeconds(4 * timeout_value)
+            
+        result = frame.EvaluateExpression("function_to_call()", options)
+
+        # Make sure the thread actually exited:
+        thread = process.GetThreadByID(tid)
+        self.assertFalse(thread.IsValid(), "The thread exited")
+
+        # Make sure the expression failed:
+        self.assertFalse(result.GetError().Success(), "Expression failed.")
+
+        # Make sure we can keep going:
+        threads = lldbutil.continue_to_breakpoint(process, return_bkpt)
+        if not threads:
+            self.fail("didn't get any threads back after continuing")
+
+        self.assertEqual(len(threads), 1, "One thread hit our breakpoint")
+        thread = threads[0]
+        frame = thread.frames[0]
+        # Now get the return value, if we successfully caused the thread to exit
+        # it should be 10, not 20.
+        ret_val = frame.FindVariable("ret_val")
+        self.assertTrue(ret_val.GetError().Success(), "Found ret_val")
+        ret_val_value = ret_val.GetValueAsSigned(error)
+        self.assertTrue(error.Success(), "Got ret_val's value")
+        self.assertEqual(ret_val_value, 10, "We put the right value in ret_val")
+        

lldb/test/API/functionalities/thread/exit_during_expression/main.c

+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <unistd.h>
+
+static unsigned int g_timeout = 200;
+
+int function_to_call() {
+
+  errno = 0;
+  while (1) {
+    int result = usleep(g_timeout);
+    if (errno != EINTR)
+      break;
+  }
+
+  pthread_exit((void *)10);
+
+  return 20; // Prevent warning
+}
+
+void *exiting_thread_func(void *unused) {
+  function_to_call(); // Break here and cause the thread to exit
+  return NULL;
+}
+
+int main() {
+  char *exit_ptr;
+  pthread_t exiting_thread;
+
+  pthread_create(&exiting_thread, NULL, exiting_thread_func, NULL);
+
+  pthread_join(exiting_thread, &exit_ptr);
+  int ret_val = (int)exit_ptr;
+  usleep(g_timeout * 4); // Make sure in the "run all threads" case
+                         // that we don't run past our breakpoint.
+  return ret_val;        // Break here to make sure the thread exited.
+}