Make sure Target::EvaluateExpression() passes up an error instead of silently dropping it.

[aprantl]

Summary

When UserExpression::Evaluate() fails and doesn't return a ValueObject there is no vehicle for returning the error in the return value.

This behavior can be observed by applying the following patch:

diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index f1a311b7252c..58c03ccdb068 100644
--- a/lldb/source/Target/Target.cpp
+++ b/lldb/source/Target/Target.cpp
@@ -2370,6 +2370,7 @@ UserExpression *Target::GetUserExpressionForLanguage(
     Expression::ResultType desired_type,
     const EvaluateExpressionOptions &options, ValueObject *ctx_obj,
     Status &error) {
+  error.SetErrorStringWithFormat("Ha ha!");  return nullptr;
   auto type_system_or_err = GetScratchTypeSystemForLanguage(language);
   if (auto err = type_system_or_err.takeError()) {
     error.SetErrorStringWithFormat(

and then running

$ lldb -o "p 1"
(lldb) p 1
(lldb)

This patch fixes this by creating an empty result ValueObject that wraps the error.

[aprantl]

In case anyone is curious about the motivation — In swift-lldb GetUserExpressionForLanguage() can actually fail and then you get into this very situation. Even though the expression still fails, a command line user has no feedback about this.

[jingham]

LGTM.

It's a bit wrong that UserExpression::Evaluate takes an error and a result_valobj_sp. What would it mean if result_valobj_sp->GetError() was something, but something different from the error parameter? ValueObjects are supposed to carry their error with them, so really this should just always make a ValueObject and put the error in it.

But that wrongness is not of your making, and this patch does what should happen, make the ValueObject carry the significant error. So as a short term fix this is fine.

It's also unfortunate that we don't have a ValueObjectError that just knows its error & its user-chosen name. ValueObjectConstResult is carrying a lot of water at this point. OTOH, it's the best choice of the classes we currently have.

[aprantl]

It's a bit wrong that UserExpression::Evaluate takes an error and a result_valobj_sp.

I agree. I might be fixing that in a follow-up commit.

[stella.stamenova]

I think this change broke the windows lldb bot: https://lab.llvm.org/buildbot/#/builders/83/builds/24905

[aprantl]

Turns out this did have an effect on the test suite: There was one test that expected a failing expression to have an invalid SBValue and an Error:

diff --git a/lldb/test/API/commands/expression/context-object/TestContextObject.py b/lldb/test/API/c
ommands/expression/context-object/TestContextObject.py
index 45f7a003837b..7c963ebd846c 100644
--- a/lldb/test/API/commands/expression/context-object/TestContextObject.py
+++ b/lldb/test/API/commands/expression/context-object/TestContextObject.py
@@ -67,7 +67,7 @@ class ContextObjectTestCase(TestBase):
 
         # Test an expression evaluation
         value = obj_val.EvaluateExpression("1")
-        self.assertFalse(value.IsValid())
+        self.assertTrue(value.IsValid())
         self.assertFalse(value.GetError().Success())
 
         #

lldb/source/Commands/CommandObjectExpression.cpp

if (result_valobj_sp->GetError().Success()) {
             error_stream.EOL();
         } else {
           error_stream.PutCString("error: unknown error\n");
         }
 
         result.SetStatus(eReturnStatusFailed);
       }
     }
+  } else {
+    error_stream.Printf("error: unknown error\n");
   }
 
   return (success != eExpressionSetupError &&
           success != eExpressionParseError);
 }
 
 void CommandObjectExpression::IOHandlerInputComplete(IOHandler &io_handler,
                                                      std::string &line) {

lldb/source/Target/Target.cpp

 #include "lldb/Core/ModuleSpec.h"
 #include "lldb/Core/PluginManager.h"
 #include "lldb/Core/SearchFilter.h"
 #include "lldb/Core/Section.h"
 #include "lldb/Core/SourceManager.h"
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/StructuredDataImpl.h"
 #include "lldb/Core/ValueObject.h"
+#include "lldb/Core/ValueObjectConstResult.h"
 #include "lldb/Expression/DiagnosticManager.h"
 #include "lldb/Expression/ExpressionVariable.h"
 #include "lldb/Expression/REPL.h"
 #include "lldb/Expression/UserExpression.h"
 #include "lldb/Expression/UtilityFunction.h"
 #include "lldb/Host/Host.h"
 #include "lldb/Host/PosixApi.h"
 #include "lldb/Interpreter/CommandInterpreter.h"

     result_valobj_sp = persistent_var_sp->GetValueObject();
     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(), error);
   }
 
   if (execution_results == eExpressionCompleted)
     m_stats.GetExpressionStats().NotifySuccess();
   else
     m_stats.GetExpressionStats().NotifyFailure();
   return execution_results;
 }