[Lldb-commits] [lldb] [lldb] Refactor UserExpression::Evaluate to only have one error channel. (PR #117186)
Adrian Prantl via lldb-commits
lldb-commits at lists.llvm.org
Thu Nov 21 08:34:18 PST 2024
https://github.com/adrian-prantl created https://github.com/llvm/llvm-project/pull/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.
>From 596eb40c641d2c580fd8a2dbfc11e8e3f41dd984 Mon Sep 17 00:00:00 2001
From: Adrian Prantl <aprantl at apple.com>
Date: Thu, 21 Nov 2024 08:32:06 -0800
Subject: [PATCH] [lldb] Refactor UserExpression::Evaluate to only have one
error channel.
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.
---
lldb/include/lldb/Expression/UserExpression.h | 10 ++--
lldb/source/Expression/REPL.cpp | 7 +--
lldb/source/Expression/UserExpression.cpp | 57 +++++++++----------
.../TSan/InstrumentationRuntimeTSan.cpp | 6 +-
.../UBSan/InstrumentationRuntimeUBSan.cpp | 6 +-
.../Utility/ReportRetriever.cpp | 6 +-
.../MemoryHistory/asan/MemoryHistoryASan.cpp | 6 +-
.../Plugins/Platform/POSIX/PlatformPOSIX.cpp | 9 ++-
.../Platform/Windows/PlatformWindows.cpp | 7 +--
lldb/source/Target/StopInfo.cpp | 8 ++-
lldb/source/Target/Target.cpp | 11 +---
.../commands/expression/fixits/TestFixIts.py | 2 +-
12 files changed, 60 insertions(+), 75 deletions(-)
diff --git a/lldb/include/lldb/Expression/UserExpression.h b/lldb/include/lldb/Expression/UserExpression.h
index 7ce463d2cb4e7e..2fde73dafa035d 100644
--- a/lldb/include/lldb/Expression/UserExpression.h
+++ b/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);
diff --git a/lldb/source/Expression/REPL.cpp b/lldb/source/Expression/REPL.cpp
index 56c50e346b39b8..4b53537e50e62a 100644
--- a/lldb/source/Expression/REPL.cpp
+++ b/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)) {
diff --git a/lldb/source/Expression/UserExpression.cpp b/lldb/source/Expression/UserExpression.cpp
index ed3734cbb943f6..c21bcbd9cfa060 100644
--- a/lldb/source/Expression/UserExpression.cpp
+++ b/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;
}
@@ -187,30 +190,29 @@ UserExpression::Evaluate(ExecutionContext &exe_ctx,
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;
}
// 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()));
+ StateAsCString(process->GetState())));
return execution_results;
}
@@ -251,13 +253,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 +271,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;
}
@@ -358,15 +358,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 +408,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;
}
diff --git a/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp b/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
index b0b17263ed6f4c..6d3e5b7e5573c4 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp
+++ b/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();
diff --git a/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp b/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
index 06d455e0676b21..8c2700cf21de9b 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp
+++ b/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();
diff --git a/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp b/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
index 2f1c78d07fc017..04ce339d8f6610 100644
--- a/lldb/source/Plugins/InstrumentationRuntime/Utility/ReportRetriever.cpp
+++ b/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();
diff --git a/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp b/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
index 7363f606d1a721..41df0e85199ceb 100644
--- a/lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp
+++ b/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;
diff --git a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp b/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
index 4a8f669a84ecb3..befc28b09d1859 100644
--- a/lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
+++ b/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();
diff --git a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp b/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
index 3936b8367fb83b..c0c26cc5f19548 100644
--- a/lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
+++ b/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();
diff --git a/lldb/source/Target/StopInfo.cpp b/lldb/source/Target/StopInfo.cpp
index f6387d47504e62..356917a45b7b34 100644
--- a/lldb/source/Target/StopInfo.cpp
+++ b/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;
diff --git a/lldb/source/Target/Target.cpp b/lldb/source/Target/Target.cpp
index d70274a4b7c7c4..4bac94f35d6cfb 100644
--- a/lldb/source/Target/Target.cpp
+++ b/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)
diff --git a/lldb/test/API/commands/expression/fixits/TestFixIts.py b/lldb/test/API/commands/expression/fixits/TestFixIts.py
index 1b22ed1c0077c4..bfe11f6c6fcb99 100644
--- a/lldb/test/API/commands/expression/fixits/TestFixIts.py
+++ b/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")
More information about the lldb-commits
mailing list