[lldb] Refactor UserExpression::Evaluate to only have one error channel. (#117186)

Prior to this patch, the function returned an exit status, sometimes a
ValueObject with an error and a Status object. This patch removes the
Status object and ensures the error is consistently returned as the
error of the ValueObject.

Author: adrian-prantl

lldb/include/lldb/Expression/UserExpression.h

@@ -240,11 +240,9 @@ class UserExpression : public Expression {
   ///     definitions to be included when the expression is parsed.
   ///
   /// \param[in,out] result_valobj_sp
-  ///      If execution is successful, the result valobj is placed here.
-  ///
-  /// \param[out] error
-  ///     Filled in with an error in case the expression evaluation
-  ///     fails to parse, run, or evaluated.
+  ///      If execution is successful, the result valobj is placed
+  ///      here. Otherwise its Error will contain an ExpressionError
+  ///      with details about the failure mode.
   ///
   /// \param[out] fixed_expression
   ///     If non-nullptr, the fixed expression is copied into the provided
@@ -266,7 +264,7 @@ class UserExpression : public Expression {
   static lldb::ExpressionResults
   Evaluate(ExecutionContext &exe_ctx, const EvaluateExpressionOptions &options,
            llvm::StringRef expr_cstr, llvm::StringRef expr_prefix,
-           lldb::ValueObjectSP &result_valobj_sp, Status &error,
+           lldb::ValueObjectSP &result_valobj_sp,
            std::string *fixed_expression = nullptr,
            ValueObject *ctx_obj = nullptr);
 

lldb/source/Expression/REPL.cpp

@@ -339,12 +339,9 @@ void REPL::IOHandlerInputComplete(IOHandler &io_handler, std::string &code) {
 
       const char *expr_prefix = nullptr;
       lldb::ValueObjectSP result_valobj_sp;
+      lldb::ExpressionResults execution_results = UserExpression::Evaluate(
+          exe_ctx, expr_options, code.c_str(), expr_prefix, result_valobj_sp);
       Status error;
-      lldb::ExpressionResults execution_results =
-          UserExpression::Evaluate(exe_ctx, expr_options, code.c_str(),
-                                   expr_prefix, result_valobj_sp, error,
-                                   nullptr); // fixed expression
-
       if (llvm::Error err = OnExpressionEvaluated(exe_ctx, code, expr_options,
                                                   execution_results,
                                                   result_valobj_sp, error)) {

lldb/source/Expression/UserExpression.cpp

@@ -144,9 +144,13 @@ lldb::ExpressionResults
 UserExpression::Evaluate(ExecutionContext &exe_ctx,
                          const EvaluateExpressionOptions &options,
                          llvm::StringRef expr, llvm::StringRef prefix,
-                         lldb::ValueObjectSP &result_valobj_sp, Status &error,
+                         lldb::ValueObjectSP &result_valobj_sp,
                          std::string *fixed_expression, ValueObject *ctx_obj) {
   Log *log(GetLog(LLDBLog::Expressions | LLDBLog::Step));
+  auto set_error = [&](Status error) {
+    result_valobj_sp = ValueObjectConstResult::Create(
+        exe_ctx.GetBestExecutionContextScope(), std::move(error));
+  };
 
   if (ctx_obj) {
     static unsigned const ctx_type_mask = lldb::TypeFlags::eTypeIsClass |
@@ -155,8 +159,7 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
     if (!(ctx_obj->GetTypeInfo() & ctx_type_mask)) {
       LLDB_LOG(log, "== [UserExpression::Evaluate] Passed a context object of "
                     "an invalid type, can't run expressions.");
-      error =
-          Status::FromErrorString("a context object of an invalid type passed");
+      set_error(Status("a context object of an invalid type passed"));
       return lldb::eExpressionSetupError;
     }
   }
@@ -168,8 +171,8 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
       LLDB_LOG(log, "== [UserExpression::Evaluate] Passed a context object of "
                     "a reference type that can't be dereferenced, can't run "
                     "expressions.");
-      error = Status::FromErrorString(
-          "passed context object of an reference type cannot be deferenced");
+      set_error(Status(
+          "passed context object of an reference type cannot be deferenced"));
       return lldb::eExpressionSetupError;
     }
 
@@ -181,37 +184,34 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
   const ResultType desired_type = options.DoesCoerceToId()
                                       ? UserExpression::eResultTypeId
                                       : UserExpression::eResultTypeAny;
-  lldb::ExpressionResults execution_results = lldb::eExpressionSetupError;
-
   Target *target = exe_ctx.GetTargetPtr();
   if (!target) {
     LLDB_LOG(log, "== [UserExpression::Evaluate] Passed a NULL target, can't "
                   "run expressions.");
-    error = Status::FromErrorString("expression passed a null target");
+    set_error(Status("expression passed a null target"));
     return lldb::eExpressionSetupError;
   }
 
   Process *process = exe_ctx.GetProcessPtr();
 
-  if (process == nullptr && execution_policy == eExecutionPolicyAlways) {
+  if (!process && execution_policy == eExecutionPolicyAlways) {
     LLDB_LOG(log, "== [UserExpression::Evaluate] No process, but the policy is "
                   "eExecutionPolicyAlways");
 
-    error = Status::FromErrorString(
-        "expression needed to run but couldn't: no process");
+    set_error(Status("expression needed to run but couldn't: no process"));
 
-    return execution_results;
+    return lldb::eExpressionSetupError;
   }
 
   // Since we might need to allocate memory, we need to be stopped to run
   // an expression.
-  if (process != nullptr && process->GetState() != lldb::eStateStopped) {
-    error = Status::FromErrorStringWithFormatv(
+  if (process && process->GetState() != lldb::eStateStopped) {
+    set_error(Status::FromErrorStringWithFormatv(
         "unable to evaluate expression while the process is {0}: the process "
         "must be stopped because the expression might require allocating "
         "memory.",
-        StateAsCString(process->GetState()));
-    return execution_results;
+        StateAsCString(process->GetState())));
+    return lldb::eExpressionSetupError;
   }
 
   // Explicitly force the IR interpreter to evaluate the expression when the
@@ -251,13 +251,14 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
       language = frame->GetLanguage();
   }
 
+  Status error;
   lldb::UserExpressionSP user_expression_sp(
-      target->GetUserExpressionForLanguage(expr, full_prefix, language,
-                                           desired_type, options, ctx_obj,
-                                           error));
+      target->GetUserExpressionForLanguage(
+          expr, full_prefix, language, desired_type, options, ctx_obj, error));
   if (error.Fail() || !user_expression_sp) {
     LLDB_LOG(log, "== [UserExpression::Evaluate] Getting expression: {0} ==",
              error.AsCString());
+    set_error(std::move(error));
     return lldb::eExpressionSetupError;
   }
 
@@ -268,10 +269,7 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
   const bool generate_debug_info = options.GetGenerateDebugInfo();
 
   if (options.InvokeCancelCallback(lldb::eExpressionEvaluationParse)) {
-    Status error = Status::FromErrorString(
-        "expression interrupted by callback before parse");
-    result_valobj_sp = ValueObjectConstResult::Create(
-        exe_ctx.GetBestExecutionContextScope(), std::move(error));
+    set_error(Status("expression interrupted by callback before parse"));
     return lldb::eExpressionInterrupted;
   }
 
@@ -287,6 +285,7 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
     fixed_expression = &tmp_fixed_expression;
 
   *fixed_expression = user_expression_sp->GetFixedText().str();
+  lldb::ExpressionResults execution_results = lldb::eExpressionSetupError;
 
   // If there is a fixed expression, try to parse it:
   if (!parse_success) {
@@ -358,15 +357,13 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
             lldb::eExpressionSetupError,
             "expression needed to run but couldn't"));
     } else if (execution_policy == eExecutionPolicyTopLevel) {
-      error = Status(UserExpression::kNoResult, lldb::eErrorTypeGeneric);
+      set_error(Status(UserExpression::kNoResult, lldb::eErrorTypeGeneric));
       return lldb::eExpressionCompleted;
     } else {
       if (options.InvokeCancelCallback(lldb::eExpressionEvaluationExecution)) {
-        error = Status::FromError(llvm::make_error<ExpressionError>(
+        set_error(Status::FromError(llvm::make_error<ExpressionError>(
             lldb::eExpressionInterrupted,
-            "expression interrupted by callback before execution"));
-        result_valobj_sp = ValueObjectConstResult::Create(
-            exe_ctx.GetBestExecutionContextScope(), std::move(error));
+            "expression interrupted by callback before execution")));
         return lldb::eExpressionInterrupted;
       }
 
@@ -410,17 +407,14 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
   }
 
   if (options.InvokeCancelCallback(lldb::eExpressionEvaluationComplete)) {
-    error = Status::FromError(llvm::make_error<ExpressionError>(
+    set_error(Status::FromError(llvm::make_error<ExpressionError>(
         lldb::eExpressionInterrupted,
-        "expression interrupted by callback after complete"));
+        "expression interrupted by callback after complete")));
     return lldb::eExpressionInterrupted;
   }
 
-  if (result_valobj_sp.get() == nullptr) {
-    result_valobj_sp = ValueObjectConstResult::Create(
-        exe_ctx.GetBestExecutionContextScope(), std::move(error));
-  }
-
+  if (error.Fail())
+    set_error(std::move(error));
   return execution_results;
 }
 

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

@@ -323,15 +323,15 @@ StructuredData::ObjectSP InstrumentationRuntimeTSan::RetrieveReportData(
 
   ValueObjectSP main_value;
   ExecutionContext exe_ctx;
-  Status eval_error;
   frame_sp->CalculateExecutionContext(exe_ctx);
   ExpressionResults result = UserExpression::Evaluate(
       exe_ctx, options, thread_sanitizer_retrieve_report_data_command, "",
-      main_value, eval_error);
+      main_value);
   if (result != eExpressionCompleted) {
     StreamString ss;
     ss << "cannot evaluate ThreadSanitizer expression:\n";
-    ss << eval_error.AsCString();
+    if (main_value)
+      ss << main_value->GetError().AsCString();
     Debugger::ReportWarning(ss.GetString().str(),
                             process_sp->GetTarget().GetDebugger().GetID());
     return StructuredData::ObjectSP();

lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp

@@ -130,15 +130,15 @@ StructuredData::ObjectSP InstrumentationRuntimeUBSan::RetrieveReportData(
 
   ValueObjectSP main_value;
   ExecutionContext exe_ctx;
-  Status eval_error;
   frame_sp->CalculateExecutionContext(exe_ctx);
   ExpressionResults result = UserExpression::Evaluate(
       exe_ctx, options, ub_sanitizer_retrieve_report_data_command, "",
-      main_value, eval_error);
+      main_value);
   if (result != eExpressionCompleted) {
     StreamString ss;
     ss << "cannot evaluate UndefinedBehaviorSanitizer expression:\n";
-    ss << eval_error.AsCString();
+    if (main_value)
+      ss << main_value->GetError().AsCString();
     Debugger::ReportWarning(ss.GetString().str(),
                             process_sp->GetTarget().GetDebugger().GetID());
     return StructuredData::ObjectSP();

lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp

@@ -84,15 +84,15 @@ ReportRetriever::RetrieveReportData(const ProcessSP process_sp) {
 
   ValueObjectSP return_value_sp;
   ExecutionContext exe_ctx;
-  Status eval_error;
   frame_sp->CalculateExecutionContext(exe_ctx);
   ExpressionResults result = UserExpression::Evaluate(
       exe_ctx, options, address_sanitizer_retrieve_report_data_command, "",
-      return_value_sp, eval_error);
+      return_value_sp);
   if (result != eExpressionCompleted) {
     StreamString ss;
     ss << "cannot evaluate AddressSanitizer expression:\n";
-    ss << eval_error.AsCString();
+    if (return_value_sp)
+      ss << return_value_sp->GetError().AsCString();
     Debugger::ReportWarning(ss.GetString().str(),
                             process_sp->GetTarget().GetDebugger().GetID());
     return StructuredData::ObjectSP();

lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp

@@ -162,7 +162,6 @@ HistoryThreads MemoryHistoryASan::GetHistoryThreads(lldb::addr_t address) {
   ExecutionContext exe_ctx(frame_sp);
   ValueObjectSP return_value_sp;
   StreamString expr;
-  Status eval_error;
   expr.Printf(memory_history_asan_command_format, address, address);
 
   EvaluateExpressionOptions options;
@@ -176,11 +175,12 @@ HistoryThreads MemoryHistoryASan::GetHistoryThreads(lldb::addr_t address) {
   options.SetLanguage(eLanguageTypeObjC_plus_plus);
 
   ExpressionResults expr_result = UserExpression::Evaluate(
-      exe_ctx, options, expr.GetString(), "", return_value_sp, eval_error);
+      exe_ctx, options, expr.GetString(), "", return_value_sp);
   if (expr_result != eExpressionCompleted) {
     StreamString ss;
     ss << "cannot evaluate AddressSanitizer expression:\n";
-    ss << eval_error.AsCString();
+    if (return_value_sp)
+      ss << return_value_sp->GetError().AsCString();
     Debugger::ReportWarning(ss.GetString().str(),
                             process_sp->GetTarget().GetDebugger().GetID());
     return result;

lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp

@@ -536,12 +536,11 @@ Status PlatformPOSIX::EvaluateLibdlExpression(
                                          // don't do the work to trap them.
   expr_options.SetTimeout(process->GetUtilityExpressionTimeout());
 
-  Status expr_error;
-  ExpressionResults result =
-      UserExpression::Evaluate(exe_ctx, expr_options, expr_cstr, expr_prefix,
-                               result_valobj_sp, expr_error);
+  ExpressionResults result = UserExpression::Evaluate(
+      exe_ctx, expr_options, expr_cstr, expr_prefix, result_valobj_sp);
   if (result != eExpressionCompleted)
-    return expr_error;
+    return result_valobj_sp ? result_valobj_sp->GetError().Clone()
+                            : Status("unknown error");
 
   if (result_valobj_sp->GetError().Fail())
     return result_valobj_sp->GetError().Clone();

lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp

@@ -798,13 +798,12 @@ extern "C" {
   options.SetTrapExceptions(false);
   options.SetTimeout(process->GetUtilityExpressionTimeout());
 
-  Status error;
   ExpressionResults result = UserExpression::Evaluate(
-      context, options, expression, kLoaderDecls, value, error);
+      context, options, expression, kLoaderDecls, value);
   if (result != eExpressionCompleted)
-    return error;
+    return value ? value->GetError().Clone() : Status("unknown error");
 
-  if (value->GetError().Fail())
+  if (value && value->GetError().Fail())
     return value->GetError().Clone();
 
   return Status();

lldb/source/Target/StopInfo.cpp

@@ -932,10 +932,9 @@ class StopInfoWatchpoint : public StopInfo {
           expr_options.SetUnwindOnError(true);
           expr_options.SetIgnoreBreakpoints(true);
           ValueObjectSP result_value_sp;
-          Status error;
           result_code = UserExpression::Evaluate(
               exe_ctx, expr_options, wp_sp->GetConditionText(),
-              llvm::StringRef(), result_value_sp, error);
+              llvm::StringRef(), result_value_sp);
 
           if (result_code == eExpressionCompleted) {
             if (result_value_sp) {
@@ -959,7 +958,10 @@ class StopInfoWatchpoint : public StopInfo {
               }
             }
           } else {
-            const char *err_str = error.AsCString("<unknown error>");
+            const char *err_str = "<unknown error>";
+            if (result_value_sp)
+              err_str = result_value_sp->GetError().AsCString();
+
             LLDB_LOGF(log, "Error evaluating condition: \"%s\"\n", err_str);
 
             StreamString strm;

lldb/source/Target/Target.cpp

@@ -2842,14 +2842,9 @@ ExpressionResults Target::EvaluateExpression(
     execution_results = eExpressionCompleted;
   } else {
     llvm::StringRef prefix = GetExpressionPrefixContents();
-    Status error;
-    execution_results = UserExpression::Evaluate(exe_ctx, options, expr, prefix,
-                                                 result_valobj_sp, error,
-                                                 fixed_expression, ctx_obj);
-    // Pass up the error by wrapping it inside an error result.
-    if (error.Fail() && !result_valobj_sp)
-      result_valobj_sp = ValueObjectConstResult::Create(
-          exe_ctx.GetBestExecutionContextScope(), std::move(error));
+    execution_results =
+        UserExpression::Evaluate(exe_ctx, options, expr, prefix,
+                                 result_valobj_sp, fixed_expression, ctx_obj);
   }
 
   if (execution_results == eExpressionCompleted)

lldb/test/API/commands/expression/fixits/TestFixIts.py

@@ -53,7 +53,7 @@ def test_with_target(self):
         expr = "struct MyTy { int m; }; MyTy x; MyTy *ptr = &x; int m = ptr.m;"
         value = frame.EvaluateExpression(expr, top_level_options)
         # A successfully parsed top-level expression will yield an
-        # unknown error . If a parsing error would have happened we
+        # unknown error. If a parsing error would have happened we
         # would get a different error kind, so let's check the error
         # kind here.
         self.assertEqual(value.GetError().GetCString(), "unknown error")