[lldb] Use a time-based timeout instead of a hardcoded instruction count in the IRInterpreter

[JDevlieghere]

Summary

At the moment the IRInterpreter will stop interpreting an expression after a hardcoded 4096 instructions. After it reaches the limit it will stop interpreting and leave the process in whatever state it was when the timeout was reached.

This patch changes the instruction limit to a timeout and uses the user-specified expression timeout value for this.

The main motivation is to allow users on targets where we can't use the JIT to run more complicated expressions if they really want to (which they can do now by just increasing the timeout).

The time-based approach also seems much more meaningful than the arbitrary (and very low) instruction limit. 4096 instructions can be interpreted in a few microseconds on some setups but might take much longer if we have a slow connection to the target. I don't think any user actually cares about the number of instructions that are executed but only about the time they are willing to wait for a result.

One problem that we have with allowing the user to change the IRInterpreter timeout is that there is currently no way to interrupt the interpreter process. As a follow-up we should check if can nicely hook up the driver's SIGINT handler to the IRInterpreter's work loop, but until this is done one can easily cause LLDB to get stuck in the interpreter by specifying no timeout and running an infinite loop-ing expression. To prevent that any user accidentally locks up their debugging session, this patch prevents that the user can pass 'no timeout' to the IRInterpreter but instead sets the limit to 30 seconds when no timeout was set. Users can still manually specify a longer timeout if they really need it.

[Michael137]

Generally LGTM, just left some minor comments

This assumes that each iteration of the loop won't misbehave and run for longer than the timeout. Which seems like an odd enough case for that not to be a concern?

lldb/source/Expression/IRInterpreter.cpp

733

Nit: should we check for timeout->count() > 0?

The Timeout docs don't really specify what < 0 means but AFAICT nothing technically stops that from happening

739

Haven't used this API before but is there a more concrete error? Perhaps something relating to timeouts?

lldb/source/Expression/LLVMUserExpression.cpp

128

Should we consider making this default timeout also configurable? So that we don't need to have a test that runs for at least 30 seconds?

[teemperor]

LGTM modulo the existing comments!

lldb/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py

27

If we ever land my ancient "optimize expr" patch then this infinite loop will be fun to debug :). Maybe just add a function to the target such as void five_min_expr() { sleep(5 * 60); } and call five_min_expr().

[JDevlieghere]

Address review feedback

lldb/source/Expression/IRInterpreter.cpp

739

There's only std::errc:: stream_timeout (ETIME) which has been deprecated. The rest of the IRInterpreter also uses the generic error codes too so I think this is the most consistent.

[Michael137]

LGTM

lldb/include/lldb/Expression/IRInterpreter.h

 //===-- IRInterpreter.h -----------------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_EXPRESSION_IRINTERPRETER_H
 #define LLDB_EXPRESSION_IRINTERPRETER_H
 
 #include "lldb/Utility/ConstString.h"
 #include "lldb/Utility/Stream.h"
+#include "lldb/Utility/Timeout.h"
 #include "lldb/lldb-public.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Pass.h"
 
 namespace llvm {
 class Function;
 class Module;
 }
static bool CanInterpret(llvm::Module &module, llvm::Function &function,
                            const bool support_function_calls);
 
   static bool Interpret(llvm::Module &module, llvm::Function &function,
                         llvm::ArrayRef<lldb::addr_t> args,
                         lldb_private::IRExecutionUnit &execution_unit,
                         lldb_private::Status &error,
                         lldb::addr_t stack_frame_bottom,
                         lldb::addr_t stack_frame_top,
-                        lldb_private::ExecutionContext &exe_ctx);
+                        lldb_private::ExecutionContext &exe_ctx,
+                        lldb_private::Timeout<std::micro> timeout);
 
 private:
   static bool supportsFunction(llvm::Function &llvm_function,
                                lldb_private::Status &err);
 };
 
 #endif

lldb/include/lldb/Target/Target.h

#endif
 
   lldb::DynamicValueType GetUseDynamic() const { return m_use_dynamic; }
 
   void
   SetUseDynamic(lldb::DynamicValueType dynamic = lldb::eDynamicCanRunTarget) {
     m_use_dynamic = dynamic;
   }
 
-  const Timeout<std::micro> &GetTimeout() const { return m_timeout; }
+  const Timeout<std::micro> &GetTimeout() const {
+    assert(m_timeout && m_timeout->count() > 0);
+    return m_timeout;
+  }
 
-  void SetTimeout(const Timeout<std::micro> &timeout) { m_timeout = timeout; }
+  void SetTimeout(const Timeout<std::micro> &timeout) {
+    // Disallow setting a non-zero timeout.
+    if (timeout && timeout->count() > 0)
+      m_timeout = timeout;
+  }
 
   const Timeout<std::micro> &GetOneThreadTimeout() const {
     return m_one_thread_timeout;
   }
 
   void SetOneThreadTimeout(const Timeout<std::micro> &timeout) {
     m_one_thread_timeout = timeout;
   }

lldb/source/Expression/IRInterpreter.cpp

 static const char *interpreter_internal_error =
     "Interpreter encountered an internal error";
 static const char *bad_value_error =
     "Interpreter couldn't resolve a value during execution";
 static const char *memory_allocation_error =
     "Interpreter couldn't allocate memory";
 static const char *memory_write_error = "Interpreter couldn't write to memory";
 static const char *memory_read_error = "Interpreter couldn't read from memory";
