[lldb] Refactor UserExpression::Evaluate to only have one error channel. #117186
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Adrian Prantl (adrian-prantl) ChangesPrior 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. Full diff: https://github.com/llvm/llvm-project/pull/117186.diff 12 Files Affected:
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")
|
jimingham
reviewed
Nov 21, 2024
There was a problem hiding this comment.
It would be marginally nicer if there were some way to have "a shared pointer that can't be empty" to express the fact that we are always going to put something in the result_valobj_sp, so you didn't have to check it for null. That's the intent of Evaluate at this point. But I'm not sure how you would express that.
This is strictly better than what was there before, so LGTM.
jimingham
approved these changes
Nov 21, 2024
JDevlieghere
approved these changes
Nov 21, 2024
| std::string *fixed_expression, ValueObject *ctx_obj) { | ||
| Log *log(GetLog(LLDBLog::Expressions | LLDBLog::Step)); | ||
| auto set_error = [&](Status error) { |
There was a problem hiding this comment.
In the interest of getting rid of Status, should this take an llvm::Error?
There was a problem hiding this comment.
ValueObject is the prime remaining user of long-lived Status objects, so it would be a purely cosmetic change.
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.
adrian-prantl
force-pushed
the
userexpression-error
branch
from
596eb40 to
208fa0a
Compare
bulbazord
reviewed
Nov 21, 2024
bulbazord
approved these changes
Nov 21, 2024
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Nov 21, 2024
…el. (llvm#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. (cherry picked from commit 6a8a4d5)
adrian-prantl
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Dec 2, 2024
…-6.1-lldb-Refactor-UserExpression-Evaluate-to-only-have-one-error-channel.-117186 [Cherry-pick into swift/release/6.1] [lldb] Refactor UserExpression::Evaluate to only have one error channel. (llvm#117186)
adrian-prantl
added a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Dec 5, 2024
…el. (llvm#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. (cherry picked from commit 6a8a4d5)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.