Use Timeout<> in Process::RunThreadPlan

[labath]

Summary

Since the function is way too big already, I tried at least to factor out the
timeout computation stuff into a separate function. I've tried to make the new
code semantically equivalent, and it also makes sense when I look at it as a done
deal, but I could use another pair of eyes for this.

[jingham]

That looks correct to me as well. Thanks for taking the time to clean this up.

source/Target/Process.cpp

4926–4928 ↗

(On Diff #79737)

without -> with

The error was in the original.

lldb/trunk/source/Target/Process.cpp

 //===-- Process.cpp ---------------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 // C Includes
 // C++ Includes
 #include <atomic>
 #include <mutex>
 
 // Other libraries and framework includes
+#include "llvm/Support/ScopedPrinter.h"
 // Project includes
 #include "Plugins/Process/Utility/InferiorCallPOSIX.h"
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Breakpoint/StoppointCallbackContext.h"
 #include "lldb/Core/Debugger.h"
 #include "lldb/Core/Event.h"
 #include "lldb/Core/Log.h"
 #include "lldb/Core/Module.h"
private:
   lldb::ThreadPlanSP m_thread_plan_sp;
   bool m_already_reset;
   bool m_private;
   bool m_is_master;
   bool m_okay_to_discard;
 };
 } // anonymous namespace
 
+static microseconds
+GetOneThreadExpressionTimeout(const EvaluateExpressionOptions &options) {
+  const milliseconds default_one_thread_timeout(250);
+
+  // If the overall wait is forever, then we don't need to worry about it.
+  if (options.GetTimeoutUsec() == 0) {
+    if (options.GetOneThreadTimeoutUsec() != 0)
+      return microseconds(options.GetOneThreadTimeoutUsec());
+    return default_one_thread_timeout;
+  }
+
+  // If the one thread timeout is set, use it.
+  if (options.GetOneThreadTimeoutUsec() != 0)
+    return microseconds(options.GetOneThreadTimeoutUsec());
+
+  // Otherwise use half the total timeout, bounded by the
+  // default_one_thread_timeout.
+  return std::min<microseconds>(default_one_thread_timeout,
+                                microseconds(options.GetTimeoutUsec()) / 2);
+}
+
+static Timeout<std::micro>
+GetExpressionTimeout(const EvaluateExpressionOptions &options,
+                     bool before_first_timeout) {
+  // If we are going to run all threads the whole time, or if we are only
+  // going to run one thread, we can just return the overall timeout.
+  if (!options.GetStopOthers() || !options.GetTryAllThreads())
+    return ConvertTimeout(microseconds(options.GetTimeoutUsec()));
+
+  if (before_first_timeout)
+    return GetOneThreadExpressionTimeout(options);
+
+  if (options.GetTimeoutUsec() == 0)
+    return llvm::None;
+  else
+    return microseconds(options.GetTimeoutUsec()) -
+           GetOneThreadExpressionTimeout(options);
+}
+
 ExpressionResults
 Process::RunThreadPlan(ExecutionContext &exe_ctx,
                        lldb::ThreadPlanSP &thread_plan_sp,
                        const EvaluateExpressionOptions &options,
                        DiagnosticManager &diagnostic_manager) {
   ExpressionResults return_value = eExpressionSetupError;
 
   std::lock_guard<std::mutex> run_thread_plan_locker(m_run_thread_plan_lock);
if (!selected_frame_sp) {
       diagnostic_manager.Printf(
           eDiagnosticSeverityError,
           "RunThreadPlan called without a selected frame on thread %d",
           thread_idx_id);
       return eExpressionSetupError;
     }
   }
 
+  // Make sure the timeout values make sense. The one thread timeout needs to be
+  // smaller than the overall timeout.
+  if (options.GetOneThreadTimeoutUsec() != 0 && options.GetTimeoutUsec() != 0 &&
+      options.GetTimeoutUsec() < options.GetOneThreadTimeoutUsec()) {
+    diagnostic_manager.PutString(eDiagnosticSeverityError,
+                                 "RunThreadPlan called with one thread "
+                                 "timeout greater than total timeout");
+    return eExpressionSetupError;
+  }
+
   StackID ctx_frame_id = selected_frame_sp->GetStackID();
 
   // N.B. Running the target may unset the currently selected thread and frame.
   // We don't want to do that either,
   // so we should arrange to reset them as well.
 
   lldb::ThreadSP selected_thread_sp = GetThreadList().GetSelectedThread();
 