-static const char *infinite_loop_error = "Interpreter ran for too many cycles";
+static const char *timeout_error =
+    "Reached timeout while interpreting expression";
 static const char *too_many_functions_error =
     "Interpreter doesn't handle modules with multiple function bodies.";
 
 static bool CanResolveConstant(llvm::Constant *constant) {
   switch (constant->getValueID()) {
   default:
     return false;
   case Value::ConstantIntVal:

 }
 
 bool IRInterpreter::Interpret(llvm::Module &module, llvm::Function &function,
                               llvm::ArrayRef<lldb::addr_t> args,
                               lldb_private::IRExecutionUnit &execution_unit,
                               lldb_private::Status &error,
                               lldb::addr_t stack_frame_bottom,
                               lldb::addr_t stack_frame_top,
-                              lldb_private::ExecutionContext &exe_ctx) {
+                              lldb_private::ExecutionContext &exe_ctx,
+                              lldb_private::Timeout<std::micro> timeout) {
   lldb_private::Log *log(GetLog(LLDBLog::Expressions));
 
   if (log) {
     std::string s;
     raw_string_ostream oss(s);
 
     module.print(oss, nullptr);
 
if (args.size() <= static_cast<size_t>(arg_index)) {
       return false;
     }
 
     lldb::addr_t ptr = args[arg_index];
 
     frame.MakeArgument(&*ai, ptr);
   }
 
-  uint32_t num_insts = 0;
-
   frame.Jump(&function.front());
 
-  while (frame.m_ii != frame.m_ie && (++num_insts < 4096)) {
+  using clock = std::chrono::steady_clock;
+
+  // Compute the time at which the timeout has been exceeded.
+  std::optional<clock::time_point> end_time;
+  if (timeout && timeout->count() > 0)
+    end_time = clock::now() + *timeout;
+
+  while (frame.m_ii != frame.m_ie) {
+    // Timeout reached: stop interpreting.
+    if (end_time && clock::now() >= *end_time) {
+      error.SetErrorToGenericError();
+      error.SetErrorString(timeout_error);
+      return false;
+    }
+
     const Instruction *inst = &*frame.m_ii;
 
     LLDB_LOGF(log, "Interpreting %s", PrintValue(inst).c_str());
 
     switch (inst->getOpcode()) {
     default:
       break;
 
case Instruction::Call: {
         frame.AssignValue(inst, returnVal, module);
       }
     } break;
     }
 
     ++frame.m_ii;
   }
 
-  if (num_insts >= 4096) {
-    error.SetErrorToGenericError();
-    error.SetErrorString(infinite_loop_error);
-    return false;
-  }
-
   return false;
 }

lldb/source/Expression/LLVMUserExpression.cpp

if (!AddArguments(exe_ctx, args, struct_address, diagnostic_manager)) {
       return lldb::eExpressionSetupError;
     }
 
     function_stack_bottom = m_stack_frame_bottom;
     function_stack_top = m_stack_frame_top;
 
     IRInterpreter::Interpret(*module, *function, args, *m_execution_unit_sp,
                              interpreter_error, function_stack_bottom,
-                             function_stack_top, exe_ctx);
+                             function_stack_top, exe_ctx, options.GetTimeout());
 
     if (!interpreter_error.Success()) {
       diagnostic_manager.Printf(eDiagnosticSeverityError,
                                 "supposed to interpret, but failed: %s",
                                 interpreter_error.AsCString());
       return lldb::eExpressionDiscarded;
     }
   } else {
     if (!exe_ctx.HasThreadScope()) {
       diagnostic_manager.Printf(eDiagnosticSeverityError,
                                 "%s called with no thread selected",
                                 __FUNCTION__);
       return lldb::eExpressionSetupError;
if (execution_result == lldb::eExpressionInterrupted ||
           "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.", 
+          " exited during its execution.",
           expr_thread_id);
       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/test/API/commands/expression/ir-interpreter/TestIRInterpreter.py

 """
 Test the IR interpreter
 """
 
 
 import lldb
 from lldbsuite.test.decorators import *
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test import lldbutil
 
 
 class IRInterpreterTestCase(TestBase):
     NO_DEBUG_INFO_TESTCASE = True
 
+    def time_expression(self, expr, options):
+        start = time.time()
+        res = self.target.EvaluateExpression(expr, options)
+        return res, time.time() - start
+
+    def test_interpreter_timeout(self):
+        """Test the timeout logic in the IRInterpreter."""
+        self.build()
+        self.target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+        self.assertTrue(self.target, VALID_TARGET)
+
+        # A non-trivial infinite loop.
+        inf_loop = "for (unsigned i = 0; i < 100; ++i) --i; 1"
+        timeout_error = "Reached timeout while interpreting expression"
+        options = lldb.SBExpressionOptions()
+
+        # This is an IRInterpreter specific test, so disable the JIT.
+        options.SetAllowJIT(False)
+
+        # No timeout means a 500ms.
+        options.SetTimeoutInMicroSeconds(0)
+        res, duration_sec = self.time_expression(inf_loop, options)
+        self.assertIn(timeout_error, str(res.GetError()))
+
+        # Depending on the machine load the expression might take quite some
+        # time, so give the time a generous upper bound.
+        self.assertLess(duration_sec, 15)
+
+        # Try a simple one second timeout.
+        options.SetTimeoutInMicroSeconds(1000000)
+        res, duration_sec = self.time_expression(inf_loop, options)
+        self.assertIn(timeout_error, str(res.GetError()))
+        self.assertGreaterEqual(duration_sec, 1)
+        self.assertLess(duration_sec, 30)
+
     def setUp(self):
         # Call super's setUp().
         TestBase.setUp(self)
         # Find the line number to break for main.c.
         self.line = line_number("main.c", "// Set breakpoint here")
 
         # Disable confirmation prompt to avoid infinite wait
         self.runCmd("settings set auto-confirm true")