[lldb] Return an llvm::Expected from DWARFExpression::Evaluate (NFCI) #94420
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
Change the interface of DWARFExpression::Evaluate and DWARFExpressionList::Evaluate to return an llvm::Expected instead of a boolean. This eliminates the Status output parameter and improves error handling.
JDevlieghere
requested review from
adrian-prantl,
labath,
clayborg and
Michael137
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesChange the interface of DWARFExpression::Evaluate and Patch is 78.24 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/94420.diff 12 Files Affected:
diff --git a/lldb/include/lldb/Expression/DWARFExpression.h b/lldb/include/lldb/Expression/DWARFExpression.h
index 1d85308d1caa7..e85ba464dea6b 100644
--- a/lldb/include/lldb/Expression/DWARFExpression.h
+++ b/lldb/include/lldb/Expression/DWARFExpression.h
@@ -132,13 +132,12 @@ class DWARFExpression {
/// \return
/// True on success; false otherwise. If error_ptr is non-NULL,
/// details of the failure are provided through it.
- static bool Evaluate(ExecutionContext *exe_ctx, RegisterContext *reg_ctx,
- lldb::ModuleSP module_sp, const DataExtractor &opcodes,
- const plugin::dwarf::DWARFUnit *dwarf_cu,
- const lldb::RegisterKind reg_set,
- const Value *initial_value_ptr,
- const Value *object_address_ptr, Value &result,
- Status *error_ptr);
+ static llvm::Expected<Value>
+ Evaluate(ExecutionContext *exe_ctx, RegisterContext *reg_ctx,
+ lldb::ModuleSP module_sp, const DataExtractor &opcodes,
+ const plugin::dwarf::DWARFUnit *dwarf_cu,
+ const lldb::RegisterKind reg_set, const Value *initial_value_ptr,
+ const Value *object_address_ptr);
static bool ParseDWARFLocationList(const plugin::dwarf::DWARFUnit *dwarf_cu,
const DataExtractor &data,
diff --git a/lldb/include/lldb/Expression/DWARFExpressionList.h b/lldb/include/lldb/Expression/DWARFExpressionList.h
index c2218ad4af0a7..f711a1cbe9bbd 100644
--- a/lldb/include/lldb/Expression/DWARFExpressionList.h
+++ b/lldb/include/lldb/Expression/DWARFExpressionList.h
@@ -9,6 +9,7 @@
#ifndef LLDB_EXPRESSION_DWARFEXPRESSIONLIST_H
#define LLDB_EXPRESSION_DWARFEXPRESSIONLIST_H
+#include "lldb/Core/Value.h"
#include "lldb/Expression/DWARFExpression.h"
#include "lldb/Utility/RangeMap.h"
#include "lldb/lldb-private.h"
@@ -113,10 +114,11 @@ class DWARFExpressionList {
void SetModule(const lldb::ModuleSP &module) { m_module_wp = module; }
- bool Evaluate(ExecutionContext *exe_ctx, RegisterContext *reg_ctx,
- lldb::addr_t func_load_addr, const Value *initial_value_ptr,
- const Value *object_address_ptr, Value &result,
- Status *error_ptr) const;
+ llvm::Expected<Value> Evaluate(ExecutionContext *exe_ctx,
+ RegisterContext *reg_ctx,
+ lldb::addr_t func_load_addr,
+ const Value *initial_value_ptr,
+ const Value *object_address_ptr) const;
private:
// RangeDataVector requires a comparator for DWARFExpression, but it doesn't
diff --git a/lldb/source/Core/ValueObjectVariable.cpp b/lldb/source/Core/ValueObjectVariable.cpp
index 67d71c90a959d..51eb11d3a189e 100644
--- a/lldb/source/Core/ValueObjectVariable.cpp
+++ b/lldb/source/Core/ValueObjectVariable.cpp
@@ -164,8 +164,11 @@ bool ValueObjectVariable::UpdateValue() {
target);
}
Value old_value(m_value);
- if (expr_list.Evaluate(&exe_ctx, nullptr, loclist_base_load_addr, nullptr,
- nullptr, m_value, &m_error)) {
+ llvm::Expected<Value> maybe_value = expr_list.Evaluate(
+ &exe_ctx, nullptr, loclist_base_load_addr, nullptr, nullptr);
+
+ if (maybe_value) {
+ m_value = *maybe_value;
m_resolved_value = m_value;
m_value.SetContext(Value::ContextType::Variable, variable);
@@ -246,6 +249,7 @@ bool ValueObjectVariable::UpdateValue() {
SetValueIsValid(m_error.Success());
} else {
+ m_error = maybe_value.takeError();
// could not find location, won't allow editing
m_resolved_value.SetContext(Value::ContextType::Invalid, nullptr);
}
diff --git a/lldb/source/Expression/DWARFExpression.cpp b/lldb/source/Expression/DWARFExpression.cpp
index 7473bb8255d0a..5c7ab49410efe 100644
--- a/lldb/source/Expression/DWARFExpression.cpp
+++ b/lldb/source/Expression/DWARFExpression.cpp
@@ -541,12 +541,12 @@ bool DWARFExpression::LinkThreadLocalStorage(
return true;
}
-static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
- ExecutionContext *exe_ctx,
- RegisterContext *reg_ctx,
- const DataExtractor &opcodes,
- lldb::offset_t &opcode_offset,
- Status *error_ptr, Log *log) {
+static llvm::Error Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
+ ExecutionContext *exe_ctx,
+ RegisterContext *reg_ctx,
+ const DataExtractor &opcodes,
+ lldb::offset_t &opcode_offset,
+ Log *log) {
// DW_OP_entry_value(sub-expr) describes the location a variable had upon
// function entry: this variable location is presumed to be optimized out at
// the current PC value. The caller of the function may have call site
@@ -593,16 +593,15 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
// 1. Find the function which pushed the current frame onto the stack.
if ((!exe_ctx || !exe_ctx->HasTargetScope()) || !reg_ctx) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no exe/reg context");
- return false;
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no exe/reg context");
}
StackFrame *current_frame = exe_ctx->GetFramePtr();
Thread *thread = exe_ctx->GetThreadPtr();
- if (!current_frame || !thread) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no current frame/thread");
- return false;
- }
+ if (!current_frame || !thread)
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no current frame/thread");
Target &target = exe_ctx->GetTargetRef();
StackFrameSP parent_frame = nullptr;
@@ -634,25 +633,23 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
break;
}
if (!parent_frame || !parent_frame->GetRegisterContext()) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no parent frame with reg ctx");
- return false;
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no parent frame with reg ctx");
}
Function *parent_func =
parent_frame->GetSymbolContext(eSymbolContextFunction).function;
- if (!parent_func) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no parent function");
- return false;
- }
+ if (!parent_func)
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no parent function");
// 2. Find the call edge in the parent function responsible for creating the
// current activation.
Function *current_func =
current_frame->GetSymbolContext(eSymbolContextFunction).function;
- if (!current_func) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no current function");
- return false;
- }
+ if (!current_func)
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no current function");
CallEdge *call_edge = nullptr;
ModuleList &modlist = target.GetImages();
@@ -663,17 +660,16 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
// produced by an ambiguous tail call. In this case, refuse to proceed.
call_edge = parent_func->GetCallEdgeForReturnAddress(return_pc, target);
if (!call_edge) {
- LLDB_LOG(log,
- "Evaluate_DW_OP_entry_value: no call edge for retn-pc = {0:x} "
- "in parent frame {1}",
- return_pc, parent_func->GetName());
- return false;
+ return llvm::createStringError(llvm::formatv(
+ "Evaluate_DW_OP_entry_value: no call edge for retn-pc = {0:x} "
+ "in parent frame {1}",
+ return_pc, parent_func->GetName()));
}
Function *callee_func = call_edge->GetCallee(modlist, parent_exe_ctx);
if (callee_func != current_func) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: ambiguous call sequence, "
- "can't find real parent frame");
- return false;
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: ambiguous call sequence, "
+ "can't find real parent frame");
}
} else {
// The StackFrameList solver machinery has deduced that an unambiguous tail
@@ -686,21 +682,19 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
}
}
}
- if (!call_edge) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: no unambiguous edge from parent "
- "to current function");
- return false;
- }
+ if (!call_edge)
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no unambiguous edge from parent "
+ "to current function");
// 3. Attempt to locate the DW_OP_entry_value expression in the set of
// available call site parameters. If found, evaluate the corresponding
// parameter in the context of the parent frame.
const uint32_t subexpr_len = opcodes.GetULEB128(&opcode_offset);
const void *subexpr_data = opcodes.GetData(&opcode_offset, subexpr_len);
- if (!subexpr_data) {
- LLDB_LOG(log, "Evaluate_DW_OP_entry_value: subexpr could not be read");
- return false;
- }
+ if (!subexpr_data)
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: subexpr could not be read");
const CallSiteParameter *matched_param = nullptr;
for (const CallSiteParameter ¶m : call_edge->GetCallSiteParameters()) {
@@ -726,28 +720,27 @@ static bool Evaluate_DW_OP_entry_value(std::vector<Value> &stack,
break;
}
}
- if (!matched_param) {
- LLDB_LOG(log,
- "Evaluate_DW_OP_entry_value: no matching call site param found");
- return false;
- }
+ if (!matched_param)
+ return llvm::createStringError(
+ "Evaluate_DW_OP_entry_value: no matching call site param found");
// TODO: Add support for DW_OP_push_object_address within a DW_OP_entry_value
// subexpresion whenever llvm does.
- Value result;
const DWARFExpressionList ¶m_expr = matched_param->LocationInCaller;
- if (!param_expr.Evaluate(&parent_exe_ctx,
- parent_frame->GetRegisterContext().get(),
- LLDB_INVALID_ADDRESS,
- /*initial_value_ptr=*/nullptr,
- /*object_address_ptr=*/nullptr, result, error_ptr)) {
+
+ llvm::Expected<Value> maybe_result = param_expr.Evaluate(
+ &parent_exe_ctx, parent_frame->GetRegisterContext().get(),
+ LLDB_INVALID_ADDRESS,
+ /*initial_value_ptr=*/nullptr,
+ /*object_address_ptr=*/nullptr);
+ if (!maybe_result) {
LLDB_LOG(log,
"Evaluate_DW_OP_entry_value: call site param evaluation failed");
- return false;
+ return maybe_result.takeError();
}
- stack.push_back(result);
- return true;
+ stack.push_back(*maybe_result);
+ return llvm::Error::success();
}
namespace {
@@ -862,19 +855,15 @@ static Scalar DerefSizeExtractDataHelper(uint8_t *addr_bytes,
return addr_data.GetAddress(&addr_data_offset);
}
-bool DWARFExpression::Evaluate(
+llvm::Expected<Value> DWARFExpression::Evaluate(
ExecutionContext *exe_ctx, RegisterContext *reg_ctx,
lldb::ModuleSP module_sp, const DataExtractor &opcodes,
const DWARFUnit *dwarf_cu, const lldb::RegisterKind reg_kind,
- const Value *initial_value_ptr, const Value *object_address_ptr,
- Value &result, Status *error_ptr) {
+ const Value *initial_value_ptr, const Value *object_address_ptr) {
- if (opcodes.GetByteSize() == 0) {
- if (error_ptr)
- error_ptr->SetErrorString(
- "no location, value may have been optimized out");
- return false;
- }
+ if (opcodes.GetByteSize() == 0)
+ return llvm::createStringError(
+ "no location, value may have been optimized out");
std::vector<Value> stack;
Process *process = nullptr;
@@ -994,11 +983,9 @@ bool DWARFExpression::Evaluate(
// retrieved from the dereferenced address is the size of an address on the
// target machine.
case DW_OP_deref: {
- if (stack.empty()) {
- if (error_ptr)
- error_ptr->SetErrorString("Expression stack empty for DW_OP_deref.");
- return false;
- }
+ if (stack.empty())
+ return llvm::createStringError(
+ "Expression stack empty for DW_OP_deref.");
Value::ValueType value_type = stack.back().GetValueType();
switch (value_type) {
case Value::ValueType::HostAddress: {
@@ -1013,11 +1000,12 @@ bool DWARFExpression::Evaluate(
LLDB_INVALID_ADDRESS);
Address so_addr;
+ Status load_err;
auto maybe_load_addr = ResolveLoadAddress(
- exe_ctx, module_sp, error_ptr, "DW_OP_deref", file_addr, so_addr);
+ exe_ctx, module_sp, &load_err, "DW_OP_deref", file_addr, so_addr);
if (!maybe_load_addr)
- return false;
+ return load_err.ToError();
stack.back().GetScalar() = *maybe_load_addr;
// Fall through to load address promotion code below.
@@ -1041,30 +1029,22 @@ bool DWARFExpression::Evaluate(
stack.back().GetScalar() = pointer_value;
stack.back().ClearContext();
} else {
- if (error_ptr)
- error_ptr->SetErrorStringWithFormat(
- "Failed to dereference pointer from 0x%" PRIx64
- " for DW_OP_deref: %s\n",
- pointer_addr, error.AsCString());
- return false;
+ return llvm::createStringError(
+ "Failed to dereference pointer from 0x%" PRIx64
+ " for DW_OP_deref: %s\n",
+ pointer_addr, error.AsCString());
}
} else {
- if (error_ptr)
- error_ptr->SetErrorString("NULL process for DW_OP_deref.\n");
- return false;
+ return llvm::createStringError("NULL process for DW_OP_deref.\n");
}
} else {
- if (error_ptr)
- error_ptr->SetErrorString(
- "NULL execution context for DW_OP_deref.\n");
- return false;
+ return llvm::createStringError(
+ "NULL execution context for DW_OP_deref.\n");
}
break;
case Value::ValueType::Invalid:
- if (error_ptr)
- error_ptr->SetErrorString("Invalid value type for DW_OP_deref.\n");
- return false;
+ return llvm::createStringError("Invalid value type for DW_OP_deref.\n");
}
} break;
@@ -1083,18 +1063,13 @@ bool DWARFExpression::Evaluate(
// expression stack.
case DW_OP_deref_size: {
if (stack.empty()) {
- if (error_ptr)
- error_ptr->SetErrorString(
- "Expression stack empty for DW_OP_deref_size.");
- return false;
+ return llvm::createStringError(
+ "Expression stack empty for DW_OP_deref_size.");
}
uint8_t size = opcodes.GetU8(&offset);
if (size > 8) {
- if (error_ptr)
- error_ptr->SetErrorStringWithFormat(
- "Invalid address size for DW_OP_deref_size: %d\n",
- size);
- return false;
+ return llvm::createStringError(
+ "Invalid address size for DW_OP_deref_size: %d\n", size);
}
Value::ValueType value_type = stack.back().GetValueType();
switch (value_type) {
@@ -1142,13 +1117,14 @@ bool DWARFExpression::Evaluate(
auto file_addr =
stack.back().GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
Address so_addr;
+ Status resolve_err;
auto maybe_load_addr =
- ResolveLoadAddress(exe_ctx, module_sp, error_ptr,
- "DW_OP_deref_size", file_addr, so_addr,
- /*check_sectionoffset=*/true);
+ ResolveLoadAddress(exe_ctx, module_sp, &resolve_err,
+ "DW_OP_deref_size", file_addr, so_addr,
+ /*check_sectionoffset=*/true);
if (!maybe_load_addr)
- return false;
+ return resolve_err.ToError();
addr_t load_addr = *maybe_load_addr;
@@ -1166,12 +1142,10 @@ bool DWARFExpression::Evaluate(
stack.back().ClearContext();
break;
} else {
- if (error_ptr)
- error_ptr->SetErrorStringWithFormat(
- "Failed to dereference pointer for DW_OP_deref_size: "
- "%s\n",
- error.AsCString());
- return false;
+ return llvm::createStringError(
+ "Failed to dereference pointer for DW_OP_deref_size: "
+ "%s\n",
+ error.AsCString());
}
}
stack.back().GetScalar() = load_addr;
@@ -1195,30 +1169,25 @@ bool DWARFExpression::Evaluate(
process->GetByteOrder(), size);
stack.back().ClearContext();
} else {
- if (error_ptr)
- error_ptr->SetErrorStringWithFormat(
- "Failed to dereference pointer from 0x%" PRIx64
- " for DW_OP_deref: %s\n",
- pointer_addr, error.AsCString());
- return false;
+ return llvm::createStringError(
+ "Failed to dereference pointer from 0x%" PRIx64
+ " for DW_OP_deref: %s\n",
+ pointer_addr, error.AsCString());
}
} else {
- if (error_ptr)
- error_ptr->SetErrorString("NULL process for DW_OP_deref_size.\n");
- return false;
+
+ return llvm::createStringError(
+ "NULL process for DW_OP_deref_size.\n");
}
} else {
- if (error_ptr)
- error_ptr->SetErrorString(
- "NULL execution context for DW_OP_deref_size.\n");
- return false;
+ return llvm::createStringError(
+ "NULL execution context for DW_OP_deref_size.\n");
}
break;
case Value::ValueType::Invalid:
- if (error_ptr)
- error_ptr->SetErrorString("Invalid value for DW_OP_deref_size.\n");
- return false;
+
+ return llvm::createStringError("Invalid value for DW_OP_deref_size.\n");
}
} break;
@@ -1239,9 +1208,9 @@ bool DWARFExpression::Evaluate(
// extended to the size of an address on the target machine before being
// pushed on the expression stack.
case DW_OP_xderef_size:
- if (error_ptr)
- error_ptr->SetErrorString("Unimplemented opcode: DW_OP_xderef_size.");
- return false;
+
+ return llvm::createStringError(
+ "Unimplemented opcode: DW_OP_xderef_size.");
// OPCODE: DW_OP_xderef
// OPERANDS: none
// DESCRIPTION: Provides an extended dereference mechanism. The entry at
@@ -1253,9 +1222,7 @@ bool DWARFExpression::Evaluate(
// retrieved from the dereferenced address is the size of an address on the
// target machine.
case DW_OP_xderef:
- if (error_ptr)
- error_ptr->SetErrorString("Unimplemented opcode: DW_OP_xderef.");
- return false;
+ return llvm::createStringError("Unimplemented opcode: DW_OP_xderef.");
// All DW_OP_constXXX opcodes have a single operand as noted below:
//
@@ -1308,9 +1275,7 @@ bool DWARFExpression::Evaluate(
// DESCRIPTION: duplicates the value at the top of the stack
case DW_OP_dup:
if (stack.empty()) {
- if (error_ptr)
- error_ptr->SetErrorString("Expression stack empty for DW_OP_dup.");
- return false;
+ return llvm::createStringError("Expression stack empty for DW_OP_dup.");
} else
stack.push_back(stack.back());
break;
@@ -1320,9 +1285,8 @@ bool DWARFExpression::Evaluate(
// DESCRIPTION: pops the value at the top of the stack
case DW_OP_drop:
if (stack.empty()) {
- if (error_ptr)
- error_ptr->SetErrorString("Expression stack empty for DW...
[truncated]
|
| @@ -2042,7 +2043,7 @@ bool RegisterContextUnwind::ReadFrameAddress( | |||
| return true; | |||
| } | |||
| UnwindLogMsg("Failed to set CFA value via DWARF expression: %s", | |||
| error.AsCString()); | |||
| llvm::toString(result.takeError()).c_str()); | |||
There was a problem hiding this comment.
Does takeError disarm the result? I am wondering if this will assert that no one has handled the error here.
adrian-prantl
approved these changes
Jun 5, 2024
| "Index %u out of range for DW_OP_pick.\n", pick_idx); | ||
| return false; | ||
| return llvm::createStringError( | ||
| "Index %u out of range for DW_OP_pick.\n", pick_idx); |
There was a problem hiding this comment.
drop the extra newline
There was a problem hiding this comment.
The capitalization, periods and newlines were the small follows up mentioned offline. There's a lot that can be improved, but I think it deserves its own PR.
| if (error_ptr) | ||
| error_ptr->SetErrorString("Unimplemented opcode DW_OP_call2."); | ||
| return false; | ||
| return llvm::createStringError("Unimplemented opcode DW_OP_call2."); |
There was a problem hiding this comment.
drop all the . at the end of the error messages
There was a problem hiding this comment.
and probably drop the uppercase at the start too?
| // Could not evaluate DW_OP_entry_value. | ||
| // FUNC4-EXPR-FAIL: couldn't get the value of variable x: could not evaluate | ||
| // DW_OP_entry_value: no matching call site param found FUNC4-EXPR: couldn't | ||
| // get the value of variable sink: could not evaluate DW_OP_entry_value: no |
There was a problem hiding this comment.
There's a missing newline that disables part of the check.
There was a problem hiding this comment.
Ugh, this is clang-format wrapping the line. I'll need to wrap that between clang-format off.
JDevlieghere
added a commit
that referenced
this pull request
Jun 5, 2024
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.
Change the interface of DWARFExpression::Evaluate and
DWARFExpressionList::Evaluate to return an llvm::Expected instead of a
boolean. This eliminates the Status output parameter and improves error
handling.