lldb::EventSP event_to_broadcast_sp;
     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.
     bool do_resume = true;
     bool handle_running_event = true;
-    const uint64_t default_one_thread_timeout_usec = 250000;
 
     // This is just for accounting:
     uint32_t num_resumes = 0;
 
-    uint32_t timeout_usec = options.GetTimeoutUsec();
-    uint32_t one_thread_timeout_usec;
-    uint32_t all_threads_timeout_usec = 0;
-
     // If we are going to run all threads the whole time, or if we are only
-    // going to run one thread,
-    // then we don't need the first timeout.  So we set the final timeout, and
-    // pretend we are after the
-    // first timeout already.
-
-    if (!options.GetStopOthers() || !options.GetTryAllThreads()) {
+    // going to run one thread, then we don't need the first timeout.  So we
+    // pretend we are after the first timeout already.
+    if (!options.GetStopOthers() || !options.GetTryAllThreads())
       before_first_timeout = false;
-      one_thread_timeout_usec = 0;
-      all_threads_timeout_usec = timeout_usec;
-    } else {
-      uint32_t option_one_thread_timeout = options.GetOneThreadTimeoutUsec();
-
-      // If the overall wait is forever, then we only need to set the one thread
-      // timeout:
-      if (timeout_usec == 0) {
-        if (option_one_thread_timeout != 0)
-          one_thread_timeout_usec = option_one_thread_timeout;
-        else
-          one_thread_timeout_usec = default_one_thread_timeout_usec;
-      } else {
-        // Otherwise, if the one thread timeout is set, make sure it isn't
-        // longer than the overall timeout,
-        // and use it, otherwise use half the total timeout, bounded by the
-        // default_one_thread_timeout_usec.
-        uint64_t computed_one_thread_timeout;
-        if (option_one_thread_timeout != 0) {
-          if (timeout_usec < option_one_thread_timeout) {
-            diagnostic_manager.PutString(eDiagnosticSeverityError,
-                                         "RunThreadPlan called without one "
-                                         "thread timeout greater than total "
-                                         "timeout");
-            return eExpressionSetupError;
-          }
-          computed_one_thread_timeout = option_one_thread_timeout;
-        } else {
-          computed_one_thread_timeout = timeout_usec / 2;
-          if (computed_one_thread_timeout > default_one_thread_timeout_usec)
-            computed_one_thread_timeout = default_one_thread_timeout_usec;
-        }
-        one_thread_timeout_usec = computed_one_thread_timeout;
-        all_threads_timeout_usec = timeout_usec - one_thread_timeout_usec;
-      }
-    }
 
     if (log)
-      log->Printf(
-          "Stop others: %u, try all: %u, before_first: %u, one thread: %" PRIu32
-          " - all threads: %" PRIu32 ".\n",
+      log->Printf("Stop others: %u, try all: %u, before_first: %u.\n",
                   options.GetStopOthers(), options.GetTryAllThreads(),
-          before_first_timeout, one_thread_timeout_usec,
-          all_threads_timeout_usec);
+                  before_first_timeout);
 
     // This isn't going to work if there are unfetched events on the queue.
     // Are there cases where we might want to run the remaining events here, and
     // then try to
     // call the function?  That's probably being too tricky for our own good.
 
     Event *other_events = listener_sp->PeekAtNextEvent();
     if (other_events != nullptr) {
#ifdef LLDB_RUN_THREAD_HALT_WITH_EVENT
     // us not
     // fetch the first event till after the halt.  So if you run a quick
     // function, it will have
     // completed, and the completion event will be waiting, when you interrupt
     // for halt.
     // The expression evaluation should still succeed.
     bool miss_first_event = true;
 #endif
-    std::chrono::microseconds timeout = std::chrono::microseconds(0);
-
     while (true) {
       // We usually want to resume the process if we get to the top of the loop.
       // The only exception is if we get two running events with no intervening
       // stop, which can happen, we will just wait for then next stop event.
       if (log)
         log->Printf("Top of while loop: do_resume: %i handle_running_event: %i "
                     "before_first_timeout: %i.",
                     do_resume, handle_running_event, before_first_timeout);
while (true) {
         // We set the timeout AFTER the resume, since the resume takes some time
         // and we
         // don't want to charge that to the timeout.
       } else {
         if (log)
           log->PutCString("Process::RunThreadPlan(): waiting for next event.");
       }
 
-      if (before_first_timeout) {
-        if (options.GetTryAllThreads())
-          timeout = std::chrono::microseconds(one_thread_timeout_usec);
-        else
-          timeout = std::chrono::microseconds(timeout_usec);
-      } else {
-        if (timeout_usec == 0)
-          timeout = std::chrono::microseconds(0);
-        else
-          timeout = std::chrono::microseconds(all_threads_timeout_usec);
-      }
-
       do_resume = true;
       handle_running_event = true;
 
       // Now wait for the process to stop again:
       event_sp.reset();
 
+      Timeout<std::micro> timeout =
+          GetExpressionTimeout(options, before_first_timeout);
       if (log) {
-        if (timeout.count()) {
-          log->Printf(
-              "Process::RunThreadPlan(): about to wait - now is %llu - "
-              "endpoint is %llu",
-              static_cast<unsigned long long>(
-                  std::chrono::system_clock::now().time_since_epoch().count()),
-              static_cast<unsigned long long>(
-                  std::chrono::time_point<std::chrono::system_clock,
-                                          std::chrono::microseconds>(timeout)
-                      .time_since_epoch()
-                      .count()));
+        if (timeout) {
+          auto now = system_clock::now();
+          log->Printf("Process::RunThreadPlan(): about to wait - now is %s - "
+                      "endpoint is %s",
+                      llvm::to_string(now).c_str(),
+                      llvm::to_string(now + *timeout).c_str());
         } else {
           log->Printf("Process::RunThreadPlan(): about to wait forever.");
         }
       }
 
 #ifdef LLDB_RUN_THREAD_HALT_WITH_EVENT
       // See comment above...
       if (miss_first_event) {
         usleep(1000);
         miss_first_event = false;
         got_event = false;
       } else
 #endif
-        got_event = listener_sp->GetEvent(event_sp, ConvertTimeout(timeout));
+        got_event = listener_sp->GetEvent(event_sp, timeout);
 
       if (got_event) {
         if (event_sp) {
           bool keep_going = false;
           if (event_sp->GetType() == eBroadcastBitInterrupt) {
             const bool clear_thread_plans = false;
             const bool use_run_lock = false;
             Halt(clear_thread_plans, use_run_lock);
#endif
         // If we didn't get an event that means we've timed out...
         // We will interrupt the process here.  Depending on what we were asked
         // to do we will
         // either exit, or try with all threads running for the same timeout.
 
         if (log) {
           if (options.GetTryAllThreads()) {
             if (before_first_timeout) {
-              if (timeout_usec != 0) {
-                log->Printf("Process::RunThreadPlan(): Running function with "
-                            "one thread timeout timed out, "
-                            "running for %" PRIu32
-                            " usec with all threads enabled.",
-                            all_threads_timeout_usec);
-              } else {
-                log->Printf("Process::RunThreadPlan(): Running function with "
-                            "one thread timeout timed out, "
-                            "running forever with all threads enabled.");
-              }
+                          "one thread timeout timed out.");
             } else
               log->Printf("Process::RunThreadPlan(): Restarting function with "
                           "all threads enabled "
-                          "and timeout: %u timed out, abandoning execution.",
-                          timeout_usec);
+                          "and timeout: %" PRIu64
+                          " timed out, abandoning execution.",
+                          timeout ? timeout->count() : -1);
           } else
             log->Printf("Process::RunThreadPlan(): Running function with "
-                        "timeout: %u timed out, "
+                        "timeout: %" PRIu64 " timed out, "
                         "abandoning execution.",
-                        timeout_usec);
+                        timeout ? timeout->count() : -1);
         }
 
         // It is possible that between the time we issued the Halt, and we get
         // around to calling Halt the target
         // could have stopped.  That's fine, Halt will figure that out and send
         // the appropriate Stopped event.
         // BUT it is also possible that we stopped & restarted (e.g. hit a
         // signal with "stop" set to false.)  In